-
Notifications
You must be signed in to change notification settings - Fork 19k
fix(core)!: fix BaseTool when args_schema
has pydantic validators that change the input keys
#32925
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
base: wip-v1.0
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@@ -643,7 +643,7 @@ def test_named_tool_decorator_return_direct() -> None: | |||
"""Test functionality when arguments and return direct are provided as input.""" | |||
|
|||
@tool("search", return_direct=True) | |||
def search_api(query: str, *args: Any) -> str: | |||
def search_api(query: str) -> str: |
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 the breaking part
CodSpeed WallTime Performance ReportMerging #32925 will not alter performanceComparing
|
args_schema
has pydantic validators that change the input keys
return { | ||
k: getattr(result, k) for k, v in result_dict.items() if k in tool_input | ||
} | ||
return {k: getattr(result, k) for k, v in result_dict.items()} |
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.
The filter here is pretty odd. I don't understand why the original code had it
CodSpeed Instrumentation Performance ReportMerging #32925 will not alter performanceComparing Summary
Footnotes |
Description:
We encountered a bug where we used a pydantic validator to patch broken llm inputs to our tools. The validator would fix the input dict, but the BaseTool would return an empty dict to the tool because our validator changed the dictionary keys. This PR changes one line of code to fix the issue and adds a test to improve the test coverage.
Concretely, the following code would fail before the PR:
Add tests and docs:
I added a test for the above use case.
Comments:
Please note that I had to remove
*args: Any
from one of the tests. The previous code would filter out theargs
keyword. I don't see any realistic use case where one would have*args
in their tool as this gives no meaningful instruction to the LLM on how to use the*args
. Please let me know if you see any issues here. I don't see a clean and robust method that would have the new test passing while still filtering out the*args
argument. Let me know if you see a better solution.This PR is a port of #30004 courtesy of @VMinB12