Deprecation warnings for v0.0.12
#209
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
Barecheck - Code coverage reportTotal: 95.09%Your code coverage diff: 0.43% ▴ Uncovered files and lines |
All the functions that have been added to will be removed in v0.0.13.
The `old_dependency_tests` now ignore deprecation warnings for v0.0.13, as those tests will be removed for that release. I've also changed the stacklevel on one warning to ensure it is raised on calling code, not in a dependency module.
The filter in pyproject.toml didn't do what I intended, so it's removed. I have used `pytestmark` on all the old dependency test modules instead.
We need to keep using an InvocationID dependency when starting actions, otherwise all of the Invocation-related dependencies will break. This commit adds an internal dependency that keeps everything working, but won't raise an error on every action call. It also removes a spurious CancelHook dependency, that isn't needed.
DirectThingClient will be removed: tests that make use of it will raise warnings. We've retained enough tests to make sure it stays working for v0.0.12, but tests to make sure other things work properly for DirectThingClient can be removed now.
I've retained tests for the dependencies, to make sure they don't break before they are removed in v0.0.13. I've marked the tests to filter out the warnings, but I also needed to manually suppress the DeprecationWarning in the test set-up code.
The only functions that needed `ActionManagerContextDep` already had access to a `Thing` object, so I've added a property to `ThingServerInterface` that allows us to access the action manager that way. This eliminates deprecation warnings related to the dependency.
The BlobDataManager dependency (which is only ever used internally) used dependencies to obtain the ThingServer, and extracted the BlobDataManager from it. This is now deprecated: Things can access the server via the ThingServerInterface. The simplest fix is to make a global BlobDataManager object. This simply allows BlobData objects to be retrieved by UUID. In the future, it may become part of a `BlobData` base class, similar to `CancelEvent`. This is only a change in behaviour if there is more than one `ThingServer` per Python instance. Even then, the only effect would be that blobs from one server could be accessed from the other server. That's not a supported scenario, and is very unlikely to lead to errors anyway - UUIDs should be unique, so there's no danger of blob IDs colliding.
This is also added to `MockThingServerInterface`. I've not attempted to simulate functionality: accessing the `action manager` on a `MockThingServerInterface` simply raises `NotImplementedError`.
rwb27
force-pushed
the
deprecation-warnings
branch
from
98f9608 to
5198d96
Compare
This dependency is no longer needed, so I've removed the vestigial code.
rwb27
force-pushed
the
deprecation-warnings
branch
from
103cc94 to
837b3ec
Compare
julianstirling
approved these changes
Nov 21, 2025
Contributor
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
A few minor points, but overall this is fine.
Co-authored-by: Julian Stirling <julian@julianstirling.co.uk>
This property was only ever intended to be used by LabThings code, so ought to be private. It is also quite likely to be removed in the future, another good reason for a leading _.
Collaborator
Author
Thanks very much - agreed on the docs and I've noted that in #212. Good shout on |
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 adds deprecation warnings for
v0.0.12.Adding the warnings resulted in many, many warnings for the test suite, so I've needed to check and filter out the warnings that are related to test code that will be deleted when dependencies are deleted. This is more or less entirely in
tests/old_dependency_tests, with one test intest_docsthat will also go once the dependency documentation is removed.Some of the deprecated dependencies were being used by internal code, which I've modified:
Action Manager
ActionManagerContextDeprelied on some of the deprecated functions. However, its function was to supply theActionManagerassociated with the current server - and theThingwas already available to the functions that used it. I now retrieve theActionManagerfrom theThingServerInterfaceof theThingwhich means no dependency is needed.I've deleted
labthings_fastapi.dependencies.action_managerbecause it was only used internally and is no longer needed. I've searched the OFM codebase to double-check that it's not used there.Blob Serialisation
BlobIOContextDepretrieved theBlobManagerfrom the server, which required a dependency.There's no real reason not to keep a global register of
BlobDataobjects rather than a per-server one: they are indexed by UUID which will be unique globally. I've now made a single, globalBlobDataManagerobject, and eliminated the use offind_server_from_request. This means that theThingServerno longer holds its own reference to a blob data manager, and it is simply referenced from the module-level global when it's needed.It would be nice, in due course, to eliminate
BlobIOContextDepentirely, but that's for another PR.Incidental changes
In doing this, I spotted some uncovered lines in
outputs.bloband have improved the tests. More work is needed here, noted in #210.Todo
Before merging, I should:
ThingServerInterface.action_manager.MockThingServerInterface.action_manager(probably raise aNotImplementedErrorfor now).Closes #203