diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 4482a597..d87381a3 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -214,6 +214,17 @@ class ThingSlotError(RuntimeError): """ +class ThingSlotCircularDependencyError(RuntimeError): + """There was no order in which the Things could be correctly started. + + This error is raised when Things have incompatible requirements about + start-up order. `~lt.thing_slot` allows Things to specify that they + should be started up only once the connected Things have been started. + If there's a cycle (e.g. A must start after B, but B must start after A), + then LabThings will fail to start with this error. + """ + + class InvocationCancelledError(BaseException): """An invocation was cancelled by the user. diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index f29991a3..f1e0f138 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -43,7 +43,7 @@ from labthings_fastapi.thing import Thing from labthings_fastapi.thing_description._model import ThingDescription from labthings_fastapi.thing_server_interface import ThingServerInterface -from labthings_fastapi.thing_slots import ThingSlot +from labthings_fastapi.thing_slots import ThingSlot, _determine_startup_order from labthings_fastapi.utilities import class_attributes __all__ = ["ThingServer"] @@ -105,9 +105,9 @@ def __init__( :param config: a `~lt.ThingServerConfig` object that configures the server, or something that may be converted to one. - :param debug: ff ``True``, set the log level for `~lt.Thing` instances to + :param debug: if ``True``, set the log level for `~lt.Thing` instances to DEBUG. - :param \**kwargs: ff keyword arguments are supplied, they will be passed + :param \**kwargs: if keyword arguments are supplied, they will be passed to the constructor of `~lt.ThingServerConfig`\ . This is not allowed if `config` is a `~lt.ThingServerConfig` object. @@ -162,6 +162,7 @@ def __init__( # The function calls below create and set up the Things. self._things = self._create_things() self._connect_things() + self._startup_order = _determine_startup_order(self.things) self._attach_things_to_server() @classmethod @@ -398,9 +399,7 @@ def _connect_things(self) -> None: """ for thing_name, thing in self.things.items(): config = self._config.thing_configs[thing_name].thing_slots - for attr_name, attr in class_attributes(thing): - if not isinstance(attr, ThingSlot): - continue + for attr_name, attr in class_attributes(thing, ThingSlot): target = config.get(attr_name, ...) attr.connect(thing, self.things, target) @@ -436,6 +435,8 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None, None]: ``__enter__`` on each Thing. The error is also saved to ``self.startup_failure`` for post mortem, as otherwise uvicorn will swallow it and replace it with SystemExit(3) and no traceback. + :raises RuntimeError: if the startup order doesn't match the Things that are + attached to the server. This should never happen. """ async with BlockingPortal() as portal: # We create a blocking portal to allow threaded code to call async code @@ -446,8 +447,13 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None, None]: # synchronous __enter__ and __exit__ methods if they exist, to initialise # and shut down the hardware. NB we must make sure the blocking portal # is present when this happens, in case we are dealing with threads. + if set(self.things.keys()) != set(self._startup_order): + raise RuntimeError( + "`self._startup_order` does not match `self.things`." + ) async with AsyncExitStack() as stack: - for thing in self.things.values(): + for name in self._startup_order: + thing = self.things[name] try: await stack.enter_async_context(thing) except BaseException as e: diff --git a/src/labthings_fastapi/thing_slots.py b/src/labthings_fastapi/thing_slots.py index 718ac467..3d160458 100644 --- a/src/labthings_fastapi/thing_slots.py +++ b/src/labthings_fastapi/thing_slots.py @@ -47,7 +47,12 @@ def say_hello(self) -> str: from weakref import ReferenceType, WeakKeyDictionary, WeakValueDictionary, ref from labthings_fastapi.base_descriptor import FieldTypedBaseDescriptor -from labthings_fastapi.exceptions import ThingNotConnectedError, ThingSlotError +from labthings_fastapi.exceptions import ( + ThingNotConnectedError, + ThingSlotCircularDependencyError, + ThingSlotError, +) +from labthings_fastapi.utilities import class_attributes if TYPE_CHECKING: from labthings_fastapi.thing import Thing @@ -102,7 +107,10 @@ class Example(lt.Thing): """ def __init__( - self, *, default: str | None | Iterable[str] | EllipsisType = ... + self, + *, + default: str | None | Iterable[str] | EllipsisType = ..., + start_first: bool = False, ) -> None: """Declare a ThingSlot. @@ -118,12 +126,21 @@ def __init__( If the type is a mapping of `str` to `~lt.Thing` the default should be of type `Iterable[str]` (and could be an empty list). + :param start_first: Whether the connected Things should be started before + the Thing on which the slot is defined. + + When this is `False` (the default), an error will be raised if the slot + is accessed during ``__enter__`` and there's no constraint on the order + in which things will be started. If it is set to `True` then LabThings + will ensure the connected Thing(s) have ``__enter__`` called before it + is called on this Thing. """ super().__init__() self._default = default self._things: WeakKeyDictionary[ "Thing", ReferenceType["Thing"] | WeakValueDictionary[str, "Thing"] | None ] = WeakKeyDictionary() + self._start_first = start_first @property def thing_type(self) -> tuple[type, ...]: @@ -165,6 +182,11 @@ def default(self) -> str | Iterable[str] | None | EllipsisType: """The name of the Thing that will be connected by default, if any.""" return self._default + @property + def start_first(self) -> bool: + """Whether the connected Things must be started before this one.""" + return self._start_first + def _pick_things( self, things: "Mapping[str, Thing]", @@ -338,8 +360,73 @@ def instance_get(self, obj: "Thing") -> ConnectedThings: return val # type: ignore[return-value] # See docstring for an explanation of the type ignore directives. + def _connected_thing_names(self, obj: "Thing") -> set[str]: + """Return the names of the Thing(s) connected to this slot. + + :param obj: the Thing instance we're considering. + :return: a set of Thing names that are connected. + """ + val = self.instance_get(obj) + if val is None: + return set() + if isinstance(val, Mapping): + return set(val.keys()) + else: + return {val.name} -def thing_slot(default: str | Iterable[str] | None | EllipsisType = ...) -> Any: + +def _determine_startup_order(things: "Mapping[str, Thing]") -> tuple[str, ...]: + r"""Determine the order in which Things should be started. + + Thing Slots may specify that connected Things must be started before the + thing on which the slot is defined. This function resolves those + dependencies and sets the order in which the Things should start. + + "start" here refers to calling ``__enter__``\ . + + :param things: a mapping of names to Things. + :return: an ordered list of Thing names. + :raises ThingSlotCircularDependencyError: if there is no order of starting + the Things that will satisfy all the constraints. + """ + dependencies: dict[str, set[str]] = {} + for name, thing in things.items(): + deps = set() + for _, slot in class_attributes(thing, ThingSlot): + if slot.start_first: + deps = deps.union(slot._connected_thing_names(thing)) + dependencies[name] = deps + + # We add Things to the list iteratively, when they are able to be added. + # Things with no dependencies will be added first, gradually working through + # until everything's done. + # If we reach an iteration where we can't add anything, we have a deadlock + # and we must raise an exception. + order: list[str] = [] + remaining = set(things.keys()) + while remaining: + # If a Thing's dependencies are a subset of the things that are already + # started, we can now add it to the order. + things_to_add = {n for n in remaining if dependencies[n].issubset(order)} + if things_to_add: + order += list(things_to_add) + remaining = remaining.difference(things_to_add) + else: + msg = ( + f"There is no order in which the Things may be started.\n" + f"We could start {order}, but the remaining Things have cyclic " + f"dependencies: {remaining}.\n\n" + ) + for name in remaining: + msg += f"'{name}' must be started after {dependencies[name]}.\n" + raise ThingSlotCircularDependencyError(msg) + return tuple(order) + + +def thing_slot( + default: str | Iterable[str] | None | EllipsisType = ..., + start_first: bool = False, +) -> Any: r"""Declare a connection to another `~lt.Thing` in the same server. ``lt.thing_slot`` marks a class attribute as a connection to another @@ -427,6 +514,14 @@ def show_connections(self) -> str: If the default is omitted or set to ``...`` the server will attempt to find a matching `~lt.Thing` instance (or instances). A default value of `None` is allowed if the connection is type hinted as optional. + :param start_first: Whether the connected Things should be started before + the Thing on which the slot is defined. + + When this is `False` (the default), an error will be raised if the slot + is accessed during ``__enter__`` and there's no constraint on the order + in which things will be started. If it is set to `True` then LabThings + will ensure the connected Thing(s) have ``__enter__`` called before it + is called on this Thing. :return: A `.ThingSlot` descriptor. Typing notes: @@ -441,4 +536,4 @@ def show_connections(self) -> str: and it is done by established libraries such as `pydantic`\ . """ - return ThingSlot(default=default) + return ThingSlot(default=default, start_first=start_first) diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index cc68c197..a37ec2da 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -26,40 +26,41 @@ __all__ = [ "RootModelWrapper", - "attributes", "class_attributes", "model_to_dict", ] -def class_attributes(obj: Any) -> Iterable[tuple[str, Any]]: +AttrT = TypeVar("AttrT") + + +def class_attributes( + obj: Any, filter_type: type[AttrT] = object +) -> Iterable[tuple[str, AttrT]]: """List all the attributes of an object's class. This function gets all class attributes, including inherited ones. It is used to obtain the various descriptors used to represent properties and actions. It calls `.attributes` on ``obj.__class__``. + If a ``filter_type`` argument is supplied, only attributes that match the + supplied type will be returned. The default (`object`) matches all + attributes. + + Attributes starting with a double underscore will be ignored. + :param obj: The instance, usually a `~lt.Thing` instance. + :param filter_type: if specified, only return attributes of this type. :yield: tuples of ``(name, value)`` giving each attribute of the class. """ cls = obj.__class__ - yield from attributes(cls) - - -def attributes(cls: Any) -> Iterable[tuple[str, Any]]: - """List all the attributes of an object not starting with `__`. - - :param cls: The object whose attributes we are listing. This may be - a class, because classes are objects too. - - :yield: tuples of ``(name, value)`` giving each attribute and its - value. - """ for name in dir(cls): if name.startswith("__"): continue - yield name, getattr(cls, name) + value = getattr(cls, name) + if isinstance(value, filter_type): + yield name, value WrappedT = TypeVar("WrappedT") diff --git a/tests/test_thing_slots.py b/tests/test_thing_slots.py index d386a1d3..944926d2 100644 --- a/tests/test_thing_slots.py +++ b/tests/test_thing_slots.py @@ -6,7 +6,15 @@ import pytest import labthings_fastapi as lt -from labthings_fastapi.exceptions import ThingSlotError +from labthings_fastapi.exceptions import ( + ThingSlotCircularDependencyError, + ThingSlotError, +) +from labthings_fastapi.testing import ( + MockThingServerInterface, +) +from labthings_fastapi.thing_slots import ThingSlot, _determine_startup_order +from labthings_fastapi.utilities import class_attributes class ThingOne(lt.Thing): @@ -32,7 +40,13 @@ class ThingN(lt.Thing): class ThingThree(lt.Thing): """A Thing that has no other attributes.""" - pass + started = False + + def __enter__(self): + self.started = True + + def __exit__(self, *args, **kwargs): + pass class ThingThatMustBeConfigured(lt.Thing): @@ -41,6 +55,45 @@ class ThingThatMustBeConfigured(lt.Thing): other_thing: lt.Thing = lt.thing_slot(None) +class ThingWithDependency(lt.Thing): + r"""A Thing that relies on a connected Thing during ``__enter__``\ .""" + + started = False + + other_thing: "ThingThree | ThingWithCircularDependency" = lt.thing_slot( + start_first=True + ) + + def __enter__(self): + assert self.other_thing.started + self.started = True + + def __exit__(self, *args, **kwargs): + pass + + +class ThingWithoutDependency(lt.Thing): + """A Thing with a slot that has start_first=False (default).""" + + started = False + + other_thing: "ThingThree | ThingWithCircularDependency" = lt.thing_slot() + + def __enter__(self): + self.started = True + + def __exit__(self, *args, **kwargs): + pass + + +class ThingWithCircularDependency(lt.Thing): + """A Thing that will cause a dependency cycle.""" + + started = False + + other_thing: ThingWithDependency = lt.thing_slot(start_first=True) + + class Dummy: """A dummy thing-like class.""" @@ -161,11 +214,11 @@ def picked_names(things, target): # If there are other Things, they should be filtered by type. for names1 in [[], ["thing1_a"], ["thing1_a", "thing1_b"]]: for names2 in [[], ["thing2_a"], ["thing2_a", "thing2_b"]]: - mixed_things = { + things = { **dummy_things(names1, Dummy1), **dummy_things(names2, Dummy2), } - assert picked_names(mixed_things, ...) == set(names1) + assert picked_names(things, ...) == set(names1) # If a string is specified, it works when it exists and it's the right type. for target in ["thing1_a", "thing1_b"]: @@ -449,3 +502,59 @@ def test_mapping_and_multiple(): assert thing_one.optional_thing is not None assert thing_one.optional_thing.name == "thing_3" assert set(thing_one.n_things.keys()) == {f"thing_{i + 3}" for i in range(3)} + + +def connected_things(classes: dict[str, type[lt.Thing]]) -> dict[str, lt.Thing]: + """Instantiate and connect a list of Things.""" + things = {} + for k, v in classes.items(): + tsi = MockThingServerInterface(name=k, class_name=v.__name__) + things[k] = v(thing_server_interface=tsi) + for thing in things.values(): + for _, attr in class_attributes(thing, ThingSlot): + attr.connect(thing, things, ...) + return things + + +THING_CLASSES_AND_ORDERS = [ + # If there are no constraints, the order doesn't matter + ({"a": ThingThree, "b": ThingThree}, {("a", "b"), ("b", "a")}), + ({"b": ThingThree, "a": ThingThree}, {("a", "b"), ("b", "a")}), + # A thing_slot that doesn't declare `start_first` can have any order + ({"a": ThingWithoutDependency, "b": ThingThree}, {("a", "b"), ("b", "a")}), + # If start_first==True, the list order can take only one value + ({"a": ThingWithDependency, "b": ThingThree}, {("b", "a")}), + # Try with swapped names, just in case something is sorting alphabetically + ({"b": ThingWithDependency, "a": ThingThree}, {("a", "b")}), +] + + +@pytest.mark.parametrize(("thing_classes", "orders"), THING_CLASSES_AND_ORDERS) +def test_determine_startup_order(thing_classes, orders): + """Check the logic to figure out the order in which Things should be started.""" + things = connected_things(thing_classes) + assert _determine_startup_order(things) in orders + + +@pytest.mark.parametrize(("thing_classes", "orders"), THING_CLASSES_AND_ORDERS) +def test_determine_startup_order_server(thing_classes, orders): + """Check the logic to figure out the order in which Things should be started.""" + server = lt.ThingServer.from_things(thing_classes) + assert server._startup_order in orders + with server.test_client(): + # There's an assertion in the relevant Thing's `__enter__` method, and + # it sets `started=True` + assert server.things["a"].started is True + assert server.things["b"].started is True + + +def test_circular_startup_dependency(): + """Check the error for a circular dependency from slots with start_first==True""" + things = connected_things( + { + "a": ThingWithDependency, + "b": ThingWithCircularDependency, + } + ) + with pytest.raises(ThingSlotCircularDependencyError): + _determine_startup_order(things)