diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 3e73b116..623a47e3 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -20,7 +20,7 @@ import inspect import logging import uuid -import weakref +import warnings from collections import deque from collections.abc import Iterator from contextlib import contextmanager @@ -39,6 +39,7 @@ TypeVar, overload, ) +from weakref import ref from fastapi import APIRouter, BackgroundTasks, Body, FastAPI, HTTPException, Response from pydantic import BaseModel, create_model @@ -118,8 +119,7 @@ class Invocation(Thread): def __init__( self, - action: ActionDescriptor, - thing: Thing, + action: ActionInfo, id: uuid.UUID, input: Optional[BaseModel] = None, dependencies: Optional[dict[str, Any]] = None, @@ -128,11 +128,7 @@ def __init__( """Create a thread to run an action and track its outputs. :param action: provides the function that we run, as well as metadata - and type information. The descriptor is not bound to an object, so we - supply the `~lt.Thing` it's bound to when the function is run. - :param thing: is the object on which we are running the ``action``, i.e. - it is supplied to the function wrapped by ``action`` as the ``self`` - argument. + and type information. :param id: is a `uuid.UUID` used to identify the invocation, for example when polling its status via HTTP. :param input: is a `pydantic.BaseModel` representing the body of the HTTP @@ -146,8 +142,7 @@ def __init__( Thread.__init__(self, daemon=True) # keep track of the corresponding ActionDescriptor - self.action_ref = weakref.ref(action) - self.thing_ref = weakref.ref(thing) + self._action = action self.input = input if input is not None else EmptyInput() self.dependencies = dependencies if dependencies is not None else {} @@ -212,32 +207,31 @@ def error_model(self) -> ProblemDetails | None: return ProblemDetails.from_exception(self._exception) @property - def action(self) -> ActionDescriptor: - """The `.ActionDescriptor` object running in this thread. + def action(self) -> ActionInfo: + """The `.ActionInfo` object we're running.""" + return self._action - :raises RuntimeError: if the action descriptor has been deleted. - This should never happen, as the descriptor is a property of - a class, which won't be deleted. + @property + def action_ref(self) -> ref[ActionDescriptor]: + """A weak reference to the descriptor object. + + Note that this is deprecated in favour of `.action` which returns + an `ActionInfo` object. """ - action = self.action_ref() - if action is None: # pragma: no cover - # Action descriptors should only be deleted after the server has - # stopped, so this error should never occur. - raise RuntimeError("The action for an `Invocation` has been deleted!") - return action + warnings.warn( + DeprecationWarning( + "Invocation.action_ref() is deprecated in favour of Invocation.action" + ), + stacklevel=2, + ) + return self.action._descriptor_ref @property def thing(self) -> Thing: - """The `~lt.Thing` to which the action is bound, i.e. this is ``self``. - - :raises RuntimeError: if the Thing no longer exists. - """ - thing = self.thing_ref() - if thing is None: # pragma: no cover - # this error block is primarily for mypy: the Thing will exist as - # long as the server is running, so we should never hit this error. - raise RuntimeError("The `Thing` on which an action was run is missing!") - return thing + """The `~lt.Thing` to which the action is bound, i.e. this is ``self``.""" + # Note: this should never error, because there's no way to start an + # Invocation without a bound Thing. + return self.action.owning_object_or_error() @property def cancel_hook(self) -> invocation_contexts.CancelEvent: @@ -267,7 +261,7 @@ def summary_model(self) -> InvocationSummary: return InvocationSummary( status=self.status, id=self.id, - action=self.thing.path + self.action.name, # type: ignore[attr-defined] + action=self.thing.path + self.action.name, href=URLFor("action_invocation", id=self.id), timeStarted=self._start_time, timeCompleted=self._end_time, @@ -287,7 +281,7 @@ def response(self) -> InvocationModel: # The line below confuses MyPy because self.action **evaluates to** a Descriptor # object (i.e. we don't call __get__ on the descriptor). with self._status_lock: - return self.action.invocation_model( # type: ignore[attr-defined] + return self.action.invocation_model( **dict(self.summary_model()), input=self.input, output=self.output_model_instance, @@ -303,7 +297,7 @@ def _publish_status(self) -> None: self.thing._thing_server_interface.publish( Message( thing=self.thing.name, - affordance=self.action.name, # type: ignore[attr-defined] + affordance=self.action.name, message_type="action", payload=self._status.value, ) @@ -336,16 +330,13 @@ def run(self) -> None: :raises RuntimeError: if there is no Thing associated with the invocation. """ - # self.action evaluates to an ActionDescriptor. This confuses mypy, - # which thinks we are calling ActionDescriptor.__get__. - action: ActionDescriptor = self.action # type: ignore[assignment] logger = self.thing.logger # The line below saves records matching our ID to ``self._log`` add_thing_log_destination(self.id, self._log) with invocation_contexts.set_invocation_id(self.id): try: self._publish_status() - + action = self.action.get_descriptor() thing = self.thing kwargs = model_to_dict(self.input) if thing is None: # pragma: no cover @@ -439,8 +430,7 @@ def append_invocation(self, invocation: Invocation) -> None: def invoke_action( self, - action: ActionDescriptor, - thing: Thing, + action: ActionInfo, input: Any, dependencies: dict[str, Any], ) -> Invocation: @@ -449,11 +439,7 @@ def invoke_action( See `.Invocation` for more details. :param action: provides the function that we run, as well as metadata - and type information. The descriptor is not bound to an object, so we - supply the `~lt.Thing` it's bound to when the function is run. - :param thing: is the object on which we are running the ``action``, i.e. - it is supplied to the function wrapped by ``action`` as the ``self`` - argument. + and type information. :param input: is a `pydantic.BaseModel` representing the body of the HTTP request that invoked the action. It is supplied to the function as keyword arguments. @@ -464,7 +450,6 @@ def invoke_action( """ thread = Invocation( action=action, - thing=thing, input=input, dependencies=dependencies, id=uuid.uuid4(), @@ -484,8 +469,8 @@ def get_invocation(self, id: uuid.UUID) -> Invocation: def list_invocations( self, - action: Optional[ActionDescriptor] = None, - thing: Optional[Thing] = None, + action: str | None = None, + thing: str | None = None, ) -> list[InvocationSummary]: """All of the invocations currently managed. @@ -494,20 +479,19 @@ def list_invocations( not yet expired. :param action: filters out only the invocations of a particular - `.ActionDescriptor`. Note that if there are two Things - of the same subclass, filtering by action will return invocations - on either `~lt.Thing`. - :param thing: returns only invocations of actions on a particular `~lt.Thing`. - This will often be combined with filtering by ``action`` to give the - list of invocations returned by a GET request on an action endpoint. + action, identified by its name. Note that this will match an action + of that name on any `~lt.Thing`. + :param thing: returns only invocations of actions on a particular `~lt.Thing`, + identified by its name. This will often be combined with filtering by + ``action``. :return: A list of invocations, optionally filtered by Thing and/or Action. """ return [ i.summary_model() for i in self.invocations - if thing is None or i.thing == thing - if action is None or i.action == action + if thing is None or i.thing.name == thing + if action is None or i.action.name == action ] def expire_invocations(self) -> None: @@ -553,7 +537,7 @@ def action_invocation(id: uuid.UUID) -> Response: return serialise_from_user_code( model_instance=invocation.response(), description=f"invocation '{id}' of ", - code=invocation.action.func, # type: ignore[attr-defined] + code=invocation.action.func, ) except KeyError as e: raise HTTPException( @@ -726,7 +710,7 @@ def output_model(self) -> type[BaseModel]: return self.get_descriptor().output_model @property - def invocation_model(self) -> type[BaseModel]: + def invocation_model(self) -> type[InvocationModel]: """A Pydantic model for an invocation of this action.""" return self.get_descriptor().invocation_model @@ -949,8 +933,7 @@ def start_action( ) -> Response: action_manager = thing._thing_server_interface._action_manager invocation = action_manager.invoke_action( - action=self, - thing=thing, + action=thing.actions[self.name], input=body, dependencies=dependencies, ) @@ -1024,7 +1007,10 @@ def start_action( ) def list_invocations() -> list[InvocationSummary]: action_manager = thing._thing_server_interface._action_manager - return action_manager.list_invocations(self, thing) + invocations = action_manager.list_invocations( + action=self.name, thing=thing.name + ) + return invocations def action_affordance( self, thing: Thing, path: Optional[str] = None diff --git a/tests/test_actions.py b/tests/test_actions.py index b8304f84..e8d3b53d 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -7,13 +7,14 @@ from pydantic import BaseModel import labthings_fastapi as lt -from labthings_fastapi.actions import ActionInfo +from labthings_fastapi.actions import ActionInfo, Invocation from labthings_fastapi.example_things import MyThing from labthings_fastapi.exceptions import ( FailedToInvokeActionError, ServerActionError, UnserialisableTypeError, ) +from labthings_fastapi.invocations import InvocationStatus from labthings_fastapi.testing import create_thing_without_server from tests.temp_client import get_link, poll_task @@ -88,6 +89,38 @@ def test_action_info(): assert move_eyes.is_bound is True +@pytest.mark.parametrize( + ("action_name", "input", "output"), + [("say_hello", None, "Hello World."), ("move_eyes", {"direction": "left"}, None)], +) +def test_invocation(action_name, input, output, mocker): + """Test the Invocation class""" + mocker.patch("labthings_fastapi.actions.add_thing_log_destination") + action_man = create_thing_without_server(ActionMan) + id = uuid.uuid4() + action = action_man.actions[action_name] + invocation = Invocation( + action=action, + id=id, + input=input, + ) + assert invocation.id == id + assert invocation.action is action + with pytest.warns(DeprecationWarning): + assert invocation.action_ref == action._descriptor_ref + assert invocation.thing is action_man + # These should change when we run it. + assert invocation.status == InvocationStatus.PENDING + assert invocation.output is None + # Run the action + invocation.start() + invocation.join(timeout=1) + assert not invocation.is_alive() # fail rather than hang + # It should complete successfully + assert invocation.status == InvocationStatus.COMPLETED + assert invocation.output == output + + def test_get_action_invocations(client): """Test that running "get" on an action returns a list of invocations.""" # When we start the action has no invocations