Make use of url_for_middleware to tidy up Blob and Invocation URLs
#244
Conversation
rwb27
force-pushed
the
refactor-blob
branch
2 times, most recently
from
18da201 to
ba1e4b3
Compare
Barecheck - Code coverage reportTotal: 96.42%Your code coverage diff: 0.11% ▴ Uncovered files and lines |
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
It may be my misunderstanding of the MR, but I am worried that actions that return blobs will be saving each blob to the blob data manager each time the blob is created. If this means this data isn't garbage collected then this will mean a possible memory leak in a long running action that calls an action that returns a blob many times.
If this is not the case some further comments or docs might be needed where the comments are
This commit makes use of the new `url_for` middleware to eliminate the Blob-specific context variables. BlobData objects are now added to a singleton BlobManager when they are created, and the URL is filled in at serialisation time. This is a slight simplification of the old behaviour, but it's equivalent in all the ways that matter.
Having now learned more about custom types in pydantic, I've done some more tidying here: * Blob is no longer a BaseModel subclass. I've separated out the model (used for serialisation/validation) and the class that user code will interact with. * BlobData is now a base class not a protocol, and there's a subclass for remote blob data that downloads on demand. This removes most of the complicated logic from `Blob` around when we do and don't need a `BlobData`: a `Blob` is **always** backed by `BlobData` whether it's local or remote. This also means we can get rid of `ClientBlobOutput` and just use `Blob` instead.
This now correctly tells clients the media type, and uses a descriptive title. I believe it's now at least as good as the old schema.
We can now use one Blob class for client and server :) I realised we had the potential to have inconsistencies between BlobData and the host Blob in the media type. We now check the types match, and allow the BlobData to override the Blob's default if it's a matching but more specific type. I've also take a pass through the blob documentation to update it where needed. Happily, as this PR only touches implementation details, not much has changed.
I got rid of the conversion of "*" to None, I think it's clearer this way. I also fixed a typo and ignored a codespell false positive.
The `codespell:ignore` directives made lines too long, and I'm happy that "ser" is an abbreviation we are stuck with.
This gets full coverage of `blob.py` and checks a few things that weren't tested directly. I actually don't quite understand why coverage thought we weren't downloading the data as I'm fairly sure that was done already - but it's no bad thing to have an explicit test that doesn't go via the ThingServer.
This adds a sequence of events for blob serialisation,
BlobManager was confusingly named: it kept track of blobs but did not manage them. I have kept the weak value dictionary of blob data, but now it's a class variable on `LocalBlobData` which feels more appropriate. I have replaced the blob manager with a FastAPI router, which is a cleaner way to add the download_blob API route.
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
I'm happy with the changes. I think the endpoints for blobs rather than the manager is clearer.
The CI is failing, but this seems to just be Flake8 complaining about a docstring or two. The base_coverage gob passed, so tests are fine. This should be ready once the docstring is fixed.
Thanks - should be done now and I'll merge when it passes. |
I am confused by this - it's possible `dmypy` confused me into changing it, but I have now changed it back and it's passing mypy. This line should be fixed by #258 in any case.
2 participants
This PR makes use of the changes in #242 to serialise
Blobs more neatly. It contains those commits and should be merged afterwards.I refactored
Blobmore significantly, making a number of under-the-hood changes that I think make it much clearer. One of the early commits includes a working version ofBlobthat usesurl_forbut hasn't yet been refactored. However, I don't think there's much point reviewing two versions of the same thing, hence not splitting the PR.Blobis no longer aBaseModelsubclass. Instead, it's a regular class that also works as apydantictype, just likeURLFor.Blobmay now refer to local (bytesor file) or remote (URL) data, withBlobDatasubclasses for each.ClientBlobOutputis gone, we can useBlobinstead. That eliminates a whole module and is a step towards client and server types matching.Protocolin favour of base classes - I don't see many (if any) reasons to add newBlobDatatypes, and if we do I don't see any reason they can't also inherit fromBlobData.Blobobjects now serialise properly when returned from any endpoint in the server. Serialising them in other contexts requires the use oflabthings_fastapi.testing.use_dummy_url_for.I've also removed the need to pass
requesttoInvocations when generating their response - we can useURLForto generate the URLs that need to show up in the output.This ended up being a larger change than I intended, but I think it results in a cleaner structure, and provides
pydanticfunctionality in a way that doesn't mess up the structure of classes that client code needs to use.