fix(rollout): abort vLLM rollout via delete-type /abort_requests - #296
Conversation
Documentation build overview
41 files changed ·
|
There was a problem hiding this comment.
Code Review
This pull request refactors the abort mechanism for the vLLM rollout backend to drop in-flight requests directly via a new /abort_requests endpoint on the workers, eliminating the need to pause and resume the scheduler. The changes include adding the endpoint to the vLLM patch, implementing the control-plane helper, updating the rollout logic to periodically re-sweep during draining, and adding corresponding unit tests. Feedback on the changes highlights critical robustness issues in the vLLM patch's endpoint, specifically regarding JSON type safety when parsing request bodies, potential string iteration bugs if a single string is passed as request_ids, and import safety when catching JSON decoding errors.
| + try: | ||
| + body = await raw_request.json() | ||
| + except json.JSONDecodeError: | ||
| + body = {} | ||
| + | ||
| + request_ids = body.get("request_ids") | ||
| + if not request_ids: | ||
| + request_ids = list(engine.output_processor.request_states.keys()) |
There was a problem hiding this comment.
There are a few robustness and correctness issues in this section of the patch:
- JSON Type Safety:
raw_request.json()can return any valid JSON type (such as a list, string, number, or boolean). If a client sends a non-dictionary JSON payload, callingbody.get(...)will raise anAttributeError: 'list' object has no attribute 'get'. We should checkisinstance(body, dict)to prevent this. - String Iteration Bug: If
request_idsis passed as a single string (e.g.,{"request_ids": "req_1"}), iterating over it or passing it directly toengine.abort()might treat it as an iterable of characters (['r', 'e', 'q', '_', '1']), attempting to abort non-existent single-character request IDs. Wrapping a single string in a list prevents this. - Import Safety: Catching
ValueErrorinstead ofjson.JSONDecodeErroris safer becausejson.JSONDecodeErrorinherits fromValueError, and this avoids any potentialNameErrorifjsonis not imported inapi_router.py.
+ try:
+ body = await raw_request.json()
+ if not isinstance(body, dict):
+ body = {}
+ except ValueError:
+ body = {}
+
+ request_ids = body.get("request_ids")
+ if isinstance(request_ids, str):
+ request_ids = [request_ids]
+ elif not request_ids:
+ request_ids = list(engine.output_processor.request_states.keys())
| count = 0 | ||
| while state.pendings: | ||
| done, state.pendings = await asyncio.wait(state.pendings, return_when=asyncio.FIRST_COMPLETED) | ||
| await abort_inflight_requests(urls) |
There was a problem hiding this comment.
Do you think that in order to keep it less "wasteful" in terms of requests - maybe its worth calling await abort_inflight_requests(urls) once before the loop and then only when asyncio.wait times out?
| paused_workers = True | ||
|
|
||
| count = 0 | ||
| while state.pendings: |
There was a problem hiding this comment.
Optional safeguard: the resweep handles transient worker blips and multi-turn stragglers fine, but if an engine crashes/hangs (or the connection goes half-open) the aborted stream never returns and await post - timeout=None - hangs forever, so the drain re-sweeps indefinitely. Could we maybe do something like:
_ABORT_MAX_DRAIN_S = 60
in the main loop
loop = asyncio.get_running_loop()
deadline = loop.time() + _ABORT_MAX_DRAIN_S
while state.pendings and loop.time() < deadline:and then outside the loop:
if state.pendings:
for task in state.pendings:
task.cancel()
await asyncio.gather(*state.pendings, return_exceptions=True)There was a problem hiding this comment.
I think for now we will keep parity with slime for this part. But will open to modify it if bug happens.
0208e38 to
7cdb69e
Compare
|
LGTM. Thanks! I ran the use-case that failed for me and this fix seems to work. |
Under --partial-rollout, abort() (pause -> drain -> resume) deadlocks: /pause?mode=abort puts the scheduler in PAUSED_NEW, and a /generate that races in after the pause parks in the waiting queue and never returns until /resume, which runs after the drain. Reordering to pause -> resume -> drain avoids the hang, but resume reopens the whole queue so the long tail runs to COMPLETION -- breaking partial rollout's "truncate the tail, resume it next step" semantics. Switch to a delete-type abort instead: - vLLM: add POST /abort_requests to the RLHF api_router -> EngineClient.abort() (removes queued requests from the waiting queue and finish-aborts running ones, whose partial output returns on the original /generate stream). It does not pause the scheduler, so there is no /resume and no deadlock. Shipped as a build-time patch in docker/patch/latest/vllm.patch. - vime: server_control.abort_inflight_requests() replaces the unused, slime-mirrored abort_servers_until_idle / _v1_loads helper (vLLM has neither /abort_request nor /v1/loads). abort() re-issues the sweep across drain waves and converges on state.pendings, with a timeout bounding how long a late multi-turn straggler can run before being truncated to partial. - vllm_engine: drop the legacy version gate in _register_to_router. vime ships its own vllm-router, so only the /workers payload path is needed. Adds delete-type abort unit tests. AI-assisted change; reviewed by a human before submission. Co-authored-by: aoshen <aoshen@inferact.ai> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai>
7cdb69e to
991473e
Compare
* fix(docker): abort-all in the /abort_requests vLLM patch must abort by internal ids The bundled /abort_requests endpoint (merged in #296) populated request_ids from output_processor.request_states (internal ids) but called engine.abort() with the default internal=False, so they were treated as external, matched nothing, and POST /abort_requests {} silently aborted no requests under default request-id randomization. Abort the all-in-flight list as internal. Mirrors vllm-project/vllm#47173. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * style: reject malformed JSON in /abort_requests patch with 400 Match the sibling dev endpoints and the Rust frontend (400 on malformed JSON) instead of silently treating it as empty. Mirrors vllm-project/vllm#47173. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * fix(docker): abort-all patch must also abort parallel-sampling parents The /abort_requests patch enumerated request_states (child internal ids only), so with n>1 the ParentRequest entry leaked. Include parent_requests keys in the abort-all set. Mirrors vllm PR #47173. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * feat(docker): cu13 image variant + TMS cu13 preload Port the cu13 build support from #307: ENABLE_CUDA_13 branches the apt dev headers, cublas header, TransformerEngine (source-built for cu13), TMS_CUDA_MAJOR auto-detect, and the cudnn pin. justfile gains a build-cu13 target and a VARIANT-prefixed manifest. actor_group preloads the cu13 TMS .so. Also switch the vLLM patch apply to --allow-empty so the build survives once the patch is emptied upstream. Excludes #307's NCCL_CUMEM_ENABLE default flip (0->1) and the glm5.2 scripts by request. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…m-project#296) Under --partial-rollout, abort() (pause -> drain -> resume) deadlocks: /pause?mode=abort puts the scheduler in PAUSED_NEW, and a /generate that races in after the pause parks in the waiting queue and never returns until /resume, which runs after the drain. Reordering to pause -> resume -> drain avoids the hang, but resume reopens the whole queue so the long tail runs to COMPLETION -- breaking partial rollout's "truncate the tail, resume it next step" semantics. Switch to a delete-type abort instead: - vLLM: add POST /abort_requests to the RLHF api_router -> EngineClient.abort() (removes queued requests from the waiting queue and finish-aborts running ones, whose partial output returns on the original /generate stream). It does not pause the scheduler, so there is no /resume and no deadlock. Shipped as a build-time patch in docker/patch/latest/vllm.patch. - vime: server_control.abort_inflight_requests() replaces the unused, slime-mirrored abort_servers_until_idle / _v1_loads helper (vLLM has neither /abort_request nor /v1/loads). abort() re-issues the sweep across drain waves and converges on state.pendings, with a timeout bounding how long a late multi-turn straggler can run before being truncated to partial. - vllm_engine: drop the legacy version gate in _register_to_router. vime ships its own vllm-router, so only the /workers payload path is needed. Adds delete-type abort unit tests. AI-assisted change; reviewed by a human before submission. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Josephasafg <ajgard7@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai>
…-project#317) * fix(docker): abort-all in the /abort_requests vLLM patch must abort by internal ids The bundled /abort_requests endpoint (merged in vllm-project#296) populated request_ids from output_processor.request_states (internal ids) but called engine.abort() with the default internal=False, so they were treated as external, matched nothing, and POST /abort_requests {} silently aborted no requests under default request-id randomization. Abort the all-in-flight list as internal. Mirrors vllm-project/vllm#47173. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * style: reject malformed JSON in /abort_requests patch with 400 Match the sibling dev endpoints and the Rust frontend (400 on malformed JSON) instead of silently treating it as empty. Mirrors vllm-project/vllm#47173. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * fix(docker): abort-all patch must also abort parallel-sampling parents The /abort_requests patch enumerated request_states (child internal ids only), so with n>1 the ParentRequest entry leaked. Include parent_requests keys in the abort-all set. Mirrors vllm PR #47173. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * feat(docker): cu13 image variant + TMS cu13 preload Port the cu13 build support from vllm-project#307: ENABLE_CUDA_13 branches the apt dev headers, cublas header, TransformerEngine (source-built for cu13), TMS_CUDA_MAJOR auto-detect, and the cudnn pin. justfile gains a build-cu13 target and a VARIANT-prefixed manifest. actor_group preloads the cu13 TMS .so. Also switch the vLLM patch apply to --allow-empty so the build survives once the patch is emptied upstream. Excludes vllm-project#307's NCCL_CUMEM_ENABLE default flip (0->1) and the glm5.2 scripts by request. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai>
What
Replace the pause/resume-based rollout abort with a delete-type abort, so that under
--partial-rolloutthe long tail is truncated to partial (and resumable next step) instead of either deadlocking or running to completion.