-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add Tools warm_up #9856
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
Add Tools warm_up #9856
Conversation
Pull Request Test Coverage Report for Build 18647136217Warning: 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 |
50c39a2 to
5b4fdea
Compare
|
Note to @anakin87 @julian-risch - @sjrl and I talked about invoking tool warm up in all chat generators via their warm_up method. Implementation in this PR will make Agent work because now ToolInvoker warms up tools. But to have pipelines working with tools warmup (ChatGenerator + ToolInvoker pipeline without Agent) we need to add warm_up to all ChatGenerators. However, that is scope for another PR. |
|
@sjrl @julian-risch @anakin87 can I 🙏 have some feedback on this PR so we can push it forward! |
julian-risch
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.
Left some comments. My my concern is the renaming of serde_utils.py to utils.py, which is a breaking change. Other than that looks quite good to me already.
|
@vblagoje I believe we should also update the |
| class _ToolsetWrapper(Toolset): | ||
| """ | ||
| A wrapper that holds multiple toolsets and provides a unified interface. | ||
| This is used internally when combining different types of toolsets to preserve | ||
| their individual configurations while still being usable with ToolInvoker. | ||
| """ |
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.
@vblagoje is it possible that this wrapper could also solve this issue deepset-ai/haystack-core-integrations#2369 if we add to_dict and from_dict methods?
|
I've tested this version of haystack with mcp and warmup and it works ok. However, as we discussed above, Agent warm_up needs to be called to warm up tools. Is that ok for no code envs? cc @julian-risch @sjrl |
|
@vblagoje how do you mean by this comment?
I think this is fine. Not entirely sure how this applies for no code envs |
|
Looking good @vblagoje I have a one more comment in addition to the few above Could we create a |
Co-authored-by: Sebastian Husch Lee <10526848+sjrl@users.noreply.github.com>
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.
@vblagoje looks good to me!
Sebastian re-reviewed and approved. My earlier comments have been addressed.
|
@sjrl Please have another look for changes since merge. I left _ToolsetWrapper because it doesn't affect main APIs but we can re-eval if we want to support + operations between Tool and Toolset |
|
@vblagoje looks good! |
Why:
Enables tools and toolsets to perform initialization during Agent/ToolInvoker warmup. This supports tools that need to establish remote connections, load models, initialize connection pools, or perform other setup operations before use.
What:
warm_up()method toToolandToolsetas extension points for initializationwarm_up_tools()utility function to warm up lists of tools or ToolsetsToolInvokerto warm up tools during initializationToolset.__add__()to avoid redundant validationHow can it be used:
How did you test it:
test/tools/test_toolset_warmup_and_wrapper.py_ToolsetWrapperto each wrapped toolsetNotes for the reviewer:
_ToolsetWrapper.warm_up()delegates to each wrapped toolset - critical for preserving individual initialization behavior when combining toolsetswarm_up()defaults to no-op in base classes