-
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.
pyproject.toml
Outdated
@@ -57,6 +57,7 @@ dependencies = [ | |||
"requests", | |||
"numpy", | |||
"python-dateutil", | |||
"docstring-parser", |
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 suggest using lazyimport for this dependency similar to jsonschema.
--- | ||
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.
test/tools/test_component_tool.py
Outdated
) | ||
|
||
pipeline = Pipeline() | ||
pipeline.add_component("llm", OpenAIChatGenerator(model="gpt-4", tools=[tool])) |
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.
gpt-4o-mini
here too or is it gpt-4
on purpose?
"Use this method to create a Tool only with Haystack component instances." | ||
) | ||
raise ValueError(message) | ||
|
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.
Here I am contradicting my comment in the experimental PR (https://github.com/deepset-ai/haystack-experimental/pull/159/files/ce864dd8f4f10c2c42f3ded35a515ef71a995585#r1892494701)
but probably now that we have clarified that components to convert into tools MUST NOT be part of a Pipeline, I think it is worth adding a check.
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.
thanks!
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.
Let's add a simple test to cover this.
haystack/tools/component_tool.py
Outdated
elif is_pydantic_v2_model(python_type): | ||
schema = self._create_pydantic_schema(python_type, description) |
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.
Our components don't use Pydantic Models, so I’m unsure why we’re supporting this.
Allowing such flexibility might be counterproductive.
If there's an important perspective or requirement that I'm overlooking, I’d appreciate it if you could elaborate.
@@ -0,0 +1,549 @@ | |||
# SPDX-FileCopyrightText: 2022-present deepset GmbH <[email protected]> |
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.
Let's add tests for:
to_dict
/from_dict
complete YAML serde (here is a good 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. |
Let's also remember to add this module to docs/pydoc/config/tools_api.yml |
Pull Request Test Coverage Report for Build 12694409064Details
💛 - Coveralls |
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.