chore: Add type checking to test/tools#11284
chore: Add type checking to test/tools#11284maxdswain wants to merge 1 commit intodeepset-ai:mainfrom
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.
@anakin87 do the changes to this file (addition of the overload) fix the typing issues you were seeing?
| ] | ||
|
|
||
| toolset = SearchableToolset(catalog=[LazyToolset()] + eager_tools) | ||
| toolset = SearchableToolset(catalog=[LazyToolset()] + eager_tools) # type: ignore[arg-type] |
There was a problem hiding this comment.
Let's leave a dev comment explaining why we are ignoring this. Also it seems odd we are ignoring this I think I would expect this to pass. Which arg is throwing the type error?
|
|
||
| assert not tool.was_warmed_up | ||
| warm_up_tools([tool]) | ||
| warm_up_tools([tool]) # type: ignore[arg-type] |
There was a problem hiding this comment.
Could you explain what error is being raised here? I'd expect this to pass since WarmupTrackingTool inherits from Tool
| assert not tool2.was_warmed_up | ||
|
|
||
| warm_up_tools([toolset]) | ||
| warm_up_tools([toolset]) # type: ignore[arg-type] |
There was a problem hiding this comment.
Same comment as here https://github.com/deepset-ai/haystack/pull/11284/changes#r3216700606
2 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.