feat: adding typing to melleatools#959
feat: adding typing to melleatools#959akihikokuroda wants to merge 3 commits intogenerative-computing:mainfrom
Conversation
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
30615cb to
7c2eccb
Compare
| from collections.abc import Callable, Generator, Iterable, Mapping, Sequence | ||
| from typing import Any, Literal, overload | ||
| from typing import Any, Generic, Literal, ParamSpec, TypeVar, overload | ||
|
|
There was a problem hiding this comment.
NIT: Generic is imported but not used — MelleaTool picks up genericity via AbstractMelleaTool[P, R]. Project ignores F401 so CI won't complain, but it's dead code.
| from typing import Any, Literal, ParamSpec, TypeVar, overload |
| @classmethod | ||
| def from_langchain(cls, tool: Any) -> "MelleaTool": | ||
| def from_langchain(cls, tool: Any) -> "MelleaTool[P, Any]": | ||
| """Create a MelleaTool from a LangChain tool object. |
There was a problem hiding this comment.
NIT: MelleaTool[P, Any] with a module-level P collapses to MelleaTool[..., Any] under pyright and MelleaTool[Any, Any] under mypy — both type-check fine, but spelling it as MelleaTool[..., Any] is the explicit form for "unknown parameter spec" and matches what pyright shows. Worth updating the docstring Returns: line too.
| """Create a MelleaTool from a LangChain tool object. | |
| def from_langchain(cls, tool: Any) -> "MelleaTool[..., Any]": |
| @classmethod | ||
| def from_smolagents(cls, tool: Any) -> "MelleaTool": | ||
| def from_smolagents(cls, tool: Any) -> "MelleaTool[P, Any]": | ||
| """Create a Tool from a HuggingFace smolagents tool object. |
There was a problem hiding this comment.
NIT: Same as from_langchain — MelleaTool[..., Any] is the idiomatic spelling for "unknown parameter spec". Worth updating the docstring Returns: line too.
| """Create a Tool from a HuggingFace smolagents tool object. | |
| def from_smolagents(cls, tool: Any) -> "MelleaTool[..., Any]": |
| from .model_options import ModelOption | ||
|
|
||
| P = ParamSpec("P") | ||
| R = TypeVar("R") |
There was a problem hiding this comment.
NIT: Both mellea/core/base.py (lines 958-959) and this module declare their own P = ParamSpec("P") / R = TypeVar("R"). Since MelleaTool subclasses AbstractMelleaTool, tools.py could import P, R from base.py for a single identity. Minor.
planetf1
left a comment
There was a problem hiding this comment.
I've left some minor nits, but otherwise LGTM
|
@planetf1 Thanks for review. I fixed them. |
243c270 to
a6163a6
Compare
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
a6163a6 to
55d8244
Compare
Misc PR
Type of PR
Description
mellea/backends/tools.py
preserving caller type information
type info is unavailable)
mellea/core/base.py
Testing
Attribution