Skip to content

[Feature]vLLM multi-node rollout engine topology - #68

Merged
aoshen02 merged 7 commits into
mainfrom
feature/multi_nodes
May 31, 2026
Merged

[Feature]vLLM multi-node rollout engine topology#68
aoshen02 merged 7 commits into
mainfrom
feature/multi_nodes

Conversation

@CalvinXKY

Copy link
Copy Markdown
Collaborator

Summary

  • Add vLLM multi-node rollout topology: one Ray actor per node-rank, head owns HTTP/router, workers launch with --headless.
  • Consolidate launch configuration into vllm_engine.py (compute_server_argsbuild_vllm_cmd_and_envlaunch_server_process).
  • Multi-node CLI: --nnodes, --node-rank, --master-*, --data-parallel-backend mp, --distributed-executor-backend mp.
  • Rename rollout per-group overrides to vllm_overrides; remove SGLang references from vllm_utils.

Part of #65 (orchestration layer; multinode integration tests and rollout cleanup in follow-up PRs).

Test plan

Completed

  • Local: pre-commit on changed files (ruff, black, isort, autoflake) — all passed
  • A800-server / vime_v22 container: pytest -q tests/unit/backends/vllm_utils/60 passed
    • Topology: single-node (rollout_num_gpus_per_engine=4, num_gpus_per_node=8, TP=4) and multi-node rank split (rollout_num_gpus_per_engine=16, num_gpus_per_node=8nnodes=2, TP=8, PP=2)
    • CLI flags: --nnodes, --node-rank, --master-addr, --master-port, --headless (worker), --data-parallel-backend mp, --distributed-executor-backend mp
    • Single-node path: no extra distributed flags appended when nnodes=1

Single-node backward compatibility (existing workloads — not re-run in this PR)

These configs should behave identically to pre-PR behavior because nnodes=1 when rollout_num_gpus_per_engine <= num_gpus_per_node; no multi-node CLI flags are injected.

  • tests/test_vllm_generate_endpoint.pyQwen3-0.6B (1 GPU): compute_server_args + launch_server_process/inference/v1/generate
  • tests/test_vllm_generate_endpoint.pyQwen3-30B-A3B (1 GPU): MoE rollout + logprob capture
  • tests/test_vllm_generate_endpoint.pyQwen3-30B-A3B + R3 routing replay (1 GPU): rollout_routed_experts shape check
  • Existing single-host training scripts (e.g. Qwen3-30B-A3B, 8 GPU colocate, rollout_num_gpus_per_engine=8, nnodes_per_engine=1) — unchanged code path; head-only HTTP/router/weight-update logic

Multi-node orchestration (supported by this PR — integration validation deferred to 2/N)

