Repository navigation
Conversation
This should silence the pip audit warnings by updating to the most recent version.
Barecheck - Code coverage reportTotal: 97.81%Your code coverage diff: 0.38% ▴ Uncovered files and lines
|
`mypy` now flags my dynamic model creation as incorrectly typed. I believe this is very close to the issue seen in: pydantic/pydantic#3585 The solution there was to use a variable typed as `Any` rather than using the argument directly. As that amounts to a stealthy type: ignore statement, I have opted to use an explicit type ignore, with a justification. I think that's clearer.
I've added "stream" as a message type, and provided "next message" and "close streams by affordance" methods. This should prepare the way for using MessageBroker for MJPEG streams.
The thing server interface now exposes the message broker.
This strips out the pub/sub logic from MJPEGStream in favour of using MessageBroker, as is already done for property notifications. The resulting class should be much simpler. I've also inherited from BaseDescriptor to save duplication in MJPEGStreamDescriptor.
This uses an event to speed up tests by eliminating `time.sleep` and adds some more checks on the data received.
The stream no longer has the notion of being "active" so we can't check for errors there. This might be a feature we want to revive - I'm not totally sure.
This is tested by a better-isolated test that mocks the server and uses a real MessageBroker.
The async generator actually works quite well with the new stream based structure, so I've resinstated it. I've taken the opportunity to rename it, and return Frame objects, not just bytes. The old name remains for backwards compatibility. This shrinks the code changes slightly, I think, and should also make it neater for client code as there's less need to create/use/type check streams directly. I've added explicit tests for the new functions in message broker.
2a7182c to
8d79b7f
Compare
|
Testing this against the OFM simulator, I can see we need to do something in the log: currently it looks like it drops a message when a client disconnects from the stream, and that results in the bytes being dumped to the log/console, which is not desirable. Also I managed to crash the simulator - I'm not sure if that's related to this change or not, but it bears investigating at least a little further. |
The old MJPEG stream silently dropped frames. We were logging every dropped frame at WARNING. I've downgraded this to DEBUG for streams. We may choose to turn it off entirely to replicate the old behaviour. I have also altered the `repr` of `Frame` objects, to avoid dumping JPEG data to console/log accidentally.
|
I have fixed the log issue in two ways:
I have not been able to reproduce the crash: what I can say is that my whole WSL2 environment crashed (including two different VSCode sessions) so it's possible that it got shut down for overloading my computer or something. The noise made by my fans when the simulator spins up suggests that is not unreasonable! |
|
One point to note: currently, stand are being made with a |
This PR strips out the custom pub/sub logic from
MJPEGStreamand replaces it with a couple of calls toMessageBroker. It goes further than #372 by actually using theMessageBrokerobject rather than just pinching a function from it. I think this results in a bigger improvement, from a smaller code change.I hope this makes the code much more readable and much more testable.
The diff has more additions than deletions - but the deletions are (complicated) code while the additions are docstrings, comments, tests, and calls to much clearer code.
The pub/sub logic in MessageBroker is tested much more carefully than the old logic was, even though it was technically covered before.
I have written a fixture that lets me write async tests for the
MJPEGStreamindependent of the server. It usesMessageBrokerbut not the server or FastAPI.This PR passes both the old tests, and the relevant bits of the improved tests in #372, which are included in this PR.
OFM-Feature-Branch: v3-mjpeg-stream-refactor
This MR contains the following
Before merge:
Merge checklist:
docs/src/public_api.rst.New or changed features have been added to (or updated in) the conceptual documentation.Any new or updated dependencies have been added to the project configuration (e.g.,pyproject.toml).Any decrease in test coverage has been justified.test-against-ofm-v3job passes, ortest-against-ofm-feature-branchpasses.New features have been used in a branch of the OpenFlexure Microscope, which is tested intest-against-ofm-feature-branch.