From 7e508e7e233d534ac50212025ea25572612a52d8 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 12 Aug 2025 15:06:21 +0100 Subject: [PATCH 1/4] Test dataclass dependencies If `from __future__ import annotations` is not present, dataclasses may be used as dependencies. I've added test code for this, and converted a few test classes to be dataclasses to satisfy the Bugbear linter rule B903. --- tests/module_with_deps.py | 29 +++++++++++++++++++++++------ tests/test_dependencies.py | 11 +++++++++-- tests/test_dependencies_2.py | 34 ++++++++++++++++++++++++++++------ 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/tests/module_with_deps.py b/tests/module_with_deps.py index a7f86fc0..95d7a9c7 100644 --- a/tests/module_with_deps.py +++ b/tests/module_with_deps.py @@ -1,11 +1,28 @@ -from __future__ import annotations +"""A module for testing dependencies. + +This module provides some classes that are used as dependencies by unit tests. +Note that `from __future__ import annotations` is not used here. If it is used, +we would need to add the following to the classes: + +.. code-block:: python + + class Whatever: + __globals__ = globals() # "bake in" globals so dependency injection works + +This relates to the way FastAPI resolves annotations to objects. There's an issue +thread that discusses the work-around above explicitly, but it's part of a bigger +issue discussed here: + +https://github.com/pydantic/pydantic/issues/2678 + +""" + +from dataclasses import dataclass from typing import Annotated from fastapi import Depends, Request class FancyID: - __globals__ = globals() # "bake in" globals so dependency injection works - def __init__(self, r: Request): self.id = 1234 @@ -13,8 +30,8 @@ def __init__(self, r: Request): FancyIDDep = Annotated[FancyID, Depends()] +@dataclass class ClassDependsOnFancyID: - __globals__ = globals() # "bake in" globals so dependency injection works + """A dataclass that will request a FancyID when used as a Dependency.""" - def __init__(self, sub: Annotated[FancyID, Depends()]): - self.sub = sub + sub: FancyIDDep diff --git a/tests/test_dependencies.py b/tests/test_dependencies.py index a8195d86..f33d634a 100644 --- a/tests/test_dependencies.py +++ b/tests/test_dependencies.py @@ -4,6 +4,7 @@ for actions. """ +from dataclasses import dataclass from fastapi import Depends, FastAPI, Request from labthings_fastapi.deps import InvocationID from fastapi.testclient import TestClient @@ -41,9 +42,15 @@ def test_dependency_needing_request(): """Test a dependency that requires Request object""" app = FastAPI() + @dataclass class DepClass: - def __init__(self, sub: Request): - self.sub = sub + r"""A class that has a dependency in its __init__. + + This is a dataclass, so __init__ is generated automatically and + will have an argument `sub` with type `Request`\ . + """ + + sub: Request @app.post("/dep") def endpoint(id: DepClass = Depends()) -> bool: diff --git a/tests/test_dependencies_2.py b/tests/test_dependencies_2.py index 8702ead0..5f682d68 100644 --- a/tests/test_dependencies_2.py +++ b/tests/test_dependencies_2.py @@ -17,6 +17,7 @@ mitigation against something changing in the future. """ +from dataclasses import dataclass from typing import Annotated from fastapi import Depends, FastAPI from fastapi.testclient import TestClient @@ -45,6 +46,8 @@ def test_dep_from_module_with_subdep(): @app.post("/endpoint") def endpoint(id: Annotated[ClassDependsOnFancyID, Depends()]) -> bool: + # Verify that the dependency is supplied, including its sub-dependency + assert id.sub.id == 1234 return True with TestClient(app) as client: @@ -101,25 +104,44 @@ def endpoint(id: DepClass = Depends()) -> bool: def test_class_dep_with_subdep(): - """Add an endpoint that uses a dependency class with sub-dependency""" + """Add an endpoint that uses a dependency class with sub-dependency. + + We do this twice, using a regular class and also a dataclass. + """ app = FastAPI() class SubDepClass: pass - class DepClass: + class DepClass: # noqa B903 + """A regular class that has sub-dependencies via __init__. + + Note that this could be a dataclass, but we want to check both + dataclasses and normal classes.""" + def __init__(self, sub: Annotated[SubDepClass, Depends()]): self.sub = sub @app.post("/dep") def endpoint(id: DepClass = Depends()) -> bool: + assert isinstance(id.sub, SubDepClass) + return True + + @dataclass + class DepDataclass: + sub: Annotated[SubDepClass, Depends()] + + @app.post("/dep2") + def endpoint2(dep: Annotated[DepDataclass, Depends()]): + assert isinstance(dep.sub, SubDepClass) return True with TestClient(app) as client: - r = client.post("/dep") - assert r.status_code == 200 - invocation = r.json() - assert invocation is True + for url in ["/dep", "/dep2"]: + r = client.post(url) + assert r.status_code == 200 + invocation = r.json() + assert invocation is True def test_invocation_id(): From aee3b545c7e0edd6c27cf410d36a174893ef6c51 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 12 Aug 2025 15:20:08 +0100 Subject: [PATCH 2/4] Enable Bugbear linter rules and fix errors I've applied lots of minor fixes that shouldn't cause any changes in code behaviour. The only fixes that are likely to affect anything were in the previous commit, related to turning a few things into dataclasses. I have not enforced B008, because it is incompatible with FastAPI Depends() (see comment in pyproject.toml). There's a work-around to use a plugin for flake8 that exempts Depends() specifically, but that won't work for Ruff. --- docs/source/quickstart/counter.py | 2 +- pyproject.toml | 4 +++- src/labthings_fastapi/actions/__init__.py | 12 ++++++------ src/labthings_fastapi/client/__init__.py | 4 ++-- src/labthings_fastapi/client/in_server.py | 4 +++- src/labthings_fastapi/example_things/__init__.py | 2 +- src/labthings_fastapi/outputs/blob.py | 8 ++++---- src/labthings_fastapi/server/__init__.py | 7 +++---- src/labthings_fastapi/server/cli.py | 6 ++++-- .../thing_description/__init__.py | 5 ++--- .../utilities/object_reference_to_object.py | 4 ++-- tests/test_action_cancel.py | 6 +++--- tests/test_action_logging.py | 4 +++- tests/test_base_descriptor.py | 4 ++-- tests/test_example_thing.py | 4 ++-- tests/test_property.py | 4 ++-- tests/test_server_cli.py | 16 ++++++++-------- 17 files changed, 51 insertions(+), 45 deletions(-) diff --git a/docs/source/quickstart/counter.py b/docs/source/quickstart/counter.py index efe22030..10f8a827 100644 --- a/docs/source/quickstart/counter.py +++ b/docs/source/quickstart/counter.py @@ -18,7 +18,7 @@ def increment_counter(self) -> None: @lt.thing_action def slowly_increase_counter(self) -> None: """Increment the counter slowly over a minute""" - for i in range(60): + for _i in range(60): time.sleep(1) self.increment_counter() diff --git a/pyproject.toml b/pyproject.toml index 772475e2..8b3e0a5f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -76,7 +76,7 @@ docstring-code-format = true [tool.ruff.lint] external = ["DOC401", "F824", "DOC101", "DOC103"] # used via flake8/pydoclint -select = ["E4", "E7", "E9", "F", "D", "DOC"] +select = ["B", "E4", "E7", "E9", "F", "D", "DOC"] ignore = [ "D203", # incompatible with D204 "D213", # incompatible with D212 @@ -84,6 +84,8 @@ ignore = [ "DOC201", # doesn't work with sphinx-style docstrings, use flake8/pydoclint "DOC501", # doesn't work with sphinx-style docstrings, use flake8/pydoclint "DOC502", # doesn't work with sphinx-style docstrings, use flake8/pydoclint + "B008", # fails on FastAPI Depends(), + # see https://github.com/fastapi/fastapi/issues/1522 ] preview = true diff --git a/src/labthings_fastapi/actions/__init__.py b/src/labthings_fastapi/actions/__init__.py index 5d51f84a..56e251fa 100644 --- a/src/labthings_fastapi/actions/__init__.py +++ b/src/labthings_fastapi/actions/__init__.py @@ -491,11 +491,11 @@ def action_invocation( try: with self._invocations_lock: return self._invocations[id].response(request=request) - except KeyError: + except KeyError as e: raise HTTPException( status_code=404, detail="No action invocation found with ID {id}", - ) + ) from e @app.get( ACTION_INVOCATIONS_PATH + "/{id}/output", @@ -530,11 +530,11 @@ def action_invocation_output(id: uuid.UUID, _blob_manager: BlobIOContextDep): with self._invocations_lock: try: invocation: Any = self._invocations[id] - except KeyError: + except KeyError as e: raise HTTPException( status_code=404, detail="No action invocation found with ID {id}", - ) + ) from e if not invocation.output: raise HTTPException( status_code=503, @@ -569,11 +569,11 @@ def delete_invocation(id: uuid.UUID) -> None: with self._invocations_lock: try: invocation: Any = self._invocations[id] - except KeyError: + except KeyError as e: raise HTTPException( status_code=404, detail="No action invocation found with ID {id}", - ) + ) from e if invocation.status not in [ InvocationStatus.RUNNING, InvocationStatus.PENDING, diff --git a/src/labthings_fastapi/client/__init__.py b/src/labthings_fastapi/client/__init__.py index c7345869..320c9fd8 100644 --- a/src/labthings_fastapi/client/__init__.py +++ b/src/labthings_fastapi/client/__init__.py @@ -57,8 +57,8 @@ def _get_link(obj: dict, rel: str) -> Mapping: raise ObjectHasNoLinksError(f"Can't find any links on {obj}.") try: return next(link for link in obj["links"] if link["rel"] == rel) - except StopIteration: - raise KeyError(f"No link was found with rel='{rel}' on {obj}.") + except StopIteration as e: + raise KeyError(f"No link was found with rel='{rel}' on {obj}.") from e def invocation_href(invocation: dict) -> str: diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index 2aedf6c6..3db66724 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -266,12 +266,14 @@ def direct_thing_client_class( """ def init_proxy(self, request: Request, **dependencies: Mapping[str, Any]): - f"""A client for {thing_class} at {thing_path}""" + """Initialise a DirectThingClient (this docstring should be replaced).""" # NB this definition isimportant, as we must modify its signature. # Inheriting __init__ means we'll accidentally modify the signature # of `DirectThingClient` with bad results. DirectThingClient.__init__(self, request, **dependencies) + init_proxy.__doc__ = f"""Initialise a client for {thing_class} at {thing_path}""" + # Using a class definition gets confused by the scope of the function # arguments - this is equivalent to a class definition but all the # arguments are evaluated in the right scope. diff --git a/src/labthings_fastapi/example_things/__init__.py b/src/labthings_fastapi/example_things/__init__.py index 3ed50647..def9bb57 100644 --- a/src/labthings_fastapi/example_things/__init__.py +++ b/src/labthings_fastapi/example_things/__init__.py @@ -90,7 +90,7 @@ def slowly_increase_counter(self, increments: int = 60, delay: float = 1): :param increments: how many times to increment. :param delay: the wait time between increments. """ - for i in range(increments): + for _i in range(increments): time.sleep(delay) self.increment_counter() diff --git a/src/labthings_fastapi/outputs/blob.py b/src/labthings_fastapi/outputs/blob.py index 8043df23..420ad7f8 100644 --- a/src/labthings_fastapi/outputs/blob.py +++ b/src/labthings_fastapi/outputs/blob.py @@ -364,11 +364,11 @@ def retrieve_data(self) -> Self: url_to_blobdata = url_to_blobdata_ctx.get() self._data = url_to_blobdata(self.href) self.href = "blob://local" - except LookupError: + except LookupError as e: raise LookupError( "Blobs may only be created from URLs passed in over HTTP." f"The URL in question was {self.href}." - ) + ) from e return self @model_serializer(mode="plain", when_used="always") @@ -398,11 +398,11 @@ def to_dict(self) -> Mapping[str, str]: blobdata_to_url = blobdata_to_url_ctx.get() # MyPy seems to miss that `self.data` is a property, hence the ignore href = blobdata_to_url(self.data) # type: ignore[arg-type] - except LookupError: + except LookupError as e: raise LookupError( "Blobs may only be serialised inside the " "context created by BlobIOContextDep." - ) + ) from e else: href = self.href return { diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 1235d7fc..0ef9abd9 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -210,7 +210,7 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: for thing in self.things.values(): await stack.enter_async_context(thing) yield - for name, thing in self.things.items(): + for _name, thing in self.things.items(): # Remove the blocking portal - the event loop is about to stop. thing._labthings_blocking_portal = None @@ -284,9 +284,8 @@ def server_from_config(config: dict) -> ThingServer: except ImportError as e: raise ImportError( f"Could not import {thing['class']}, which was " - f"specified as the class for {path}. The error is " - f"printed below:\n\n{e}" - ) + f"specified as the class for {path}." + ) from e instance = cls(*thing.get("args", {}), **thing.get("kwargs", {})) assert isinstance(instance, Thing), f"{thing['class']} is not a Thing" server.add_thing(instance, path) diff --git a/src/labthings_fastapi/server/cli.py b/src/labthings_fastapi/server/cli.py index 068490b9..5a1aefbf 100644 --- a/src/labthings_fastapi/server/cli.py +++ b/src/labthings_fastapi/server/cli.py @@ -96,8 +96,10 @@ def config_from_args(args: Namespace) -> dict: try: with open(args.config) as f: config = json.load(f) - except FileNotFoundError: - raise FileNotFoundError(f"Could not find configuration file {args.config}") + except FileNotFoundError as e: + raise FileNotFoundError( + f"Could not find configuration file {args.config}" + ) from e else: config = {} if args.json: diff --git a/src/labthings_fastapi/thing_description/__init__.py b/src/labthings_fastapi/thing_description/__init__.py index 845167a1..a06497f1 100644 --- a/src/labthings_fastapi/thing_description/__init__.py +++ b/src/labthings_fastapi/thing_description/__init__.py @@ -75,9 +75,8 @@ def look_up_reference(reference: str, d: JSONSchema) -> JSONSchema: return resolved except KeyError as ke: raise KeyError( - f"The JSON reference {reference} was not found in the schema " - f"(original error {ke})." - ) + f"The JSON reference {reference} was not found in the schema." + ) from ke def is_an_object(d: JSONSchema) -> bool: diff --git a/src/labthings_fastapi/utilities/object_reference_to_object.py b/src/labthings_fastapi/utilities/object_reference_to_object.py index d95e543c..72ddceb3 100644 --- a/src/labthings_fastapi/utilities/object_reference_to_object.py +++ b/src/labthings_fastapi/utilities/object_reference_to_object.py @@ -26,9 +26,9 @@ def object_reference_to_object(object_reference: str) -> Any: for attr in qualname.split("."): try: obj = getattr(obj, attr) - except AttributeError: + except AttributeError as e: raise ImportError( f"Cannot import name {attr} from {obj} " f"when loading '{object_reference}'" - ) + ) from e return obj diff --git a/tests/test_action_cancel.py b/tests/test_action_cancel.py index 7ec923cf..5e7c5742 100644 --- a/tests/test_action_cancel.py +++ b/tests/test_action_cancel.py @@ -20,7 +20,7 @@ class CancellableCountingThing(lt.Thing): @lt.thing_action def count_slowly(self, cancel: lt.deps.CancelHook, n: int = 10): - for i in range(n): + for _i in range(n): try: cancel.sleep(0.1) except lt.exceptions.InvocationCancelledError as e: @@ -35,7 +35,7 @@ def count_slowly_but_ignore_cancel(self, cancel: lt.deps.CancelHook, n: int = 10 Used to check that cancellation alter task behaviour """ counting_increment = 1 - for i in range(n): + for _i in range(n): try: cancel.sleep(0.1) except lt.exceptions.InvocationCancelledError: @@ -52,7 +52,7 @@ def count_and_only_cancel_if_asked_twice( """ cancelled_once = False counting_increment = 1 - for i in range(n): + for _i in range(n): try: cancel.sleep(0.1) except lt.exceptions.InvocationCancelledError as e: diff --git a/tests/test_action_logging.py b/tests/test_action_logging.py index 86fecf9e..c526e574 100644 --- a/tests/test_action_logging.py +++ b/tests/test_action_logging.py @@ -31,7 +31,9 @@ def test_invocation_logging(caplog): invocation = poll_task(client, r.json()) assert invocation["status"] == "completed" assert len(invocation["log"]) == len(ThingOne.LOG_MESSAGES) - for expected, entry in zip(ThingOne.LOG_MESSAGES, invocation["log"]): + for expected, entry in zip( + ThingOne.LOG_MESSAGES, invocation["log"], strict=True + ): assert entry["message"] == expected diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index e283d438..34e37637 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -161,7 +161,7 @@ def test_basedescriptor_orphaned(): """Check the right error is raised if we ask for the name outside a class.""" prop = MockProperty() with pytest.raises(DescriptorNotAddedToClassError): - prop.name + _ = prop.name def test_basedescriptor_fallback(): @@ -185,7 +185,7 @@ def test_basedescriptor_get(): assert isinstance(Example.my_property, MockProperty) with pytest.raises(NotImplementedError): # BaseDescriptor requires `instance_get` to be overridden. - e.base_descriptor + _ = e.base_descriptor class MockFunctionalProperty(MockProperty): diff --git a/tests/test_example_thing.py b/tests/test_example_thing.py index ab6e454c..14ebe23f 100644 --- a/tests/test_example_thing.py +++ b/tests/test_example_thing.py @@ -49,13 +49,13 @@ def test_thing_with_broken_affordances(): def test_thing_that_cannot_instantiate(): - with pytest.raises(Exception): + with pytest.raises(RuntimeError): ThingThatCantInstantiate() def test_thing_that_cannot_start(): thing = ThingThatCantStart() assert isinstance(thing, ThingThatCantStart) - with pytest.raises(Exception): + with pytest.raises(RuntimeError): with thing: pass diff --git a/tests/test_property.py b/tests/test_property.py index e15ab1be..aad5db5b 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -157,9 +157,9 @@ def test_baseproperty_type_and_model(): # By default, we have no type so `.type` errors. with pytest.raises(tp.MissingTypeError): - prop.value_type + _ = prop.value_type with pytest.raises(tp.MissingTypeError): - prop.model + _ = prop.model # Once _type is set, these should both work. prop._type = str | None diff --git a/tests/test_server_cli.py b/tests/test_server_cli.py index dce81b0e..e2d63f15 100644 --- a/tests/test_server_cli.py +++ b/tests/test_server_cli.py @@ -14,12 +14,8 @@ def monitored_target(target, conn, *args, **kwargs): """Monitor stdout and exceptions from a function""" # The lines below copy stdout messages to a pipe # which allows us to monitor STDOUT and STDERR - for output, name in [(sys.stdout, "stdout"), (sys.stderr, "stderr")]: - - def write_wrapper(message): - conn.send((name, message)) - - output.write = write_wrapper + sys.stdout.write = lambda message: conn.send(("stdout", message)) + sys.stderr.write = lambda message: conn.send(("stderr", message)) try: ret = target(*args, **kwargs) @@ -44,8 +40,10 @@ def __init__(self, target=None, **kwargs): self, target=monitored_target, args=args, **kwargs ) - def run_monitored(self, terminate_outputs=[], timeout=10): + def run_monitored(self, terminate_outputs=None, timeout=10): """Run the process, monitoring stdout and exceptions""" + if terminate_outputs is None: + terminate_outputs = [] self.start() try: while self._pconn.poll(timeout): @@ -84,8 +82,10 @@ def test_server_from_config(): assert isinstance(server, ThingServer) -def check_serve_from_cli(args: list[str] = []): +def check_serve_from_cli(args: list[str] | None = None): """Check we can create a server from the command line""" + if args is None: + args = [] p = MonitoredProcess(target=serve_from_cli, args=(args,)) p.run_monitored(terminate_outputs=["Application startup complete"]) From 691202a2620fe715515e5e1eb9f8b30c4d126469 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 12 Aug 2025 16:48:35 +0100 Subject: [PATCH 3/4] Format and docstring fixes. Flake8 was flagging errors on an init method that was missing type hints and parameter docs. This docstring gets overwritten dynamically anyway, but I've added the docs as they may still be helpful to someone reading the code. --- src/labthings_fastapi/client/in_server.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index 3db66724..d959d650 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -265,8 +265,17 @@ def direct_thing_client_class( This class may be used as a FastAPI dependency: see :ref:`things_from_things`. """ - def init_proxy(self, request: Request, **dependencies: Mapping[str, Any]): - """Initialise a DirectThingClient (this docstring should be replaced).""" + def init_proxy( + self: DirectThingClient, request: Request, **dependencies: Mapping[str, Any] + ): + r"""Initialise a DirectThingClient (this docstring will be replaced). + + :param self: The DirectThingClient instance we're initialising. + :param request: a FastAPI Request option (will be supplied by FastAPI). + :param \**dependencies: Other keyword arguments will be saved as + dependencies. FastAPI will look at the signature (which we will + manipulate below) to determine these. + """ # NB this definition isimportant, as we must modify its signature. # Inheriting __init__ means we'll accidentally modify the signature # of `DirectThingClient` with bad results. From acb1e308dcbc52dd90a8fac4aec0d449b5f78c14 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 12 Aug 2025 18:43:43 +0100 Subject: [PATCH 4/4] Add a comment explaining B008 in pyproject.toml. --- pyproject.toml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 8b3e0a5f..052f0a65 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -84,7 +84,8 @@ ignore = [ "DOC201", # doesn't work with sphinx-style docstrings, use flake8/pydoclint "DOC501", # doesn't work with sphinx-style docstrings, use flake8/pydoclint "DOC502", # doesn't work with sphinx-style docstrings, use flake8/pydoclint - "B008", # fails on FastAPI Depends(), + "B008", # This disallows function calls in default values. + # FastAPI Depends() breaks this rule, and FastAPI's response is "disable it". # see https://github.com/fastapi/fastapi/issues/1522 ] preview = true