-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add DynamicToolset support in Temporal
#3682
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 DynamicToolset support in Temporal
#3682
Conversation
pydantic_ai_slim/pydantic_ai/durable_exec/temporal/_dynamic_toolset.py
Outdated
Show resolved
Hide resolved
tests/test_temporal.py
Outdated
| assert output == 'Dynamic result received' | ||
|
|
||
|
|
||
| # Test with explicit id parameter |
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'd rather test the new id arg where we test the @agent.toolset decorator, since this part is not Temporal-specific
tests/test_temporal.py
Outdated
| pytest.fail('TemporalDynamicToolset not found in toolsets') | ||
|
|
||
|
|
||
| async def test_dynamic_toolset_get_tools_outside_workflow(): |
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 should test this by running the same agent we created earlier outside of a workflow, and checking that we still get the correct result
tests/test_temporal.py
Outdated
| pytest.fail('TemporalDynamicToolset not found') | ||
|
|
||
|
|
||
| async def test_dynamic_toolset_call_tool_outside_workflow(): |
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.
Not needed
tests/test_temporal.py
Outdated
| pytest.fail('TemporalDynamicToolset not found') | ||
|
|
||
|
|
||
| async def test_dynamic_toolset_tool_not_found_in_activity(): |
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 fine with pragma: no cover for this UserError like we have here:
pydantic-ai/pydantic_ai_slim/pydantic_ai/durable_exec/temporal/_function_toolset.py
Lines 41 to 47 in f42e523
| try: | |
| tool = (await toolset.get_tools(ctx))[name] | |
| except KeyError as e: # pragma: no cover | |
| raise UserError( | |
| f'Tool {name!r} not found in toolset {self.id!r}. ' | |
| 'Removing or renaming tools during an agent run is not supported with Temporal.' | |
| ) from e |
Core changes: - DynamicToolset: convert to plain class with custom __init__, add copy() - TemporalDynamicToolset: use _call_tool_in_activity shared method - TemporalFunctionToolset: use _call_tool_in_activity shared method - temporalize_toolset: move DynamicToolset import to top Test changes: - Add test_fastmcp_dynamic_toolset_in_workflow for MCP lifecycle in DynamicToolset - Add test_dynamic_toolset_id and test_agent_toolset_decorator_id - Update test_visit_and_replace for DynamicToolset plain class 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
docs/durable_execution/temporal.md
Outdated
|
|
||
| When `TemporalAgent` dynamically creates activities for the wrapped agent's model requests and toolsets (specifically those that implement their own tool listing and calling, i.e. [`FunctionToolset`][pydantic_ai.toolsets.FunctionToolset] and [`MCPServer`][pydantic_ai.mcp.MCPServer]), their names are derived from the agent's [`name`][pydantic_ai.agent.AbstractAgent.name] and the toolsets' [`id`s][pydantic_ai.toolsets.AbstractToolset.id]. These fields are normally optional, but are required to be set when using Temporal. They should not be changed once the durable agent has been deployed to production as this would break active workflows. | ||
|
|
||
| For dynamic toolsets created with the [`@agent.toolset`][pydantic_ai.Agent.toolset] decorator, the `id` parameter can be set explicitly or it will default to the function name: |
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.
Please link to the dynamic toolset doc
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 it's actually better to NOT default to the function name and require id to be set explicitly when Temporal is used, as the ID is used in activity names that need to be stable (i.e. can't change from one release to the next). So it's better for it to be separate field with a clear docstring that the user won't accidentally rename when they're doing a code refactor
docs/durable_execution/temporal.md
Outdated
| class MyDeps: | ||
| ... | ||
|
|
||
| @agent.toolset(id='my_dynamic_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.
I don't think we need the example
docs/durable_execution/temporal.md
Outdated
| ... | ||
| ``` | ||
|
|
||
| Note that with Temporal, `per_run_step=False` is not respected, as the toolset always needs to be created on-the-fly in the activity. |
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 can be on the same single line about dynamic toolsets as the stuff above
docs/toolsets.md
Outdated
|
|
||
| By default, the function will be called again ahead of each agent run step. If you are using the decorator, you can optionally provide a `per_run_step=False` argument to indicate that the toolset only needs to be built once for the entire run. | ||
|
|
||
| When using [Temporal durable execution](./durable_execution/temporal.md), the decorator also accepts an `id` parameter to uniquely identify the toolset. If not provided, the function name is used as the ID. |
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 makes it seem like it only accepts the parameter when Temporal is used, while of course it always accepts the parameter :)
If we don't use the function name by default as I suggested above, we may not need this line here at all, as this won't be relevant unless they use Temporal, and if they do they'll get an error or read it in the docs
| per_run_step: bool | ||
| _id: str | None | ||
| _toolset: AbstractToolset[AgentDepsT] | None | ||
| _run_step: int | None |
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 wonder if we could solve this "reuse a dynamic toolset across multiple agent runs" issue not by copying it for each run (which is tricky as we see here), but by storing the _toolset and _run_step on a dict keyed by ctx.run_id.
That way, we can keep using the same toolset instance, but state from different runs wouldn't interfere. I think that's worth trying in this PR, as an alternative to the new copy method
tests/test_temporal.py
Outdated
| # --- DynamicToolset / @agent.toolset tests --- | ||
|
|
||
|
|
||
| def get_dynamic_weather(location: str) -> 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.
We don't need this at the top level; we can register it to the FunctionToolset inline inside my_dynamic_toolset using @toolset.tool
| # --- MCP-based DynamicToolset test --- | ||
| # Tests that @agent.toolset with an MCP toolset works with Temporal workflows. | ||
| # Uses FastMCPToolset (HTTP-based) rather than MCPServerStdio (subprocess-based) because | ||
| # MCPServerStdio has issues when created dynamically inside Temporal activities. |
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 link to the issue
| """Dynamic toolset that returns an MCP toolset. | ||
| This tests MCP lifecycle management (context manager enter/exit) within DynamicToolset + Temporal. | ||
| Uses per_run_step=False so the toolset persists across run steps within an activity. |
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.
Does this actually work?
tests/test_temporal.py
Outdated
|
|
||
| @dynamic_toolset_agent.toolset | ||
| def my_dynamic_toolset(ctx: RunContext[None]) -> FunctionToolset[None]: | ||
| return FunctionToolset(tools=[get_dynamic_weather], id='dynamic_weather') |
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 use a value from deps inside the function (when we inline it), since the point of DynamicToolset is to build it dynamically. We can do that in the MCP server example as well, if that seems more appropriate
| ] | ||
| ) | ||
| visited_toolset = toolset.visit_and_replace(lambda toolset: WrapperToolset(toolset)) | ||
| assert visited_toolset == CombinedToolset( |
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.
Is this change necessary? The new code is a lot harder to read
Closes #3390
@agent.toolsetdecorator