fix(serve): ensure a single event loop for python 3.11#1528
Conversation
WalkthroughAdds KeyboardInterrupt handling to the NAT CLI start command and refactors the FastAPI front-end startup from blocking Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1779e43 to
588fe18
Compare
|
/ok to test 588fe18 |
yczhang-nv
left a comment
There was a problem hiding this comment.
I like the fix - well-scoped and easy to understand
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/nat/front_ends/fastapi/fastapi_front_end_plugin.py`:
- Around line 202-218: The code currently constructs uvicorn.Config and calls
server.serve() which ignores multiprocess/reload; add a pre-check in the method
that builds the uvicorn.Config (the code block that reads self.front_end_config
and creates uvicorn.Config / server) to validate that if
self.front_end_config.workers > 1 or self.front_end_config.reload is True and
use_gunicorn (or the flag that controls using Gunicorn) is False, you reject the
combination: raise a clear ValueError (or log an error and exit) explaining that
workers>1 and reload=True require the Gunicorn/supervisor path and instruct the
user to enable use_gunicorn or set workers=1 and reload=False; reference
self.front_end_config.workers, self.front_end_config.reload, and the
uvicorn.Server(config).serve() call so the guard runs before creating
uvicorn.Server.
Signed-off-by: Will Killian <wkillian@nvidia.com>
588fe18 to
7661b64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/nat/front_ends/fastapi/fastapi_front_end_plugin.py`:
- Around line 214-217: In the except KeyboardInterrupt block that wraps await
server.serve(), replace the logger.info call with logger.exception (or
logger.error with exc_info=True) so the KeyboardInterrupt stack trace is
recorded; locate the try/except around server.serve() in
fastapi_front_end_plugin and change the logging call in the except
KeyboardInterrupt: handler to logger.exception("Received interrupt, shutting
down FastAPI server.") to preserve exception details.
|
/merge |
This PR ensures that a single event loop is in use at all times when issuing `nat serve` - catch KeyboardInterrupt cleanly to reduce error propagation - manually start the server rather than call `uvicorn.run()` Closes ## By Submitting this PR I confirm: - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing/index.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. ## Summary by CodeRabbit * **Bug Fixes** * Improved graceful shutdown handling across startup paths to properly catch interrupts, log the interruption, and ensure clean termination of the application and web front-end. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> Authors: - Will Killian (https://github.com/willkill07) Approvers: - Yuchen Zhang (https://github.com/yczhang-nv) URL: NVIDIA#1528
Backport the fix from PR NVIDIA#1528 (commit 2d1230b) to the release/1.4 branch. Python 3.11 introduced stricter event loop requirements. When running 'nat serve', users encounter this error: Error: Runner.run() cannot be called from a running event loop This happens because uvicorn.run() tries to create a new event loop when one already exists from asyncio.run(). The fix changes how we start the uvicorn server: - Use uvicorn.Config + uvicorn.Server instead of uvicorn.run() - This lets us await server.serve() instead of blocking - Add KeyboardInterrupt handling for clean shutdown This maintains the same behavior while working correctly with Python 3.11's event loop restrictions. Tested on develop branch in PR NVIDIA#1528. This is a clean backport with no modifications to the original fix. Fixes NVIDIA#1554 Signed-off-by: Brandon Ledden <bledden@users.noreply.github.com>
Description
This PR ensures that a single event loop is in use at all times when issuing
nat serveuvicorn.run()Closes
By Submitting this PR I confirm:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.