-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix component tool parameters #9342
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
Pull Request Test Coverage Report for Build 15039406888Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
…ent-tool-parameters
Current integration error is resulting from this issue huggingface/text-generation-inference#2876 where it doesn't look like HuggingFace supports the I'd suggest a separate issue for this is opened to resolve since this is already an existing bug in main when using the Let me know if you agree and I can remove that test for now and open an issue. |
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 left some comments.
I would appreciate it if @vblagoje could review too, since he originally worked on this.
I agree. (When I originally worked on tools support for HF API, I found several inconsistencies (deepset-ai/haystack-experimental#120 (comment)). I hope that most of them are fixed now, but I am not sure.) |
@anakin87 do you think it's worth updating |
Since we expect basic python types haystack/haystack/tools/from_function.py Line 61 in 42b3789
I would leave it unchanged for now |
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.
Very very nice!
Related Issues
parameters
inComponentTool
does not work for more complex types #9292Proposed Changes:
Properties Schema Generation
from_function.py
where we utilize Pydantic'smodel_json_schema
. This adds full support of basic types.parameters_schema_utils.py
. The only semi-interesting thing we do is specially handle the conversion of our dataclasses into a pydantic model. We do this so we can utilize the docstring descriptions of the parameter names. By default pydantic ignores docstrings.Remaining TODOs
oneOf
,null
,additionalProperties
) are generally supported by LLM providersHow did you test it?
Notes for the reviewer
As a heads up most of the line changes ~500 are for tests.
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
and added!
in case the PR includes breaking changes.