-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: Add ComponentTool
to Haystack tools
#8693
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.
Looks quite good to me. My main request is to make the docstring-parser dependency optional. Let's discuss if @anakin87 disagrees or if we decide to make jsonschema a required dependency too.
--- | ||
features: | ||
- | | ||
Introduced the ComponentTool, a new tool that wraps Haystack components allowing them to be utilized as tools for LLMs (various ChatGenerators). This tool supports automatic function schema generation, input type validation, and offers enhanced integration with various data types. |
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.
You mentioned limitations in the call regarding which components are supported. Could you please add them to the release note? If there is a particular Haystack component that is not supported mention it here as an example.
Thank you @julian-risch and @anakin87 - great feedback. I haven’t removed support for Pydantic models yet but if both of you are okay with its removal - I’ll proceed. The support is largely effortless thanks to TypeAdapter conversion, but as @anakin87 mentioned it could be counterproductive. I’ll remove it in a separate commit to maintain a reference for its implementation in case our users ask for it or an internal need for it arises in the future. |
|
Pull Request Test Coverage Report for Build 12715813181Warning: 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 |
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 am quite happy with the progress here.
Some of my comments probably went unnoticed. I was proposing to add some tests:
ComponentTool
to_dict
/from_dict
methods- check that an attempt to convert a Component that is part of a Pipeline fails
@dfokina could you please take a look at docstrings and release note? |
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.
Looks good!
(Nit: I have the impression that some methods can be converted to static methods. See if it makes sense...)
Let's wait for final reviews by @julian-risch and @dfokina
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.
Mostly unifying the capitalization and punctuation in docstrings. Everything else seems good, inc the release note and the error messages
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
@julian-risch @dfokina have a look again 🙏 |
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.
Left one last comment, otherwise LGTM
Co-authored-by: Daria Fokina <[email protected]>
Why:
Adds
ComponentTool
enabling Haystack components to be wrapped and used as LLM-compatible tools thus integrating them into Haystack's new tooling architecture.What:
ComponentTool
class to facilitate the use of Haystack components as callable tools.__init__.py
to includeComponentTool
in the module exports.pyproject.toml
to include thedocstring-parser
dependency for documentation extraction.How can it be used:
This implementation enables the creation of LLM-compatible tools from existing components. Here’s an example of usage:
With
weather_tool
assigned to your ChatGenerator and ToolInvoker, you can now invoke the component automatically with given a ChatMessage that will trigger this tool, e.gChatMessage.from_user("What's the weather like in S.F.?")
How did you test it:
Extensive unit and integration tests were created, validating the functionality of
ComponentTool
with various components. Tests included invoking tools and ensuring correct parameter handling and responses from LLM integrations.Notes for the reviewer:
Review the implementation of the
ComponentTool
for potential edge cases with non-component inputs. The integration tests require an OpenAI API key to run successfully, so consider verifying this before executing the tests.