diff --git a/python/google_adk/src/temporalio/google_adk/workflow.py b/python/google_adk/src/temporalio/google_adk/workflow.py index 693f778d..15e5dd45 100644 --- a/python/google_adk/src/temporalio/google_adk/workflow.py +++ b/python/google_adk/src/temporalio/google_adk/workflow.py @@ -278,8 +278,8 @@ def _is_method(fn: Callable) -> bool: A module-level or nested function, or a staticmethod, is called with every parameter. A function defined in a class body is found in that class to - tell a method from a staticmethod; a class that can't be found raises - ``ValueError``. + tell a method from a staticmethod; a class that can't be found, or whose + attribute doesn't wrap the same function as ``fn``, raises ``ValueError``. """ qualname = fn.__qualname__.split(".") if len(qualname) < 2 or qualname[-2] == "": @@ -305,7 +305,22 @@ def _is_method(fn: Callable) -> bool: " (wrap decorators with functools.wraps), pass a bound method for a" " method, or rename a staticmethod's parameter." ) - return not isinstance(attr, staticmethod) + static = isinstance(attr, staticmethod) + # Compare the innermost functions: a decorator applied to Class.method + # wraps the attribute's function, and a wrapt-style one stores a wrapper in + # the class that class access wraps again. + if inspect.unwrap(fn) is not inspect.unwrap(attr.__func__ if static else attr): + # The class's name or attribute was rebound after fn was taken from it, + # for example by reloading its module, so the attribute's binding isn't + # fn's. + 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. Import the class again" + " if its module was reloaded, pass a bound method for a method, or" + " rename a staticmethod's parameter." + ) + return not static def _tool_signature( @@ -420,8 +435,9 @@ def activity_as_tool(activity_def: Callable, **kwargs: Any) -> Callable: staticmethod, classmethod or ``functools.partialmethod``, can't be read, or takes keyword-only arguments or ``**kwargs`` raises ``ValueError``. So does a method taking ``self`` whose class can't be reached from the function's - globals, such as a class defined in a function: its ``self`` could be a - staticmethod's input. + globals, such as a class defined in a function, or whose class's name or + attribute now holds another object: its ``self`` could be a staticmethod's + input. Outside a Workflow, as in local ADK runs, the tool calls ``activity_def`` itself. A method passed as ``Class.method`` and a class passed as itself diff --git a/python/google_adk/tests/helpers/decorators.py b/python/google_adk/tests/helpers/decorators.py index 1fd50832..10068273 100644 --- a/python/google_adk/tests/helpers/decorators.py +++ b/python/google_adk/tests/helpers/decorators.py @@ -2,6 +2,7 @@ import functools import inspect +import types from collections.abc import Awaitable, Callable from typing import Any @@ -26,3 +27,18 @@ def unwrapped( delattr(wrapper, "__wrapped__") setattr(wrapper, "__signature__", inspect.signature(fn)) return wrapper + + +class WrapOnAccess: + """Stays in the class; each access returns a new ``functools.wraps`` wrapper, as wrapt does.""" + + __wrapped__: Callable[..., Awaitable[Any]] + + def __init__(self, fn: Callable[..., Awaitable[Any]]) -> None: + self.__wrapped__ = fn + + def __get__( + self, obj: object, cls: type | None = None + ) -> Callable[..., Awaitable[Any]]: + fn = self.__wrapped__ + return passthrough(fn if obj is None else types.MethodType(fn, obj)) diff --git a/python/google_adk/tests/test_google_adk_agents.py b/python/google_adk/tests/test_google_adk_agents.py index bbb82a2a..6933ab97 100644 --- a/python/google_adk/tests/test_google_adk_agents.py +++ b/python/google_adk/tests/test_google_adk_agents.py @@ -14,6 +14,7 @@ """Integration tests for ADK Temporal support.""" +import asyncio import functools import inspect import json @@ -69,7 +70,7 @@ ) from temporalio.worker import Worker from temporalio.workflow import ActivityConfig -from tests.helpers.decorators import passthrough, unwrapped +from tests.helpers.decorators import WrapOnAccess, passthrough, unwrapped from tests.helpers.opentelemetry import dump_spans from tests.helpers.postponed_annotations import PostponedHolder @@ -1004,6 +1005,12 @@ async def count_decorated(self, passengers: int) -> str: """Method wrapped by a decorator from another module.""" return f"{self.prefix}:{passengers}" + @WrapOnAccess + @activity.defn + async def count_wrapped_on_access(self, passengers: int) -> str: + """Method whose decorator wraps it again on each access.""" + return f"{self.prefix}:{passengers}" + class __Private: """Its attribute in the class is mangled: ``_ComplexActivityMethodHolder__Private``.""" @@ -1679,14 +1686,31 @@ async def nested_echo(self) -> str: # type: ignore[no-untyped-def] # pyright: i (ComplexActivityMethodHolder.private_count, ["passengers"]), (PostponedHolder.track, ["parcel_id"]), (nested_echo, ["self"]), + # A decorator applied to Class.method wraps the class's own function. + (passthrough(ComplexActivityMethodHolder.echo_static), ["self"]), + (passthrough(ComplexActivityMethodHolder.count_passengers), ["passengers"]), + (passthrough(ComplexActivityMethodHolder.echo_decorated), ["self"]), + (ComplexActivityMethodHolder.count_wrapped_on_access, ["passengers"]), ]: tool = temporalio.google_adk.workflow.activity_as_tool( fn, start_to_close_timeout=timedelta(seconds=30) ) assert list(inspect.signature(tool).parameters) == params, fn.__qualname__ + wrapped_static_tool = temporalio.google_adk.workflow.activity_as_tool( + passthrough(ComplexActivityMethodHolder.echo_static), + start_to_close_timeout=timedelta(seconds=30), + ) + declaration = FunctionTool(wrapped_static_tool)._get_declaration() + assert declaration is not None + assert declaration.parameters_json_schema is not None + assert list(declaration.parameters_json_schema["properties"]) == ["self"] + assert asyncio.run(wrapped_static_tool(self="model-self")) == "static:model-self" + -def test_activity_as_tool_rejects_method_of_unreachable_class() -> None: +def test_activity_as_tool_rejects_method_of_unreachable_class( + monkeypatch: pytest.MonkeyPatch, +) -> None: """A method whose class can't be found can't be told from a staticmethod.""" class LocalHolder: @@ -1706,6 +1730,40 @@ async def method(self, trip: str) -> str: UnwrappedHolder.echo, start_to_close_timeout=timedelta(seconds=30) ) + # A reference kept after its class's name is rebound: the class now found + # holds another object under that name, so the reference is refused rather + # than given that object's binding. + echo_static = ComplexActivityMethodHolder.echo_static + count_passengers = ComplexActivityMethodHolder.count_passengers + monkeypatch.setitem(globals(), "ComplexActivityMethodHolder", ReboundMethodHolder) + with pytest.raises(ValueError, match="is another object"): + temporalio.google_adk.workflow.activity_as_tool( + echo_static, start_to_close_timeout=timedelta(seconds=30) + ) + with pytest.raises(ValueError, match="is another object"): + temporalio.google_adk.workflow.activity_as_tool( + count_passengers, start_to_close_timeout=timedelta(seconds=30) + ) + # Wrapping a kept reference doesn't make it the rebound attribute. + for fn in (echo_static, count_passengers): + with pytest.raises(ValueError, match="is another object"): + temporalio.google_adk.workflow.activity_as_tool( + passthrough(fn), start_to_close_timeout=timedelta(seconds=30) + ) + + +class ReboundMethodHolder: + """Takes ComplexActivityMethodHolder's name, with the opposite bindings.""" + + @activity.defn + async def echo_static(self) -> str: + return "rebound" + + @staticmethod + @activity.defn + async def count_passengers(self, passengers: int) -> str: # type: ignore[no-untyped-def] # pyright: ignore[reportMissingParameterType, reportSelfClsParameterName] + return str(passengers) + class UnwrappedHolder: @staticmethod