-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: Update tools param to Optional[Union[list[Union[Tool, Toolset]], Toolset]] #9886
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
a8ae92f to
1af7a0a
Compare
Pull Request Test Coverage Report for Build 18595249452Warning: 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 |
|
@sjrl @julian-risch @anakin87 I'm still double checking everything but the transition to new signature seems to work well except in one remote edge case: So there were 2 tests that failed before adding ffa6436 :
Both tests were creating a ComponentTool from OpenAIChatGenerator. So when ComponentTool tried to generate the JSON schema for OpenAIChatGenerator's run method parameters, it hit the tools parameter with the new signature:
"tools": {
"anyOf": [
{ "type": "array", "items": { "anyOf": [] } },
{ "type": "null" }
],
"default": null,
"description": "A list of tools or a Toolset the model can use. Overrides the `tools` parameter from initialization.\nAccepts either a list of `Tool` objects or a `Toolset` instance."
},instead of the expected "tools": {
"type": "null",
"default": null,
"description": "A list of tools or a Toolset the model can use. Overrides the `tools` parameter from initialization.\nAccepts either a list of `Tool` objects or a `Toolset` instance."
}for the old signature. Which triggered this failure: Full failure trace available here |
haystack/tools/component_tool.py
Outdated
| # Parameters that should be excluded from the schema generation | ||
| # These are typically configuration parameters that aren't meant to be provided by LLMs | ||
| excluded_params = {"tools", "tools_strict"} |
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'm not fully understanding the error that's caused when switching to the new tools type but I'm not convinced this is the right solution.
I'd rather get the parameter schema generation to work properly for tools such that is valid from OpenAI's perspective.
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.
No no, I agree - it was just to isolate the error and pinpoint the cause. The issue is that now with the new signature Pydantic tries to generate the schema it goes down the nested Union and ends up producing an empty anyOf array while previously it produced:
"tools": {
"type": "null",
"default": null,
"description": "A list of tools or a Toolset the model can use. Overrides the `tools` parameter from initialization.\nAccepts either a list of `Tool` objects or a `Toolset` instance."
}which was valid.
But also, think about it - tools parameter configures the component's internal behavior not input arguments the LLM is supposed to supply! This is kinda recursion into tools that doesn't make much sense.
|
I think this solution should work now. The outcome is that tools can be translated in to OpenAI api schema "tools": {
"anyOf": [
{
"type": "array",
"items": {
"anyOf": [
{ "$ref": "#/$defs/_ToolSchemaPlaceholder" },
{ "$ref": "#/$defs/_ToolsetSchemaPlaceholder" }
]
}
},
{ "$ref": "#/$defs/_ToolsetSchemaPlaceholder" },
{ "type": "null" }
],
"default": null,
"description": "A list of tools or a Toolset for which the model can prepare calls. If set, it will override the `tools` parameter set during component initialization. This parameter can accept either a list of `Tool` objects or a `Toolset` instance."
},which is acceptable to Draft202012Validator.check_schema() |
|
Very minor contribution, but I suggest replacing |
Very minor but brilliant - would simplify things quite a lot. Where would we put the definition of this alias? |
|
Nice! I didn't go too much into detail, but signatures and (de)serialization logics are looking good. |
|
Thanks @tstadel I'll go through the details tonight and this one should be ready for review tomorrow |
|
Ok the PR should be ready for review now. I'll add more edge test cases today but I wanted to give you heads up to start reviewing. thx all @sjrl @tstadel @julian-risch @anakin87 @mpangrazzi |
haystack/components/agents/agent.py
Outdated
| return tools | ||
|
|
||
| if isinstance(tools, list): | ||
| return cast(list[Union[Tool, Toolset]], tools) |
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 also add a dev comment here explaining the need for cast
test/tools/test_tools_utils.py
Outdated
| def test_flatten_nested_toolsets(self, add_tool, multiply_tool, subtract_tool): | ||
| """Test flattening multiple levels of Toolsets.""" |
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.
nit: I don't think this is actually testing mutliple levels here.
| raise TypeError("tools must be Toolset, list[Union[Tool, Toolset]], or None") | ||
|
|
||
|
|
||
| def deserialize_tools_or_toolset_inplace(data: dict[str, Any], key: str = "tools") -> None: |
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.
We should update the docstrings of this function to reflect the new type serialized tools can have. E.g. Deserialize a list of Tools or a Toolset in a dictionary inplace. is no longer fully correct
| class TestToolsetList: | ||
| """Tests for list[Toolset] functionality.""" | ||
|
|
||
| def test_tool_invoker_with_list_of_toolsets(self, weather_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.
nit: mixed case (Tool and Toolset) seems to be missing here
tstadel
left a comment
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 great already. Left a few nits.
Co-authored-by: Sebastian Husch Lee <[email protected]>
Co-authored-by: tstadel <[email protected]>
This reverts commit ebdec91.
| generation_kwargs: Optional[dict[str, Any]] = None, | ||
| streaming_callback: Optional[StreamingCallbackT] = None, | ||
| tools: Optional[Union[list[Tool], Toolset]] = None, | ||
| tools: Optional[ToolsType] = None, |
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.
docstring for this function needs to be updated
| generation_kwargs: Optional[dict[str, Any]] = None, | ||
| *, | ||
| tools: Optional[Union[list[Tool], Toolset]] = None, | ||
| tools: Optional[ToolsType] = None, |
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.
docstring for this needs to be updated below
| generation_kwargs: Optional[dict[str, Any]] = None, | ||
| *, | ||
| tools: Optional[Union[list[Tool], Toolset]] = None, | ||
| tools: Optional[ToolsType] = None, |
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.
docstring for this needs to be updated below
|
|
||
| @staticmethod | ||
| def _validate_and_prepare_tools(tools: Union[list[Tool], Toolset]) -> dict[str, Tool]: | ||
| def _validate_and_prepare_tools(tools: ToolsType) -> dict[str, 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.
docstring for tools needs to be updated for this function
| *, | ||
| enable_streaming_callback_passthrough: Optional[bool] = None, | ||
| tools: Optional[Union[list[Tool], Toolset]] = None, | ||
| tools: Optional[ToolsType] = None, |
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.
update docstring for tools below
| *, | ||
| enable_streaming_callback_passthrough: Optional[bool] = None, | ||
| tools: Optional[Union[list[Tool], Toolset]] = None, | ||
| tools: Optional[ToolsType] = None, |
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.
update docstrings below
releasenotes/notes/mixed-tools-toolsets-support-d944c5770e2e6e7b.yaml
Outdated
Show resolved
Hide resolved
sjrl
left a comment
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! A few minor comments regarding docstrings
…7b.yaml Co-authored-by: Sebastian Husch Lee <[email protected]>
|
Nice, thank you all, please let me see how to coordinate merging this PR and #9856 i.e. what to merge first and how to minimize pain |
Why:
Enhances tool flexibility across Haystack components by enabling mixed lists of Tool and Toolset objects, allowing developers to organize tools into logical groupings while maintaining the ability to include standalone tools all within the same
toolsparameter.This change is fully backward compatible, existing code continues to work unchanged.
What:
ToolsTypetype alias (Union[list[Union[Tool, Toolset]], Toolset]) to standardize tool parameter typesflatten_tools_or_toolsets()utility function for consistent tool flattening across componentsserialize_tools_or_toolset()anddeserialize_tools_or_toolset_inplace()to preserve Tool/Toolset boundaries during serialization_ToolSchemaPlaceholderand_ToolsetSchemaPlaceholderto enable JSON schema generation for non-serializable Tool/Toolset typesAgent,ToolInvoker,OpenAIChatGenerator,AzureOpenAIChatGenerator,HuggingFaceAPIChatGenerator,HuggingFaceLocalChatGeneratorHow can it be used:
How did you test it:
test_agent.py,test_openai.py,test_serde_utils.py, andtest_tools_utils.pyNotes for the reviewer:
Please review the
flatten_tools_or_toolsets()function inhaystack/tools/utils.py—this is the core utility that enables the feature and is used consistently across all components. Also note the serialization logic inserde_utils.pythat preserves the original list structure (not automatically flattening) to ensure perfect round-trip serialization. The_ToolSchemaPlaceholdertypes inparameters_schema_utils.pysolve the JSON schema generation issue for callable-containing types.