Repository navigation
Conversation
…ing them DeepAgentsPlugin.configure_worker kept the first activity of each name in the Worker's whole activity list. With two plugin instances on one side, the Worker silently dropped the second instance's activities, so every model call used the first instance's model_provider, with its endpoint and key. A caller activity whose name repeated another activity's name was dropped or replaced the same way. The plugin now skips its own activities only in the case its docs describe: one instance on the Client and one on the Worker. The Worker keeps the Client instance's activities, as before. Every other repeated name reaches the Worker's own check. Breaking change: a Worker with two DeepAgentsPlugin instances on the same side, or with a caller activity whose name repeats another activity's name (including deepagents.invoke_model, deepagents.invoke_model_streaming, deepagents.invoke_tool and deepagents.backend_op), now fails at construction with "ValueError: More than one activity named ...". Before, it started and used the first registration. Remove or rename the extra registration. Workflow code and histories do not change.
Review of the first version found setups that start on main and broke on it. With one DeepAgentsPlugin on the Client and one on the Worker, the Worker instance now skips only an activity that is already registered: the same callable, a callable that wraps it (__wrapped__), or, for a second instance, the Client instance's copy of the same built-in activity. It no longer restores the whole earlier list, so other activities given to the Worker instance stay, and a Client plugin that wraps the activities keeps working. The check keeps no state between calls, so a Client plugin that copies the config and Workers built from several threads behave the same. The built-in activities are kept apart from the public activities setting. Breaking change, unchanged from the previous commit: two DeepAgentsPlugin instances on one side, or a caller activity whose name repeats another activity's name with a different function, now fail at Worker construction with "ValueError: More than one activity named ...".
Review revisions after 253dcce: the Worker instance skips a repeat only while a registration with the same activity name, or the dynamic slot, stays in the list, and only when that registration is the same callable, wraps it, or is the Client instance's copy of the same built-in activity. Registrations are now compared by identity (two bound methods of one function on one object count as one callable), never with __eq__. Two distinct callables that compare equal, such as callable dataclass instances, are different registrations, so they reach the Worker's duplicate-name or duplicate-dynamic check instead of one being dropped. Breaking change, unchanged: two DeepAgentsPlugin instances on one side, or a repeated activity name or second dynamic activity with a different callable, fail at Worker construction.
6abb9e0 to
01b5e42
Compare
DABH
left a comment
There was a problem hiding this comment.
First review, at head 01b5e42 (three commits over c4fb1ae). On a worktree at the head, make lint is clean and make test passes 118 of 118. Each finding below was checked with temporary tests against a local dev server, since removed: a real Worker with one DeepAgentsPlugin on the Client and a differently configured one on the Worker; the error text for two instances on one side; a Replayer with two instances; and a one-shot activities iterable, compared with main's SimplePlugin path. I also prototyped the fix proposed for the first finding on the head. Checked and fine: the same instance on both sides (list, tuple and callable activities). Two instances on one side are refused, and so is a caller Activity named deepagents.invoke_tool. The before/after comparison covers both forms of SimplePlugin's activities parameter (a list to append, or a callable). Replayer construction is unaffected: configure_replayer never touches Activities, so two instances on a Replayer construct, as on main. I left nothing material unverified.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Where the problems cluster: the refusals are correct, and the 25 new cases cover them well. The open question is the exemption. It accepts any two instances, one on each side, not only the same plugin twice. A Worker-side instance's model_provider and backends are therefore still replaced, without an error, by the Client instance's. That is #174's impact in a different placement, and the rule is looser than the one #161, #162 and #202 apply on the TypeScript side. The other two items are the error message for two instances and a regression for one-shot iterables.
Commenting rather than requesting changes: the remaining P2 is behaviour main already has, not a regression from this PR, but I think it should be settled before this closes #174.
PR body: it says 'Draft while these questions are open', but the PR is not marked draft. 'Closes #174' holds for the issue's two reproductions (two instances on one Client, two caller fetch Activities). The issue's Impact paragraph (model calls reaching another provider's endpoint with its key) still applies with one instance on each side; see the first inline comment. On open question 1 (a caller's extra copy of the shared instance's own built-in Activity): it registers the same function under the same name, so accepting it cannot change which code runs. I would leave it as is.
| on_worker = [ | ||
| p for p in config.get("plugins") or [] if isinstance(p, DeepAgentsPlugin) | ||
| ] | ||
| if len(on_client) == 1 and len(on_worker) == 1 and on_worker[0] is self: |
There was a problem hiding this comment.
P2: the exemption accepts any two instances, one on each side, so a Worker-side DeepAgentsPlugin configured differently from the Client's has its model_provider and backends replaced without an error; that is #174's impact in another placement. Probe on the dev server:
- Client:
plugins=[DeepAgentsPlugin(model_provider=mock_model_provider(['answer from CLIENT provider']), model_activity_options={'start_to_close_timeout': timedelta(seconds=11)})]. - Worker:
plugins=[DeepAgentsPlugin(model_provider=mock_model_provider(['answer from WORKER provider']), model_activity_options={'start_to_close_timeout': timedelta(seconds=22)}, backends={'fs': ...})].
The Worker starts. Every registered plugin Activity is bound to the Client instance's DeepAgentActivities, and the run answers answer from CLIENT provider. A TemporalBackend('fs') call would fail with the non-retryable Backend factory 'fs' is not configured on this worker. Configure DeepAgentsPlugin(backends=...) (_activity.py:398-406), even though the Worker plugin does configure it.
Meanwhile _serde.get_settings().model_activity_options holds the Worker instance's 22 s. model_activity_options, streaming_topic and tool_activity_options are process globals set by whichever instance was constructed last (_serde.py:69-76, _tools.py:77-84), so the Worker runs a mix of the two configurations. The only signal is the SDK's generic warning ('The same plugin type ... may run twice'), which describes neither effect.
The setup is plausible. The README's 'Composing with other plugins' connect() helper puts a default DeepAgentsPlugin() on the Client, and the class docstring says API keys live in the Worker-side model_provider. #161, #162 and #202 accept a repeated plugin only when both instances were built from the same provider and options.
Fix, prototyped on this head:
- Record the constructor's
model_provider,backendsandstreaming_batch_interval. - Return the Client instance here only when it
is selfor matches on those:model_providerby identity,backendsas a dict of factories, the interval by equality. Optionally compare the dispatch settings too, since they are globals.
With that change, two default instances and every setup in test_one_instance_on_each_side_keeps_the_client_registration still start (all 25 registration tests pass). The probe above then fails with ValueError: More than one activity named deepagents.invoke_model.
Also add a case with a different model_provider to test_repeated_activity_names_are_refused. In the README, say in user terms which setups start (the same instance on both sides, or two instances built from the same provider and backends) and that any other second instance fails. If you keep the current rule, the README needs to say that the Worker instance's model_provider and backends are ignored in this case.
| ] | ||
| if len(on_client) == 1 and len(on_worker) == 1 and on_worker[0] is self: | ||
| return on_client[0] | ||
| return None |
There was a problem hiding this comment.
P3: two instances on one side fail with the SDK's ValueError('More than one activity named deepagents.invoke_model'), which names neither DeepAgentsPlugin nor the fix. Probe: [DeepAgentsPlugin(), DeepAgentsPlugin()] on the Client, or on the Worker, gives exactly that text. A user who added a second instance to reach a second provider (#174's setup) sees only an Activity name they never wrote. The lists built at 268-271 already tell this method how many instances there are, so it can raise first. For example: ValueError('DeepAgentsPlugin is registered 2 times for this Worker (2 on the Client, 0 on the Worker). Register one instance: its model_provider receives each model name, so one provider can route to several, and backends= takes every factory.'). Caller-Activity collisions can stay with the SDK's check. The TypeScript siblings (#161, #162, #166, #202) raise a plugin-specific message that says what to change. Non-blocking.
| def configure_worker(self, config: Any) -> Any: | ||
| """Deduplicate activity registrations after the base configuration.""" | ||
| """Skip activities the Client's instance already registered.""" | ||
| before = list(config.get("activities") or []) |
There was a problem hiding this comment.
P3: this line consumes a one-shot iterable before SimplePlugin.configure_worker reads the same object, so caller Activities passed as a generator are dropped without an error whenever this plugin configures the Worker first. Probe: Worker(client_with_plugin, workflows=[...], activities=(a for a in [fetch])) registers only the four deepagents.* names at this head. Main's path (SimplePlugin.configure_worker, then the name dedupe) also registers fetch, and the SDK on its own iterates activities only once. This happens with or without a second instance. The parameter is typed Sequence[Callable], so this input is outside the contract, but the failure has #174's shape: an Activity goes missing with no error until a Workflow schedules it. Fix: after taking the copy, write a separate list back when the key is set (if config.get('activities') is not None: config['activities'] = list(before)). It must be a separate list, so that a callable activities setting that appends in place cannot change before. Non-blocking.
What was changed
DeepAgentsPlugin.configure_workerno longer drops every repeated Activity name from the Worker's Activity list. Repeated names now fail at Worker start, as they do without the plugin, except in the one case the module docstring describes: one instance on the Client and one on the Worker.DeepAgentsPluginon the Client and one on the Worker, the same object or two), the Worker instance skips an Activity it added when an entry from before its call is still registered after it, under the same Activity name (for a dynamic Activity, the Worker's one dynamic slot), and is:__wrapped__);__eq__, so two distinct callables that compare equal reach the Worker's checks.alias.deepagents.invoke_tool) never removes the Worker instance's own Activities.activitiessetting, which callers may change._dedupe_activitiesis removed.ValueError: More than one activity named ..., and a second, different dynamic Activity reaches itsTypeError: More than one dynamic activity.Breaking change: Workers that start today with a repeated name now fail at construction: two
DeepAgentsPlugininstances on one side, two caller Activities with one name, or a caller Activity named like a plugin Activity (deepagents.invoke_model,deepagents.invoke_model_streaming,deepagents.invoke_tool,deepagents.backend_op). Remove or rename the extra registration. The title starts withBreaking:because release notes come from commit subjects.Rollback: no version gate. The change runs at Worker construction, not in Workflow code. Activity names, commands, payloads and History don't change, so recorded histories replay with this change and without it. A rolled-back Worker starts again with the silent drop.
The branch keeps the first version and two review revisions as separate commits; review the combined diff against
main.Why?
See #174. With two plugin instances on one Client, the Worker silently drops the second instance's Activities, so every model call goes to the first instance's
model_provider, with its endpoint and key. A caller Activity can be dropped or replaced the same way. Nothing warns.Checklist
Closes [Bug] deepagents: Worker silently drops Activities that share a name #174
How was this tested:
tests/test_activity_registration.py, 25 cases, building Workers from a dev-server Client:test_repeated_activity_names_are_refused, 7 setups: two instances on the Client, two on the Worker, one on the Client and two on the Worker, two caller Activities namedfetch, a caller Activity nameddeepagents.invoke_tool, an Activity namedfetchappended to the Worker instance'sactivitiesbeside a callerfetch, and two compare-equal callables with one name. Each must raiseValueError: More than one activity named.test_two_dynamic_activities_are_refused, 4 setups: two different dynamic Activities, as two functions and as two compare-equal callables, with one object and with two. Each must raiseTypeError: More than one dynamic activity.test_one_instance_on_each_side_keeps_the_client_registration, 14 setups: the same object or two; a Client wrapper plugin; an extra Activity on the Worker instance (list and callable setting); a dynamic Activity on one object on both sides (list and callable); a Client plugin that copies the config; a caller copy of the object's owninvoke_model; a Client plugin that replaces the plugin Activities withalias.deepagents.*wrappers; a callable on the Worker instance that replaces the Client instance's built-ins with its own. The Worker must hold each plugin name andfetchonce, with the expected callables.main, the 7 named refusals fail withDID NOT RAISE ValueErrorand the other 18 pass. With the patch, all 25 pass. The keep setups guard behaviormainhas and earlier revisions of this branch broke.pytest: 118 passed.ruff check --select I,ruff format --check,mypy,basedpyright,pyrightandpydocstylepass. These ran in Docker with uv 0.13.1, outside the plugin'srequired-version, so not throughmake. CI coversmake lintandmake test.defect reproducedon 0.0.3 andmain. With the patch, it stops at the two-plugins Worker withValueError: More than one activity named deepagents.invoke_model.main, with the Client instance's provider answering except where the Worker instance replaces it. With a dynamic Activity, a Workflow calling an unregistered name gets its answer. Two threads that build Workers from one plugin at the same time both start.Any docs updates needed?
The README paragraph and module docstring, included here.
Not covered:
__wrapped__. With one object on both sides, a Client plugin that replaces the Activities with callables that don't set it makes the Worker fail at start with the SDK'sValueError; it does not start with a wrong registration. Not run.temporalio1.34.x still bundles the old code astemporalio.contrib.deepagents. No backport here.Open questions:
maindoes, because both entries are one function.PluginAPI givesconfigure_workerno way to know which call of a plugin on both sides is which; this PR avoids needing it. Would an SDK hook for this be worth adding, so plugins can be stricter?temporalio1.34.x get a backport?Draft while these questions are open.