feat: request migration for trtllm - #5599
Conversation
b63c680 to
7b9f735
Compare
WalkthroughThe changes implement graceful shutdown event coordination across the TensorRT LLM runtime. An asyncio.Event is threaded through initialization and handler configuration to enable coordinated shutdown signaling during request generation and cancellation handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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. 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 |
kthui
left a comment
There was a problem hiding this comment.
LGTM! Minor comment on the redundant try...except block.
Will let @tanmayv25 to give the final check mark on TRT-LLM, as updated offline that he will be able to review by the EOD.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/src/dynamo/trtllm/request_handlers/handler_base.py`:
- Around line 182-199: The comment incorrectly claims
context.async_killed_or_stopped() is "already a Future"; it is an async def and
returns a coroutine, so change the comment and wrap that coroutine in a Task
before passing to asyncio.wait: replace the current cancellation_future =
context.async_killed_or_stopped() with creating a task via
asyncio.create_task(context.async_killed_or_stopped()) (keep the variable name
cancellation_future or rename to cancellation_task if you prefer), ensure
shutdown_event handling still creates shutdown_task via
asyncio.create_task(self.shutdown_event.wait()), and pass these tasks to
asyncio.wait; update the misleading comment to state that the coroutine is
wrapped into a Task for compatibility with asyncio.wait.
🧹 Nitpick comments (2)
components/src/dynamo/trtllm/main.py (1)
141-146: Store the task reference to prevent potential garbage collection.The task created by
asyncio.create_task()should be stored to prevent it from being garbage collected before completion. While this is in a shutdown context, storing the reference is best practice.♻️ Suggested fix
+ _shutdown_task = None + # Set up signal handler for graceful shutdown def signal_handler(): # Schedule the shutdown coroutine instead of calling it directly - asyncio.create_task(graceful_shutdown(runtime, shutdown_event)) + nonlocal _shutdown_task + _shutdown_task = asyncio.create_task(graceful_shutdown(runtime, shutdown_event))components/src/dynamo/trtllm/request_handlers/handler_base.py (1)
252-259: Consider logging unexpected exceptions instead of silently ignoring them.The
except Exception: passblock will silently swallow any unexpected errors from the monitoring task, which could hide bugs. Adding debug-level logging would aid troubleshooting without changing the control flow.♻️ Suggested improvement
else: # Task completed, check if it was due to shutdown try: monitor_task.result() except GeneratorExit: raise except Exception: - pass + logging.debug( + f"Monitor task completed with unexpected exception for context {context.id()}", + exc_info=True, + )
|
Are these shutdown changes being tested somewhere? |
Right now, we rely on the fault_tolerance/migration to test. The change yield same result as before for trtllm, we will need to add better simulation and test when the migration story is fully implemented. |
Overview:
Implement request migration when worker shuts down for trtllm
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.