Skip to content

refactor: replace deprecated on_event with lifespan context manager#46

Open
VishalDhariwal wants to merge 2 commits intosugarlabs:mainfrom
VishalDhariwal:refactor-lifespan-deprecation
Open

refactor: replace deprecated on_event with lifespan context manager#46
VishalDhariwal wants to merge 2 commits intosugarlabs:mainfrom
VishalDhariwal:refactor-lifespan-deprecation

Conversation

@VishalDhariwal
Copy link

This PR migrates the application's startup logic from the deprecated @app.on_event("startup") decorator to the modern lifespan context manager.
According to the FastAPI 0.93.0+ and Starlette documentation, on_event and app.add_event_handler are now deprecated. The lifespan approach is the recommended way to handle startup and shutdown as it provides a cleaner way to manage the state and shared resources (like database sessions) across the app's lifecycle.

Changes Made
Introduced lifespan logic: Created an asynccontextmanager to encapsulate the database synchronization (sync_env_keys_to_db) and logging.
Modernized App Initialization: Updated the app configuration to use the lifespan parameter.
Improved Error Handling: Added a basic try/except block around the startup routine to ensure initialization failures are logged clearly.

main.py Outdated
logger.info(f"Starting Sugar-AI on port {port}")
uvicorn.run(app, host="0.0.0.0", port=port)

uvicorn.run(app, host="0.0.0.0", port=port) No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should remove the no new line at EOF, you can do so with echo >> filename.

If you view your changes with git diff before making a commit, you'll detect issues like this and can resolve it before committing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Done! I've added the missing newline at the end of the file. Thank you for the suggestion of echo >> filename and for the tip on using git diff to catch these issues before committing. I'll make sure to include that in my workflow moving forward!"

@AjayBandiwaddar
Copy link

Hi, I tested this locally and the lifespan approach works correctly.
The deprecation warning is gone after this change. Hope this helps
with the review!

@VishalDhariwal
Copy link
Author

Thanks for testing this locally!
The lifespan implementation has been updated and all previous review comments have been addressed (including the EOF newline fix). Looking forward to your approval.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants