-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add Runtime builtin_tools
Support
#3009
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?
Add Runtime builtin_tools
Support
#3009
Conversation
…t and implement merge_builtin_tools function
builtin_tools
Support #2821builtin_tools
Support (#2821)
builtin_tools
Support (#2821)builtin_tools
Support
Heyy @DouweM |
…dle builtin_tools directly + update unit tests
Heyy @DouweM Thanks for the feedback ! |
{ | ||
**({type(tool): tool for tool in self._builtin_tools or []}), | ||
**({type(tool): tool for tool in builtin_tools or []}), | ||
}.values() |
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.
@safina57 I suggest moving this to a variable further up, with a comment explaining what it does. Like:
if builtin_tools:
# Deduplicate builtin tools passed to the agent and the run based on type
builtin_tools = list(
{
**({type(tool): tool for tool in self._builtin_tools or []}),
**({type(tool): tool for tool in builtin_tools}),
}.values()
)
else:
builtin_tools = self._builtin_tools
I like it a bit better than the 10-line method because this is still sufficiently clear to someone familiar with Python, and much more concise. The separate method makes it seem like something complicated is going on, while it's really just deduplication.
We could also do it like this if you think it's more clear:
if builtin_tools:
if self.builtin_tools:
builtin_tool_types = [type(tool) for tool in builtin_tools]
builtin_tools.extend([tool for tool in self._builtin_tools if type(tool) not in builtin_tool_types])
else:
builtin_tools = self._builtin_tools
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.
okyy got it
thanks ^^
…tools merge logic placement
Add Runtime Builtin Tools Support with Priority-Based Merging
Overview
This PR Fixes issue #2821 adds support for providing builtin tools at runtime in
Agent.run()
,Agent.run_sync()
,Agent.run_stream()
andAgent.iter()
methods, with runtime tools taking priority over agent initialization tools. The implementation follows the exact same pattern as the existing model settings priority system.Motivation
Previously, builtin tools could only be configured when creating an Agent instance. This limitation made it difficult to:
Implementation Approach
Priority System Design
The implementation follows the established pattern used for model settings
merge_model_settings()
behavior:Core Changes
1. Settings Module Enhancement
merge_builtin_tools()
function that implements type-based deduplicationWebSearchTool
)2. Agent Interface Updates
builtin_tools: Sequence[AbstractBuiltinTool] | None = None
parameter to all agent method signaturesNone
3. Core Agent Implementation
iter
method (the base method all others delegate to)GraphAgentDeps
4. Wrapper Agent Support
WrapperAgent
to pass through thebuiltin_tools
parameter