Add Tool/Toolset warm_up #9849
Add Tool/Toolset warm_up #9849
Conversation
vblagoje
added
ignore-for-release-notes
Pull Request Test Coverage Report for Build 18124993824Details
💛 - Coveralls |
| return self.tools[index] | ||
|
|
||
|
|
||
| class _ToolsetWrapper(Toolset): |
There was a problem hiding this comment.
could you explain why we need _ToolsetWrapper?
There was a problem hiding this comment.
Not sure we need it in this form (there should be something simpler), it was a hack to enable:
all_tools = perplexity_mcp + routing_mcp + some_other_tools
If they are all MCPToolset and not warmed up when addition above is executed (with current code) we'll just get garbage. We need to preserve their configs (URLs) and then after they've been warmed up they could be added (using our current code) - this way we effectively add them and preserve configs so that when we warm them up they all connect to the right servers.
| if isinstance(value, (Tool, Toolset)): | ||
| return (value,) | ||
|
|
||
| if isinstance(value, Iterable) and not isinstance(value, (str, bytes, dict)): |
There was a problem hiding this comment.
Iterables are needed for Toolset? Could you please explain?
There was a problem hiding this comment.
Ah that's just trick to search for Tools. Essentially if it's an iterable (like a list) BUT NOT a string, bytes, or dict then return it so we can iterate through it. See above how we search for (Tool, Toolset) without encoding specifically ChatGenerator ToolInvoker or any other component (Agent in pipeline has tools) also some others tomorrow perhaps. Rather than hardcoding this we look through fields of a component to find tools to warmup
There was a problem hiding this comment.
Perhaps an overkill, can simplify as well.
|
The issue is the following (somewhat elaborated in #9807):
The key is the following: Secret.resolve_value() happens when creating the client, but the actual connection attempt happens in init before the Secret is fully resolved in the deserialization context. The solution proposed: Have lazy init for some tools via warm_up. These are tools that need connection. Like MCP. All the Secret values get properly set in init but connection is made in Tool warm_up or first connect to server. First connect to server would work nicely if we didn't have two separate instances of MCPTool/MCPToolset in ChatGenerator and ToolInvoker (in pipeline scenario). Nobody calls "warm_up" on MCPTool/MCPToolset given to deserialized ChatGenerator and we end up with dysfunctional Pipeline. So we need "warm_up" on tools. Truth to be told, there might be another solution that also might be possible but I'm not sure 100%. Revamp MCPToolset/MCPTool to pass Secret directly to its init and call resolve on it prior to connection init. But I'm not sure this is possible (in terms of getting correct values from Secret passed). Do you know? That's why I went with the warm_up solution because it seemed less kludgey to me. I prepared the most basic test to demonstrate the above. LMK how we should proceed @anakin87 @sjrl |
|
Is it possible to add a simple test to show what was failing before this PR? I think that this might help clarifying the issue and looking for the best solution. |
|
Maybe as a general comment I would have thought we should add a And I agree with @anakin87 seeing a test/example of showing the behavior being fixed would help a lot. |
That was my instinct as well, clean, make sense, has nice hierarchy of calls but I'm afraid users will forget and we'll get tons of confused devs. Plus we'll have to add warm_up to every single chat generator with the same code. We can warm_up tools regardless (for Pipelines and for Agents) and if they are idempotent - we are ok. wdyt? |
|
Closing - superseded by #9856 |
4 participants
DRAFT only - for feedback