-
Notifications
You must be signed in to change notification settings - Fork 5
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
[REF] Refactor startup/shutdown events to lifespan events #407
Conversation
…espan events * Replaced deprecated startup and shutdown events with lifespan events. * Improved environment variable validation and vocabulary loading during app lifecycle. * Ensured compatibility with FastAPI’s modern event handling approach.
Reviewer's Guide by SourceryThis pull request refactors the application's startup and shutdown processes by replacing the deprecated startup/shutdown events with lifespan events. It also centralizes environment variable validation, authentication checks, and vocabulary loading/cleanup within the application lifecycle. Sequence diagram for application startup with lifespan eventssequenceDiagram
participant FastAPI App
participant validate_environment_variables
participant check_client_id
participant initialize_vocabularies
activate FastAPI App
FastAPI App->validate_environment_variables: Call
activate validate_environment_variables
validate_environment_variables-->>FastAPI App: Return
deactivate validate_environment_variables
FastAPI App->check_client_id: Call
activate check_client_id
check_client_id-->>FastAPI App: Return
deactivate check_client_id
FastAPI App->initialize_vocabularies: Call
activate initialize_vocabularies
initialize_vocabularies-->>FastAPI App: Return
deactivate initialize_vocabularies
deactivate FastAPI App
Sequence diagram for application shutdown with lifespan eventssequenceDiagram
participant FastAPI App
participant cleanup_temp_vocab_dir
activate FastAPI App
FastAPI App->cleanup_temp_vocab_dir: Call
activate cleanup_temp_vocab_dir
cleanup_temp_vocab_dir-->>FastAPI App: Return
deactivate cleanup_temp_vocab_dir
deactivate FastAPI App
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hi @KhushiRajurkar,
Thanks for your PR! The CI tests reproduced 2/5 of the failures you mentioned encountering in the issue: https://github.com/neurobagel/api/actions/runs/13127168535/job/36625878480?pr=407
These are due to your code changes breaking assumptions of our existing unit tests of the app startup checks, which can be fixed by updating either the tests or your code - see my detailed comments below.
As for the other 3 test failures you reported, these are integration tests (marked with @pytest.mark.integration
, see example here), which means that you need a synthetic Neurobagel node running in order to run the tests. The httpx.Connect
errors you encountered suggest that in the integration tests, requests involving the synthetic node are failing, so the node may not have been set up correctly in your local environment.
More details about the integration test setup are in our README, but actually, I'd recommend simply running the test suite using:
pytest
which should skip the integration tests by default. If you're developing in a Docker container as opposed to a normal Python environment, you may need extra steps to ensure that the tests can reach the test node (which is its own Docker Compose stack). This may not be worthwhile for the purposes of your PR, which shouldn't affect the integration tests. As a sanity check, you can see that the same integration tests which are failing for you locally are also running and passing in the CI tests on this PR :)
I've left some other comments below, including suggestions for more detail in the new docstrings. Have a look and let me know if you have questions.
Hello @alyssadai, Thank you for the detailed review and guidance! |
Sounds great @KhushiRajurkar - please mark your PR as ready to review once you have made the changes! |
Hi @KhushiRajurkar, just checking in: do you have any questions about addressing the review comments? Please feel free to let us know if you no longer have capacity to work on this issue, so that one of the maintainers can take over for your PR. |
Hello @alyssadai, I believe the issue might be related to my local system environment. As a workaround, I had planned to run the tests in Docker as a final approach before pushing the changes. I have modified the following files:
I’ll proceed with the Docker approach and push the updates soon. Please let me know if there are any alternative suggestions or if you’d like me to take a different approach. Thank you! |
Hi @KhushiRajurkar, I wouldn't recommend running the tests in Docker, as that will be difficult for us to replicate on our end (our CI tests run just in a bare Python environment). Instead, I'd suggest first committing and pushing your current changes to this branch (our CI tests will run on them so we can see directly in this PR if there are any tests still failing), and then setting up a fresh Python virtual environment locally to ensure that you have the correct dependencies and linters as defined for this repo. For example, from the root of this repo: python -m venv venv # ensure you have Python 3.10+
pip install -r requirements.txt
# following https://github.com/neurobagel/api?tab=readme-ov-file#testing
git submodule init
git submodule update
# following https://neurobagel.org/contributing/CONTRIBUTING/#follow-repository-code-style
pre-commit install
pre-commit run then, you can try running the tests again using the command Let me know if that works. For us to better track progress on your PR, I'd encourage you to always push your changes first! |
Hello @alyssadai Regarding pytest, I had encountered an issue while running the command: Thank you for your patience and support! |
- Restored previously deleted docstrings - Fixed spacing issues in the `validate_environment_variables` - Changed raised error from `ValueError` to `RuntimeError` in `security.py` - Updated corresponding assertion in `test_security.py`
Added a comment that was previously deleted
Revert deletions of comments in `overridden_swagger` and `overridden_redoc`
Hello @alyssadai, I've committed and pushed the changes in PR #407. Please review my changes and provide your valuable feedback. Thank you! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #407 +/- ##
==========================================
+ Coverage 97.10% 97.24% +0.13%
==========================================
Files 24 24
Lines 830 835 +5
==========================================
+ Hits 806 812 +6
+ Misses 24 23 -1 ☔ View full report in Codecov by Sentry. |
Revert deletion of docstring in the `root` function
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.
Hi @KhushiRajurkar, thanks for the changes! The tests are now passing 🎉
I left a few final minor suggestions below. I also notice that the pre-commit check is still failing, which I'm assuming may be related to your difficulties installing the dependencies locally.
Since this PR is close to ready, for the sake of being able to merge it soon (we have a couple PRs being worked on in parallel, and we don't want them to get too out of sync and run into merge conflicts), I will apply the remaining changes directly and run pre-commit locally so that we do not merge with the failing check.
Regarding pytest, I had encountered an issue while running the command:
pip install -r requirements.txt
I was unable to install the dependencies, which prevented me from running pytest in the virtual environment. I'll attempt to resolve this after pushing my changes.
If you are still having trouble with the dependencies, could you please open an issue or reach out to us on Discord with the exact error message you encountered, so we can better help you debug it? Without seeing the error, it is hard to know whether the issue is related to your Python installation or something else about your dev setup.
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.
Ready to merge! 🧑🍳 Thanks again @KhushiRajurkar for the contribution!
As mentioned above. please feel free to open an issue or message us directly on Discord for further help debugging the issue you encountered with installing the requirements.
Hello @alyssadai, |
Thank you, @alyssadai, for the opportunity to contribute to Neurobagel and for your guidance and support throughout this PR! 🙇🏻♀️ This experience has helped me grow as a programmer, and I’m grateful for the chance to learn and improve! |
Changes Made
main.py
.Issue Reference
Notes
Changes proposed in this pull request
✅ Updated FastAPI event handling to lifespan events.
✅ Ensured vocabulary loading and cleanup occur within the app lifecycle.
✅ Maintains compatibility with existing API functionality.
Summary by Sourcery
Migrates the application from deprecated startup/shutdown events to FastAPI lifespan events. This change improves the management of the application lifecycle, including initialization and cleanup tasks such as environment variable validation and vocabulary loading.
Bug Fixes:
Enhancements: