Skip to content

Update tool.py to include **kwargs #7918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions dspy/primitives/tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ def __init__(
self.args = args
self.arg_types = arg_types
self.arg_desc = arg_desc

self.has_var_kwargs = False

self._parse_function(func, arg_desc)

def _resolve_pydantic_schema(self, model: type[BaseModel]) -> dict:
Expand Down Expand Up @@ -86,6 +87,8 @@ def _parse_function(self, func: Callable, arg_desc: dict[str, str] = None):

# Use inspect.signature to get all arg names
sig = inspect.signature(annotations_func)
# set has_var_kwargs to True if the function has a **kwargs parameter
self.has_var_kwargs = any([sig.parameters[k].kind == 4 for k in sig.parameters.keys()])
# Get available type hints
available_hints = get_type_hints(annotations_func)
# Build a dictionary of arg name -> type (defaulting to Any when missing)
Expand Down Expand Up @@ -116,7 +119,10 @@ def _parse_function(self, func: Callable, arg_desc: dict[str, str] = None):
def __call__(self, **kwargs):
for k, v in kwargs.items():
if k not in self.args:
raise ValueError(f"Arg {k} is not in the tool's args.")
if self.has_var_kwargs:
continue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much @ButterflyAtHeart ! Is continue the right thing to do here?

I think the right thing to do might be more like "find the name of the kwargs argument" and then "wrap the non-matching arguments into a dict with that kwargs name".

Maybe we need to test this with a real function that takes **kwargs as inputs and describes those kwargs in the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with checking for a kwargs argument is that needs to happen before the loop, because kwargs["kwargs"] doesn't necessarily exist so changing the dictionary kwargs while iterating raises an error. This would add quite a lot of code. I think continue is a more elegant solution as it just says we do not need to do any further tests for this keyword argument. Maybe adding a comment there before continue would be good. So if you agree we should stick with continue I would add this comment.

Here is the test I have used to test this:

import dspy

def func(x:int, y=1, **kwargs:dict):
    """This is a test function.
    Args:
        x (int): The first argument.
        y (int): The second argument.
    """
    return "Completed."

f = dspy.Tool(func)
f(x=0, y = 0, z=2)

If you think there need to be any other cases included into the test. Please let me now.

Copy link
Collaborator

@TomeHirata TomeHirata Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ButterflyAtHeart Should we make sure the insufficient arguments are detected?

import dspy

def func(x:int, **kwargs:dict):
    """This is a test function.
    Args:
        x (int): The first argument.
    """
    return x

f = dspy.Tool(func)
f(z=2) # should raise exception

else:
raise ValueError(f"Arg {k} is not in the tool's args.")
try:
instance = v.model_dump() if hasattr(v, "model_dump") else v
if not self.args[k] == "Any":
Expand Down
Loading