Repository navigation
Conversation
0af8613 to
7fe708b
Compare
|
The rebase is pushed as One blocking review finding: [P1] Preserve annotation resolution when wrapping I ran a callable class whose constructor and I also changed the existing The new schema is built before the patch gate is checked, so the legacy path cannot protect these older runs. Resolve the annotations on the original |
5ea0c2a to
68ad367
Compare
|
Fixed in
Coverage: With the gate gone, I removed |
activity_as_tool accepted a callable-class activity by its class, the reference workflow code uses, but built the tool from the class's signature, which is its constructor's. The model was offered the constructor's parameters and its values went to __call__ by position, so a run could complete with a wrong result. For a class, build the schema from __call__ without self, keep the class name, and take the description from __call__'s docstring (or the class docstring). Resolve __call__'s annotations in its own module, since the partial that wraps it has no module globals. Breaking change: there is no version gate. A run that called such a tool before this change recorded the constructor's arguments. On replay the tool rejects them and the replay fails with a non-determinism error. Let those runs finish before upgrading, or use Worker Versioning.
68ad367 to
a42ec46
Compare
brianstrauch
left a comment
There was a problem hiding this comment.
One P2 finding on callable-class argument handling.
| if inspect.isclass(fn): | ||
| # Workflow code references a callable-class activity by its class, but the worker's | ||
| # registered instance runs __call__, so build the tool from __call__ without `self`. | ||
| call = functools.partial(fn.__call__, None) |
There was a problem hiding this comment.
[P2] Preserve arguments for static and class __call__ methods
Unconditionally binding None assumes fn.__call__ has an unbound instance receiver. For @staticmethod async def __call__(city: str) and @classmethod async def __call__(cls, city: str), the exposed signature is already (city: str), so this consumes the real city parameter. I reproduced both with a constructor taking city: str: the base tool exposes city, while this branch exposes an empty schema and schedules the activity with args=[], producing TypeError: ...__call__() missing 1 required positional argument: 'city'. The SDK recognizes both registered instances with argument types [str].
Please bind a dummy receiver only for an unbound instance method, preserving static and already-bound class methods, and add regression coverage for both forms.
There was a problem hiding this comment.
Thanks, fixed in 8dffd34. activity_as_tool now drops the receiver only when __call__ is a plain function on the class (inspect.isfunction(inspect.getattr_static(cls, "__call__"))), so a @staticmethod or @classmethod __call__ keeps all its parameters.
Coverage: test_tool_workflow gains ActivityWeatherStaticCall and ActivityWeatherClassMethodCall, each with __call__(city: str), and checks that both are scheduled with the model's city. Without the fix, both tools offer an empty schema and the Workflow fails on the ActivityWeatherStaticCall Activity. With it, the full suite (390 tests) and the make lint steps pass locally.
…tools activity_as_tool dropped the first parameter of every callable-class __call__, assuming an unbound `self`. A @staticmethod or @classmethod __call__ has no `self` to drop, so the tool lost its first real parameter and the Activity failed with a missing argument. Drop the receiver only when __call__ is a plain function.
brianstrauch
left a comment
There was a problem hiding this comment.
One P2 finding on callable classes using functools.partialmethod for call. Lint and 59 focused tests pass; an additional regression probe fails on this revision.
| if inspect.isfunction(inspect.getattr_static(fn, "__call__")): | ||
| call = functools.partial(fn.__call__, None) | ||
| else: | ||
| call = functools.partial(fn.__call__) |
There was a problem hiding this comment.
[P2] Handle partialmethod implementations of __call__
A functools.partialmethod descriptor can still expose an unbound instance receiver, but inspect.isfunction(inspect.getattr_static(fn, "__call__")) returns false for it. For example:
@activity.defn
class Weather:
def __init__(self, city: str) -> None:
pass
async def run(self, conditions: str, city: str) -> str:
return f"{city}: {conditions}"
__call__ = functools.partialmethod(run, "sunny")Here Weather.__call__ has signature (self, city: str). The new wrapper leaves self in place, so execution falls through to the existing receiver-removal block, which raises AttributeError: 'functools.partial' object has no attribute '__temporal_activity_definition' at line 198. Consequently, activity_as_tool(Weather, ...) fails during tool construction.
I verified that the base implementation exposes city and completes a call through a local worker registered with Weather("Tokyo"), returning Tokyo: sunny; this revision fails before scheduling the activity. Please handle the receiver for partialmethod descriptors and add regression coverage.
There was a problem hiding this comment.
Thanks, fixed in abaae82. Rather than adding another descriptor check, activity_as_tool now binds the class's __call__ descriptor to a stand-in instance, as the registered instance would bind it. Python then drops self from a method or partialmethod and keeps every parameter of a static or class method. Annotations and the docstring come from the function underneath, so your example offers city: string with run's docstring.
Coverage: a new test_partialmethod_call_tool_drops_only_self checks the schema and description for __call__ = functools.partialmethod(get_weather, "Rainy"). On 8dffd34 it fails with the same AttributeError. It's a schema test, not part of test_tool_workflow, because on Python 3.10 the SDK registers an async partialmethod __call__ as sync (is_async=False), so the Worker can't run it there. With the change, all four forms offer {'city': 'string'} on Python 3.10, 3.13 and 3.14. Locally, the full suite (391 tests) and the make lint steps pass.
Checking for a plain-function __call__ left `self` in a functools.partialmethod __call__, so building its tool failed with AttributeError. Bind the __call__ descriptor to a stand-in instance instead, so Python drops `self` where the registered instance would: from a method or partialmethod, but not from a static or class method.
There was a problem hiding this comment.
Review of the __call__-based class tool. Each finding below was reproduced against the PR build (Python 3.10 to 3.14, SDK 1.34.0, Agents SDK 0.22.0) unless marked otherwise. Items already raised in earlier review rounds (static/class method receivers, partialmethod receiver, annotation resolution) are not repeated.
Not anchored to a diff line:
- Replay coverage.
tests/histories/tools-workflow-history.jsonhas noActivityWeather*schedules, so no committed history replays an invoked class tool. A later change to the scheduled command (for example scheduling underschema.name, or re-adding constructor args) keepstest_openai_replay.pygreen while production histories break. - Same bug elsewhere.
python/google_genai/.../workflow.pystill builds class tools from the constructor, so #205 applies there too and may deserve its own issue. - Title. The PR is a deliberate breaking change but the title has no
Breaking:prefix. Release notes are generated from commit subjects, so the squash subject should carry it.
| "Input activity must have a name to be made into a tool", | ||
| "invalid_tool", | ||
| ) | ||
| if inspect.isclass(fn): |
There was a problem hiding this comment.
Replay compatibility (judgment call). This switches the tool's schema and arguments with no workflow.patched branch. run_activity always executes inside the workflow, so an invoke-time gate is feasible: build both the legacy constructor-derived schema and the new __call__ schema once, then branch inside run_activity on workflow.patched(...). Build-time gating is impossible because activity_as_tool can run at import. I understand from the thread this was removed on purpose; if so, please state in the PR body why that is acceptable for a GA plugin (for example that every affected run was already producing wrong results), since a run whose model already called a class tool now fails replay with [TMPRL1100].
There was a problem hiding this comment.
Yes, that's deliberate. We're not adding version gates to these fixes: breaking old histories is accepted with the plugins' 1.0 releases. The runs affected had the constructor's schema, which gave the right result only when the constructor's parameters matched __call__'s. I've added that to the "Breaking change" note in the description.
| # registered instance runs __call__, so build the tool from __call__ as an instance binds | ||
| # it: without the `self` of a method or partialmethod, and with every parameter of a | ||
| # static or class method. The tool never calls this stand-in binding. | ||
| method = inspect.getattr_static(fn, "__call__") |
There was a problem hiding this comment.
Keyword-only __call__ parameters are offered to the model and then dropped. The SDK's keyword-only check at @activity.defn time inspects the constructor for a class, so async def __call__(self, *, city: str) decorates fine. function_schema then advertises city as required, to_call_args returns it in the kwargs half, and run_activity discards that half (the comment at the call site says the SDK already rejected keyword-only parameters, which is no longer true for classes). The activity is scheduled with args=[] and the worker fails every attempt with TypeError: missing 1 required keyword-only argument. Same for functools.partialmethod(get, conditions="Rainy"), whose signature turns later parameters keyword-only. Suggest rejecting inspect.Parameter.KEYWORD_ONLY parameters of the bound __call__ here with the same ApplicationError(..., "invalid_tool") the function uses.
There was a problem hiding this comment.
Fixed in e0e076a. activity_as_tool now rejects keyword-only parameters of the bound __call__, including the ones a partialmethod keyword creates, with ApplicationError(..., "invalid_tool"). test_invalid_callable_class_tool covers it.
| # static or class method. The tool never calls this stand-in binding. | ||
| method = inspect.getattr_static(fn, "__call__") | ||
| if hasattr(method, "__get__"): | ||
| method = method.__get__(object(), fn) |
There was a problem hiding this comment.
Malformed classes raise raw exceptions instead of ApplicationError('invalid_tool'). @activity.defn class NoCall: def __init__(self, c: str): ... is accepted by the SDK, getattr_static returns the metaclass slot type.__call__, and binding it to object() raises TypeError: descriptor '__call__' for 'type' objects doesn't apply to a 'object' object. A zero-parameter __call__ = zero raises ValueError: invalid method signature later. Inside workflow code these become workflow task failures that retry indefinitely. Please guard for a class-level __call__ and wrap signature errors in ApplicationError naming the class. (Passing the registered instance, which the PR body calls out of scope, has the same opaque AttributeError on __name__; a one-line invalid_tool error would be kinder.)
There was a problem hiding this comment.
Fixed in e0e076a. A class with no __call__ in its MRO now raises ApplicationError("Activity class X has no __call__ method", "invalid_tool"), and a signature error such as the zero-parameter __call__ raises ApplicationError("Cannot build a tool from X: invalid method signature", "invalid_tool"). Both are cases in test_invalid_callable_class_tool. I left passing the instance as it is on main, since this PR doesn't touch that path.
| method = inspect.getattr_static(fn, "__call__") | ||
| if hasattr(method, "__get__"): | ||
| method = method.__get__(object(), fn) | ||
| call = functools.partial(method) |
There was a problem hiding this comment.
Two stand-in builders that disagree. This partial plus hand-copied __name__/__annotations__/__doc__ duplicates the existing self branch below, which still copies raw string annotations. Result: the Class.method form (which the new docstring advertises) still raises NameError: name 'Weather' is not defined under from __future__ import annotations, while the class form now works (reproduced). One helper that builds a single stand-in with resolved hints, called from both branches, removes the divergence. function_schema(fn, name_override=...) also exists, so setattr(call, '__name__', ...) is not needed.
There was a problem hiding this comment.
Fixed in e0e076a. The class branch now only picks the callable and the function to read annotations and docs from. The self check and one stand-in, with resolved hints, name and docstring, are shared by both forms. So Class.method resolves postponed annotations too: ActivityWeatherService.get_weather_method now takes city: "CityName", and on abaae82 that fails test_tool_workflow and both ToolsWorkflow replays with NameError.
I kept setting __name__: generate_func_documentation in agents/function_schema.py reads func.__name__ even when name_override is given, so a partial without one fails.
| # A partial has no module globals, so resolve postponed annotations against the | ||
| # function's own. | ||
| func = method | ||
| while isinstance(func, functools.partial): |
There was a problem hiding this comment.
Python 3.14 (the CI max) silently mis-builds a bare functools.partial __call__. On 3.14 functools.partial is a method descriptor whose __get__ returns a bound method wrapping the partial, so this loop stops at the method and the tool is built from functools.partial's own annotations and docstring: __call__ = functools.partial(free, 'x') gives properties == {} and description 'Create a new function with partial application of the given arguments and keywords.' with no error. On 3.10 to 3.13 the same class gives the right city schema, so behavior diverges across the CI matrix. Unwrap types.MethodType via __func__ as well as partial layers.
There was a problem hiding this comment.
Unwrapped in e0e076a: annotations and the docstring now come from the function under bound methods as well as partials, so the description is free's.
The empty schema on 3.14 is what the instance accepts, though. There the partial binds like a method and gets the instance as its next argument, so inspect.signature(instance) is (). On 3.10 to 3.13 the class doesn't reach the tool: @activity.defn raises TypeError, because the SDK can't read a partial's type hints. So neither version can run this form, and I didn't add a test for it. I listed it under "Not covered".
| while isinstance(func, functools.partial): | ||
| func = func.func | ||
| call.__annotations__ = typing.get_type_hints(func, include_extras=True) | ||
| call.__doc__ = func.__doc__ or fn.__doc__ |
There was a problem hiding this comment.
Docstring fallback misbehaves in both directions. (1) It picks up synthesized class docs: @activity.defn @dataclass class DC: conditions: str with an undocumented __call__ yields description "DC(conditions: 'str')", the constructor signature, which also becomes the activity summary. (2) It reads only the class's own __doc__, so a documented base class is lost, whereas the pre-PR function_schema(cls) used inspect.getdoc, which walks the MRO: class Derived(Base) with a documented Base now gets description == ''. Suggest inspect.getdoc(func) and no dataclass-synthesized fallback.
There was a problem hiding this comment.
Fixed in e0e076a. The description is __call__'s own docstring, then inspect.getdoc(cls), which walks base classes, but not for a dataclass, whose generated docstring is its constructor's signature. A dataclass with its own class docstring loses the fallback too; documenting __call__ covers that. I didn't call inspect.getdoc on __call__: for an undocumented one it walks up to object.__call__ and returns "Call self as a function.". test_callable_class_tool_schema now covers a dataclass and a class documented on its base class.
| func = func.func | ||
| call.__annotations__ = typing.get_type_hints(func, include_extras=True) | ||
| call.__doc__ = func.__doc__ or fn.__doc__ | ||
| fn = typing.cast(Callable, call) |
There was a problem hiding this comment.
The existing self branch now runs on this stand-in and can crash. When the bound __call__ still begins with self (for example functools.singledispatchmethod, whose __get__ returns a plain function), execution falls into the self branch below, which does getattr(fn, '__temporal_activity_definition') on the partial and raises AttributeError: 'functools.partial' object has no attribute '__temporal_activity_definition'. Nothing reads that copied attribute after ret is computed at the top of the function, so deleting that setattr removes the crash. typing.cast here is also redundant.
There was a problem hiding this comment.
Fixed in e0e076a. The unused __temporal_activity_definition copy and the typing.cast are gone, and the shared self check now runs on the bound callable before the stand-in is built. A singledispatchmethod __call__ now offers city on 3.10, 3.12, 3.13 and 3.14.
| events.append(e) | ||
|
|
||
| assert len(events) == 11 | ||
| attrs = e.activity_task_scheduled_event_attributes |
There was a problem hiding this comment.
This reads activity_task_scheduled_event_attributes on every history event and relies on protobuf returning an empty default message so activity_type.name == '' fails the startswith. The rest of the suite guards the same read with e.HasField("activity_task_scheduled_event_attributes") (for example further down this file). Using elif e.HasField(...) makes the intent explicit.
|
|
||
|
|
||
| def test_partialmethod_call_tool_drops_only_self(): | ||
| # On Python 3.10 the SDK runs an async partialmethod as sync, so test_tool_workflow can't |
There was a problem hiding this comment.
Two corrections for the partialmethod form. (1) The version note is too narrow: inspect.iscoroutinefunction(cls.__call__) is False on 3.10, 3.11 and 3.12 and True only from 3.13 (measured), so the SDK registers the activity as sync on all three, and Worker(activities=[PM()]) then demands an activity_executor. (2) Support is schema-only beyond JSON primitives: the SDK's _type_hints_from_func sees only the unannotated partialmethod wrapper (arg_types == [None]), so a non-primitive parameter such as a dataclass is decoded on the worker as a plain dict and __call__ fails with AttributeError: 'dict' object has no attribute .... Consider scoping the docstring claim and adjusting this comment to "before Python 3.13".
There was a problem hiding this comment.
Thanks. I measured is_async=False on 3.10 and 3.12 and True on 3.13 and 3.14, so the comment now says "Before Python 3.13". The argument decoding is the SDK's, so I listed it under "Not covered" in the description. The fn docstring doesn't mention partialmethod, so I left it as is.
…orms
Address review findings on callable-class activity tools:
- Build one stand-in for both the class and the Class.method forms,
with annotations resolved in the function's own module, so a
Class.method with string annotations no longer fails with NameError.
Drop the unused __temporal_activity_definition copy, which crashed
when a bound __call__ still started with `self`.
- Raise ApplicationError("invalid_tool") for a class with no __call__,
a __call__ whose signature can't be read, and keyword-only
parameters, which the tool can't pass.
- Read annotations and docs from under bound methods as well as
partials. Take the description from __call__'s own docstring, then
the class's or a base class's, but not a dataclass's generated one.
- Add a ToolsWorkflow History whose model calls each class tool to the
replay test.
|
Thanks for the review. The fixes are in
|
brianstrauch
left a comment
There was a problem hiding this comment.
Two remaining findings, reproduced against e0e076a on Python 3.14.4. make lint and 16 focused tests passed, including all nine replay histories. Additional probes fail for the cases below; corresponding baseline checks pass.
| partial, | ||
| "__temporal_activity_definition", | ||
| getattr(fn, "__temporal_activity_definition"), | ||
| if params and params[0].name == "self": |
There was a problem hiding this comment.
[P2] Preserve real parameters named self after descriptor binding
The class branch has already bound the descriptor, so a remaining parameter named self can be an actual input rather than a receiver. For example, both @staticmethod async def __call__(self: str) and @classmethod async def __call__(cls, self: str) are valid, but this check consumes their required input. I reproduced both producing properties == {}. The tool consequently schedules with no arguments, leaving the registered callable missing its required parameter.
As a baseline control, classes with a city: str constructor and either of these call signatures successfully receive "Tokyo" through the base implementation's tool. Please track whether an unbound receiver remains, instead of applying the name-only heuristic to every already-bound class callable, and cover both signatures.
There was a problem hiding this comment.
Fixed in bfc2003. __call__ must now be a function, staticmethod, classmethod or functools.partialmethod, and binding any of them already drops the receiver, so the class form no longer strips a parameter named self. test_callable_class_tool_schema covers @staticmethod __call__(self: str) and @classmethod __call__(cls, self: str); both offer self.
| func = func.func if isinstance(func, functools.partial) else func.__func__ | ||
| doc = func.__doc__ | ||
| # A dataclass without a docstring gets its constructor's signature as one. | ||
| if not doc and not dataclasses.is_dataclass(fn): |
There was a problem hiding this comment.
[P2] Preserve explicit dataclass documentation in the fallback
dataclasses.is_dataclass(fn) identifies all dataclasses, including ones with an explicitly written class docstring; it does not distinguish a generated constructor signature. Consequently, @activity.defn @dataclass class Weather with a class docstring such as "Get the weather for a city." and an undocumented __call__ now produces description == "". I reproduced the same class retaining its description on the base implementation. The guard also suppresses inherited documentation for dataclass subclasses.
This removes the description the model uses to select the tool. Please filter the synthesized constructor documentation while preserving explicit class documentation and the nearest documented base, and add a documented-dataclass regression case.
There was a problem hiding this comment.
Fixed in bfc2003. Only the Name(...) docstring that dataclasses and namedtuples write is skipped now, so an explicit dataclass docstring is used again. ActivityWeatherDocumentedDataclass covers it.
I didn't keep the nearest documented base, though. As DABH found, walking the bases sends library docstrings (abc.ABC, typing.Generic) to the model and into the Activity summary. The description is now __call__'s docstring, or that of a __call__ it overrides, else the class's own.
DABH
left a comment
There was a problem hiding this comment.
Second pass, on the current head. The previous round's keyword-only gap, the dead attribute copy, the postponed-annotation gap for Class.method, the 3.14 functools.partial binding, the raw errors for malformed classes, the missing replay history and the Breaking: subject are all addressed. Lint and the suite pass. The findings below were reproduced in the plugin venv on 3.14 (and on a 3.10 interpreter where the version matters).
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Where the remaining problems cluster: the new docstring fallback (lines 197-200) now has three independent failure modes, and the plugin's own __call__ resolution differs from the SDK's _type_hints_from_func, so the schema the model fills and the types the Worker decodes can disagree with no error at tool-build time (__call__(this, loc: Loc) offers loc as an object schema while ret.arg_types is [None, Loc], so the worker decodes raw and __call__ receives a dict; a partialmethod __call__ gets arg_types=[None]). The right depth is one resolution shared by both sides (upstreaming this binding into temporalio.common._type_hints_from_func); until then the plugin holds ret.arg_types and can refuse the disagreement it can see (arg_types is None, a length mismatch with the schema's parameters, or a None hint for a non-primitive parameter) with invalid_tool, turning the PR's 'not covered' caveats into enforced ones. The keyword-only check at line 213 is compensation for the same SDK gap (@activity.defn validates the constructor's signature, not __call__'s); a comment naming that reason would help the next reader.
As noted last round, google_genai (workflow.py:79-91, byte-for-byte the pre-PR code), strands_agents (_temporal_activity_tool.py:28-29) and google_adk (workflow.py:159,222,252) still build a callable class's tool from inspect.signature(cls), so #205 stays reachable in three siblings; worth tracking issues.
| func = func.func if isinstance(func, functools.partial) else func.__func__ | ||
| doc = func.__doc__ | ||
| # A dataclass without a docstring gets its constructor's signature as one. | ||
| if not doc and not dataclasses.is_dataclass(fn): |
There was a problem hiding this comment.
P2: the dataclass guard discards every dataclass docstring, not just the generated one. dataclasses only writes Name(sig) into __doc__ when the docstring is empty, so an explicit docstring is never the generated text, yet this guard skips the fallback for the whole dataclass family (and for plain subclasses of a dataclass, since is_dataclass is true through inherited __dataclass_fields__). @activity.defn @dataclass class Lookup: """Look up a parcel.\n\nArgs:\n parcel_id: ...""" with an undocumented __call__ gives tool.description == '' and loses the per-parameter descriptions and the activity summary, where the pre-PR function_schema(cls) returned 'Look up a parcel.' (reproduced, 8 of 8 finders). The test enshrines the symptom ((ActivityWeatherDataclass, "")) and has no explicit-docstring dataclass case. Compare fn.__doc__ against f"{fn.__name__}{inspect.signature(fn)}".replace(" -> None", "") (the exact string dataclasses writes) and skip only that; a typing.NamedTuple base produces a similar generated doc (NTTool(conditions,)) that is not filtered today.
There was a problem hiding this comment.
Fixed in bfc2003 (see Brian's thread). A docstring is skipped only when it starts with Name( on a dataclass or a tuple subclass, which covers dataclasses and namedtuples; your NTTool now gets ''. I used the prefix instead of rebuilding the exact signature string. There are now cases for both a documented and an undocumented dataclass.
| doc = func.__doc__ | ||
| # A dataclass without a docstring gets its constructor's signature as one. | ||
| if not doc and not dataclasses.is_dataclass(fn): | ||
| doc = inspect.getdoc(fn) |
There was a problem hiding this comment.
P2: inspect.getdoc(fn) walks the class MRO, so unrelated library docstrings become the tool description and the activity summary. A callable class with no docstring on __call__ or itself that derives from abc.ABC yields 'Helper class that provides a standard way to create an ABC using\ninheritance.'; from typing.Generic, the multi-paragraph Generic docstring (reproduced). Any user class deriving from a documented framework base (clients, mixins, pydantic models) sends that base's docstring to the model and records it as the ScheduleActivity summary. If the intent is the nearest documented user base, walk fn.__mro__ explicitly; otherwise use only fn.__doc__.
There was a problem hiding this comment.
Fixed in bfc2003: the class fallback is the class's own docstring only, so abc.ABC's and typing.Generic's no longer reach the model. ActivityWeatherStaticCall derives from abc.ABC and expects an empty description.
| func = call | ||
| while isinstance(func, (functools.partial, types.MethodType)): | ||
| func = func.func if isinstance(func, functools.partial) else func.__func__ | ||
| doc = func.__doc__ |
There was a problem hiding this comment.
P3: the inheritance is inverted. func.__doc__ reads only the resolved __call__'s own attribute, so a subclass that overrides a documented base __call__ without repeating the docstring gets '' (or the base class docstring from line 200), while inspect.getdoc semantics would inherit the method docstring as function_schema does for plain functions (reproduced: inspect.getdoc(vars(Derived)['__call__']) returns the base text). The same applies to the Class.method form overriding a documented base method. Walk fn.__mro__ for the first __call__ in vars(c) with a docstring; do not use inspect.getdoc(cls.__call__), which falls through to type.__call__ and returns 'Call self as a function.' for a docless __call__.
There was a problem hiding this comment.
Fixed in bfc2003: the description is the first docstring among the __call__s in fn.__mro__ (functions, staticmethods, classmethods and partialmethods), then the class's own. ActivityWeatherDerived overrides a documented base __call__ and gets that docstring, not the base class's. The Class.method form now reads inspect.getdoc, so it inherits a base method's docstring too.
| partial, | ||
| "__temporal_activity_definition", | ||
| getattr(fn, "__temporal_activity_definition"), | ||
| if params and params[0].name == "self": |
There was a problem hiding this comment.
P2: the receiver is still dropped by the literal parameter name self, now on top of descriptor binding. For the Class.method form, a method whose first parameter is named anything else (async def get(this, city: str)) is exposed to the model as a required this parameter, to_call_args passes the model's value positionally, and the worker's bound method fails with TypeError: takes 2 positional arguments but 3 were given on every attempt (reproduced). For the class form the strip runs after __get__ binding, so @staticmethod async def __call__(self: str, city: str) has a real positional parameter removed: the tool offers only city, arg_types is [str, str], and the worker fails with 'missing 1 required positional argument' (reproduced). Bind the method form the same way (fn.__get__(object()) drops the first parameter regardless of its name, resolves string annotations in the method's module, and forwards __name__/__doc__, producing an identical FuncSchema for every tested form), and do not apply the name strip inside the class branch.
There was a problem hiding this comment.
For the class form, fixed in bfc2003 (see Brian's thread): no name check after binding.
For Class.method I kept the name check. The plugin gets a plain function there, the same as a module-level one, so it can't bind it without knowing it's a method. The name is also how main and the SDK's _type_hints_from_func find the receiver: for get(this, city: str) the SDK already records [None, str] as the argument types, whatever schema the plugin builds. I'd fix that in the SDK.
| raise ApplicationError( | ||
| f"Activity class {fn.__name__} has no __call__ method", "invalid_tool" | ||
| ) | ||
| call = method.__get__(object(), fn) if hasattr(method, "__get__") else method |
There was a problem hiding this comment.
P2: only ValueError is translated, so descriptor and type errors still escape as raw exceptions. Binding to a bare object() makes C slot wrappers and any descriptor that type-checks its instance raise TypeError: @activity.defn class PartialSub(functools.partial): pass (nearest __call__ in the MRO is functools.partial's slot wrapper) raises TypeError: descriptor '__call__' for 'functools.partial' objects doesn't apply to a 'object' object, a cached/bound-caching descriptor whose __get__ checks isinstance(obj, cls) raises likewise, and __call__ = 42 (accepted by @activity.defn, which inspects the constructor) raises TypeError: 42 is not a callable object from inspect.signature (all reproduced on 3.10 and 3.14). Inside a workflow these are task failures retried forever rather than the promised invalid_tool. Catch TypeError alongside ValueError around the bind and the signature read. Two related version splits: on 3.13 functools.partial.__get__ exists but only emits FutureWarning: functools.partial will be a method descriptor... and returns itself, so a __call__ = functools.partial(...) class warns on every tool build (and fails under -W error); and a callable instance as __call__ (__call__ = Impl()) makes typing.get_type_hints raise TypeError on 3.10-3.12 while returning {} on 3.14. The hasattr(method, '__get__') fallback is otherwise untested; either bind unconditionally and document that __call__ must be a function, staticmethod or classmethod, or add a callable-instance case to test_callable_class_tool_schema.
There was a problem hiding this comment.
Fixed in bfc2003. A __call__ that isn't a function, staticmethod, classmethod or functools.partialmethod over a function now gets invalid_tool before any binding. That covers your functools.partial subclass, a callable instance, a bare functools.partial on 3.14 and a singledispatchmethod (which last round offered city; it's now rejected). With no binding of a functools.partial, there's no 3.13 FutureWarning either. Binding, the signature read and get_type_hints are in one try that turns NameError, TypeError and ValueError into invalid_tool. On 3.10 to 3.14 @activity.defn already rejects __call__ = 42 here, with TypeError.
| # own module, and the activity's name and docstring. | ||
| stand_in = functools.partial(call) | ||
| setattr(stand_in, "__name__", fn.__name__) | ||
| stand_in.__annotations__ = typing.get_type_hints(func, include_extras=True) |
There was a problem hiding this comment.
P3: get_type_hints here is outside the try/except, and on 3.10 it changes the method-path schema. (1) For a partialmethod __call__ whose underlying method has a TYPE_CHECKING-only annotation, @activity.defn accepts the class (the SDK sees the annotation-less _method wrapper) but this call raises NameError out of workflow code (reproduced), a task-failure loop rather than invalid_tool. (2) On Python 3.10 only, get_type_hints wraps a parameter with a None default in Optional[...], so class Svc: @activity.defn async def m(self, city: Annotated[str, 'The city'] = None) now yields anyOf [string, null] with the Annotated description lost (the SDK's _strip_annotated only strips a top-level Annotated), where the old partial (no __code__) gave {'type': 'string', 'description': 'The city'}; on 3.11+ both give str. Plain-function tools already behave this way on 3.10, so this only needs a decision: accept the 3.10 divergence for methods, or unwrap the 3.10 Optional when the raw annotation is not Optional.
There was a problem hiding this comment.
(1) Fixed in bfc2003: get_type_hints is inside the try, so a TYPE_CHECKING-only annotation gets invalid_tool.
(2) I'll accept the 3.10 difference. Class.method now resolves hints the way plain function tools always have, so on 3.10 both wrap a None default in Optional; 3.12 and later keep str and the description.
| call: Callable = fn | ||
| func: Callable = fn | ||
| doc = fn.__doc__ | ||
| if inspect.isclass(fn): |
There was a problem hiding this comment.
P3: the instance form still crashes with a raw AttributeError. activity_as_tool(ActivityWeatherClass('Cloudy')), the exact object passed to Worker(activities=[...]), passes _Definition.from_callable (the definition is found through the class attribute), skips this branch, and dies in generate_func_documentation with AttributeError: 'ActivityWeatherClass' object has no attribute '__name__' (reproduced); inside a workflow that is a task failure retried forever, and only AgentsWorkflowError is in workflow_failure_exception_types. The PR body leaves this out of scope, which is fine, but then not inspect.isclass(fn) and not inspect.isroutine(fn) should raise invalid_tool naming the three supported forms, or map the instance to type(fn). The docstring's Raises: section (line 150) also still lists only the 'not properly decorated' case and none of the new invalid_tool cases (no __call__, unreadable signature, keyword-only parameters).
There was a problem hiding this comment.
Fixed in bfc2003: passing an instance of a callable-class activity raises ApplicationError("Pass the activity class X, not an instance of it", "invalid_tool"), and Raises: lists the new invalid_tool cases. test_invalid_callable_class_tool covers the instance.
| call = functools.partial(call, None) | ||
| params = params[1:] | ||
| # run_activity passes only positional arguments. | ||
| if any(p.kind is inspect.Parameter.KEYWORD_ONLY for p in params): |
There was a problem hiding this comment.
P3 simplification: this check, the params = params[1:] slice that feeds it, and the ValueError translation at line 203 are reachable only for classes. @activity.defn already runs inspect.signature(fn) on a decorated function or method and raises TypeError('Activity cannot have keyword-only arguments'), so for every non-class form these branches are dead; for a class, after method.__get__(object(), fn) the first parameter is never self, so the strip at line 209 is unreachable in the class branch. Moving the try/except and the keyword-only check inside if inspect.isclass(fn): (with the comment that the SDK validated the constructor's signature, not __call__'s) and binding the method form with fn.__get__(object()) removes the call is not fn gate, the two stacked partials, the hand-copied __name__/__annotations__/__doc__ trio, and the call/func/doc prologue initialised at the top and overwritten; a restructured version produced identical FuncSchema fields and identical ApplicationError messages for all 15 forms tried, including the three invalid classes. An alternative that also removes the pre-resolved annotations dict: one stand-in carrying __wrapped__ and __signature__ (functools.update_wrapper plus stand_in.__signature__ = sig.replace(parameters=params)), so function_schema's own inspect.signature/get_type_hints handle every form and the plugin no longer has to stay in lock-step with what the Agents SDK reads.
There was a problem hiding this comment.
Mostly done in bfc2003. Binding, the signature read and the keyword-only check (now with a comment on why it's needed) live in the class branch. The method form keeps its own self check (see the thread on line 209). Both build their stand-in with one _tool_stand_in helper, so the call is not fn gate and the shared prologue are gone. I kept the resolved-annotations dict rather than __wrapped__ and __signature__, since it's the version tested across these forms.
| stand_in.__annotations__ = typing.get_type_hints(func, include_extras=True) | ||
| stand_in.__doc__ = doc | ||
| fn = stand_in | ||
| schema = function_schema(fn) |
There was a problem hiding this comment.
P3 (pre-existing, adjacent): function_schema(fn) never forwards the caller's strict_json_schema, so ensure_strict_json_schema always runs and raises a raw agents.exceptions.UserError ('additionalProperties should not be set for object types ...') for a dict[str, Any] or **kwargs parameter even when the user passed strict_json_schema=False (reproduced with a class __call__(self, city: str, **extra: str)). The new keyword-only guard also does not cover VAR_KEYWORD, whose values run_activity would drop. Pass strict_json_schema=strict_json_schema through and treat VAR_KEYWORD like keyword-only.
There was a problem hiding this comment.
**kwargs in __call__ is now rejected with the keyword-only check (ActivityWeatherKwargsCall). Passing strict_json_schema through to function_schema changes the schema of every tool built with strict_json_schema=False, so I'd rather do that in its own PR.
| # test_tool_workflow can't run this form; the tool's schema is the part this plugin builds. | ||
| (ActivityWeatherPartialMethodCall, "Get the weather for a given city."), | ||
| # Not the docstring a dataclass writes from its constructor. | ||
| (ActivityWeatherDataclass, ""), |
There was a problem hiding this comment.
P3 (tests): the dataclass case pins the empty description (see line 199); add an explicit-docstring dataclass expecting its docstring and a generated-docstring dataclass expecting the base/empty fallback. Running ActivityWeatherStaticCall and ActivityWeatherClassMethodCall through the end-to-end workflow duplicates what this parametrized schema test already proves per binding form (the end-to-end leg, run_activity passing positionals and the SDK calling instance(*args), is the same code path for all three), at the cost of coordinated edits in five places plus the new 1,081-line replay history; keeping one class form in ToolsWorkflow and adding the other two here would be cheaper while the history file is still new. And test_tool_workflow now has 17 copies of the six-line index-based assert block; a decoded results list zipped against an expected list reports the failing index and makes a new tool two list entries.
There was a problem hiding this comment.
Done in bfc2003: the static and class forms moved from ToolsWorkflow into test_callable_class_tool_schema, the History is re-recorded with only ActivityWeatherClass (23 events), and there are documented and undocumented dataclass cases. I left the index-based asserts in test_tool_workflow as they are. That's the test's existing style, and this PR now adds two of them.
…n fallback
Address review findings on callable-class activity tools:
- Accept a __call__ that is a function, staticmethod, classmethod or
functools.partialmethod, and raise ApplicationError("invalid_tool")
for anything else. Binding these already drops the receiver, so the
class form no longer strips a real parameter named `self`.
- Turn NameError and TypeError, as well as ValueError, from binding,
the signature read and type hints into invalid_tool, and reject
**kwargs in __call__ like keyword-only arguments.
- Reject an instance of a callable-class activity, and list the
invalid_tool cases in the docstring.
- Take the description from __call__ or a __call__ it overrides, else
the class's own docstring, skipping only the one dataclasses and
namedtuple generate. Base classes' docstrings, such as abc.ABC's, are
no longer used.
- Test the static and class forms in the schema test instead of
ToolsWorkflow, and re-record the History with the one class tool.
|
Thanks both. The fixes are in
|
What was changed
activity_as_toolnow builds a callable-class Activity's tool from its__call__, not from its constructor.__call__as an instance binds it, keeps the class name as the tool name, and takes the description from__call__'s docstring or that of a__call__it overrides, else the class's own docstring, but not theName(...)one that dataclasses and namedtuples generate.__call__must be a function, staticmethod, classmethod orfunctools.partialmethod: binding it drops the receiver of a method, classmethod or partialmethod and keeps every parameter of a staticmethod. Calls then pass the model's arguments to__call__, which the Worker's registered instance runs.__call__'s annotations are resolved in its own module, keepingAnnotatedmetadata, so string annotations andfrom __future__ import annotationswork. Methods passed asClass.methodnow get the same resolution; before, a string annotation failed withNameError. They still drop a first parameter namedself, as onmainand in the SDK.ApplicationError(..., "invalid_tool"): an instance of a callable-class activity in place of its class, a class whose__call__is missing or of another kind, a__call__whose signature or type hints can't be read, and a__call__with keyword-only parameters or**kwargs, which the tool can't pass.fndocstring now names the three forms.No public name or option changes.
Breaking change: the change has no version gate. We are not adding version gates to these fixes: breaking old histories is accepted with the plugins' 1.0 releases. The runs affected had the constructor's schema, which gave the right result only when the constructor's parameters matched
__call__'s. A run that called such a tool before this change recorded the constructor's arguments. Built from__call__, the tool rejects them on replay, returns an argument error to the model instead of scheduling the Activity, and the replay fails with[TMPRL1100]. This holds whether the tool was built in Workflow code or once at import time. Runs that only used function orClass.methodtools are not affected. Also, a class whose__call__takes a parameter with no JSON schema now fails tool construction, as a function Activity with that parameter does; before, it got the constructor's schema. The same holds for the classes in theinvalid_toolbullet above.Upgrade: let runs that called a callable-class Activity tool passed as its class finish on the earlier version before you upgrade, or keep them there with Worker Versioning.
Why?
See #205. Workflow code references a callable-class Activity by its class, as
workflow.execute_activity_classdoes, butactivity_as_toolbuilt the tool frominspect.signature(cls), the constructor's. The model was offered the constructor's parameters and its values went to__call__by position: with a one-string constructor and a one-string call, the run completed with a wrong result and no error. Other constructors failed tool construction in Workflow code, or failed every call withTypeError.Checklist
Closes [Bug] openai_agents:
activity_as_toolbuilds a callable-class Activity's tool from its constructor #205How was this tested:
test_tool_workflowintests/test_openai.pygains a callable-class Activity,ActivityWeatherClass(constructor takesconditions,__call__takescity: "CityName", a string naming a module-level alias), passed toactivity_as_toolas its class and registered asActivityWeatherClass("Cloudy"). The test now also checks that the class's Activity is scheduled with the model'scity.mainatffda4f6it fails:assert [] == [['"Tokyo"']](the model'scityfailed the constructor schema, so the Activity was never scheduled).tools-workflow-history.jsonfail withNameError: name 'CityName' is not defined.test_callable_class_tool_schemachecks the schema and description for a@staticmethod __call__(self: str)on anabc.ABCsubclass and a@classmethod __call__(cls, self: str)(both offerself), a__call__set withfunctools.partialmethod, an undocumented and a documented dataclass, and a__call__that overrides a documented one. It checks the schema only, because before Python 3.13 the SDK registers an asyncpartialmethod__call__as sync.test_invalid_callable_class_toolchecks theinvalid_toolerrors for a class with no__call__, a zero-parameter function as__call__, keyword-only and**kwargsparameters, and an instance.ActivityWeatherService.get_weather_methodnow takescity: "CityName", sotest_tool_workflowand theToolsWorkflowreplays cover theClass.methodannotation resolution.e0e076a'sactivity_as_tool, 8 of these 21 tests fail: the twoselfparameters (empty schema), the documented dataclass and the overriding__call__(wrong description), the**kwargsand instance cases (rawUserErrorandAttributeError), and the two cases whose error message changed.make lint(ruff check --select I,ruff format --check,pyright,mypy,basedpyright,pydocstyle) passes. These ran in a cached environment with uv 0.13.1, outside the plugin'srequired-version, withUV_RUNpointed at it. CI coversmake lintandmake test.activity_as_toolbuilds a callable-class Activity's tool from its constructor #205's reproduction exits 1 on 1.1.0, onmainatc664a34and ontemporalio.contrib.openai_agentsintemporalio1.33.0. On this branch the model is offeredparcel_idand the Activity returnsparcel P-1717 via acme-post.tests/histories/tools-workflow-callable-class-history.json, recorded on this branch, has the model callActivityWeatherClass(23 events), andtest_replayreplays it on Python 3.10, 3.12, 3.13 and 3.14. A version of it recorded with all three class tools (ine0e076a) failed onmain'sactivity_as_toolwith[TMPRL1100], whiletools-workflow-history.jsonreplayed.mainfrom aToolsWorkflowrun whose model called the class tool fails on this branch with[TMPRL1100] Nondeterminism error: Activity type of scheduled event 'ActivityWeatherClass' does not match activity type of activity command 'invoke_model_activity'. Earlier checks on a gated version of this change, with Workers with no Workflow cache, covered a class tool built in Workflow code and one built at import time, each for one and two turns around a held Activity, and a function tool as a control. Without the gate, all eightmainhistories with a class tool failed with[TMPRL1100], and the two function-tool histories replayed.Any docs updates needed?
The
activity_as_tooldocstring is updated here. Release notes should say which runs stop replaying (see "Breaking change").Not covered:
functools.partialmethod__call__runs only as far as the SDK supports it. The SDK registers an async one as sync before Python 3.13, and it reads no argument types for it (arg_types == [None]), so arguments that aren't JSON primitives are not converted._type_hints_from_funcfinds a__call__or method receiver by the nameself, so for__call__(this, loc: Loc)the Worker decodeslocwithout its type (reported in review, not rerun here). That is the SDK's, and affectsworkflow.execute_activity_classtoo.function_schemadoesn't get the caller'sstrict_json_schema.Class.methodtools now wrap a parameter with aNonedefault inOptional, as function tools already do there.AttributeError. Workflow code references the class, so this PR leaves it alone.temporalio1.34.x still bundles the old code astemporalio.contrib.openai_agents. No backport here.Open questions:
temporalio1.34.x get a backport?Draft while the breaking change and this question are open.