feat(RL): add RL worker admin routes - #9680
Conversation
84f0357 to
d4a15b6
Compare
d4a15b6 to
4d18d72
Compare
81a65d6 to
15cf6d5
Compare
7a2a3cd to
68f5c1c
Compare
4d18d72 to
31f9cbc
Compare
31f9cbc to
9d1d61c
Compare
9d1d61c to
3ca7ddd
Compare
- shield the deferred-abort await so a cancelled admin route / disconnected
client cannot cancel the in-flight engine abort
- wire the deferred-abort guard into _generate_text_mode (disagg decode text
mode) so admin abort_request also defers there and pre-first-token aborts are
safe
- run flush_cache under _pause_lock so it cannot race a locked weight update
- invalidate prefix/KV cache after weight updates (disk + distributed) before
resume so stale cache is not reused under new weights
- roll back the engine adapter (remove new, re-add old), not just metadata,
when a LoRA hot-swap's prefix-cache reset fails
- coerce non-dict admin request bodies to {} so malformed JSON returns a typed
error dict instead of a 500
…ilures
- admin routes now reject non-object JSON bodies with a typed error instead of
normalizing them into a default action (None still treated as empty {})
- _DeferredAbort records the engine-abort exception; abort_request reports it
(and escalates EngineDeadError) instead of a false 'ok'
- shield the abort task in _DeferredAbort.close() too
- regression tests for body rejection and deferred-abort failure surfacing
…ngine death - _DeferredAbort.abort() returns immediately for a pre-first-token deferred abort (the real abort fires in the background on first token) so the admin abort_request route and disconnect monitor never hang waiting for a token - thread an on_engine_dead hook so a deferred/background abort that hits EngineDeadError shuts the runtime down on any path (admin, monitor, deferred), not just the synchronous admin route - regressions for no-block-before-first-token and engine-dead escalation
abort() no longer blocks pre-first-token (the real abort is deferred to a background task), so the tests now await guard.abort() directly and observe the parked _abort_task / drive it via signal_first_token, and the _capture mock accepts the new on_engine_dead arg.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/src/dynamo/vllm/handlers.py (1)
3303-3304:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't signal
first_tokenon the synthetic no-output error path.
generate_tokens()emits a synthetic error chunk whenres.outputsis empty. This loop callsabort_guard.signal_first_token()for every yielded chunk, so a queued admin/disconnect abort can wake up and executeengine_client.abort()even though no real token was produced yet. That reopens the exact pre-first-token abort window the deferred guard is meant to avoid.Suggested fix
- if abort_guard is not None: + if abort_guard is not None and tok.get("token_ids"): abort_guard.signal_first_token()If you want to preserve the logic in one place, moving the signal into the normal-output branch inside
generate_tokens()is even safer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/vllm/handlers.py` around lines 3303 - 3304, The current loop calls abort_guard.signal_first_token() for every yielded chunk including the synthetic no-output error from generate_tokens() (when res.outputs is empty), which reopens the pre-first-token abort window; fix it by only signaling the abort guard when a real token/output is produced—i.e., guard the call to abort_guard.signal_first_token() with a check that res.outputs (or the actual token) is non-empty or move the signal into the normal-output branch inside generate_tokens() so synthetic error chunks never trigger signal_first_token(); update the code paths that yield chunks to use this check and reference abort_guard.signal_first_token() and generate_tokens() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/src/dynamo/vllm/handlers.py`:
- Around line 299-301: The defer-abort guard is being removed from registry
before awaiting guard.close(), causing a race where abort_request() can miss the
guard and call engine_client.abort(request_id); change the teardown so the guard
remains registered until close() completes — i.e., await guard.close() while the
registry still maps request_id to the guard, then remove
registry.pop(request_id, None) after the await (or wrap close() in a try/finally
that pops in the finally) so the guard is always discoverable by abort_request()
until close() finishes.
In `@components/src/dynamo/vllm/tests/test_vllm_worker_handler.py`:
- Around line 1310-1388: Add a "unit" test-type marker to these new tests by
applying pytest.mark.unit at module or class scope (e.g., set pytestmark =
[pytest.mark.unit] near the top of the file or add `@pytest.mark.unit` above class
TestRLAdminRouteHardening) so the test suite's marker validation
(tests/report_pytest_markers.py) recognizes the required Test Type category;
keep existing markers intact and ensure the new marker is imported from pytest
if necessary.
---
Outside diff comments:
In `@components/src/dynamo/vllm/handlers.py`:
- Around line 3303-3304: The current loop calls abort_guard.signal_first_token()
for every yielded chunk including the synthetic no-output error from
generate_tokens() (when res.outputs is empty), which reopens the pre-first-token
abort window; fix it by only signaling the abort guard when a real token/output
is produced—i.e., guard the call to abort_guard.signal_first_token() with a
check that res.outputs (or the actual token) is non-empty or move the signal
into the normal-output branch inside generate_tokens() so synthetic error chunks
never trigger signal_first_token(); update the code paths that yield chunks to
use this check and reference abort_guard.signal_first_token() and
generate_tokens() when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ffe9e48e-91ac-4779-8c97-7be2b14372f2
📒 Files selected for processing (4)
components/src/dynamo/common/rl/admin.pycomponents/src/dynamo/common/tests/test_rl_admin.pycomponents/src/dynamo/vllm/handlers.pycomponents/src/dynamo/vllm/tests/test_vllm_worker_handler.py
🚧 Files skipped from review as they are similar to previous changes (2)
- components/src/dynamo/common/tests/test_rl_admin.py
- components/src/dynamo/common/rl/admin.py
Awaiting guard.close() before popping request_id from the registry closed a teardown race: an out-of-band admin abort_request during close() could miss the guard and take the unsafe direct engine_client.abort path in the pre-first-token window (CodeRabbit).
Signed-off-by: Yao, Qing <qing.yao@intel.com>
Signed-off-by: shenls <shenlinshan@kanzhun.com>
Overview
Adds the worker-side RL admin surface needed by prime-rl on top of
bis/nvext-tito-vllm. This PR keeps the frontend out of admin execution and prepares workers to advertise route metadata over the request plane.Changes
dynamo.common.rlhelpers for RL route descriptors and LoRA request validation./engine/<route>.dyn://<namespace>.<component>.rlonly as a route-descriptor request-plane endpoint.system_urlthrough the Python runtime binding.load_lora/unload_lorahandlers when LoRA is enabled.Validation
.venv/bin/python -m pytest components/src/dynamo/common/tests/test_rl_admin.py -q.venv/bin/python -m py_compile components/src/dynamo/common/rl/admin.py components/src/dynamo/vllm/handlers.py components/src/dynamo/vllm/worker_factory.pyNotes
supersedes #9382
Summary by CodeRabbit
New Features
Tests