-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add ToolCall.execute for smoother tool execution #8825
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a convenient execute
method to ToolCall
objects that allows direct execution of tool calls without requiring manual function lookup and parameter passing boilerplate.
- Adds
execute
method toToolCall
class for direct tool execution - Supports multiple function lookup modes including automatic discovery from caller's scope
- Provides comprehensive test coverage for the new execution functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
dspy/adapters/types/tool.py |
Implements the execute method with flexible function lookup |
tests/adapters/test_tool.py |
Adds comprehensive test cases for the new execute functionality |
docs/docs/learn/programming/tools.md |
Updates documentation to demonstrate the new execute method usage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
docs/docs/learn/programming/tools.md
Outdated
print(f"Result: {result}") | ||
# Execute individual tool calls with different options: | ||
|
||
# Option 1: Pass tools as a dict (most explicit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like Option 3! Option 1 and 2 will be much less popular IMO though. My understanding is they handle the cases:
- users forget to include the tool in runtime, in which case
weather
will be None as well. - Users want to search functions in a tailored namespace, which is cool but I have never seen any use cases.
Can we remove the functions
arg from execute
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
option 3 cannot cover some usecases where the tool name is different from the local variable name too. I like 3 too and this is the main intention, but can we keep the function arg for flexibility? Since it's optional, it doesn't increase friction and I can change the order in this tutorial and explain option 3 first. what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the late reply.
I am curious about "where the tool name is different from the local variable name", why is this a valid case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this totally possible? In this case LM recognizes this tool as "weather" while it's registered as "my_tool" in globals.
my_tool = dspy.Tool(name="weather", func: lambda city: f"the weather in {city} is rainy")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is possible, but doesn't seem to be a valid use case IMO. When users want to set name="weather"
, I don't see why they want to change the tool instance name, and pass it in functions
later on.
Since this is not breaking and doesn't add too much complexity, I don't want to block on this, and will let you make the call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few nonblocking comments
docs/docs/learn/programming/tools.md
Outdated
print(f"Result: {result}") | ||
# Execute individual tool calls with different options: | ||
|
||
# Option 1: Pass tools as a dict (most explicit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is possible, but doesn't seem to be a valid use case IMO. When users want to set name="weather"
, I don't see why they want to change the tool instance name, and pass it in functions
later on.
Since this is not breaking and doesn't add too much complexity, I don't want to block on this, and will let you make the call
|
||
# Test locals take precedence over globals | ||
try: | ||
globals()["local_add"] = lambda a, b: a + b + 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we define it as a function before def main()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's still registered in locals in that case since globals are module-level variables.
This PR adds a helper function to ToolCall so that users can run tool calls with less boilerplate.