- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.5k
 
fix: warm up individual tools inside Toolsets in warm_up_tools() #10002
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
base: main
Are you sure you want to change the base?
fix: warm up individual tools inside Toolsets in warm_up_tools() #10002
Conversation
Related Issues: * Follows up on PR deepset-ai#9942 (feat: Add warm_up() method to ChatGenerators) * Addresses bug discovered during implementation of PR deepset-ai#9942 for issue deepset-ai#9907 Proposed Changes: The warm_up_tools() utility function was only calling warm_up() on Toolset objects themselves, but not on the individual Tool instances contained within them. This meant tools inside a Toolset were not properly initialized before use. This PR modifies warm_up_tools() to iterate through Toolsets and call warm_up() on each individual tool, in addition to calling warm_up() on the Toolset itself. Changes: - Modified warm_up_tools() in haystack/tools/utils.py to iterate through Toolsets when encountered (both as single argument and within lists) - Added iteration to call warm_up() on each individual Tool inside Toolsets - Added comprehensive test class TestWarmUpTools with 7 test cases How did you test it: - Added 7 comprehensive unit tests in test/tools/test_tools_utils.py: * test_warm_up_tools_with_none - handles None input * test_warm_up_tools_with_single_tool - single tool in list * test_warm_up_tools_with_single_toolset - KEY TEST: verifies both Toolset and individual tools are warmed * test_warm_up_tools_with_list_containing_toolset - toolset within list * test_warm_up_tools_with_multiple_toolsets - multiple toolsets * test_warm_up_tools_with_mixed_tools_and_toolsets - mixed scenarios * test_warm_up_tools_idempotency - safe to call multiple times Notes for the reviewer: I discovered this bug while implementing PR deepset-ai#9942 (for issue deepset-ai#9907). When a Toolset object is passed to a component's tools parameter, the warm_up_tools() function only calls Toolset.warm_up(), which is a no-op. It doesn't iterate through the individual tools inside the Toolset to warm them up. acknowledged by @vblagoje and @sjrl This implementation: - Modified warm_up_tools() to iterate through Toolsets and call warm_up() on each individual tool - Added comprehensive tests for Toolset warming behavior - Verified both the Toolset and its contained tools are warmed up Checklist: I have read the contributors guidelines and the code of conduct I have updated the related issue with new insights and changes I added unit tests and updated the docstrings I've used one of the conventional commit types for my PR title: fix: I documented my code I ran pre-commit hooks and fixed any issue
| 
           @HamidOna13 is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it.  | 
    
          Pull Request Test Coverage Report for Build 18990344825Details
 
 💛 - Coveralls | 
    
| 
           @Amnah199 let me take this one  | 
    
| 
           @HamidOna let’s think this through together. I don’t think we should assume that every Toolset instance or subclass must always maintain an internal list of Tool objects. If you think about it a bit it's clear that’s a pretty restrictive design choice if we have Toolset warm_up already. For the default Toolset it makes sense having an explicit list is straightforward and works fine. But for more complex implementations that assumption can get in the way. Case in point, MCPToolset multiplexes a single connection for all its tools. It might not want to expose or even manage its internal tools individually just to satisfy some warm_up contract. So my proposal is: by default, let the base Toolset handle warming up its tools if it has them yet allow subclasses to override that behavior and decide how (or whether) to warm up internal tools. That way we get the best of both worlds - we’re flexible instead of enforcing a rigid interface and we handle warmup consistently. LMK wdyt! cc @sjrl  | 
    
          
 @vblagoje Hmmm I see what you mean now. I have taken a second look at the code I can see _ToolsetWrapper.warm_up() already does this the right way . it just delegates to each toolset's warm_up() without poking around inside them which my implementation basically broke that pattern. The real issue is that the base Toolset.warm_up() is just a pass statement right now. So the actual fix should be: 
 I totally agree with that flexibility! Let me rework the PR with this approach. Thanks for catching this!  | 
    
Related Issues:
warm_up()method to ChatGenerators for tool initialization #9907Proposed Changes:
The warm_up_tools() utility function was only calling warm_up() on Toolset objects themselves, but not on the individual Tool instances contained within them. This meant tools inside a Toolset were not properly initialized before use. This PR modifies warm_up_tools() to iterate through Toolsets and call warm_up() on each individual tool, in addition to calling warm_up() on the Toolset itself.
Changes:
Toolsets when encountered (both as single argument and within lists)
How did you test it:
Toolset and individual tools are warmed
Notes for the reviewer:
I discovered this bug while implementing PR #9942 (for issue #9907). When a Toolset object is passed to a component's tools parameter, the warm_up_tools() function only calls Toolset.warm_up(), which is a no-op. It doesn't iterate through the individual tools inside the Toolset to warm them up.
Acknowledged by @vblagoje and @sjrl
This implementation:
Checklist:
fix: