chore: Add type checking to test/tools#11284
Conversation
|
@maxdswain is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||
| # - list[PipelineTool]: List of PipelineTool objects | ||
| # - Toolset: Single Toolset (not in a list) | ||
| ToolsType = list[Tool] | list[Toolset] | list[Tool | Toolset] | Toolset | ||
| ToolsType = list[Tool] | list[Toolset] | list[Tool | Toolset] | list[ComponentTool] | list[PipelineTool] | Toolset |
There was a problem hiding this comment.
I don't think we need to explicitly include ComponentTool and PipelineTool since they both inherit from Tool.
There was a problem hiding this comment.
I get a bunch of type errors in test_component _tool and test_pipeline_tool. This is because lists are invariant meaning that even though they both inherit from Tool, they are not considered subtypes.
There was a problem hiding this comment.
Ahh okay I see, let me take a deeper look at this
There was a problem hiding this comment.
Could we try updating this to be ToolsType = Sequence[Tool | Toolset] | Toolset and see if that works? Using Sequence should help us overcome the list invariance issue.
Also @anakin87 I thought we considered using Sequence for this before but can't find the old convo/conclusion on that.
There was a problem hiding this comment.
That resolves the invariance issues, I've pushed that change in 29e4101 and all your comments should be addressed :)
There was a problem hiding this comment.
@sjrl I tried looking back in the history but my impression is that we never considered Sequence. Happy if it works 🙂
There was a problem hiding this comment.
@anakin87 do the changes to this file (addition of the overload) fix the typing issues you were seeing?
There was a problem hiding this comment.
yes!
from haystack.components.generators.chat.openai import OpenAIChatGenerator
from haystack.tools import tool
@tool
def my_tool(number: int) -> int:
return number * 2
generator = OpenAIChatGenerator(tools=[my_tool])running mypy on this example no longer gives errors
maxdswain
force-pushed
the
tools-tests-type-checking
branch
from
d61e022 to
a0cd909
Compare
|
@maxdswain thanks! One last comment is could you add a brief release note explaining the update to the ToolsType to mention the changes that were made to it and how that improves type checking? |
I've now added a release note. |
3 participants
Related Issues
Proposed Changes:
This PR extends type checking to
test/tools.How did you test it?
Re-ran tests - unit, integration and type checkng.
Notes for the reviewer
I didn't want to add
WarmupTrackingToolor other classes created just for tests toToolsType, so I used# type: ignorefor these situations.I modified
haystack/tools/__init__.pyexpanding theToolsTypeto includelist[ComponentTool]&list[PipelineTool], I think this was the right thing to do but I'm not sure on this.Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.