Use ActionInfo in Invocations

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 @@
 
     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 @@
         """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 @@
         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 @@
                 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 @@
             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 @@
         # 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 @@
         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 @@
 
         :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 invoke_action(
         self,
-        action: ActionDescriptor,
-        thing: Thing,
+        action: ActionInfo,
         input: Any,
         dependencies: dict[str, Any],
     ) -> Invocation:
@@ -449,11 +439,7 @@
         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 @@
         """
         thread = Invocation(
             action=action,
-            thing=thing,
             input=input,
             dependencies=dependencies,
             id=uuid.uuid4(),
@@ -484,8 +469,8 @@
 
     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 @@
         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 @@
                 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(
@@ -598,8 +582,8 @@
             with self._invocations_lock:
                 try:
                     invocation: Any = self._invocations[id]
                 except KeyError as e:
                     raise HTTPException(
                         status_code=404,
                         detail="No action invocation found with ID {id}",
                     ) from e
@@ -612,7 +596,7 @@
                     invocation.output.response
                 ):
                     # TODO: honour "accept" header
                     return invocation.output.response()
                 try:
                     return serialise_from_user_code(
                         model_instance=invocation.output_model_instance,
@@ -648,8 +632,8 @@
             with self._invocations_lock:
                 try:
                     invocation: Any = self._invocations[id]
                 except KeyError as e:
                     raise HTTPException(
                         status_code=404,
                         detail="No action invocation found with ID {id}",
                     ) from e
@@ -726,7 +710,7 @@
         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
 
@@ -850,7 +834,7 @@
         """
         super().__set_name__(owner, name)
         if self.name != self.func.__name__:
             raise ValueError(
                 f"Action name '{self.name}' does not match function name "
                 f"'{self.func.__name__}'",
             )
@@ -949,8 +933,7 @@
         ) -> 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,
             )
@@ -991,14 +974,14 @@
         try:
             responses[200]["model"] = self.output_model
             pass
         except AttributeError:
             print(f"Failed to generate response model for action {self.name}")
         # Add an additional media type if we may return a file
         if hasattr(self.output_model, "media_type"):
             responses[200]["content"][self.output_model.media_type] = {}
         # Now we can add the endpoint to the app.
         if thing.path is None:
             raise NotConnectedToServerError(
                 "Can't add the endpoint without thing.path!"
             )
         app.post(
@@ -1024,7 +1007,10 @@
         )
         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
@@ -1046,7 +1032,7 @@
         """
         path = path or thing.path
         if path is None:
             raise NotConnectedToServerError("Can't generate forms without a path!")
         forms = [
             Form[ActionOp](href=path + self.name, op=[ActionOp.invokeaction]),
         ]

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