Repository navigation
Conversation
activity_as_tool tells a method from a staticmethod by reading the attribute that the function's __qualname__ names in its class. If the class's name or that attribute was rebound after the reference was taken, the attribute is another object, and the reference was classified by it: a staticmethod could lose its self, so the model's value reached the wrong parameter. Check that the attribute is the function, or a staticmethod of it, and raise ValueError otherwise, as for a class that can't be reached.
DABH
left a comment
There was a problem hiding this comment.
Review at head 53ba069 (two commits on 88745dc, current main). On a worktree at the head: make lint is clean and make test gives 180 passed, 5 skipped (Python 3.14.3, temporalio 1.34.0, google-adk 2.12.0); CI is green. Both findings were reproduced with probes against installed builds of 0.0.1, main, the head and a one-line fix; the first also end to end with real Workers on the pinned dev server under the sandboxed runner, with histories recorded on 0.0.1 and on main replayed across the builds. The body's claims hold: #362's reproduction exits 1 on main and 0 on the head; the rejection test fails on main with DID NOT RAISE ValueError and the metadata test passes there; four single-line mutants of the new check (no check, the first commit's is not fn, ignoring a staticmethod's __func__, unwrapping without stop) are each caught by the new tests. Checked and refuted: false refusals for functools.wraps-style decorators (tenacity's @retry, warnings.deprecated, pydantic's validate_call), for dataclass(slots=True) and attrs @define classes, which rebuild the class, and inside the Workflow sandbox (the suite's integration test builds the method, staticmethod, decorated and private-class forms in Workflow code); any change to a tool the head still builds (_is_method returns main's value whenever it does not raise). No other Python plugin has this classification, and Go and TypeScript take function values or registered names, so the languages stay consistent. Nothing material was left unverified.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Requesting changes for one item: the new check also refuses wrapt-decorated methods that nothing rebound, so 0.0.1 runs that build such a tool stop replaying on upgrade, a break the body does not disclose; the one-line fix in the inline comment removes it and keeps every refusal this PR adds.
Replay, at the head: for every tool the head still builds, nothing changes. The 60 replays of the #312 review's histories (function, bound-method, staticmethod, method, callable-instance, class, tool_context and module-placement forms, recorded on #312's base and head, sandboxed and unsandboxed, finished and held) give identical outcomes on main and on the head: 49 replay, and the same 11 fail on #312's own changes. That agrees with the body's 16-history check, and the disclosed break holds as described (a staticmethod taking self, referenced after its class was rebound, now raises when the tool is built). The undisclosed break is the P2: histories recorded on 0.0.1 with a wrapt-decorated Class.method tool, including two runs that completed without calling it, replay on 0.0.1, on main and on the patched copy, and fail on the head. Those replays ran with debug_mode=True, because 0.0.1's own slow first activation intermittently trips the deadlock detector.
| target = attr.__func__ if static else attr | ||
| # fn may wrap the attribute's function, as a functools.wraps decorator | ||
| # applied to Class.method does. | ||
| if inspect.unwrap(fn, stop=lambda f: f is target) is not target: |
There was a problem hiding this comment.
P2: the identity check also refuses methods decorated with a wrapt-based decorator, which nothing rebound, so 0.0.1 runs that build such a tool stop replaying on upgrade, and the body does not disclose it. wrapt keeps a FunctionWrapper in the class __dict__, and class access returns a new BoundFunctionWrapper whose __wrapped__ is the plain function, so the attribute is never on fn's unwrap chain and this line raises. Probe, wrapt 2.4.1, with class WraptHolder: @activity.defn @traced async def meth(self, trip: str) (traced a @wrapt.decorator): main offers ['trip']; the head raises Cannot tell whether 'WraptHolder.meth' is a method or a staticmethod: 'WraptHolder.meth' reachable from the function's globals is another object, in either decorator order. The Deprecated package's @deprecated(reason=...) (3.0.1, a wrapt.FunctionWrapper subclass) does the same. Replay, with real Workers on the pinned dev server: 0.0.1 offers the tool as ['self', 'trip'] and completes a run whose tools are track and WraptHolder.meth when the model calls only track, and a run with only WraptHolder.meth that calls nothing. Both histories replay on 0.0.1 and on main, and fail on the head with that ValueError, so a run still open at the upgrade fails every Workflow Task until the code changes. On main the same tool runs end to end (the Worker's WraptHolder("worker").meth returns wrapt-meth:worker:meth.trip); on the head a new run fails its first Workflow Task. A wrapt staticmethod works on all three. wrapt is already in this plugin's lockfile through the dev group's OpenInference instrumentation. Fix: compare the innermost functions, if inspect.unwrap(fn) is not inspect.unwrap(target):, and reword the comments here and the _is_method docstring to match. On a patched copy the wrapt and Deprecated methods offer ['trip'] again, the 0.0.1 and main histories replay, #362's reproduction still exits 0, references kept across a reload are still refused, and make lint and the full suite pass (180 passed, 5 skipped). To pin it without depending on wrapt, a 12-line decorator class in tests/helpers/decorators.py whose __get__ returns a fresh functools.wraps wrapper of self.__wrapped__.__get__(obj, cls) reproduces the refusal (main and the patched copy offer ['passengers'], the head raises) and fits as a row in the metadata loop. If you keep the stricter check instead, the Breaking change paragraph and the docstring need to name wrapt-style decorated methods and their 0.0.1 runs.
| raise ValueError( | ||
| f"Cannot tell whether '{fn.__qualname__}' is a method or a" | ||
| f" staticmethod: '{'.'.join(qualname)}' reachable from the" | ||
| " function's globals is another object. Pass a bound method for a" |
There was a problem hiding this comment.
P3: the refusal fires after any in-process reload, not only one that changed the class, and neither remedy in the message fits that case. importlib.reload re-executes the module into the same globals, so every reference taken before it fails the check even when the file is unchanged. Probe: from acts import Holder, keep Holder.count (method) and Holder.echo (staticmethod taking self), then reload the unchanged acts: main offers ['passengers'] and ['self']; the head raises for both, and also for Holder.count read again through the caller's Holder, which is still the old class (a re-run notebook cell does exactly this). Refusing is the trade the body chooses, but 'as after importlib.reload of a changed module' understates it for the release notes, and that user needs to take the reference again, not pass a bound method or rename a parameter. Suggested: say 'after a reload' in the body, and end the message with 'Take the reference again if its module was reloaded, pass a bound method for a method, or rename a staticmethod's parameter.'
What was changed
activity_as_toolingoogle_adktells a method passed asClass.methodfrom a staticmethod by finding the class that the function's__qualname__names, in the function's globals, and reading that attribute (#312). It now also checks that the attribute is the function it was given, or a staticmethod of it. Afunctools.wrapswrapper of that function, such asdecorate(Class.method), still counts as that function. If the class's name or that attribute now holds another object, as afterimportlib.reloadof a changed module,activity_as_toolcan't tell what the reference is, so it raisesValueError, as it does for a class it can't reach. The message says to pass a bound method for a method, or to rename a staticmethod's parameter.The
activity_as_tooldocstring names the new case.Breaking change: compared with 0.0.1, a staticmethod taking a parameter named
self, referenced after its class's name or attribute was rebound, now fails withValueErrorwhen the tool is built, and its runs stop replaying. On 0.0.1 its tool offeredself. #312 already does the same for a class that can't be reached. A method referenced this way didn't work on 0.0.1 either. #312 isn't released yet, so both land in the same release, and the release notes can list this case with #312's. No version gate.Why?
See #362. On
mainsince #312, such a reference is classified by the object now under the name. A staticmethodecho(self, value), kept before its class's name was rebound to a class with a methodecho, gets a tool withoutself. The model'svaluereachesself, and the call returns a wrong result with no error. In the reverse case, a method keepsselfin its tool.Checklist
Closes [Bug] google_adk:
activity_as_toolclassifies a method reference by its class's rebound attribute (main only) #362. Follow-up to Breaking: support method and callable-class Activities in Google ADK activity_as_tool #312.How was this tested:
test_activity_as_tool_rejects_method_of_unreachable_classnow also rebinds the class's module-level name to a class with the opposite kinds. It checks that a kept staticmethod reference and a kept method reference both raiseValueError, bare and wrapped with afunctools.wrapsdecorator. Withmain'sworkflow.py(88745dc) installed, it fails withDID NOT RAISE ValueError.test_activity_as_tool_preserves_metadatanow passes afunctools.wrapswrapper of an unchangedClass.staticmethod, of aClass.method, and of an already decorated staticmethod. It checks their parameters, the wrapped staticmethod's ADK schema, and that calling its tool returns the model'sself. It passes onmaintoo.activity_as_toolclassifies a method reference by its class's rebound attribute (main only) #362's reproduction exits 0 on 0.0.1 (the tool offers['self', 'value']and returnsself=model-self value=model-value), 1 onmain(['value'],self=model-value value=default), and 0 with this change, whereactivity_as_toolraisesValueError.make lintpasses.make testgives 175 passed and 5 skipped withouttests/test_optional_mcp.py, and 5 passed for that file on its own.main, and so does the control. No new histories were recorded: the change only adds a refusal when the tool is built, and changes no command of a tool it builds.Any docs updates needed?
The
activity_as_tooldocstring is updated here. The release notes should list this case with Breaking: support method and callable-class Activities in Google ADK activity_as_tool #312's breaking changes.