Skip to content

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

Merged
aoshen02 merged 1 commit into
feature/multi_nodesfrom
fix/vllm-health-wait-timeout
May 31, 2026
Merged

fix(vllm): wait for engine health with no time limit (SGLang-style), gated by liveness#90
aoshen02 merged 1 commit into
feature/multi_nodesfrom
fix/vllm-health-wait-timeout

Conversation

@aoshen02

@aoshen02 aoshen02 commented May 31, 2026

Copy link
Copy Markdown
Collaborator

What

_wait_server_healthy hardcoded a 300s deadline. A large engine exceeds it while still loading weights + compiling + capturing CUDA graphs — a Qwen3-30B-A3B-FP8 dp=2 colocate engine hit TimeoutError: Timeout waiting for vLLM server healthy at 300s even though it became healthy seconds later.

Fix

Match slime's SGLang backend _wait_server_healthy: loop until /health returns 200, or — for a managed subprocess — until it dies (fail fast via process.is_alive()), with no overall deadline. The timeout_s parameter is removed entirely (cleaner). The per-probe timeout=3 on each /health GET is kept so a single stuck socket can't wedge the loop.

_wait_worker_process_alive (headless-worker stability check, different semantics) is unchanged.

Note on external mode

In external mode (process is None) there is no subprocess whose death signals failure, so a permanently-unreachable URL loops indefinitely — by design, since the external engine is caller-managed (this mirrors how SGLang's own health wait behaves). test_vllm_generate_endpoint.py updated to drop the now-removed timeout_s kwarg.

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

@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 increases the startup timeout for vLLM processes from 300 to 1200 seconds to accommodate large MoE engines. However, a critical issue was identified in _wait_worker_process_alive: because this function blocks until the timeout expires for a running process, increasing the timeout to 1200 seconds will cause healthy worker nodes to block startup for 20 minutes. It is recommended to keep a much smaller timeout (e.g., 10 seconds) for worker process verification.



def _wait_worker_process_alive(process: multiprocessing.Process, timeout_s: float = 300.0) -> None:
def _wait_worker_process_alive(process: multiprocessing.Process, timeout_s: float = 1200.0) -> None:

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.

critical

Critical Efficiency & Logic Issue: Healthy Workers Will Block Startup for 20 Minutes. In _wait_worker_process_alive, the loop continues as long as the process is alive. Because a healthy worker process stays alive indefinitely, this function has no success exit condition other than the timeout expiring. Consequently, any healthy worker node will always block and sleep for the entire timeout_s duration (now bumped to 1200 seconds / 20 minutes) before returning. Since _wait_worker_process_alive is called synchronously during the Ray Actor's init() on all non-head nodes, this change will introduce a mandatory 20-minute delay to the startup of any multi-node vLLM engine group. ### Suggested Fix: The head node's health check (_wait_server_healthy) already acts as the barrier for the entire distributed engine's readiness. Therefore, the worker nodes only need to verify that their subprocess does not crash immediately on startup. We should use a much smaller, dedicated timeout (e.g., 10 seconds) for _wait_worker_process_alive instead of sharing the 1200-second startup patience timeout.

Suggested change
def _wait_worker_process_alive(process: multiprocessing.Process, timeout_s: float = 1200.0) -> None:
def _wait_worker_process_alive(process: multiprocessing.Process, timeout_s: float = 10.0) -> None:

@aoshen02 aoshen02 changed the title fix(vllm): health-wait timeout 300s -> 1200s for large DP/MoE engine startup fix(vllm): wait for engine health with no time limit (SGLang-style), gated by liveness May 31, 2026
@aoshen02
aoshen02 force-pushed the fix/vllm-health-wait-timeout branch from c05245e to bce9b80 Compare May 31, 2026 02:02
…gated by liveness

_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>
@aoshen02
aoshen02 force-pushed the fix/vllm-health-wait-timeout branch from bce9b80 to 4ce883e Compare May 31, 2026 02:13
@aoshen02
aoshen02 merged commit ba4d796 into feature/multi_nodes May 31, 2026
0 of 2 checks passed
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>
@aoshen02
aoshen02 deleted the fix/vllm-health-wait-timeout branch June 8, 2026 14:17
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