Allow dynamic TemporalWrapperToolset in Temporal workflows #3775
Conversation
qyinm
force-pushed
the
add-allow-cross-agent-tool-registration
branch
from
c733278 to
9a6fe3b
Compare
|
|
||
| from ...exceptions import UserError | ||
| from ._agent import TemporalAgent | ||
| from ._function_toolset import TemporalFunctionToolset |
There was a problem hiding this comment.
This feature should work for any toolset, so we need to export the other wrappers (MCP etc) as well
| in_workflow = workflow.in_workflow() | ||
|
|
||
| if toolsets: | ||
| if in_workflow and any(not isinstance(t, TemporalWrapperToolset) for t in toolsets): |
There was a problem hiding this comment.
CombinedToolsets and others that don't require temporal wrappers are fine, so we should look at the ones covered bytemporalize_toolset- We should look down the hierarchy of toolsets using
toolset.visit(), if we find a temporal wrapper, we know we don't need to look at its children, but otherwise we error if we encountered an unwrapped toolset
| 'Model cannot be set at agent run time inside a Temporal workflow, it must be set at agent creation time.' | ||
| ) | ||
| if toolsets is not None: | ||
| if toolsets is not None and any(not isinstance(t, TemporalWrapperToolset) for t in toolsets): |
There was a problem hiding this comment.
Same as above; the check is not precise enough
tests/test_temporal_dynamic.py
Outdated
There was a problem hiding this comment.
This should be in the existing test file
tests/test_temporal_dynamic.py
Outdated
| activity_name_prefix='shared_tools', | ||
| activity_config=ActivityConfig(start_to_close_timeout=timedelta(minutes=1)), | ||
| tool_activity_config={}, | ||
| deps_type=type(None), |
There was a problem hiding this comment.
These 4 args should not be required
tests/test_temporal_dynamic.py
Outdated
| # We use a known name "test_agent" so the dynamic agent can share it. | ||
| base_model = TestModel() | ||
| base_agent = Agent(base_model, name='test_agent') | ||
| base_temporal_agent = TemporalAgent(base_agent) |
There was a problem hiding this comment.
I want to also support the same kind of pre-registering we're adding in #3537 for models, so you'd pass toolsets={'funcs': toolset, 'other': ...} and then you can pass run(toolsets=['funcs']) or run(toolsets=[toolset]) later, in addition to the instance you've pre-wrapped yourself.
There was a problem hiding this comment.
In this scenario, we should make sure that TemporalAgent.toolset_activity_config and TemporalAgent.tools_activity_config work for toolsets that are not manually wrapped like that, as otherwise the user wouldn't have a way of specifying activity config
qyinm
force-pushed
the
add-allow-cross-agent-tool-registration
branch
from
9a6fe3b to
ad5ba4f
Compare
…meters optional - Fix undefined '_model' attribute by using '_temporal_model' - Add 'model' parameter to '_temporal_overrides' with 'using_model' support - Skip model override outside workflow to allow direct model parameter usage - Add 'force' parameter for toolsets property compatibility - Make TemporalDynamicToolset __init__ parameters optional to match temporalize_toolset signature
|
@DouweM I've addressed all review feedback:
Ready for re-review. |
qyinm
force-pushed
the
add-allow-cross-agent-tool-registration
branch
from
feab98e to
2064ceb
Compare
2 participants
Closes #3768.
Summary
This PR enables the use of dynamic toolsets in Temporal workflows by allowing
toolsetsto be passed toagent.run()at runtime, provided they are instances ofTemporalWrapperToolset.Changes
TemporalAgent: Updated_temporal_overridesto accept runtime toolsets if they are properly wrapped.tests/test_temporal_dynamic.pyto verify that aTemporalFunctionToolsetcan be successfully passed and executed dynamically within a workflow.Rationale
As discussed in #3768, this change allows for: