Update tool.py to include **kwargs #7918
Conversation
Before the tool call of a function containing **kwargs had to be done like this: {"kwargs": {"foo":"bar"} now {"foo":bar"} is directly supported.
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
|
Hi, @ButterflyAtHeart. Would you mind adding some test cases for this logic? |
|
Incorporate this change in #8102 |
Before the tool call of a function containing **kwargs had to be done like this: {"kwargs": {"foo":"bar"} now {"foo":bar"} is directly supported.