Thing slots to replace Thing dependencies #185
Merged
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
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 kept `ThingServer.connect_things` as simple as possible, by moving error checks into `ThingConnection`.
Fixed a bug that did the wrong thing if a connection was configured to be None, and fixed the remaining test that was failing because of an error I'd not anticipated.
I've covered 100% of `thing_connections.py` with unit tests, with bottom-up tests that don't use the server or `Thing`, as well as more realistic tests that do use `Thing` and `ThingServer`. This uncovered a few issues that I've fixed, in particular with `thing_type` and with error handling logic in `connect`. I've now split `connect` into `_pick_things` (which finds the Thing(s) to connect) and `connect` (which makes sure they match the type hint and supplies context for error messages). I think this is clearer. I think the split also makes it more testable, as I can test the logic in `_pick_things` separately.
FunctionalProperty was overriding `value_type` provided by FieldTypedBaseProperty. The override wasn't typed properly. I've removed it - there is no need to override the base implementation.
I had hoped not to need to ignore these. It would probably be possible if I made ThingConnection non-generic, but I think it is worth keeping that feature: it allows us to specify a subscripted class, if for whatever reason it's better to do that than use a type hint on the attribute. I have added explanation in the docstring as to why I have ignored type checking, and why I think it's the right thing to do.
Types are now evaluated lazily, but we check for the existence of a type hint in __set_name__. This used to happen only for `DataProperty` but now happens for `BaseProperty` too, so I needed to adjust some tests.
This tests the remaining 1 line that was uncovered.
typing.get_type_hints automatically resolves forward references using sensible values for locals and globals. This is used to permit forward references in field-typed descriptors. There's no corresponding way to resolve forward references in type subscripts, so for now I have made this an error - descriptors that are subscripted may not be subscripted with a string. Given that I don't anticipate type subscripts will be used much, this is probably not a major limitation. It could be fixed by implementing our own type evaluator based on `get_type_hints` but I would prefer not to do this if I can avoid it.
An exception was being ignored because of a missing `as excinfo` in a `pytest.raises` block.
This adds an argument to `ThingServer.add_thing` that captures configuration for thing connections.
Pass thing_connections through from configuration.
This was causing two of the tests to fail.
Exceptions raised during class definition are wrapped in a RuntimeError prior to Python 3.12, so I use `raises_or_is_called_by` to check the error. It's not ideal that our error gets wrapped in a RuntimeError, but there's relatively little we can do about it.
I've added an explanation comment with a link to the open pydantic issue.
Contributor
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
I have looked through the code, and read the docs. We have talked through it so I understand what it does, and can see it is tested. However, I do not understand what is happening in the actual code.
Merged
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.
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>
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.
Drop dependencies feedback
Invocation context variables
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 MR introduces a new descriptor,
lt.thing_connection. This provides a way to connect Things together, without using dependencies. As withlt.property, it uses the type hint to determine the type of the Thing it should supply. Most of the time, this is all that's needed:When one Thing of each type is added to a server, the descriptor will automatically be connected to the Thing of the specified type. It's also possible to specify connections, for example if there's more than one Thing of a given type.
There is support for optional connections, which don't cause an error if no Thing is available, and for connections to multiple Things. At the moment, this is only documented in docstrings in the
thing_connectionsmodule, it should probably be a conceptual page.Note: this PR builds on #183 and includes its commits.
Things to do before it's finished:
lt.ThingServer.add_thing