This PR adds the Ray-actor topology and vLLM launch flags; cross-host E2E is planned in follow-up PRs (see PR #66 sweep matrix). Target configs:

Config Model Hosts rollout_num_gpus_per_engine num_gpus_per_node nnodes_per_engine vLLM TP × PP What this exercises
s3 Qwen3-30B-A3B 2 × 8 GPU 16 8 2 16 × 1 One logical engine spans both hosts; worker --headless; mp backends
s4 Qwen3-30B-A3B 2 × 8 GPU 8 4 2 8 × 1 Multi-engine + nnodes=2 per engine
s9 Qwen3-30B-A3B 2 × 8 GPU 16 8 2 8 × 2 Cross-host vLLM PP=2 × nnodes=2 (eager; CUDA-graph capture hang is a known vLLM PP issue)

Planned test artifacts (2/N, aligned with #65 / PR #66):

  • tests/test_qwen3_30B_A3B_pr66_sweep.py — parametrized 11-config × 2-step sweep
  • tests/multinode/{head,worker,launch_cross_host}.sh — cross-host Ray + container orchestration

Part of #65

@CalvinXKY
CalvinXKY requested a review from aoshen02 May 29, 2026 03:26

@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 refactors the vLLM rollout backend to support multi-node configurations by introducing a structured VllmEngineTopology and separating server argument computation from process launching. It also cleans up various HTTP endpoints and adds corresponding unit and integration tests. The feedback highlights three key issues: first, _wait_worker_process_alive incorrectly blocks the main thread for the full timeout duration when the worker process is healthy; second, the override checks for vllm_data_parallel_backend and vllm_distributed_executor_backend are dead code because these fields are in SKIPPED_DESTS; and third, consecutive checks for the colocate attribute in build_vllm_subprocess_env should be consolidated for better readability.


def _redact_cmd_for_log(cmd: list[str]) -> str:
"""Stringify ``cmd`` for logging, replacing values of sensitive flags with '***'.
def _wait_worker_process_alive(process: multiprocessing.Process, timeout_s: float = 300.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.

high

The current implementation of _wait_worker_process_alive will block the main thread for the entire duration of timeout_s (defaulting to 300.0 seconds / 5 minutes) if the worker process is healthy and remains alive. This introduces a mandatory 5-minute delay on every multi-node launch for non-head nodes.

Since the head node's _wait_server_healthy already handles the full model loading and cluster synchronization health check, the worker nodes only need a brief startup check (e.g., 10 seconds) to ensure the subprocess didn't immediately crash due to invalid arguments or environment issues.

We should reduce the default timeout_s to a much smaller value like 10.0 seconds.

Suggested change
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 = 10.0) -> None:

Comment on lines +194 to +197
if not _user_overrode(args, "vllm_data_parallel_backend"):
cmd += ["--data-parallel-backend", "mp"]
if not _user_overrode(args, "vllm_distributed_executor_backend"):
cmd += ["--distributed-executor-backend", "mp"]

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

In arguments.py, data_parallel_backend and distributed_executor_backend are added to SKIPPED_DESTS. This means they are skipped during parser construction and do not exist in get_vllm_cli_action_table().

As a result, _user_overrode(args, "vllm_data_parallel_backend") and _user_overrode(args, "vllm_distributed_executor_backend") will always return False because get_vllm_cli_action_table().get(dest) returns None.

If these backends are strictly orchestrator-controlled and should never be overridden by the user, the _user_overrode checks here are dead code and can be removed. If they are meant to be overridable, they should not be in SKIPPED_DESTS or the override detection logic needs to be adjusted.

Comment on lines +321 to +329
if getattr(args, "colocate", False):
import slime

vime_root = os.path.dirname(os.path.dirname(os.path.abspath(slime.__file__)))
existing_pp = env.get("PYTHONPATH", "")
if vime_root not in {p for p in existing_pp.split(os.pathsep) if p}:
env["PYTHONPATH"] = os.pathsep.join(filter(None, [vime_root, existing_pp]))
if getattr(args, "colocate", False):
env.setdefault("VLLM_ALLOW_INSECURE_SERIALIZATION", "1")

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

The getattr(args, "colocate", False) check is performed twice consecutively. We can consolidate these checks into a single conditional block to improve readability and maintainability.

Suggested change
if getattr(args, "colocate", False):
import slime
vime_root = os.path.dirname(os.path.dirname(os.path.abspath(slime.__file__)))
existing_pp = env.get("PYTHONPATH", "")
if vime_root not in {p for p in existing_pp.split(os.pathsep) if p}:
env["PYTHONPATH"] = os.pathsep.join(filter(None, [vime_root, existing_pp]))
if getattr(args, "colocate", False):
env.setdefault("VLLM_ALLOW_INSECURE_SERIALIZATION", "1")
if getattr(args, "colocate", False):
import slime
vime_root = os.path.dirname(os.path.dirname(os.path.abspath(slime.__file__)))
existing_pp = env.get("PYTHONPATH", "")
if vime_root not in {p for p in existing_pp.split(os.pathsep) if p}:
env["PYTHONPATH"] = os.pathsep.join(filter(None, [vime_root, existing_pp]))
env.setdefault("VLLM_ALLOW_INSECURE_SERIALIZATION", "1")

@CalvinXKY

CalvinXKY commented May 29, 2026

Copy link
Copy Markdown
Collaborator Author

Multi-node validation on A800-server + A800-server2 (vime_v22)

Environment

  • Hosts: A800-server (7.216.199.149) + A800-server2 (7.216.196.62)
  • Container: vime_v22 on both nodes (shared NFS mount /data/nfs_87)
  • vLLM: 0.21.1rc1.dev38+gff712f644
  • Model: Qwen3-30B-A3B copied to container-local disk (/root/local_models/Qwen3-30B-A3B, 57G, 16 shards) to avoid slow NFS weight load

Connectivity pre-check

  • Management SSH 7.216.199.1497.216.196.62: PASS

s3 — cross-host colocate, nnodes_per_engine=2 ✅ PASS

Field Value
Config s3
Model Qwen3-30B-A3B
Hosts 2 × 8 GPU (16 GPU logical engine)
rollout_num_gpus_per_engine 16
num_gpus_per_node 8
nnodes_per_engine 2
vLLM TP × PP 16 × 1
dist_init_addr 7.216.199.149:38271

Head log (excerpt)

[node_rank=0] config=s3 nnodes=2 local_gpus=8 tp=16 pp=1 headless=False dist_init_addr=7.216.199.149:38271
non-default args: {..., 'distributed_executor_backend': 'mp', 'master_addr': '7.216.199.149', 'master_port': 38271, 'nnodes': 2, 'tensor_parallel_size': 16, ...}
DP group leader: node_rank=0, ..., world_size=16, local_world_size=8
[head] server healthy, running generate
PASS config=s3 response_len=16 status=Status.TRUNCATED tokens_tail=[220, 18, 488, 220]

Worker log (excerpt)

[node_rank=1] config=s3 nnodes=2 local_gpus=8 tp=16 pp=1 headless=True dist_init_addr=7.216.199.149:38271
Launching vLLM headless multiproc executor, with head node address 7.216.199.149:38271
world_size=16 rank=8..15 local_rank=0..7 distributed_init_method=tcp://7.216.199.149:38271
PASS worker config=s3 stayed alive until test-done

@CalvinXKY

Copy link
Copy Markdown
Collaborator Author

A800 dual-node multinode smoke test results

Environment

  • Head: A800-server (7.216.199.149) · Worker: A800-server2 (7.216.196.62)
  • Container: vime_v22 on both nodes
  • Model: Qwen3-30B-A3B (local copy at /root/local_models/Qwen3-30B-A3B)
  • vLLM: 0.21.1rc1.dev38+gff712f644
Config TP×PP rollout_num_gpus_per_engine num_gpus_per_node Result Log
s3 16×1 16 8 ✅ PASS logs/pr68_s3_local4/head.log
s4 8×1 8 4 ✅ PASS logs/pr68_s4_20260529_131533/head.log
s9 8×2 (enforce_eager) 16 8 ✅ PASS logs/pr68_s9_20260529_142251/head.log

s9 excerpt (cross-host PP=2, world_size=16, generate returned 200 OK):

[node_rank=0] config=s9 nnodes=2 local_gpus=8 tp=8 pp=2 headless=False dist_init_addr=7.216.199.149:54313
(APIServer pid=311) INFO: ... "POST /inference/v1/generate HTTP/1.1" 200 OK
PASS config=s9 response_len=16 status=Status.TRUNCATED tokens_tail=[220, 18, 488, 220]

Notes

  • s9 requires enforce_eager=True (same MoE+PP CUDA graph issue as PR Mirror SGLang rollout architecture for vLLM #66).
  • Barrier sync fix: head no longer deletes worker-ready after worker touches it.
  • Manual test scripts live under NFS multinode_test/ (not committed to the vime repo).
  • Occasional TCPStore Broken pipe warnings on the worker after teardown are benign; PASS is determined by PASS config=... in head/worker logs.

@CalvinXKY
CalvinXKY force-pushed the feature/multi_nodes branch from 87c6a1a to 290c708 Compare May 29, 2026 06:31
@CalvinXKY

Copy link
Copy Markdown
Collaborator Author

A800 dual-node end-to-end training validation (follow-up)

How this differs from the earlier smoke tests

The inference smoke tests above only exercise the rollout / inference side:

  • Standalone vLLM engine launch across 2 nodes (s3 / s4 / s9 topologies)
  • One-shot POST /inference/v1/generate through the router
  • Validates cross-node TP/PP wiring, Ray barrier sync, and worker registration

This E2E run exercises the full RL training loop on the same hardware and configs:

  • train.py submitted as a Ray job on 2 nodes × 8 GPUs
  • Megatron actors load the torch_dist checkpoint and run 2 rollout steps (num-rollout=2)
  • Each step: rollout → reward → IPC weight sync (train → inference) → Megatron PPO update
  • Confirms the orchestration path used in real training, not just isolated inference

Same model (Qwen3-30B-A3B), same container image, same s3/s4/s9 engine layouts as the smoke tests.

Results

Config TP×PP Rollout layout E2E steps Result
s3 16×1 1 engine, TP16 across both nodes 2 ✅ PASS
s4 8×1 2 engines, TP8 each (4 GPU/node) 2 ✅ PASS
s9 8×2 (enforce_eager) 2 engines, TP8 PP2 (16 GPU/engine) 2 ✅ PASS

Pass criteria: Ray job completed successfully; both training steps finished; update_weights between rollout and train had no fatal errors; grad_norm showed no NaN/inf (small TIGHT batch — grad_norm=0 is expected).

Observed behavior (s9): first step is slow (~2–3 min) while Megatron loads the distributed checkpoint and performs the initial weight sync into vLLM; this is normal, not a hang.

Code change uncovered during E2E

While running s9, E2E hit missing/invalid PP CLI wiring. Fixed in arguments.py (dffb2da):

  • Add --vllm-pp-size and validate via getattr
  • Exclude vllm_pp_size from generic orchestration forwarding (was incorrectly emitted as --pp-size); build_vllm_cmd_and_env sends --pipeline-parallel-size instead

@CalvinXKY

Copy link
Copy Markdown
Collaborator Author

Single-node R3 training validation (follow-up)

How this differs from the multinode E2E above

The multinode E2E tests validate cross-host rollout topologies (s3/s4/s9) with 2 nodes × 8 GPUs.

This follow-up checks that the original single-machine training recipes still work on the same A800 hardware — in particular routing replay (R3) with vLLM:

  • One node, 8 GPUs, Ray head only (no cross-host vLLM TP/PP)
  • Full train.py path: rollout → R3 routed experts → IPC weight sync → Megatron update
  • Two layouts from our run_script r3 work:
    • disagg 4+4 — 4 train + 4 rollout GPUs (same as qwen3-30B-A3B-4t4i-r3-300step.sh)
    • colocate — train + rollout share all 8 GPUs (--colocate)

Each layout only needs to pass on one machine (no need to duplicate on both nodes).

Results

Mode GPUs Steps Result Notes
disagg (4+4) 4 train + 4 rollout, TP1 EP4 2 ✅ PASS Matches run_script r3 layout; --use-rollout-routing-replay
colocate 8 shared, TP4 EP8 1 ✅ PASS 1-step smoke; 2-step colocate hits host RAM limit on 1TB nodes during weight sync

Pass criteria: Ray job succeeded; training step(s) completed with R3 enabled; no fatal update_weights errors.

Together with the inference smoke + multinode E2E above, this covers single-node r3, cross-node rollout, and cross-node full training loop on Qwen3-30B-A3B.

@CalvinXKY CalvinXKY changed the title [Feature][1/N] vLLM multi-node rollout engine topology [Feature]vLLM multi-node rollout engine topology May 30, 2026
@CalvinXKY

Copy link
Copy Markdown
Collaborator Author

Cross-node Expert Parallel validation (follow-up)

Follow-up on the dual-node A800 setup (2×8 GPUs, vime_v22, Qwen3-30B-A3B, branch feature/multi_nodes). Two independent EP test groups — no vime code changes required; validation scripts only.

How this differs from prior tests

Prior test What it covered
Inference smoke (s3/s4/s9) Cross-host vLLM TP/PP topologies, generate-only
Multinode E2E (s3/s4/s9) Full train loop with Megatron TP=8 / EP=8 / CP=2, but rollout side was dense TP (no vLLM EP)
Single-node R3 Routing replay on one machine

This follow-up explicitly validates MoE expert parallelism spanning both nodes on each stack:

Test A — vLLM cross-node EP (inference)

Config Nodes vLLM layout EP Result
vllm_ep16 2×8 TP=8, DP=2 16 ✅ PASS

Head runs one generate after cross-node vLLM startup (--vllm-enable-expert-parallel, --vllm-data-parallel-size 2). Worker stays alive headless until teardown.

Test B — Megatron cross-node EP (E2E, 2 steps)

Config Actor Megatron Rollout vLLM Result
megatron_ep8_xnode 2×8 TP=8, EP=8, CP=2 TP=16 cross-node (no vLLM EP) ✅ PASS

Full colocate path: rollout → IPC weight sync → train × 2 steps. grad_norm=0 on both steps; no NaN/inf.

Note on EP=16: A megatron_ep16 (TP=1, EP=16) attempt hung at load_checkpoint — the existing torch_dist checkpoint was converted on 8 GPUs (TP4/EP8) and reshard to EP=16 does not complete in practice. EP=8 across both nodes is sufficient to validate Megatron cross-node EP with the current checkpoint; full EP=16 would need a 16-GPU checkpoint convert.

Summary

Stack Cross-node EP Code changes Result
vLLM EP=16 (TP8×DP2) None ✅ PASS
Megatron EP=8 (TP8/CP2) None ✅ PASS

Together with inference smoke, multinode E2E, and single-node R3, this covers vLLM cross-node EP inference and Megatron cross-node EP training on Qwen3-30B-A3B without additional changes on feature/multi_nodes.

@CalvinXKY
CalvinXKY force-pushed the feature/multi_nodes branch 3 times, most recently from 86c42c7 to 630183e Compare May 30, 2026 06:32
- 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
aoshen02 and others added 2 commits May 31, 2026 00:12
…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>
…ic 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>
aoshen02 and others added 4 commits May 31, 2026 10:04
…able 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>
…e-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>
…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>
# Conflicts:
#	slime/backends/vllm_utils/vllm_engine.py
@aoshen02
aoshen02 merged commit 3de05ee into main May 31, 2026
9 of 12 checks passed
aoshen02 added a commit that referenced this pull request Jun 4, 2026
…r_process API

`test_vllm_generate_endpoint.py` still called the pre-#68 multi-kwarg
`launch_server_process(bind_host=, server_port=, args=, rank=, visible_devices=,
model_path=)` form. PR #68 (multi-node rollout topology) refactored
`launch_server_process` to take a single `server_args` dict built by
`_compute_server_args(...)`, so the test raised at runtime:

    TypeError: launch_server_process() got an unexpected keyword argument 'bind_host'

i.e. the test has been failing since #68 and never caught (the unit/e2e job is
gated behind pre-commit, which currently fails on every PR).

Fix the test to mirror how `VLLMEngine` itself launches the server: build a
`server_args` dict via `_compute_server_args(args, rank=0, dist_init_addr=None,
host, port)` then call `launch_server_process(server_args)`. The `args` Namespace
is expanded with the attrs that `_compute_server_args` / `build_vllm_cmd_and_env`
/ `get_base_gpu_id` read (`num_gpus_per_node`, `hf_checkpoint`,
`vllm_enable_sleep_mode`, `vllm_dp_size`, and a single-colocate placement:
`colocate`, `actor_num_nodes`, `actor_num_gpus_per_node`, `use_critic`,
`debug_rollout_only`). This lets the GPU base be derived through the real
`get_base_gpu_id()`/`_to_local_gpu_id()` path — identical to `VLLMEngine` — so the
launched server tracks `CUDA_VISIBLE_DEVICES` rather than a hardcoded base id.
Drop the now-unused `_visible_devices` helper. No production code change.

Verified: `test_qwen3_0_6b_vllm_inference_generate_endpoint` passes on a single
GPU (1 passed, ~100s, Qwen3-0.6B, real vLLM server + /inference/v1/generate).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CalvinXKY pushed a commit that referenced this pull request Jun 8, 2026
…r_process API (#149)

`test_vllm_generate_endpoint.py` still called the pre-#68 multi-kwarg
`launch_server_process(bind_host=, server_port=, args=, rank=, visible_devices=,
model_path=)` form. PR #68 (multi-node rollout topology) refactored
`launch_server_process` to take a single `server_args` dict built by
`_compute_server_args(...)`, so the test raised at runtime:

    TypeError: launch_server_process() got an unexpected keyword argument 'bind_host'

i.e. the test has been failing since #68 and never caught (the unit/e2e job is
gated behind pre-commit, which currently fails on every PR).

Fix the test to mirror how `VLLMEngine` itself launches the server: build a
`server_args` dict via `_compute_server_args(args, rank=0, dist_init_addr=None,
host, port)` then call `launch_server_process(server_args)`. The `args` Namespace
is expanded with the attrs that `_compute_server_args` / `build_vllm_cmd_and_env`
/ `get_base_gpu_id` read (`num_gpus_per_node`, `hf_checkpoint`,
`vllm_enable_sleep_mode`, `vllm_dp_size`, and a single-colocate placement:
`colocate`, `actor_num_nodes`, `actor_num_gpus_per_node`, `use_critic`,
`debug_rollout_only`). This lets the GPU base be derived through the real
`get_base_gpu_id()`/`_to_local_gpu_id()` path — identical to `VLLMEngine` — so the
launched server tracks `CUDA_VISIBLE_DEVICES` rather than a hardcoded base id.
Drop the now-unused `_visible_devices` helper. No production code change.

Verified: `test_qwen3_0_6b_vllm_inference_generate_endpoint` passes on a single
GPU (1 passed, ~100s, Qwen3-0.6B, real vLLM server + /inference/v1/generate).

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 feature/multi_nodes 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.

2 participants