toolset refactor POC - #12271
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@anakin87 I like the proposed changes! And I do think that it helps simplify the implementation of Toolset quite a bit. I agree with your sentiment that we should just deprecate |
| if spawned is not item: | ||
| # spawn() returned a per-run copy that already restricts itself to the selected tools. | ||
| selected.append(spawned) | ||
| else: | ||
| selected.append(item) | ||
| # No per-run copy: extract the selected tools from `selectable`, the same list the names were | ||
| # validated against. Iterating a dynamic Toolset instead could silently miss some of them. | ||
| selected.extend(tool for tool in selectable if tool.name in matched) | ||
| return selected |
There was a problem hiding this comment.
Ahh I think I understand now (referring to the question in this comment). We do the check spawned is not item which is actually False for normal Toolsets so then we fall into the else branch.
I think that this is worth at least a dev comment explaining this behavior because reading this code in isolation I get the impression that spawned should always be different if the incoming item is a Toolset
There was a problem hiding this comment.
Agree that was difficult to understand. Clarified in 3bc0b51
2 participants
Related Issues
Toolsetclass has grown over time to support different use cases.toolsonly acceptedlist[Tool]|Toolset, so+support forToolsetenabled merging multiple toolsetswarm_upwas added but this created issues with+, so_ToolsetWrapperwas added to keep+workingSearchableToolset. So in fix: Fix usage ofSearchableToolsetwithAgentwhen selecting a subset of tools to be active #11564, the base class gainedspawn()andget_selectable_tools()All these different aspects contributed to making the class complex and prone to some bugs, like #12009. While working on this, I also found some other potential bugs:
_is_warmed_upflag prevented warming tools added later;Agent.warm_up()skipped toolsets that are empty until warmed up (MCPToolsetimplements a workaround for this).Proposed Changes:
+support; makeaddonly supportTool, no longerToolset. This removes the need for all duplicated logic in_ToolsetWrapper. Users can still passtools=[my_toolset, another_toolset]spawn()andget_selectable_tools()implementations in the base class, but move specific behaviors toSearchableToolsetwarm_up: remove_is_warmed_upflag; makeAgent.warm_up()callwarm_up_toolson every call. This is more aligned with otherwarm_upmethods in Haystack, but now a specific implementation must know how to avoid doing expensive work multiple times. (A similar change I think it would also be needed to make the Agent editable after initialization)How did you test it?
CI, new tests, tested also with MCP and other Toolset integrations
Notes for the reviewer
In this PR, I am sharing my idea for the refactor.
I realize that it contains some breaking changes. For this reason, I'd be more inclined to just deprecate
+andadd(toolset)in the next Haystack version (e.g. 3.1) and then remove them in the following versions (e.g. 3.2). The other changes do not seem too breaking to me and I'd ship them right away once we agree on the shape. WDYT?Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.