-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add builtin_tools
to Agent
#2102
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?
Conversation
Co-authored-by: Marcelo Trylesinski <[email protected]>
…dantic#1752) Co-authored-by: Marcelo Trylesinski <[email protected]>
- Added builtin_tools field to ModelRequestParameters - Merged new output_mode and output_object fields from main - Updated test snapshots to include all fields - Resolved import conflicts to include both builtin tools and profiles
Co-authored-by: David Montague <[email protected]>
c4aa1ef
to
4376fc2
Compare
@Kludex unless we want to break these out into individual classes this feels like its in a pretty good spot for a first pass and to prevent it already feels like a giant PR |
@@ -307,6 +311,8 @@ def __init__( | |||
output_retries: The maximum number of retries to allow for output validation, defaults to `retries`. | |||
tools: Tools to register with the agent, you can also register tools via the decorators | |||
[`@agent.tool`][pydantic_ai.Agent.tool] and [`@agent.tool_plain`][pydantic_ai.Agent.tool_plain]. | |||
builtin_tools: The builtin tools that the agent will use. This depends on the model, as some models may not | |||
support certain tools. On models that don't support certain tools, the tool will be ignored. |
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.
I feel like silently ignoring built-in tools is unexpected, because the model is not going to behave as intended if it can't search the web or execute code. I'd prefer to raise an error from the model class if it sees an unsupported built-in tool, similarly to how we do for unsupported file/binary content types.
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.
I'm not sure it's the same case. I expect builtin tools to fail far more than the different content inputs.
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.
@Kludex I agree, but wouldn't you want the user to realize that they asked for something the model can't actually do?
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.
@mattbrandman Please make it a hard error if an unsupported built-in tool was provided -- I talked about it with Marcelo and he's fine with it.
That means every model class will need to check model_request_parameters.builtin_tools
and error if it doesn't implement any at all
@@ -734,6 +754,7 @@ async def _responses_create( | |||
) -> responses.Response | AsyncStream[responses.ResponseStreamEvent]: | |||
tools = self._get_tools(model_request_parameters) | |||
tools = list(model_settings.get('openai_builtin_tools', [])) + 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.
Should we deprecate this setting and push people to use the new builtin_tools
?
This setting currently supports FileSearchToolParam
and ComputerToolParam
as well, should we implement those as AbstractBuiltinTool
s?
Should we have a way to build an AbstractBuiltinTool
with arbitrary built-in tool JSON supported by a given model, so users can start using them without having to wait for us to add support to the model class?
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.
Should we deprecate this setting and push people to use the new builtin_tools?
Yes.
This setting currently supports FileSearchToolParam and ComputerToolParam as well, should we implement those as AbstractBuiltinTools?
I don't think they work properly right now, but the idea is to support them yes.
Should we have a way to build an AbstractBuiltinTool with arbitrary built-in tool JSON supported by a given model, so users can start using them without having to wait for us to add support to the model class?
Yes, but that needs to be model specific, so I left it out of this PR. I think we should have that possibility tho.
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.
Let's leave openai_builtin_tools
for now, as I don't want to further delay this PR by having to support FileSearch and ComputerTool. We'll add ComputerUse later as Anthropic also supports it: https://docs.anthropic.com/en/docs/agents-and-tools/tool-use/computer-use-tool, but FileSearch is currently OpenAI-specific so it seems too early to define a generic interface.
Co-authored-by: Douwe Maan <[email protected]>
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.
@mattbrandman Thanks for your patience, he's another review! 22 comments sounds like like a lot but it should all be pretty straightforward...
|
||
|
||
@dataclass(repr=False) | ||
class ServerToolCallPart(BaseToolCallPart): |
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.
This (plus docstring and part_kind) also still needs to be renamed to Builtin!
@@ -1145,6 +1181,29 @@ def tool_call_id(self) -> str: | |||
__repr__ = _utils.dataclasses_no_defaults_repr | |||
|
|||
|
|||
@dataclass(repr=False) | |||
class ServerToolCallEvent: |
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.
This and the next event should also be renamed
# This is currently never returned from huggingface | ||
pass | ||
elif isinstance(item, BuiltinToolReturnPart): # pragma: no cover | ||
# BuiltinToolReturnPart represents a tool return from a remote server |
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.
This is ambiguous as "remote server" could mean e.g. MCP. Let's make it clear this is specifically about built-in tools and drop "server" everywhere
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.
I think we can just drop this line FYI
# This is currently never returned from cohere | ||
pass | ||
elif isinstance(item, BuiltinToolReturnPart): # pragma: no cover | ||
# BuiltinToolReturnPart represents a tool return from a remote server |
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.
Like I mentioned elsewhere, let's drop this line as the "remote server" bit is potentially confusing and the class name is clear enough
type='approximate', **tool.user_location | ||
) | ||
tools.append(web_search_tool) | ||
return 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.
We can support code execution here as well: https://platform.openai.com/docs/guides/tools-code-interpreter
@@ -893,6 +928,9 @@ async def _map_messages( | |||
openai_messages.append(responses.EasyInputMessageParam(role='assistant', content=item.content)) | |||
elif isinstance(item, ToolCallPart): | |||
openai_messages.append(self._map_tool_call(item)) | |||
# OpenAI doesn't return server tool calls | |||
elif isinstance(item, (ServerToolCallPart, BuiltinToolReturnPart)): | |||
continue |
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.
continue | |
pass |
""" | ||
|
||
city: str | ||
country: str |
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.
On OpenAI, these needs to be a 2-letter country code: https://platform.openai.com/docs/guides/tools-web-search?api-mode=responses#user-location
@@ -298,6 +298,9 @@ async def _completions_create( | |||
model_request_parameters: ModelRequestParameters, | |||
) -> chat.ChatCompletion | AsyncStream[ChatCompletionChunk]: | |||
tools = self._get_tools(model_request_parameters) | |||
web_search_options = self._get_web_search_options(model_request_parameters) | |||
|
|||
# standalone function to make it easier to override |
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.
# standalone function to make it easier to override |
@@ -734,6 +754,7 @@ async def _responses_create( | |||
) -> responses.Response | AsyncStream[responses.ResponseStreamEvent]: | |||
tools = self._get_tools(model_request_parameters) | |||
tools = list(model_settings.get('openai_builtin_tools', [])) + 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.
Let's leave openai_builtin_tools
for now, as I don't want to further delay this PR by having to support FileSearch and ComputerTool. We'll add ComputerUse later as Anthropic also supports it: https://docs.anthropic.com/en/docs/agents-and-tools/tool-use/computer-use-tool, but FileSearch is currently OpenAI-specific so it seems too early to define a generic interface.
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.
@mattbrandman Thanks for your patience, he's another review! 22 comments sounds like like a lot but it should all be pretty straightforward...
Fixes test and merge conflicts for #1722
Closes #840