-
Notifications
You must be signed in to change notification settings - Fork 508
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
Update Tool dataclass to accept a custom function_schema argument #881
base: main
Are you sure you want to change the base?
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.
LGTM overall, thanks for the contribution! I commented with one test change request :)
tests/test_tools.py
Outdated
# make sure the function schema for y_tool is used instead of the default of x_tool. | ||
assert 'x' not in agent._function_tools['x_tool']._parameters_json_schema['properties'] | ||
assert 'y' in agent._function_tools['x_tool']._parameters_json_schema['properties'] |
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.
Maybe we could just do assert agent._function_tools['x_tool']._parameters_json_schema == snapshot(...)
?
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.
@sydney-runkle updated
I like this, but if we expose a way to override the function_schema (which I see could be a useful convenience in some scenarios where a function is defined using *args and **kwargs), I think we should move |
@dmontagu - Any suggestions? There's quite a lot/almost all of
I think maybe we can pull out only the |
a0b36dd
to
17df4fc
Compare
I don't think we should make That being said, let's expose |
@sydney-runkle - how does the latest commit look? I had to change a few things around due to the circular dependency. |
c30b05f
to
0a3a97d
Compare
Unsure if the tests should be importing
_pydantic
like that, but the idea behind the test is to add a function schema for a different function and checking that the generated json schema matches the function schema passed rather than the one that would have been generated without it.Resolves #878.