Skip to content

fix(vllm): tolerate empty 200 body in _response_json (/sleep, /wake_up) - #80

Closed
aoshen02 wants to merge 2 commits into
mainfrom
fix/vllm-empty-200-response
Closed

fix(vllm): tolerate empty 200 body in _response_json (/sleep, /wake_up)#80
aoshen02 wants to merge 2 commits into
mainfrom
fix/vllm-empty-200-response

Conversation

@aoshen02

Copy link
Copy Markdown
Collaborator

Problem

VLLMEngine.release_memory_occupation() (colocate memory offload, via RolloutManager.offload()) does POST /sleep and then _response_json(response). vLLM's /sleep (and /wake_up) return HTTP 200 with an empty body, but _response_json unconditionally calls response.json():

def _response_json(response: requests.Response) -> dict:
    response.raise_for_status()      # 200 OK
    return response.json()           # 💥 empty body -> JSONDecodeError

So colocate offload crashes on the first offload (before rollout #1):

ray::VLLMEngine.release_memory_occupation()
  File ".../slime/backends/vllm_utils/vllm_engine.py", line 749, in release_memory_occupation
    return _response_json(response)
  File ".../slime/backends/vllm_utils/vllm_engine.py", line 41, in _response_json
    return response.json()
requests.exceptions.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

Fix

Treat an empty 200 body as an empty result ({}). This covers /sleep and /wake_up, which carry no JSON payload.

How it was found

A colocate Qwen3-4B GRPO run (DP4 / TP1, --colocate) on the vLLM R3 image (vllm 0.21.1rc1.dev38), as part of a slime(SGLang)-vs-vime(vLLM) convergence A/B. The vime arm crashed at the first release_memory_occupation; the slime arm was unaffected.

Notes / follow-up

  • resume_memory_occupation() (/wake_up) hits the same code path and is fixed by the same change.
  • Behavior for endpoints that do return JSON is unchanged.
  • Follow-up: a unit test for _response_json (empty body → {}, non-empty → parsed, non-200 → raises with response.text note) would lock this in.

🤖 Generated with Claude Code

vLLM control-plane endpoints POST /sleep and /wake_up return HTTP 200 with an
empty body. _response_json() unconditionally called response.json(), which
raises JSONDecodeError("Expecting value: line 1 column 1 (char 0)") and crashes
colocate memory offload (VLLMEngine.release_memory_occupation /
resume_memory_occupation, via RolloutManager.offload()).

Treat an empty 200 body as an empty result ({}).

Found via a colocate Qwen3-4B GRPO run (DP4/TP1) on the vLLM R3 image
(vllm 0.21.1rc1.dev38): the rollout engine crashed on the first offload, before
rollout #1, with the JSONDecodeError above.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the _response_json function in slime/backends/vllm_utils/vllm_engine.py to handle empty HTTP 200 response bodies from vLLM control-plane endpoints, preventing a JSONDecodeError. The reviewer suggested optimizing the check by using response.content.strip() directly on the raw bytes instead of response.text.strip(), which avoids the CPU-intensive character encoding detection overhead.

Comment on lines +46 to +47
if not response.content or not response.text.strip():
return {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using response.text can be inefficient because it triggers automatic character encoding detection (which can be slow and CPU-intensive if charset_normalizer or chardet is invoked) and decodes the entire response body into a string.\n\nSince response.content is a bytes object, and bytes supports the .strip() method in Python, we can check if the body is empty or contains only whitespace directly on the raw bytes. This is much more efficient and avoids any encoding detection overhead.

    if not response.content.strip():\n        return {}

@aoshen02 aoshen02 closed this May 30, 2026
aoshen02 added a commit that referenced this pull request May 30, 2026
… tested

test_response_json_empty_body_returns_ok built _MockResponse(text="") and expected
{"ok": True}, but _MockResponse had no `content` attribute while _response_json checks
`response.content` — so the test raised AttributeError instead of exercising the
empty-body branch (the /sleep, /wake_up empty-200 handling, cf. #80).

Give _MockResponse a `content` (JSON-body bytes when json_data is set, else the text
bytes, i.e. b"" when empty) so the empty-body handling is actually verified. Other
_MockResponse usages set json_data and thus get non-empty content — unaffected.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aoshen02 added a commit that referenced this pull request May 30, 2026
…85)

* fix(vllm): derive rollout-engine TP per-engine + strict external config check

Two fixes of one root cause — TP/parallel sizing read the *global*
rollout_num_gpus_per_engine instead of the per-engine value — across the two
engine launch paths.

P2 (managed launch): validate_args unconditionally set a global args.vllm_tp_size
(= global rollout_num_gpus_per_engine // pp) and _resolve_vllm_parallel_sizes
preferred it, so the per-engine `tp = gpus_per_engine // pp` branch was dead in
real runs. A heterogeneous per-group engine (e.g. num_gpus_per_engine=2, tp=2)
thus launched with the global TP while the trainer sized the NCCL weight-transfer
rendezvous from the per-group engine_gpu_counts — they disagreed and the
rendezvous hung 300s ("3/4 clients joined"). TP is now derived per engine in
_resolve_vllm_parallel_sizes (no global shadow), matching upstream slime's
sglang_engine; the global vllm_tp_size computation is removed. dp>1 raises
NotImplementedError (DP/EP wiring is a follow-up); pp divisibility validated per
engine.

P1 (external engine): _wait_external_config_ready compared the engine's reported
TP against the same global flag and only warned, and ran on headless workers
(node_rank>0) that own no HTTP. Replaced with _sanity_check_external_server_args:
checks tp/pp/dp/nnodes against the per-engine expectation and raises on mismatch
(fail fast instead of a later rendezvous hang), gated to node_rank 0, skipping
fields /server_info does not report (vLLM may omit nnodes).

Note: #66 (aoshen/vllm-mirror-sglang-arch) carries the identical global-tp
shadow; this is a fix both branches need, not a port.

Tests: unit tests for per-engine/heterogeneous TP, the dp>1 guard, and the strict
external check (match / mismatch-raises / unreported-skipped / missing-config).
Existing topology unit tests are unchanged and still pass.

AI assistance (Claude Code) was used for this change.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* cleanup(vllm): drop forced 0.55 gpu-mem default + redundant router fallback

Remove two silent/ambiguous defaults in the vLLM launch path:

- launch_server_process no longer forces --gpu-memory-utilization=0.55. In colocate,
  training and rollout do not occupy the GPU simultaneously (sleep/offload cycles), so
  vLLM's own default is appropriate; a user value via --vllm-gpu-memory-utilization is
  still auto-forwarded by _forward_vllm_cli_args. (This reverts the unset default to
  vLLM's; memory-tight large-model colocate setups can set the flag explicitly.)
- VLLMEngine.init: drop the `else self.args.vllm_router_ip/port` fallback. rollout always
  calls engine.init(router_ip=self.router_ip, router_port=self.router_port) and
  _start_router always returns a real address, so the fallback was dead/redundant.
- Update the arguments.py note that referenced the removed 0.55 default.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* cleanup(vllm): use _response_json helper + drop dead fields/aliases

- _sanity_check_external_server_args now uses the _response_json helper (consistent
  error handling — annotates HTTP errors with response text) instead of a manual
  raise_for_status + .json().
- Remove unused VllmEngineTopology.master_host/master_port fields: never set or read
  (append_vllm_distributed_launch_flags takes the master addr via its own param).
- Remove three test-only back-compat aliases (_append_vllm_distributed_launch_flags,
  _redact_cmd_for_log, _serialize_for_cli); tests now call the public names directly.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(vllm): central node_rank guard for control-plane HTTP (headless workers)

Mirror SGLang's _make_request choke point: route the node_rank guard through the
single shared POST helper so every control-plane method that POSTs is guarded by
construction, instead of scattered (and incomplete) per-method `if node_rank != 0`
checks.

Before, 7 control-plane HTTP methods were unguarded (release/resume_memory_occupation,
init_weight_transfer_engine, start/finish_weight_update, init_weights_update_group,
update_weights_from_distributed): on a headless worker (node_rank>0, no HTTP server)
they would hit a non-existent endpoint instead of no-op'ing.

- `_post_json` (the central POST path, = SGLang's `_make_request`) now short-circuits
  to None on node_rank>0; `_response_json(None)` returns None so the no-op propagates
  to callers with no per-call special-casing and no return-type change (mocked tests
  unaffected).
- `/sleep` and `/wake_up` bypass `_post_json` (query params), so they keep an explicit
  guard — same shape as SGLang's explicit guards on its non-_make_request methods.

Tests: headless worker no-ops all control-plane methods with zero HTTP;
`_post_json` short-circuits; `_response_json(None) -> None`.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(vllm): route control-plane POSTs through _make_request (SGLang-style)

Add `_make_request` (guard + POST + JSON parse), mirroring SGLang's
HttpServerEngineAdapter._make_request, and route the control-plane POST callers
through it: _post_vllm_update_weights_http, init_weight_transfer_engine,
init_weights_update_group, start_weight_update, finish_weight_update. Each becomes a
one-liner (no more `response = self._post_json(...); return _response_json(response)`).

The node_rank guard stays centralized in `_post_json` (which `_make_request` wraps),
so behavior is unchanged and tests that mock `_post_json` are unaffected (no
return-type change, no test ripple).

Verified: headless workers (node_rank>0) no-op every control-plane method with zero
HTTP; node-0 posts + parses normally.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(vllm): collapse _post_json into _make_request (single choke point)

Per review feedback, `_make_request -> _post_json -> _response_json` was one layer
too many. SGLang's `_make_request` is self-contained (guard + POST inline), so inline
`_post_json` into `_make_request` (node_rank guard + POST + parse via the shared
`_response_json`, which the query-param endpoints /sleep, /wake_up also reuse) and drop
`_post_json` entirely.

`_response_json` reverts to strict (the None-tolerance is unneeded now that nothing
passes it None). Tests that mocked `_post_json` now mock `_make_request` (which returns
parsed JSON), with no other behavior change.

Verified: headless workers (node_rank>0) no-op every control-plane method with zero
HTTP; node-0 posts + parses; `_post_json` is fully removed.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(vllm): model _MockResponse.content so the empty-200-body path is tested

test_response_json_empty_body_returns_ok built _MockResponse(text="") and expected
{"ok": True}, but _MockResponse had no `content` attribute while _response_json checks
`response.content` — so the test raised AttributeError instead of exercising the
empty-body branch (the /sleep, /wake_up empty-200 handling, cf. #80).

Give _MockResponse a `content` (JSON-body bytes when json_data is set, else the text
bytes, i.e. b"" when empty) so the empty-body handling is actually verified. Other
_MockResponse usages set json_data and thus get non-empty content — unaffected.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aoshen02 added a commit that referenced this pull request May 31, 2026
* [Feature][1/N] Add vLLM multi-node rollout engine topology

- VllmEngineTopology / compute_server_args for cross-node vLLM engines
- Merge origin/main: add processed_logprobs default and weight-transfer comments
- Fix _response_json for vLLM sleep/wake empty HTTP 200 body
- Update unit tests for multi-node SKIPPED_DESTS and vllm_router_ip API

* fix(vllm): per-engine TP + strict external config check (harden #68) (#85)

* fix(vllm): derive rollout-engine TP per-engine + strict external config check

Two fixes of one root cause — TP/parallel sizing read the *global*
rollout_num_gpus_per_engine instead of the per-engine value — across the two
engine launch paths.

P2 (managed launch): validate_args unconditionally set a global args.vllm_tp_size
(= global rollout_num_gpus_per_engine // pp) and _resolve_vllm_parallel_sizes
preferred it, so the per-engine `tp = gpus_per_engine // pp` branch was dead in
real runs. A heterogeneous per-group engine (e.g. num_gpus_per_engine=2, tp=2)
thus launched with the global TP while the trainer sized the NCCL weight-transfer
rendezvous from the per-group engine_gpu_counts — they disagreed and the
rendezvous hung 300s ("3/4 clients joined"). TP is now derived per engine in
_resolve_vllm_parallel_sizes (no global shadow), matching upstream slime's
sglang_engine; the global vllm_tp_size computation is removed. dp>1 raises
NotImplementedError (DP/EP wiring is a follow-up); pp divisibility validated per
engine.

P1 (external engine): _wait_external_config_ready compared the engine's reported
TP against the same global flag and only warned, and ran on headless workers
(node_rank>0) that own no HTTP. Replaced with _sanity_check_external_server_args:
checks tp/pp/dp/nnodes against the per-engine expectation and raises on mismatch
(fail fast instead of a later rendezvous hang), gated to node_rank 0, skipping
fields /server_info does not report (vLLM may omit nnodes).

Note: #66 (aoshen/vllm-mirror-sglang-arch) carries the identical global-tp
shadow; this is a fix both branches need, not a port.

Tests: unit tests for per-engine/heterogeneous TP, the dp>1 guard, and the strict
external check (match / mismatch-raises / unreported-skipped / missing-config).
Existing topology unit tests are unchanged and still pass.

AI assistance (Claude Code) was used for this change.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* cleanup(vllm): drop forced 0.55 gpu-mem default + redundant router fallback

Remove two silent/ambiguous defaults in the vLLM launch path:

- launch_server_process no longer forces --gpu-memory-utilization=0.55. In colocate,
  training and rollout do not occupy the GPU simultaneously (sleep/offload cycles), so
  vLLM's own default is appropriate; a user value via --vllm-gpu-memory-utilization is
  still auto-forwarded by _forward_vllm_cli_args. (This reverts the unset default to
  vLLM's; memory-tight large-model colocate setups can set the flag explicitly.)
- VLLMEngine.init: drop the `else self.args.vllm_router_ip/port` fallback. rollout always
  calls engine.init(router_ip=self.router_ip, router_port=self.router_port) and
  _start_router always returns a real address, so the fallback was dead/redundant.
- Update the arguments.py note that referenced the removed 0.55 default.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* cleanup(vllm): use _response_json helper + drop dead fields/aliases

- _sanity_check_external_server_args now uses the _response_json helper (consistent
  error handling — annotates HTTP errors with response text) instead of a manual
  raise_for_status + .json().
- Remove unused VllmEngineTopology.master_host/master_port fields: never set or read
  (append_vllm_distributed_launch_flags takes the master addr via its own param).
- Remove three test-only back-compat aliases (_append_vllm_distributed_launch_flags,
  _redact_cmd_for_log, _serialize_for_cli); tests now call the public names directly.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(vllm): central node_rank guard for control-plane HTTP (headless workers)

Mirror SGLang's _make_request choke point: route the node_rank guard through the
single shared POST helper so every control-plane method that POSTs is guarded by
construction, instead of scattered (and incomplete) per-method `if node_rank != 0`
checks.

Before, 7 control-plane HTTP methods were unguarded (release/resume_memory_occupation,
init_weight_transfer_engine, start/finish_weight_update, init_weights_update_group,
update_weights_from_distributed): on a headless worker (node_rank>0, no HTTP server)
they would hit a non-existent endpoint instead of no-op'ing.

- `_post_json` (the central POST path, = SGLang's `_make_request`) now short-circuits
  to None on node_rank>0; `_response_json(None)` returns None so the no-op propagates
  to callers with no per-call special-casing and no return-type change (mocked tests
  unaffected).
- `/sleep` and `/wake_up` bypass `_post_json` (query params), so they keep an explicit
  guard — same shape as SGLang's explicit guards on its non-_make_request methods.

Tests: headless worker no-ops all control-plane methods with zero HTTP;
`_post_json` short-circuits; `_response_json(None) -> None`.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(vllm): route control-plane POSTs through _make_request (SGLang-style)

Add `_make_request` (guard + POST + JSON parse), mirroring SGLang's
HttpServerEngineAdapter._make_request, and route the control-plane POST callers
through it: _post_vllm_update_weights_http, init_weight_transfer_engine,
init_weights_update_group, start_weight_update, finish_weight_update. Each becomes a
one-liner (no more `response = self._post_json(...); return _response_json(response)`).

The node_rank guard stays centralized in `_post_json` (which `_make_request` wraps),
so behavior is unchanged and tests that mock `_post_json` are unaffected (no
return-type change, no test ripple).

Verified: headless workers (node_rank>0) no-op every control-plane method with zero
HTTP; node-0 posts + parses normally.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(vllm): collapse _post_json into _make_request (single choke point)

Per review feedback, `_make_request -> _post_json -> _response_json` was one layer
too many. SGLang's `_make_request` is self-contained (guard + POST inline), so inline
`_post_json` into `_make_request` (node_rank guard + POST + parse via the shared
`_response_json`, which the query-param endpoints /sleep, /wake_up also reuse) and drop
`_post_json` entirely.

`_response_json` reverts to strict (the None-tolerance is unneeded now that nothing
passes it None). Tests that mocked `_post_json` now mock `_make_request` (which returns
parsed JSON), with no other behavior change.

Verified: headless workers (node_rank>0) no-op every control-plane method with zero
HTTP; node-0 posts + parses; `_post_json` is fully removed.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(vllm): model _MockResponse.content so the empty-200-body path is tested

test_response_json_empty_body_returns_ok built _MockResponse(text="") and expected
{"ok": True}, but _MockResponse had no `content` attribute while _response_json checks
`response.content` — so the test raised AttributeError instead of exercising the
empty-body branch (the /sleep, /wake_up empty-200 handling, cf. #80).

Give _MockResponse a `content` (JSON-body bytes when json_data is set, else the text
bytes, i.e. b"" when empty) so the empty-body handling is actually verified. Other
_MockResponse usages set json_data and thus get non-empty content — unaffected.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(vllm): reorder vllm_engine.py functions by lifecycle (no logic change)

Pure top-level reordering for readability — group module functions by stage:
shared helpers -> topology -> launch config (compute_server_args) -> command/env
build (build_vllm_cmd_and_env) -> process spawn (launch_server_process) -> misc ->
the VLLMEngine actor.

No behavior change (module-level functions resolve names at call time). Verified the
module imports cleanly and the topology / control-plane (headless no-op) drivers still
pass; a reorder script asserted no non-blank line was added or removed (diff is a
balanced 71/71 move).

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(rollout): always use the full Rust router for PD; remove the unusable MiniLB path (#88)

_start_router defaulted PD to MiniLB (mini_lb=True unless SLIME_VLLM_ROUTER_USE_RUST=1).
MiniLB (vllm_router/mini_lb.py) is a debug-only load balancer that REQUIRES static
prefill/decode URLs at construction; slime never provides those (it launches the router
first and engines register dynamically via POST /workers), so MiniLB can never work here.

Remove the MiniLB activation and the dead SLIME_VLLM_ROUTER_USE_RUST gate. PD now uses the
full Rust router (accepts dynamic registration like the non-PD path). MiniLB stays off via
RouterArgs' own default (mini_lb=False) — no explicit setter needed. Also drop the stale
MiniLB mention from the router-startup-failure error message.

Note: routing-layer fix only; PD end-to-end still needs the engine-side KV transport
(--kv-transfer-config) to initialize, a separate open blocker.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(vllm): update validate_args unit tests to the post-#85 per-engine-TP contract (#91)

PR #85 removed the global vllm_tp_size from validate_args (TP is derived per-engine
in vllm_engine._resolve_vllm_parallel_sizes) and moved the pp-divisibility check out
of validate_args. But three test_arguments.py cases still asserted the old contract
and now fail on feature/multi_nodes:
  - test_validate_args_pp1            (expected ns.vllm_tp_size == 4)
  - test_validate_args_pp2_dp2_derives_tp (expected ns.vllm_tp_size == 2)
  - test_validate_args_pp_indivisible_asserts (expected validate_args to raise)
Rewrite them to the new contract: validate_args records pp/dp but sets no global TP,
and no longer raises on pp-indivisibility (that check moved per-engine, covered in
test_vllm_engine.py).

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(vllm): wait for engine health with no time limit (SGLang-style), gated by liveness (#90)

_wait_server_healthy hardcoded a 300s deadline that a large engine exceeds while still
loading/compiling/capturing CUDA graphs (a 30B FP8 dp=2 colocate engine timed out at 300s
though it was healthy seconds later). Match slime's SGLang backend: loop until /health 200
or — for a managed subprocess — until it dies (fail fast via process.is_alive()), with no
overall deadline. Drop the timeout_s parameter entirely (cleaner); keep the per-probe
timeout=3 so a single stuck socket can't wedge the loop. External mode (process is None)
has no liveness signal and loops until reachable, by design (caller-managed engine).

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
momo609 pushed a commit that referenced this pull request Jun 8, 2026
* [Feature][1/N] Add vLLM multi-node rollout engine topology

- VllmEngineTopology / compute_server_args for cross-node vLLM engines
- Merge origin/main: add processed_logprobs default and weight-transfer comments
- Fix _response_json for vLLM sleep/wake empty HTTP 200 body
- Update unit tests for multi-node SKIPPED_DESTS and vllm_router_ip API

* fix(vllm): per-engine TP + strict external config check (harden #68) (#85)

* fix(vllm): derive rollout-engine TP per-engine + strict external config check

Two fixes of one root cause — TP/parallel sizing read the *global*
rollout_num_gpus_per_engine instead of the per-engine value — across the two
engine launch paths.

P2 (managed launch): validate_args unconditionally set a global args.vllm_tp_size
(= global rollout_num_gpus_per_engine // pp) and _resolve_vllm_parallel_sizes
preferred it, so the per-engine `tp = gpus_per_engine // pp` branch was dead in
real runs. A heterogeneous per-group engine (e.g. num_gpus_per_engine=2, tp=2)
thus launched with the global TP while the trainer sized the NCCL weight-transfer
rendezvous from the per-group engine_gpu_counts — they disagreed and the
rendezvous hung 300s ("3/4 clients joined"). TP is now derived per engine in
_resolve_vllm_parallel_sizes (no global shadow), matching upstream slime's
sglang_engine; the global vllm_tp_size computation is removed. dp>1 raises
NotImplementedError (DP/EP wiring is a follow-up); pp divisibility validated per
engine.

P1 (external engine): _wait_external_config_ready compared the engine's reported
TP against the same global flag and only warned, and ran on headless workers
(node_rank>0) that own no HTTP. Replaced with _sanity_check_external_server_args:
checks tp/pp/dp/nnodes against the per-engine expectation and raises on mismatch
(fail fast instead of a later rendezvous hang), gated to node_rank 0, skipping
fields /server_info does not report (vLLM may omit nnodes).

Note: #66 (aoshen/vllm-mirror-sglang-arch) carries the identical global-tp
shadow; this is a fix both branches need, not a port.

Tests: unit tests for per-engine/heterogeneous TP, the dp>1 guard, and the strict
external check (match / mismatch-raises / unreported-skipped / missing-config).
Existing topology unit tests are unchanged and still pass.

AI assistance (Claude Code) was used for this change.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* cleanup(vllm): drop forced 0.55 gpu-mem default + redundant router fallback

Remove two silent/ambiguous defaults in the vLLM launch path:

- launch_server_process no longer forces --gpu-memory-utilization=0.55. In colocate,
  training and rollout do not occupy the GPU simultaneously (sleep/offload cycles), so
  vLLM's own default is appropriate; a user value via --vllm-gpu-memory-utilization is
  still auto-forwarded by _forward_vllm_cli_args. (This reverts the unset default to
  vLLM's; memory-tight large-model colocate setups can set the flag explicitly.)
- VLLMEngine.init: drop the `else self.args.vllm_router_ip/port` fallback. rollout always
  calls engine.init(router_ip=self.router_ip, router_port=self.router_port) and
  _start_router always returns a real address, so the fallback was dead/redundant.
- Update the arguments.py note that referenced the removed 0.55 default.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* cleanup(vllm): use _response_json helper + drop dead fields/aliases

- _sanity_check_external_server_args now uses the _response_json helper (consistent
  error handling — annotates HTTP errors with response text) instead of a manual
  raise_for_status + .json().
- Remove unused VllmEngineTopology.master_host/master_port fields: never set or read
  (append_vllm_distributed_launch_flags takes the master addr via its own param).
- Remove three test-only back-compat aliases (_append_vllm_distributed_launch_flags,
  _redact_cmd_for_log, _serialize_for_cli); tests now call the public names directly.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(vllm): central node_rank guard for control-plane HTTP (headless workers)

Mirror SGLang's _make_request choke point: route the node_rank guard through the
single shared POST helper so every control-plane method that POSTs is guarded by
construction, instead of scattered (and incomplete) per-method `if node_rank != 0`
checks.

Before, 7 control-plane HTTP methods were unguarded (release/resume_memory_occupation,
init_weight_transfer_engine, start/finish_weight_update, init_weights_update_group,
update_weights_from_distributed): on a headless worker (node_rank>0, no HTTP server)
they would hit a non-existent endpoint instead of no-op'ing.

- `_post_json` (the central POST path, = SGLang's `_make_request`) now short-circuits
  to None on node_rank>0; `_response_json(None)` returns None so the no-op propagates
  to callers with no per-call special-casing and no return-type change (mocked tests
  unaffected).
- `/sleep` and `/wake_up` bypass `_post_json` (query params), so they keep an explicit
  guard — same shape as SGLang's explicit guards on its non-_make_request methods.

Tests: headless worker no-ops all control-plane methods with zero HTTP;
`_post_json` short-circuits; `_response_json(None) -> None`.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(vllm): route control-plane POSTs through _make_request (SGLang-style)

Add `_make_request` (guard + POST + JSON parse), mirroring SGLang's
HttpServerEngineAdapter._make_request, and route the control-plane POST callers
through it: _post_vllm_update_weights_http, init_weight_transfer_engine,
init_weights_update_group, start_weight_update, finish_weight_update. Each becomes a
one-liner (no more `response = self._post_json(...); return _response_json(response)`).

The node_rank guard stays centralized in `_post_json` (which `_make_request` wraps),
so behavior is unchanged and tests that mock `_post_json` are unaffected (no
return-type change, no test ripple).

Verified: headless workers (node_rank>0) no-op every control-plane method with zero
HTTP; node-0 posts + parses normally.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(vllm): collapse _post_json into _make_request (single choke point)

Per review feedback, `_make_request -> _post_json -> _response_json` was one layer
too many. SGLang's `_make_request` is self-contained (guard + POST inline), so inline
`_post_json` into `_make_request` (node_rank guard + POST + parse via the shared
`_response_json`, which the query-param endpoints /sleep, /wake_up also reuse) and drop
`_post_json` entirely.

`_response_json` reverts to strict (the None-tolerance is unneeded now that nothing
passes it None). Tests that mocked `_post_json` now mock `_make_request` (which returns
parsed JSON), with no other behavior change.

Verified: headless workers (node_rank>0) no-op every control-plane method with zero
HTTP; node-0 posts + parses; `_post_json` is fully removed.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(vllm): model _MockResponse.content so the empty-200-body path is tested

test_response_json_empty_body_returns_ok built _MockResponse(text="") and expected
{"ok": True}, but _MockResponse had no `content` attribute while _response_json checks
`response.content` — so the test raised AttributeError instead of exercising the
empty-body branch (the /sleep, /wake_up empty-200 handling, cf. #80).

Give _MockResponse a `content` (JSON-body bytes when json_data is set, else the text
bytes, i.e. b"" when empty) so the empty-body handling is actually verified. Other
_MockResponse usages set json_data and thus get non-empty content — unaffected.

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(vllm): reorder vllm_engine.py functions by lifecycle (no logic change)

Pure top-level reordering for readability — group module functions by stage:
shared helpers -> topology -> launch config (compute_server_args) -> command/env
build (build_vllm_cmd_and_env) -> process spawn (launch_server_process) -> misc ->
the VLLMEngine actor.

No behavior change (module-level functions resolve names at call time). Verified the
module imports cleanly and the topology / control-plane (headless no-op) drivers still
pass; a reorder script asserted no non-blank line was added or removed (diff is a
balanced 71/71 move).

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(rollout): always use the full Rust router for PD; remove the unusable MiniLB path (#88)

_start_router defaulted PD to MiniLB (mini_lb=True unless SLIME_VLLM_ROUTER_USE_RUST=1).
MiniLB (vllm_router/mini_lb.py) is a debug-only load balancer that REQUIRES static
prefill/decode URLs at construction; slime never provides those (it launches the router
first and engines register dynamically via POST /workers), so MiniLB can never work here.

Remove the MiniLB activation and the dead SLIME_VLLM_ROUTER_USE_RUST gate. PD now uses the
full Rust router (accepts dynamic registration like the non-PD path). MiniLB stays off via
RouterArgs' own default (mini_lb=False) — no explicit setter needed. Also drop the stale
MiniLB mention from the router-startup-failure error message.

Note: routing-layer fix only; PD end-to-end still needs the engine-side KV transport
(--kv-transfer-config) to initialize, a separate open blocker.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(vllm): update validate_args unit tests to the post-#85 per-engine-TP contract (#91)

PR #85 removed the global vllm_tp_size from validate_args (TP is derived per-engine
in vllm_engine._resolve_vllm_parallel_sizes) and moved the pp-divisibility check out
of validate_args. But three test_arguments.py cases still asserted the old contract
and now fail on feature/multi_nodes:
  - test_validate_args_pp1            (expected ns.vllm_tp_size == 4)
  - test_validate_args_pp2_dp2_derives_tp (expected ns.vllm_tp_size == 2)
  - test_validate_args_pp_indivisible_asserts (expected validate_args to raise)
Rewrite them to the new contract: validate_args records pp/dp but sets no global TP,
and no longer raises on pp-indivisibility (that check moved per-engine, covered in
test_vllm_engine.py).

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(vllm): wait for engine health with no time limit (SGLang-style), gated by liveness (#90)

_wait_server_healthy hardcoded a 300s deadline that a large engine exceeds while still
loading/compiling/capturing CUDA graphs (a 30B FP8 dp=2 colocate engine timed out at 300s
though it was healthy seconds later). Match slime's SGLang backend: loop until /health 200
or — for a managed subprocess — until it dies (fail fast via process.is_alive()), with no
overall deadline. Drop the timeout_s parameter entirely (cleaner); keep the per-probe
timeout=3 so a single stuck socket can't wedge the loop. External mode (process is None)
has no liveness signal and loops until reachable, by design (caller-managed engine).

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Signed-off-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@CalvinXKY
CalvinXKY deleted the fix/vllm-empty-200-response branch June 16, 2026 11:35
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.

1 participant