From 9034289a7f37e6bd6160f1a621f8bf195f3cfa0e Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 30 Oct 2026 00:35:28 +0000 Subject: [PATCH 1/3] Use ActionInfo in Invocations Passing around descriptor instances is hard to type properly and gets really confusing. I've changed `Invocation` so that `action` is an `ActionInfo` object rather than an `ActionDescriptor`, which should fix that problem. I've also changed `list_invocations` to use thing and action names, instead of objects. --- src/labthings_fastapi/actions.py | 96 +++++++++++--------------------- 1 file changed, 33 insertions(+), 63 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 3e73b116..dbf1e7e2 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -20,7 +20,6 @@ import inspect import logging import uuid -import weakref from collections import deque from collections.abc import Iterator from contextlib import contextmanager @@ -118,8 +117,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 +126,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 +140,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 +205,16 @@ def error_model(self) -> ProblemDetails | None: return ProblemDetails.from_exception(self._exception) @property - def action(self) -> ActionDescriptor: - """The `.ActionDescriptor` object running in this thread. - - :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. - """ - 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 + def action(self) -> ActionInfo: + """The `.ActionInfo` object we're running.""" + return self._action @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 +244,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 +264,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 +280,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 +313,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 +413,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 +422,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 +433,6 @@ def invoke_action( """ thread = Invocation( action=action, - thing=thing, input=input, dependencies=dependencies, id=uuid.uuid4(), @@ -484,8 +452,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 +462,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 +520,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 +693,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 +916,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 +990,11 @@ 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 + ) + print(f"list invocations: {invocations}") + return invocations def action_affordance( self, thing: Thing, path: Optional[str] = None From e332749930488325ccd122e9e28d879c6336c20f Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 30 Oct 2026 01:14:10 +0000 Subject: [PATCH 2/3] Provide `Invocation.action_ref` for backwards compatibility. Added a test for the new property, which also checks a few other features of Invocation. --- src/labthings_fastapi/actions.py | 17 ++++++++++++++++ tests/test_actions.py | 34 +++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index dbf1e7e2..dea9b41b 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -20,6 +20,7 @@ import inspect import logging import uuid +import warnings from collections import deque from collections.abc import Iterator from contextlib import contextmanager @@ -38,6 +39,7 @@ TypeVar, overload, ) +from weakref import ref from fastapi import APIRouter, BackgroundTasks, Body, FastAPI, HTTPException, Response from pydantic import BaseModel, create_model @@ -209,6 +211,21 @@ def action(self) -> ActionInfo: """The `.ActionInfo` object we're running.""" return self._action + @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. + """ + 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``.""" diff --git a/tests/test_actions.py b/tests/test_actions.py index b8304f84..7efbeee5 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,37 @@ 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() + # It should complete successfully + assert invocation.status == InvocationStatus.COMPLETED + assert invocation.output is 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 From dae42db02cd5d20c24250abb5b859cf005ba5737 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 2 Nov 2026 20:48:02 +0000 Subject: [PATCH 3/3] Fixes from code review This removes a left-over print statement and swaps `is` for `==` in a string comparison. It also adds a timeout to a test that had the potential to hang indefinitely. --- src/labthings_fastapi/actions.py | 1 - tests/test_actions.py | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index dea9b41b..623a47e3 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -1010,7 +1010,6 @@ def list_invocations() -> list[InvocationSummary]: invocations = action_manager.list_invocations( action=self.name, thing=thing.name ) - print(f"list invocations: {invocations}") return invocations def action_affordance( diff --git a/tests/test_actions.py b/tests/test_actions.py index 7efbeee5..e8d3b53d 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -114,10 +114,11 @@ def test_invocation(action_name, input, output, mocker): assert invocation.output is None # Run the action invocation.start() - invocation.join() + 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 is output + assert invocation.output == output def test_get_action_invocations(client):