Drop dependencies #194
Merged
Drop dependencies #194
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In order to improve the Thing lifecycle, we're going to move to having the server instantiate the Thing. This will allow us to inject arguments to `__init__` that simplify the set-up process. This commit changes `add_thing` to accept a class rather than an object, and adds `args` and `kwargs` parameters to enable values to be passed to `__init__`. This commit swaps the `path` for a `name` (in all known cases this simply removes the leading `/` from the string) in preparation for making a distinction between names and paths. This commit does not change `Thing` at all: that will come later. It also doesn't change the test code - that will touch many more files!
This should help catch code that called the old version of add_thing more easily. Also, we no longer want `/` in the thing names, so I've removed that tfrom the regex.
For now, DirectThingClient should locate Things by name. It will be removed in due course, once thing connections are implemented.
I've taken this opportunity to move nearly all of the server set-up into fixtures. It would be nice to deduplicate this code in the future, but I think it's already neater than it was. Some tests use things that are not connected to a server. These will require mocking in the future.
This updates the code that's included as separate python files. I will also do a search for `.add_thing` and update docstrings.
I can't believe there are only two mentions - but these are the only two I can find so far.
This now consistently defines the arguments and return type.
There were #noqa directives left over from when we were double-checking `D` codes with flake8 and ruff. Ruff works better, so the #noqa is no longer needed.
Each Thing is now provided with a ThingServerInterface which allows it to interact with the server. The ThingServerInterface is specific to each Thing, and is how the Thing obtains its name and path. This removes the need to pass around the BlockingPortal: the ThingServerInterface provides a method to call async code in the event loop on the server. I hope this is clearer. The ThingServerInterface takes care of making sure we're connected to a server: I've therefore been able to remove a lot of error-handling code and tests for NotConnectedToServerError. This will need to be tested for explicitly, on the ThingServerInterface.
This came to light during testing of the new ThingServerInterface.. There's no reason errors setting up the logger should be swallowed by a catch-all exception: errors there are a bug in LabThings. Also, setting up the logger after emitting an event meant that an exception raised there caused another exception because of the missing logger. I have moved the creation of the logger out of the `try:` block to fix both these issues.
I now reference ThingServerInterface rather than BlockingPortal when describing how to run async code in the event loop.
We no longer need `get_blocking_portal` and can use `ThingServerInterface` instead. This change in `MJPEGStreamDescriptor` will require code that adds frames to delete an argument.
Some tests (specifically those that spin up an HTTP server) are much slower than the rest. I have marked these as "slow" so the test suite may be run with `-m "not slow"` when repeatedly re-testing. Obviously we should run all the tests in CI, this is a convenience feature for development.
This removes the blocking portal argument, that's no longer needed.
Now that we require a ThingServerInterface to create a Thing, I have provided a convenience test harness, `create_thing_without_server()` to consistently and cleanly allow `Thing` subclasses to be instantiated in test code. This auto-generates a name and path for the Thing, and mocks the required functionality. I have removed some tests that were checking for conditions where the Thing was not connected. Things are now created in a connected state, so this isn't needed any more. I should add tests for ThingServerInterface, particularly to make sure that it raises sensible errors when the event loop is not running. That's for a future commit.
This is a full, bottom-up set of tests for ThingServerInterface, as well as the associted mock/test harness code.
I've added a couple of ignores, for thing instantiation. Given that this is very much dynamically typed code, I don't think it loses anything.
This change makes it possible to load settings during __init__, though it does not yet do so. As the settings path is always available, we can remove some error-checking code that handled the period between the Thing being created and it having a settings path to use: this period no longer exists.
I had the same logic in `ThingServer.path_for_thing` and also `ThingServerInterface.path`. I've now called the former from the latter. I've then duplicated the logic again in `MockThingServerInterface` but I am less concerned about that, as duplication in a mock is not such a problem. There is a test to check the mock server interface generates a sensible path, and a test that asking for the path of a Thing that's not on the server raises an error.
I'm going to re-use the field-like type annotations for thing_connection, so it makes sense to move this into BaseDescriptor.
This implements a descriptor that will be used to connect Things together. It will need logic in the ThingServer before it is useful.
This commit adds the minimal set of features needed to use thing_connection in Thing code: * defines a function `thing_connection()` to make the class (primarily so we can type it as `Any` to avoid type clashes with field-style type annotation - see the related discussion for `property` in the typing notes section of the docstring). * adds a function to make thing connections in the server. * expose `thing_connection` in the API. * add a `name` property to `Thing` for convenience.
This checks that two things can be connected to each other. In writing this test, I realised my initial assumption that it would be easy to have circular dependencies and that these would not cause a problem wasn't true. See next commit for my solution to the circular dependency problem. The test covers the expected functionailty and anticipated errors in what I hope is a fairly bottom-up way.
The previous implementation of dataclass-style field typing (i.e. annotating class attributes like `foo: int = lt.property()`) used `typing.get_type_hints()`. This was clean and readable but wasn't very efficient (it retrieves every type hint every time) and un-string-ized the annotations immediately. I have switched to an implementation that is lower-level (using `inspect.get_annotations` and manually traversing the MRO) which has two key benefits: * We only evaluate the one type hint we care about each time. * The type hint is lazily evaluated as it's needed. The latter is important: it means that annotations given as strings or modules using `from __future__ import annotations` will work as expected. In implementing and type checking this I realised that it's far simpler to have both types of `Property` inherit all the typing behaviour (rather than trying to have `FunctionalProperty` inherit only part of it). This changes a few things, none of which I believe matter in code using the library: * Field-style type hints are now checked for functional properties. Previously, they were just ignored. Now, we'll check them and raise an error if a type is specified that disagrees with the supplied function This means errors will appear in some places where they'd have been ignored, but the behaviour for valid code is unchanged. * Types must always wait for the descriptor to be added to a class before they work. This mostly means tests that used bare `FunctionalProperty` instances had to be updated to put these in the context of a class (any class will do)..
`typing.get_type_hints` is the recommended way of getting the type hints for a class. Using this during `__set_name__` immediately evaluates any string annotations, i.e. it makes it impossible to do forward references. I'd previously rolled my own, using `__annotations__` and `eval` but this flagged security warnings with the linter, and misses some subtleties that are present in `get_type_hints`. While most of those subtleties aren't needed, I am prioritising code clarity over performance: instead of lazily evaluating the string type hint, I am just calling `get_type_hints` at a later point. This means there's one less bit of my code to go wrong.
It's now possible to specify the type as a Mapping or Union, in order to either permit multiple Things, or allow None.
I've expanded this docstring as suggested.
I've also fixed the import of `os` - I was previously importing `os.path` but using `os`, now I import `os` directly.
I've imported symbols directly and eliminated `from labthings_fastapi import <module> as <shortname>`. This ought to make the tests a bit more readable.
I've deleted a chunk of text that was in a documentation file, but didn't show (it was deliberately hidden). This may or may not get added back in somewhere else in due course, but I think it's confusing to leave it in its current state.
This was a utility function for testing, but it wasn't clear or helpful. I've replaced the use of this function with more direct tests.
The module under test was renamed, so I'm renaming the test module to match.
Collaborator
Author
|
@beth thanks for your review, I think I've addressed everything but please let me know if not. I ended up applying your suggestions locally rather than via the web interface so they might not show as applied (I'd responded to some of Julian's feedback so the diffs were outdated) - but they are all in the PR now, one commit per comment. |
Co-authored-by: Beth <167304066+bprobert97@users.noreply.github.com>
Co-authored-by: Beth <167304066+bprobert97@users.noreply.github.com>
Co-authored-by: Beth <167304066+bprobert97@users.noreply.github.com>
@bprobert97 :) |
I've removed a test that mixed old and new syntax (the code it tests is tested elsewhere), and added a README to explain why we have a folder of almost-duplicated tests.
bprobert97
approved these changes
Nov 13, 2025
This was referenced Nov 13, 2025
This was referenced Nov 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes all the commits from #183, #185, #187, and #191. Those four PRs form a linear history, they're just broken up to provide a bit more explanation of each set of related changes.
The Goal
Dependency injection is a cool FastAPI feature that I really like, but it really confuses everyone else who has used LabThings. Removing the need for them would simplify code that uses LabThings, make it easier to work with
Thinginstances directly, eliminating the need forDirectThingClient, and vastly reduce the number of arguments of many functions. This is especially helpful when large actions are split into multiple smaller actions.The implementation
There are three distinct changes needed to get rid of dependencies:
Things. This means resources that come from the server (BlockingPortal,MetadataGetter) may be requested asself._thing_server_interface.get_thing_states()etc. without dependencies. This is implemented in Server instantiates things #183. Prior to this, the only way to access theThingServerwas using a dependency.Things easier to access usinglt.thing_slotas introduced in Thing slots to replace Thing dependencies #185 (and renamed in Drop dependencies feedback #191).In implementing these, there were a few more changes that made sense:
Things. This reduces the number of phases of theThinglifecycle we need to think about: the server interface is available duringThing.__init__andThings are connected together as soon as they are all instantiated inThingServer.__init__(implemented in Drop dependencies feedback #191).FieldTypedBaseDescriptor(rather than being a part ofDataProperty). It's also now possible to use deferred type hints/string type hints, and they won't be resolved until required. This is possibly the most complicated bit of code in the PR, though it's mostly moved fromDataPropertywith some changes to work with deferred type hints.This PR is the one we should approve and merge once we're ready. The others are intended to give more detail about the code changes.