-
Notifications
You must be signed in to change notification settings - Fork 2
Adapt to FastAPI changes #216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
82c33e3 to
f872465
Compare
|
Note to reviewers: either of you is sufficient. This does not need double review! |
julianstirling
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wrote this a couple of hours ago but failed to submit it.
|
I'm happy with the code changes but we need to get test passing first |
|
Happy with the code changes - need to get the test passing |
|
It seems #216 is dying because I still don't really understand the logic of pinned dependency tests as a concept, as we need the tests to pass for any version we support. But it seems that only the python version they were created from would be guaranteed to work? |
|
Urgh, this is more of a mess than I thought. I will have a think about the nicest way to keep everything working. I believe I've fixed the only actual problem (that FastAPI gets confused by |
My motivation is to make the best use of limited developer resources by ensuring we don't need to hit a moving target with every PR. Pinning the environment for testing means that updates to upstream packages won't cause test failures that need fixing during a PR: the PR can concentrate on adding a feature or fixing a bug, and once it's done it can be merged. Testing with unpinned dependencies is helpful because it catches problems that arise due to upstream updates - but if that happens when there is an open pull request, we then can't merge until the problem is fixed and the fix is rebased into the feature branch. My compromise was to do both: test with pinned dependencies (and don't merge unless that passes) but also test with unpinned dependencies. I am not sure I ever wrote it down, but my intention was that it should be OK to merge so long as the pinned tests work: if the unpinned tests fail, we will need to log an issue and fix it, but that can be done in a future PR. Accepting that LabThings is not currently replete with developer hours, I think this is a pretty good solution for ensuring that we catch problems, without interrupting development every time something comes up. Clearly we shouldn't make a release if the unpinned tests are failing: perhaps that could even be written into the CI somehow? With all of that said, clearly I do need to get all the tests passing here! It may be that I need to be less eager about updating the pinned dependencies, or perhaps even just restrict pinned dependencies to one Python version. |
The docs suggest specifying a send type is optional, but recent FastAPI errors if it's not there. This fixes the error and does not change any code.
Barecheck - Code coverage reportTotal: 95.01%Your code coverage diff: 0.00% ▴ Uncovered files and lines
|
julianstirling
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. It would be good to understand why the changes to FastAPI caused this just for our own knowledge. But clearly this is the needed fix.
This changes type declarations of
AsyncGeneratorto explicitly includeNonefor the send type. This is required by recent FastAPI versions, which fail with a confusing error if dependencies are typed asAsyncGenerator[<whatever>]. The fix is very simple: explicitly specifyAsyncGenerator[<whatever>, None].The standard library docs suggest either form is equivalent, but clearly there's an implementation detail in FastAPI somewhere that disagrees.
In doing this, I have run into some issues updating
dev-requirements.txtfor all supported Python versions. I'm going to try to fix that in a separate PR, as it's clear it requires more thought than I have given it here. For now, I will leave the pinned dependencies alone: this PR only fixes the FastAPI problem.As part of this, I've upgraded the pinned dependencies used in the tests. I couldn't come up with a set of pinned dependencies that works for all Python versions we currently support (3.10-3.13). I have dropped 3.10 from the maintestjob, and will now pin dependencies on 3.11.Python 3.10 is still supported, and will continue to be tested without pinned dependencies, until we decide to drop support for it. This feels like a reasonable compromise: I don't intend to release without tests passing on 3.10, but I also don't want to hold up development with painful dependency management.