Mirror SGLang rollout architecture for vLLM - #66
Closed
aoshen02 wants to merge 12 commits into
Closed
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the rollout backend from SGLang to vLLM, introducing vLLM-specific argument parsing, engine management via VLLMEngine, and rollout generation logic. It also refactors weight synchronization to use vLLM's native weight transfer engines (both colocated IPC and distributed NCCL). The reviewer feedback highlights three key areas for improvement: handling missing routed_experts gracefully on aborted samples in vllm_rollout.py to avoid crashes, reusing init_weight_transfer_engine in vllm_engine.py to eliminate duplicate initialization logic, and guarding _update_weights_vllm_packed in update_weight_from_distributed.py to prevent crashes on non-DP0 ranks where _model_update_groups is uninitialized.
Comment on lines
+184
to
+189
| if routed is None: | ||
| raise RuntimeError( | ||
| "vLLM routing replay: missing choices[0].routed_experts on /inference/v1/generate response. " | ||
| "Check vLLM 0.22+ was launched with --enable-return-routed-experts." | ||
| ) | ||
| if not isinstance(routed, str): |
There was a problem hiding this comment.
If a rollout generation task is aborted, the vLLM server might return an incomplete response or omit the routed_experts metadata. Raising a RuntimeError here when sample.status == Sample.Status.ABORTED will cause the abort task gathering loop to crash, preventing graceful cleanup and recovery. We should handle missing routed_experts gracefully (e.g., by logging a warning and returning) if the sample is already marked as aborted.
if routed is None:
if sample.status == Sample.Status.ABORTED:
logger.warning("vLLM routing replay: missing routed_experts on aborted sample.")
return
raise RuntimeError(
"vLLM routing replay: missing choices[0].routed_experts on /inference/v1/generate response. "
"Check vLLM 0.22+ was launched with --enable-return-routed-experts."
)
Comment on lines
+928
to
+959
| def init_weights_update_group(self, master_address, master_port, rank_offset, world_size, group_name, backend): | ||
| """Call ``POST /init_weight_transfer_engine`` with an ``init_info`` block. | ||
|
|
||
| ``group_name`` / ``backend`` are accepted for a uniform caller signature but are not sent to vLLM. | ||
| Always uses the vllm-native weight transfer engine; reload-on-continue fallback is no longer supported. | ||
| """ | ||
| del group_name, backend | ||
| payload = { | ||
| "init_info": { | ||
| "master_address": master_address, | ||
| "master_port": master_port, | ||
| "rank_offset": rank_offset, | ||
| "world_size": world_size, | ||
| } | ||
| } | ||
| init_timeout_s = float(os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_HTTP_TIMEOUT_SEC", "900")) | ||
| last_error = None | ||
| for attempt in range(1, 4): | ||
| try: | ||
| response = self._post_json("init_weight_transfer_engine", payload, timeout=init_timeout_s) | ||
| response.raise_for_status() | ||
| try: | ||
| return response.json() | ||
| except Exception: | ||
| return {"ok": True, "raw": response.text} | ||
| except Exception as e: | ||
| last_error = e | ||
| if attempt < 3: | ||
| logger.warning("init_weight_transfer_engine attempt %s/3 failed: %s", attempt, e) | ||
| time.sleep(2 * attempt) | ||
| raise RuntimeError(f"vLLM init_weight_transfer_engine failed: {last_error}") from last_error | ||
|
|
There was a problem hiding this comment.
The init_weights_update_group method contains identical retry and HTTP POST logic to init_weight_transfer_engine. To improve maintainability and reduce code duplication, init_weights_update_group should reuse init_weight_transfer_engine directly.
Suggested change
| def init_weights_update_group(self, master_address, master_port, rank_offset, world_size, group_name, backend): | |
| """Call ``POST /init_weight_transfer_engine`` with an ``init_info`` block. | |
| ``group_name`` / ``backend`` are accepted for a uniform caller signature but are not sent to vLLM. | |
| Always uses the vllm-native weight transfer engine; reload-on-continue fallback is no longer supported. | |
| """ | |
| del group_name, backend | |
| payload = { | |
| "init_info": { | |
| "master_address": master_address, | |
| "master_port": master_port, | |
| "rank_offset": rank_offset, | |
| "world_size": world_size, | |
| } | |
| } | |
| init_timeout_s = float(os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_HTTP_TIMEOUT_SEC", "900")) | |
| last_error = None | |
| for attempt in range(1, 4): | |
| try: | |
| response = self._post_json("init_weight_transfer_engine", payload, timeout=init_timeout_s) | |
| response.raise_for_status() | |
| try: | |
| return response.json() | |
| except Exception: | |
| return {"ok": True, "raw": response.text} | |
| except Exception as e: | |
| last_error = e | |
| if attempt < 3: | |
| logger.warning("init_weight_transfer_engine attempt %s/3 failed: %s", attempt, e) | |
| time.sleep(2 * attempt) | |
| raise RuntimeError(f"vLLM init_weight_transfer_engine failed: {last_error}") from last_error | |
| def init_weights_update_group(self, master_address, master_port, rank_offset, world_size, group_name, backend): | |
| """Call ``POST /init_weight_transfer_engine`` with an ``init_info`` block. | |
| ``group_name`` / ``backend`` are accepted for a uniform caller signature but are not sent to vLLM. | |
| Always uses the vllm-native weight transfer engine; reload-on-continue fallback is no longer supported. | |
| """ | |
| del group_name, backend | |
| payload = { | |
| "init_info": { | |
| "master_address": master_address, | |
| "master_port": master_port, | |
| "rank_offset": rank_offset, | |
| "world_size": world_size, | |
| } | |
| } | |
| return self.init_weight_transfer_engine(payload) |
Comment on lines
+223
to
+226
| def _update_weights_vllm_packed(self, converted_named_tensors: list[tuple[str, torch.Tensor]]) -> None: | ||
| """Single-shot vLLM weight update using packed broadcast.""" | ||
| while not ray.get(self.rollout_engine_lock.acquire.remote()): | ||
| time.sleep(0.1) |
There was a problem hiding this comment.
In multi-GPU setups with Data Parallelism (DP > 1), self._is_pp_src_rank can be True on multiple DP ranks (e.g., DP=0 and DP=1 at PP=0). However, self._model_update_groups is only initialized on the global source rank (DP=0, TP=0, PP=0). If other DP ranks call _update_weights_vllm_packed, they will pass self._model_update_groups as None to update_weights_from_distributed, causing a crash in NCCLWeightTransferEngine.trainer_send_weights. We should guard the execution of _update_weights_vllm_packed to ensure it only runs on the rank where self._model_update_groups is initialized.
def _update_weights_vllm_packed(self, converted_named_tensors: list[tuple[str, torch.Tensor]]) -> None:
"""Single-shot vLLM weight update using packed broadcast."""
if self._model_update_groups is None:
return
while not ray.get(self.rollout_engine_lock.acquire.remote()):
time.sleep(0.1)
aoshen02
force-pushed
the
aoshen/vllm-mirror-sglang-arch
branch
5 times, most recently
from
May 28, 2026 13:58
fceb31e to
db45cf6
Compare
aoshen02
commented
May 28, 2026
aoshen02
left a comment
aoshen02
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code review focused on whether this PR actually delivers what its title and the linked RFC (#65) promise — "mirror SGLang's multi-node rollout architecture". The plumbing is mostly in place, but I found several issues that block multi-node from actually working, plus a couple of regressions on the single-node path. Details inline.
Top-level concerns:
- Multi-node (nnodes>1) will crash inside vLLM — missing
--data-parallel-backend mp. This is the constraint #65 explicitly called out. - The multi-node code path has zero end-to-end coverage. plan.md Phase 5 is all
[ ]. Every "passed" test in Phase 1 is nnodes=1, i.e. effectively the old code path. launch_server_processnow leaks the OS process on health-check failure.- External engines silently stop deregistering from the router on shutdown.
_init_externalescalates TP mismatch from a logged warning to a hardAssertionError— undocumented breaking change.
Please either run the simulated multi-node scenario (--num-gpus-per-node 4 --rollout-num-gpus-per-engine 8) before merge, or split this PR so the SGLang-symmetric refactor lands first and the multi-node CLI flag work lands as a follow-up.
| "--master-addr", | ||
| str(server_args["master_addr"]), | ||
| "--master-port", | ||
| str(server_args["master_port"]), |
Collaborator
Author
There was a problem hiding this comment.
Blocker for the stated multi-node goal. vLLM itself asserts in ParallelConfig (reference/vllm/vllm/engine/arg_utils.py:1819):
assert self.data_parallel_backend == "mp" or self.nnodes == 1, (
"nnodes > 1 is only supported with data_parallel_backend=mp"
)This PR unconditionally passes --nnodes, but never sets --data-parallel-backend mp. The instant nnodes >= 2, the vLLM subprocess will fail this assertion before the server starts. RFC #65 explicitly listed this as a vLLM constraint ("--distributed-executor-backend mp required when nnodes > 1") — the constraint is real, only the flag name in the RFC was slightly off (it's data_parallel_backend, not distributed_executor_backend).
Fix: when server_args["nnodes"] > 1, auto-append --data-parallel-backend mp here (and probably also --distributed-executor-backend mp — check the matrix in arg_utils.py around line 2266 to see if PP needs it too).
This bug is undetected because plan.md Phase 5 (multi-node) was never run; every "passing" integration test in Phase 1 has rollout_num_gpus_per_engine <= num_gpus_per_node, so nnodes==1 and the assertion is trivially satisfied.
| _wait_server_healthy( | ||
| base_url=f"http://{server_args['host']}:{server_args['port']}", | ||
| process=p, | ||
| ) |
Collaborator
Author
There was a problem hiding this comment.
Process-leak regression on health-check failure.
Old code:
self.process = launch_server_process(...) # always returns
_wait_server_healthy(self._http_base(), process=self.process) # may raiseIf the wait raised, self.process was still assigned, and shutdown() could clean up.
New code (here):
p = _spawn_ctx.Process(...); p.start()
if server_args["node_rank"] != 0:
return p
_wait_server_healthy(base_url=..., process=p) # may raise → function never returns
return pIf _wait_server_healthy raises, the caller's self.process = launch_server_process(server_args) never executes — self.process stays None, but p is still a live OS process. shutdown() then short-circuits (if self.process is None: return) and the subprocess leaks.
Fix: either (a) accept engine: VLLMEngine and assign engine.process = p before the health check, or (b) wrap the wait in try/except, set process state, then re-raise.
| sglang_overrides: dict | None = None, | ||
| num_gpus_per_engine: int | None = None, | ||
| ) -> dict: | ||
| del nccl_port, worker_type, disaggregation_bootstrap_port, sglang_overrides |
Collaborator
Author
There was a problem hiding this comment.
Four of the eleven parameters (nccl_port, worker_type, disaggregation_bootstrap_port, sglang_overrides) are accepted and immediately del-ed. This is a private function with exactly one caller (VLLMEngine.init). If the intent is signature symmetry with SGLang's _compute_server_args for a future shared interface, please add a one-line docstring saying so. Otherwise drop them — keeping dead params (especially nccl_port, which looks like it should matter for NCCL weight sync) invites someone later to wire them up wrongly.
| if self.args.rollout_external: | ||
| self._init_external() | ||
| if self.node_rank == 0: | ||
| self._init_external(server_args, external_engine_need_check_fields=("tp_size",)) |
Collaborator
Author
There was a problem hiding this comment.
Sanity check is hardcoded to TP only. The PR's stated goal is parity with SGLang's multi-node PP/EP, but here the external-engine cross-check is restricted to tp_size. An external vLLM server with mismatched pipeline_parallel_size, data_parallel_size, or nnodes would silently pass.
/server_info returns the full parallel_config; extending _get_actual_server_args to also return pp_size/dp_size/nnodes and adding them to external_engine_need_check_fields is a one-line change here. Worth doing now while the API is being designed; harder to add once external-engine users depend on the lenient behavior.
| for name in external_engine_need_check_fields: | ||
| expect_value = expect_server_args.get(name) | ||
| actual_value = actual_server_args.get(name) | ||
| assert ( |
Collaborator
Author
There was a problem hiding this comment.
Undocumented breaking change: warning → AssertionError.
Old behavior in _wait_external_config_ready:
if actual_tp is not None and actual_tp != expect_tp:
logger.warning(
"External vLLM server_info TP mismatch: expect=%s actual=%s (weak check)",
expect_tp, actual_tp,
)The (weak check) annotation was deliberate.
New behavior here:
assert actual_value == expect_value, f"..."This turns a previously survivable mismatch into a hard AssertionError that prevents startup. I agree the new behavior is probably correct, but it's a breaking change for users running external vLLM servers with intentional mismatches (e.g. dev-time A/B). Please call it out in the PR description.
Also related: if /server_info's shape changes or the field is missing, actual_value is None and the assert message just says actual_value=None expect_value=4. Consider including the raw response body so failures are debuggable without re-running.
| response.raise_for_status() | ||
| parallel_cfg = response.json().get("vllm_config", {}).get("parallel_config", {}) | ||
| return { | ||
| "tp_size": parallel_cfg.get("tensor_parallel_size"), |
Collaborator
Author
There was a problem hiding this comment.
Defensive nit: if /server_info doesn't have vllm_config.parallel_config (older vLLM, different API shape, or proxy in front), this returns {"tp_size": None}. Downstream, the assert at line 640 fires with message expect_value=4 actual_value=None, which doesn't tell you whether the server is misconfigured or the response shape is unexpected.
Suggest: when parallel_cfg is empty or the field is None, either log response.json() at warning level or raise a clearer error explaining the response shape mismatch.
|
|
||
| ## Phase 5: New Multi-Node-Rank Coverage For This Refactor | ||
|
|
||
| - [ ] Single-host simulated multi-node vLLM topology |
Collaborator
Author
There was a problem hiding this comment.
The actual reason this PR exists is untested. Phase 5 is the only phase that exercises the new code path — nnodes > 1, node_rank != 0, --headless worker spawning, head-only router registration, head-only health-check. Both items here are [ ].
Everything in Phase 1 that's marked [x] uses rollout_num_gpus_per_engine <= num_gpus_per_node, which means nnodes = max(1, gpus_per_engine // num_gpus_per_node) == 1 and node_rank = rank % 1 == 0 for every actor. That is functionally identical to the pre-PR code path (modulo the dead --nnodes 1 --node-rank 0 flags now appended to the cmd).
The single-host simulation listed right below — --num-gpus-per-node 4 --rollout-num-gpus-per-engine 8 on one H200 — is the minimum viable test for this refactor. It would have caught the missing --data-parallel-backend mp flag I flagged in vllm_engine.py:348. Please run it before merge.
| @@ -0,0 +1,290 @@ | |||
| # vLLM Mirror-SGLang Architecture Test Plan | |||
Collaborator
Author
There was a problem hiding this comment.
Meta: this looks like a personal test-tracking scratchpad — 290 lines of run IDs, absolute paths under /home/aoshen/vime-test-runs/..., ssh-host-specific notes, and harness commands referencing /home/aoshen/vime/.worktree/.... None of this is useful to future readers of the repo, and the absolute paths will rot immediately.
Suggest moving to a personal scratchpad outside the repo and removing from this PR. If you want a permanent record of the test matrix design, a much shorter docs/dev/multi-node-rollout-test-matrix.md (without run logs / personal paths) would carry that intent without the noise.
| "master_addr", | ||
| "master_port", | ||
| "nnodes", | ||
| "node_rank", |
Collaborator
Author
There was a problem hiding this comment.
These four dests are added to SKIPPED_DESTS so users can't pass --vllm-master-addr etc., which is correct since the orchestrator computes them. But the comment says "Ray ServerGroup computes these exactly like SGLang" — that's only half true: SGLang's _compute_server_args computes them per-engine, and so does this PR's new _compute_server_args in vllm_engine.py. The slime/ray/rollout.py ServerGroup just allocates dist_init_addr (host:port); the splitting into master_addr/master_port happens in vllm_engine.py:283. Tightening the comment to "vllm_engine._compute_server_args derives these from dist_init_addr" would point readers at the right code.
| assert process.started is True | ||
| assert waits == [] | ||
|
|
||
|
|
Collaborator
Author
There was a problem hiding this comment.
This test stubs out _build_vllm_cmd_and_env (line 273 of the test) and only verifies that _wait_server_healthy is/isn't called based on node_rank. The actual command construction — which is where the bugs I flagged live (missing --data-parallel-backend mp, --headless placement, --master-addr/--master-port wiring) — has zero unit coverage.
Suggest adding a separate test that calls _build_vllm_cmd_and_env(server_args) with nnodes=2, node_rank=0 and nnodes=2, node_rank=1, then asserts the cmd contains the expected --nnodes 2 --node-rank {0,1} --master-addr ... --master-port ... --data-parallel-backend mp flags, and that --headless is present iff node_rank != 0. That single test would have caught the --data-parallel-backend mp omission.
aoshen02
force-pushed
the
aoshen/vllm-mirror-sglang-arch
branch
from
May 28, 2026 14:21
db45cf6 to
8c8fcde
Compare
aoshen02
commented
May 28, 2026
aoshen02
left a comment
aoshen02
left a comment
Collaborator
Author
There was a problem hiding this comment.
Second pass. Most v1 blockers were addressed; flagging the remaining issues + one regression v2 introduced.
Addressed in v2 — confirmed
- ✅
--data-parallel-backend mp --distributed-executor-backend mpare now appended whennnodes > 1. Matches the vLLMParallelConfigassert. Backed bytest_build_vllm_cmd_multi_node_topology_flagswhich validates the cmd shape. - ✅
launch_server_processnow wraps_wait_server_healthyin try/except and terminates the subprocess on failure. Tested bytest_launch_server_process_terminates_on_health_failure. - ✅ External sanity check expanded from TP-only to
(tp_size, pp_size, dp_size, nnodes), with explicitRuntimeErrorwhen/server_infois missingparallel_config, and a clearerAssertionErrormessage including both expect/actual dicts. - ✅ External engine deregister regression is gone —
shutdown()is back to pre-PR ordering (_deregister_worker_from_router()runs first, then short-circuits forrollout_external). - ✅
plan.mdremoved from the diff. - ✅
arguments.pycomment now points atvllm_engine._compute_server_args. - ✅
_compute_server_argsdead params now have a docstring explaining the SGLang-symmetry intent.
Remaining
- 🔴
self.node_rankno longer initialized in__init__— this v2 also droppedself.node_rank = 0; the conftest workaround attests/unit/backends/vllm_utils/conftest.py:34(engine.node_rank = 0) is the tell. Inline comment below. - 🟡 Subprocess-tree leak in
launch_server_processfailure path —p.terminate()/p.kill()only kill the direct child; vLLM serve forks worker processes that escape. The goodshutdown()path useskill_process_tree(pid). Inline comment below. - 🟡 Multi-node still has zero end-to-end coverage — the unit test only validates the cmd string; the actual
vllm serve --nnodes 2 --data-parallel-backend mp --distributed-executor-backend mp ...invocation has never been launched. The single-host simulation (--num-gpus-per-node 4 --rollout-num-gpus-per-engine 8) is still the cheapest way to gain real confidence. - 🟡 PR description still references deleted
plan.md— "Detailed logs and running checklist are inplan.md." Needs to go. - 🟡
_update_weights_vllm_packedsilent no-op masks rank-routing bugs — inline below. - 🟡
vllm_rollout.pyaborted-sample early-return — needs a comment explaining the training-side consequence. Inline below.
Not blocking, but worth resolving before merge.
| @@ -448,8 +548,6 @@ def __init__( | |||
| self.num_gpus_per_engine = num_gpus_per_engine | |||
| self.process: multiprocessing.Process | None = None | |||
| self._weight_version: str | None = None | |||
Collaborator
Author
There was a problem hiding this comment.
Regression: self.node_rank is no longer initialized in __init__.
The pre-PR __init__ had self.node_rank = 0; v1 removed it ("multi-node worker rank is not used"), and v2 still doesn't restore it. self.node_rank is now only set inside init() (line 591). That leaves a window in which the attribute simply doesn't exist on the instance:
tests/unit/backends/vllm_utils/conftest.py:34had to addengine.node_rank = 0after constructingVLLMEngine— that workaround is a strong signal you already hit this.- In production:
slime/ray/rollout.py:474collectseng.shutdown.remote()calls on dispose. Ifinit()raises before line 591 (e.g._compute_server_argsthrows, port allocation fails, or external health check times out), the orchestrator's cleanup invokesshutdown()→_deregister_worker_from_router()→ readsself.node_rank→AttributeError. The except in rollout.py logs it as "non-fatal" and continues, so the bug would be invisible in logs except as a stack trace.
Fix: restore the default in __init__:
self.process: multiprocessing.Process | None = None
self._weight_version: str | None = None
self.node_rank: int = 0 # overwritten in init(); kept here so error-path shutdown is safeThis also makes the conftest workaround unnecessary.
| @@ -241,27 +252,78 @@ def _serialize_weight_transfer_config(value) -> str: | |||
| return serialized | |||
|
|
|||
|
|
|||
Collaborator
Author
There was a problem hiding this comment.
Subprocess-tree leak on health-check failure.
p.terminate() / p.kill() only signal the direct child process. vllm serve forks N worker processes (one per TP rank, plus the API server); when the wrapper dies, those orphan workers stay alive holding the GPUs until something else reaps them. The orphaned workers will block the next Ray actor from re-binding the same --port/GPU.
The existing shutdown() (line 784) deliberately uses kill_process_tree(pid) from vllm.utils.system_utils to walk the whole tree — the failure path should do the same:
except Exception:
logger.exception("vLLM server health check failed; terminating subprocess pid=%s", p.pid)
try:
from vllm.utils.system_utils import kill_process_tree
kill_process_tree(p.pid)
except Exception as kill_err:
logger.warning("kill_process_tree failed (%s); falling back to terminate.", kill_err)
if p.is_alive():
p.terminate(); p.join(timeout=15)
if p.is_alive():
p.kill(); p.join(timeout=15)
raiseAdding the test test_launch_server_process_terminates_on_health_failure is good, but it asserts on _FakeProcess.terminated is True — it doesn't verify the tree is actually reaped. If you take the kill_process_tree suggestion, extend the test to assert that path is called too.
| def _update_weights_vllm_packed(self, converted_named_tensors: list[tuple[str, torch.Tensor]]) -> None: | ||
| """Single-shot vLLM weight update using packed broadcast.""" | ||
| if self._model_update_groups is None: | ||
| return |
Collaborator
Author
There was a problem hiding this comment.
Functionally correct (mirrors disconnect_rollout_engines's guard at line 104), but the silent return hides a real failure mode: if _is_pp_src_rank is True on a rank but _model_update_groups is unexpectedly None, weight update silently skips and training continues with stale rollout weights — much harder to debug than a crash.
Minimal change:
if self._model_update_groups is None:
logger.debug(
"_update_weights_vllm_packed: skipping rank without _model_update_groups "
"(expected on non-DP-0 ranks where _is_pp_src_rank is True)"
)
returnThis way the no-op is visible to anyone running with verbose logging if rollout weights ever look stale.
| if sample.status == Sample.Status.ABORTED and sample.response_length == 0: | ||
| return | ||
| if routed is None: | ||
| if sample.status == Sample.Status.ABORTED: |
Collaborator
Author
There was a problem hiding this comment.
Two concerns with this branch:
(1) The aborted sample still gets trained on without routing info. The early return at line 183 (response_length == 0) drops the sample entirely. This new branch fires when response_length > 0 (some tokens were generated) AND status == ABORTED AND routed_experts is None. So the sample IS included in training, just without rollout_routed_experts. If downstream training code assumes routing replay is always available when use_rollout_routing_replay=True, it will hit a separate (probably worse) error later. Worth a one-line comment explaining the training-side contract:
if sample.status == Sample.Status.ABORTED:
# Aborted mid-generation may yield partial tokens but no routed_experts.
# Sample stays in the batch; downstream training must tolerate rollout_routed_experts=None.
logger.warning("vLLM routing replay: missing routed_experts on aborted sample.")
return(2) The warning has no sample identifier. When this fires repeatedly under load you'll get an unattributable wall of warnings. Include sample.idx (or whatever the canonical id is) in the log message so debugging is possible.
| logger.warning("init_weight_transfer_engine attempt %s/3 failed: %s", attempt, e) | ||
| time.sleep(2 * attempt) | ||
| raise RuntimeError(f"vLLM init_weight_transfer_engine failed: {last_error}") from last_error | ||
| return self.init_weight_transfer_engine(payload) |
Collaborator
Author
There was a problem hiding this comment.
Refactor is correct (init_weight_transfer_engine carries the same 3-attempt retry loop, so behavior is preserved). One leftover concern from the pairing standpoint:
destroy_weights_update_group() at line 937 was a no-op even before this PR. With init_weights_update_group now delegating to init_weight_transfer_engine, there's still no symmetric destroy_weight_transfer_engine call anywhere — meaning the lifecycle is init_weight_transfer_engine ➜ … ➜ (nothing). If vLLM 0.21+ relies on the trainer to call a teardown to release NCCL communicators / handles cleanly, leaks accumulate across reconnects.
Not necessarily wrong (vLLM may own the destroy internally), but please verify against vLLM 0.21+ and either add a one-line comment here "vLLM owns the destroy; no teardown call needed" or wire up the destroy call. The current state leaves a future reader unsure which interpretation is intended.
| ) | ||
| else: | ||
| self._init_normal() | ||
| self._init_normal(server_args) |
Collaborator
Author
There was a problem hiding this comment.
Behavioral question on external + multi-node mode: when rollout_external=True and nnodes > 1, this branch spawns nnodes Ray actors per logical engine but only node_rank == 0 does any work — the other nnodes - 1 actors just log "skip HTTP health check" and sit on a GPU placement slot.
For managed engines that's fine because the workers are actually running vllm serve --headless. For external mode there's no vllm serve subprocess at all on node_rank > 0, so those slots are pure dead weight in the Ray placement group.
If this is intentional symmetry (so the Ray placement shape is identical between managed and external), please add a one-line comment saying so. Otherwise consider sizing the placement group based on whether external mode is active.
| ) -> dict: | ||
| """Compute vLLM launch args with a signature kept parallel to SGLang's helper.""" | ||
| del nccl_port, worker_type, disaggregation_bootstrap_port, sglang_overrides | ||
| gpus_per_engine = num_gpus_per_engine or args.rollout_num_gpus_per_engine |
Collaborator
Author
There was a problem hiding this comment.
Nit on the docstring you added: "signature kept parallel to SGLang's helper" is good, but the four del-ed parameters (nccl_port, worker_type, disaggregation_bootstrap_port, sglang_overrides) make this function impossible to grep for "who passes nccl_port to vLLM" — they look like real plumbing.
At minimum, name the parameter _nccl_port (and friends), or annotate them # noqa: ARG001 so it's clear at the call site that the unused-ness is intentional and not a forgotten wire-up.
aoshen02
force-pushed
the
aoshen/vllm-mirror-sglang-arch
branch
from
May 28, 2026 14:39
8c8fcde to
a72f502
Compare
aoshen02
commented
May 28, 2026
aoshen02
left a comment
aoshen02
left a comment
Collaborator
Author
There was a problem hiding this comment.
Third pass. Every v2 inline comment is resolved. Remaining items are minor polish, not blockers.
Resolved in v3
- ✅
self.node_rank(plusserver_host,server_port,router_ip,router_port) initialized in__init__— error-pathshutdown()no longerAttributeErrors. Conftest workaround removed cleanly. - ✅
launch_server_processfailure path now useskill_process_tree(p.pid)withterminate/killas fallback, mirroringshutdown(). Test updated to mockvllm.utils.system_utils.kill_process_treeand assert it was called with the right pid. - ✅
_update_weights_vllm_packedaddslogger.debugon the silent skip; future debugging has a breadcrumb. - ✅ Aborted-sample warning includes
sample.indexand the comment explains the training-side contract ("training masks aborted samples out"). - ✅
destroy_weights_update_groupdocstring now explicitly states vLLM owns transfer-engine teardown via server-process lifetime. - ✅ External + multi-node log line explains the Ray actor topology symmetry rationale.
- ✅
_deregister_worker_from_routergained defensive checks against missingserver_host/server_port. - ✅ PR body updated —
plan.mdreference removed, single-host simulated multi-node test gap explicitly called out as a pre-merge requirement.
Remaining (non-blocking polish)
- 🟡 Forced multi-node backend flags don't respect user override (could result in duplicate cmd flags). Inline comment below.
- 🟡
nnodesdivisibility silently rounds down on misconfig. Inline comment below. - 🟢 Per-rank seed semantics in multi-rank-per-engine — pre-existing pattern, just worth a comment.
None of these block merge. Main outstanding item is the H200 single-host multi-node smoke run you already flagged in the PR description — please run it and post the log.
| if node_rank != 0: | ||
| cmd.append("--headless") | ||
| if server_args["nnodes"] > 1: | ||
| cmd += ["--data-parallel-backend", "mp", "--distributed-executor-backend", "mp"] |
Collaborator
Author
There was a problem hiding this comment.
Two small redundancy/cleanliness issues with this block:
(a) --data-parallel-backend mp is the vLLM default. From reference/vllm/vllm/config/parallel.py:133:
data_parallel_backend: DataParallelBackend = "mp"The nnodes>1 assertion (data_parallel_backend == "mp" or nnodes == 1) is therefore satisfied by default. Adding the flag explicitly is harmless but redundant. The defensive value of explicitness here is OK; just worth a comment so future readers don't think this flag is load-bearing.
(b) Both flags bypass _user_overrode(). Compare to lines 412-418 (the --gpu-memory-utilization pattern), which checks _user_overrode("vllm_gpu_memory_utilization") and emits a single --gpu-memory-utilization flag with either user value or vime default.
Current code unconditionally appends mp, then _forward_vllm_cli_args later appends the user's --vllm-data-parallel-backend X (if any). vLLM argparse takes last-wins, so user choice still wins, but the cmd ends up with duplicate --data-parallel-backend mp ... --data-parallel-backend ray. Messy logs.
Minor refactor:
if server_args["nnodes"] > 1:
dpb = args.vllm_data_parallel_backend if _user_overrode("vllm_data_parallel_backend") else "mp"
deb = args.vllm_distributed_executor_backend if _user_overrode("vllm_distributed_executor_backend") else "mp"
cmd += ["--data-parallel-backend", dpb, "--distributed-executor-backend", deb]This also means the auto-forwarder will skip these dests (since the value matches what we already passed), avoiding the duplicate-flag pattern.
| """Compute vLLM launch args with a signature kept parallel to SGLang's helper.""" | ||
| del nccl_port, worker_type, disaggregation_bootstrap_port, sglang_overrides | ||
| gpus_per_engine = num_gpus_per_engine or args.rollout_num_gpus_per_engine | ||
| nnodes = max(1, gpus_per_engine // args.num_gpus_per_node) |
Collaborator
Author
There was a problem hiding this comment.
Latent misconfig: this rounds down silently when gpus_per_engine is not a multiple of num_gpus_per_node. E.g. rollout_num_gpus_per_engine=12, num_gpus_per_node=8 → nnodes = 12 // 8 = 1, then local_num_gpus = min(8, 12) = 8, so vime asks vLLM to run TP=12 on a single 8-GPU node. vLLM will fail in a confusing way (likely a CUDA visibility error or NCCL world-size mismatch).
The SGLang side has the same formula, so this is consistent — but "matches SGLang's pre-existing latent bug" isn't a strong argument. Suggest a single guard so users see the problem at orchestration time, not at vLLM startup:
if gpus_per_engine % args.num_gpus_per_node != 0 and gpus_per_engine > args.num_gpus_per_node:
raise ValueError(
f"rollout_num_gpus_per_engine ({gpus_per_engine}) must be a multiple of "
f"num_gpus_per_node ({args.num_gpus_per_node}) for multi-node engines."
)| model = model_path | ||
| tp = args.rollout_num_gpus_per_engine | ||
| host_for_subprocess = server_args["host"].strip("[]") | ||
| seed = getattr(args, "seed", 1234) + rank |
Collaborator
Author
There was a problem hiding this comment.
Pre-existing pattern that the multi-node refactor quietly amplifies: seed = base + rank, where rank is the global Ray-actor rank. With one engine per actor (pre-PR), this meant "engine N gets seed base+N" — clean.
Now with multi-rank-per-engine, engine 0 actor 0 gets base+0, engine 0 actor 1 gets base+1, engine 1 actor 0 gets base+2, etc. Head and worker of the same engine get different --seed values.
In practice this is probably fine — vLLM workers don't use --seed for sampling (only the API server does), and weight init is overwritten by weight sync anyway. But it's a subtle reproducibility shift: two engines running the same model on the same input now have different head-rank seeds depending on nnodes.
Not blocking, but please leave a one-line comment so future-you doesn't spend an afternoon debugging "why do my two runs produce different rollouts when I added a node":
# Per-actor seed so each Ray actor's RNG is independent; vLLM headless workers ignore --seed,
# so head/worker seed divergence within one logical engine is benign.
seed = getattr(args, "seed", 1234) + rank
aoshen02
force-pushed
the
aoshen/vllm-mirror-sglang-arch
branch
from
May 28, 2026 14:51
a72f502 to
872adb9
Compare
…for PR #66 Adds the parametrized test file and orchestration scripts used to validate the new vLLM nnodes>1 path end-to-end on 2× H200×8 with Qwen3-30B-A3B: - test_qwen3_30B_A3B_pr66_sweep.py: 11 configs (ref/s1-s10) covering single-host colocate, multi-engine, vLLM PP=2, Megatron PP=2, disagg, and cross-host nnodes=2 colocate + disagg. Picked via PR66_CONFIG env var. - head.sh / worker.sh: container entrypoints that fix /etc/hosts (Gloo full-mesh reachability), build the Ray cluster across two hosts, run the test on rank 0, and tear down via NFS-shared barrier files. - launch_cross_host.sh: driver that spawns both containers with --network host + the env vars required for cross-host Ray / NCCL / Gloo (NCCL_SOCKET_IFNAME, NCCL_IB_DISABLE, GLOO_SOCKET_IFNAME, SLIME_HOST_IP, RAY_NODE_IP_ADDRESS, SLIME_VLLM_HEALTH_TIMEOUT_SEC). - README.md: config matrix, single-host and cross-host run instructions, and the deployment requirements list discovered during validation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per review feedback the examples/ directory is the wrong home for these —
they are an integration test (the .py) plus the orchestration scripts that
drive it across two hosts (the .sh trio). Moves:
examples/multinode_validation/test_qwen3_30B_A3B_pr66_sweep.py
→ tests/test_qwen3_30B_A3B_pr66_sweep.py
examples/multinode_validation/{head,worker,launch_cross_host}.sh
→ tests/multinode/{head,worker,launch_cross_host}.sh
README.md is removed — its content (config matrix, run instructions,
cross-host deployment requirements) is fully covered inline in the PR
description.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A single step only proves the path wires up; bumping --num-rollout to 2 exercises a second rollout→IPC-weight-sync→train cycle so the sweep also catches multi-step issues (weight-version drift, grad_norm blowup, IPC state-machine reuse across updates). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vLLM pipeline-parallel CUDA-graph capture deadlocks for Qwen3-30B-A3B in the colocate setup — all ranks pin 100% GPU with no forward progress (observed hang at 2% of the PIECEWISE capture for >25 min). The hang is inside vLLM's PP graph capture, orthogonal to the rollout topology this sweep validates, so the PP configs (s8/s9) now pass --vllm-enforce-eager to exercise the real pipeline-parallel rollout path without tripping the capture deadlock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 tasks
…nfigs
Enable vLLM data-parallel + expert-parallel ("wide-EP") MoE rollout:
- arguments.py validate_args: vLLM grabs tp*pp*dp GPUs per engine under the
mp/ray backend (ParallelConfig.world_size_across_dp = tp*pp*dp). The previous
tp = engine_gpus // pp ignored DP, so any --vllm-data-parallel-size > 1
over-committed GPUs (e.g. dp=2 on an 8-GPU engine asked vLLM for 16). Now
tp = engine_gpus // (pp * dp); for dp=1 this is unchanged.
- test sweep: add s11 (dp=2/tp=4), s12 (dp=4/tp=2), s13 (cross-host dp=2),
all with --vllm-enable-expert-parallel. DP/PP configs run --vllm-enforce-eager
(vLLM multi-dim CUDA-graph capture deadlocks for this MoE in colocate).
Validated on 2x H200 (Qwen3-30B-A3B): s12 (dp=4) PASS both steps end-to-end;
the 2 DP EngineCores each take IPC weight updates and rollout/train complete.
s11 (dp=2) intermittently hits a CUDA illegal-memory-access in the DP engine's
process-group watchdog during the 2nd release_memory_occupation (sleep/offload)
— a vLLM sleep-mode x data-parallel interaction, under investigation; not in
the rollout-topology path this PR adds.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…TP+PP+DP) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… H200)
Implement vLLM-native prefill/decode (PD) disaggregation for the rollout
backend, using the NIXL KV connector (the stock vllm_router only does
SGLang-style PD).
- vllm_engine._compute_server_args/_build_vllm_cmd_and_env: stop discarding
worker_type; when worker_type is "prefill"/"decode", launch the engine with
--kv-transfer-config '{"kv_connector":"NixlConnector","kv_role":"kv_both",...}'
and a unique VLLM_NIXL_SIDE_CHANNEL_HOST/PORT (derived from the HTTP port or
the orchestrator-allocated bootstrap port).
- pd_proxy.py: vLLM-native PD proxy. Speaks the surface slime's rollout already
targets (POST /inference/v1/generate, /health, worker listing, broadcast
control) and drives the two-step NIXL handshake: prefill with
kv_transfer_params={do_remote_decode:true} + max_tokens=1, then decode with
{do_remote_prefill:true, **prefill_params}. vLLM's GenerateRequest/Response
both carry kv_transfer_params, so the relay rides the existing endpoint.
- tests/multinode/nixl_pd_proof.sh: standalone 1P1D proof. Launches a prefill
(GPU0) + decode (GPU1) engine, both NixlConnector kv_both, relays one request
P->D. Verified on 2x H200 (r3 image, Qwen3-0.6B): prefill returns NIXL handles,
decode pulls KV and generates correct output. PROOF: PASS.
Remaining for full RL-loop PD: rollout.py builds prefill/decode ServerGroups
for the vllm backend and launches pd_proxy instead of the SGLang mini_lb.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- pd_proxy: workers now register dynamically via the same surface the engines already use against the router (POST/GET/DELETE /workers with worker_type), so the proxy is a drop-in for the "router starts first, engines register later" rollout flow. Round-robins across registered prefill/decode pools. - rollout._start_router: when a model has PD disaggregation and the vLLM backend is selected (opt-in SLIME_VLLM_PD_NIXL=1), spawn run_pd_proxy instead of the SGLang mini_lb. The rest of the rollout path is already backend-generic: from_prefill_num_servers builds prefill/decode ServerGroups, start_engines creates VLLMEngine actors with worker_type, and the engine emits the NIXL --kv-transfer-config + side channel (previous commit). End-to-end chain now in place: prefill/decode VLLMEngines launch with NIXL, register with pd_proxy, and /inference/v1/generate relays P->D. NIXL transfer itself is proven (tests/multinode/nixl_pd_proof.sh PASS); full slime-RL-loop PD smoke is the next test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the proxy-level proof and the one required vLLM-fork fix.
tests/multinode/nixl_pd_proxy_test.sh launches prefill+decode NIXL engines +
slime's pd_proxy, registers the engines, and sends one GenerateRequest through
the proxy over /inference/v1/generate (the exact endpoint slime's rollout uses).
vllm_disagg_serving_kv_transfer.patch: the disagg serving handler
(vllm/entrypoints/serve/disagg/serving.py) accepts kv_transfer_params in its
GenerateRequest schema but never threaded it into the engine, so the NIXL
handshake never engaged on /inference/v1/generate (it worked on /v1/completions).
The fix injects request.kv_transfer_params into SamplingParams.extra_args (vLLM
v1 reads it there). This lives in the vLLM image's disagg server, not slime —
applied here as a documented patch.
Verified on 2x H200 (r3 image, Qwen3-0.6B) WITH the patch: proxy returns HTTP
200 with kv_transfer_params populated and correct decode output
(" Paris. The capital of France is ..."). Without the patch the same request
returns "prefill returned no kv_transfer_params". PROOF: PASS.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…colocate) Mirrors the miles/slime-sibling run-qwen3-4B.sh convergence config: GRPO + clip-higher 0.28 + deepscaler reward, dapo-math-17k train / aime-2024 eval, response 8192. 2-host 16-GPU colocate, pure data-parallel (Megatron TP1 PP1 CP1 => DP16; vLLM rollout TP1 => 16 single-GPU engines). For the PR #66 cross-host colocate path under a real converging workload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aoshen02
added a commit
that referenced
this pull request
May 30, 2026
…p TP
launch_server_process / _init_normal derived tensor-parallel size and
CUDA_VISIBLE_DEVICES from the global --rollout-num-gpus-per-engine, ignoring
the per-engine num_gpus_per_engine already carried on the VLLMEngine actor.
A ServerGroup configured with num_gpus_per_engine greater than the global
flag (e.g. tp=2) therefore launched as tp=1, while the NCCL weight-sync
rendezvous sized world_size from engine_gpu_counts (the per-group value).
The two disagreed: the trainer waited for a rank the under-sized engine
never started, so init_weight_transfer_engine hung for 300s
("3/4 clients joined") and the job failed.
Honor the per-engine num_gpus_per_engine at launch, falling back to the
global flag when unset (matches the SGLang path and PR #66's
_compute_server_args).
Verified on H200: tests/test_qwen2.5_0.5B_vllm_config_distributed now
launches engine0 tp=2 / engine1 tp=1, update_weights completes in 1.1s
(was a 301s timeout), and rollout+eval proceed.
AI assistance (Claude Code) was used for this change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CalvinXKY
pushed a commit
that referenced
this pull request
May 30, 2026
* [Clean] Remove SGLang runtime code
Rebuilt against current main so the PR contains only the SGLang runtime
removal -- the docs / tests-ci / examples / scripts / docker portions were
split into separate PRs that have since merged.
- Delete dead SGLang server/runtime code: sglang_utils/{arguments,sglang_engine}.py,
rollout/sglang_rollout.py, the megatron_utils/sglang.py re-export shim, and all
docker/**/sglang.patch files.
- Rename the rollout config module sglang_utils/sglang_config.py ->
vllm_utils/vllm_config.py (SglangConfig -> VllmConfig, _resolve_sglang_config ->
_resolve_vllm_config, --sglang-config -> --vllm-config); inline the
GPU_MEMORY_TYPE_* constants in rollout.py.
- Add megatron_utils/fp8_helpers.py for the UE8M0 fp8 helpers formerly re-exported
through the sglang shim; repoint quantizer_fp8 to it.
- Swap sglang_router -> vllm_router in http_utils/wandb_utils; drop the dead
sglang-router dependency from requirements.txt.
- Finish the SGLang->vLLM rename in the runtime so it is internally consistent and
matches the tests landing in the tests/CI PR:
* router args --router-* -> --vllm-router-* (vllm_router_ip/port/timeout);
* get_model_url reads vllm_model_routers (aligning with rollout.py);
* --opd-type sglang -> vllm; engine_overrides rename;
* sglang_enable_deterministic_inference -> vllm_enable_deterministic_inference,
wired to a real --vllm-enable-deterministic-inference flag (exports
VLLM_BATCH_INVARIANT=1);
* consistent_hash session-id routing uses vllm-router's x-session-id header;
* drop dead trace helper build_sglang_meta_trace_attrs; de-SGLang comments/docstrings.
- Rename test_sglang_config.py -> test_vllm_config.py and de-SGLang the
plugin-contract tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Address review: finish de-SGLang + fold OPD/router-policy into runtime
- naming: replace residual generic "rollout engine"/"engine"/"comm" wording
with concrete vLLM (engine_overrides -> vllm_overrides; arguments help text;
http_utils comments; rollout.py "inference workers"). sglang->vllm is correct,
sglang->generic is not.
- megatron_to_hf: drop the q_a_proj/kv_a_proj_with_mqa pairing + _cached_tensors
global. That was sglang-only: sglang's loader torch.cat's both shards within a
single load_weights call (needs them co-bucketed), whereas vLLM loads each shard
independently via stacked_params_mapping into fused_qkv_a_proj. Also fix the
misleading "merge into single fused name" comment.
- docker/Dockerfile: remove now-dead sglang/sglang-router --no-deps stubs + the
build-time `import sglang` smoke check (slime no longer imports sglang_router).
- OPD: migrate on_policy_distillation.py teacher logprobs to vLLM /v1/completions
(prompt_logprobs) instead of sglang return_logprob / meta_info.input_token_logprobs.
- routing replay: register --vllm-router-policy (dest=router_policy) so the
consistent_hash x-session-id session-affinity path is actually wired (was dead).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Review follow-ups: mirror slime vllm_config parsing + restore vLLM process cleanup
- vllm_config.from_yaml: drop the needless `models_raw` intermediate and iterate
`data["vllm"]` directly, restoring the "Accept both server_groups / legacy
engine_groups" comment -- mirrors slime's sglang_config.from_yaml line-for-line.
- command_utils.execute_train: re-add a process kill for leftover rollout engines
as `pkill -9 -f "vllm serve"` (the old `pkill -9 sglang` was dropped with no
vLLM equivalent), so stale engines don't hold GPUs/ports across runs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drop test changes from runtime PR; tests live in the tests+CI PR (#40)
The plugin_contracts tests and the test_sglang_config -> test_vllm_config rename
are coupled to the test/CI rename effort and are owned by #40. Restore them to
main here so #18 is purely the SGLang runtime removal. #18 merges first; #40
rebases and re-lands the vLLM test versions.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fp8_helpers: copy SGLang verbatim (fix import crash); pin vLLM deep_gemm env
- fp8_helpers.py: replace the bespoke rewrite with SGLang's exact implementations
of quant_weight_ue8m0 / transform_scale_ue8m0 and their DeepGEMM helpers
(per_block_cast_to_fp8, ceil_to_ue8m0, ceil_div, ceil_align, the torch-impl
packer). deep_gemm is imported lazily inside the functions (as SGLang does), so
module import no longer requires deep_gemm. This fixes the module-level
`NameError: _get_tma_aligned_size` that crashed `import megatron_to_hf` on any
deep_gemm image, and drops the invented sf-stride fixup block that was not in
upstream. Only should_deepgemm_weight_requant_ue8m0 stays vLLM-adapted
(is_deep_gemm_e8m0_used) since SGLang's reads SGLang-internal deep_gemm_wrapper.
- vllm_engine.launch_server_process: set VLLM_USE_DEEP_GEMM=1 +
VLLM_DEEP_GEMM_WARMUP=relax explicitly (setdefault) alongside VLLM_BATCH_INVARIANT,
replacing SGLang's removed deep_gemm precompile/warmup envs. All vLLM engine env
now lives in the subprocess env builder (single source of truth).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fp8_helpers: revert to vLLM impl + fix the import NameError; add vLLM trace attrs
- fp8_helpers.py: keep the vLLM-based implementation (uses vllm.utils.deep_gemm,
consistent with the vLLM runtime) rather than the SGLang verbatim copy. Fix the
module-level crash: the `try` block referenced `_get_tma_aligned_size` before it
was bound (the "pre-imported with fallback" import was never written), which
raised NameError whenever deep_gemm imported successfully -- and NameError is not
caught by `except ImportError`, so `import megatron_to_hf` crashed on any
deep_gemm image. Replace the bogus self-assignment with the real import:
`from vllm.utils.deep_gemm import get_tma_aligned_size as _get_tma_aligned_size`.
- trace_utils/vllm_rollout: add build_vllm_meta_trace_attrs and attach finish_reason
+ token usage to the vllm_inference_generate span (mirrors SGLang's
build_sglang_meta_trace_attrs; vLLM responses lack the pd_* timing, which lives
in vLLM's own OTLP traces).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(opd): score teacher via /inference/v1/generate with prompt_logprobs
Move the vllm OPD teacher path off the OpenAI /v1/completions endpoint onto
vime's native /inference/v1/generate (the same endpoint the rollout engines
use), and fix three latent issues:
1. model field: /inference/v1/generate takes `model` as OPTIONAL. Stop
defaulting to args.hf_checkpoint (the *student* name, which mis-names a
teacher!=student server). Add --opd-teacher-model; send `model` only when
set, otherwise omit it (single-model teacher servers use their loaded model).
2. multimodal: the old code sent image_data to a token-only endpoint, which is
invalid. Raise NotImplementedError until the
/v1/chat/completions/render -> /inference/v1/generate flow is wired (mirrors
slime.rollout.vllm_rollout.generate).
3. logprob robustness: read top-level GenerateResponse.prompt_logprobs, assert
it is present and length-aligned with token_ids, assert the per-sample tensor
covers response_length, and raise (not silently return 0.0) on a missing
token logprob. vLLM always includes the actual prompt token in
prompt_logprobs, so a miss is a real error.
Alignment is unchanged (plp[i] <-> tokens[i], skip pos 0, take [-response_length:]).
Follow-up (separate, in the tests PR): the OPD e2e test must launch a teacher
that exposes /inference/v1/generate and point --rm-url at it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore(clean-sglang): purge SGLang from tools, train scripts, and build infra
tools/: drop dead `args.sglang_enable_ep_moe` shim (read nowhere); reword
profile/replay helpers to vLLM and map analyzer hints to vLLM flags
(--enforce-eager, --gpu-memory-utilization). train{,_async}.py: comments
SGLang -> vLLM.
build infra: remove build_conda.sh (SGLang-only conda path); drop the GB300
sgl-kernel install from the Dockerfile; delete docker/npu_patch/ wholesale.
docker base image: bump to vLLM v0.22.0. justfile ARM recipes now pin the real
multi-arch vLLM base images instead of the dead SGLANG_IMAGE_TAG/
ENABLE_SGLANG_PATCH build-args -- cu129-arm64 -> v0.22.0-cu129-ubuntu2404
(CUDA 12.9), cu13-arm64 -> v0.22.0-ubuntu2404 (the default-CUDA tag is already
CUDA 13.0) + ENABLE_CUDA_13=1. vLLM tags are multi-arch manifests, so docker
selects the arm64 image automatically on an ARM host.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci(clean-sglang): drop conda-build workflow (ran deleted build_conda.sh on SGLang image)
The single build-conda job ran `bash build_conda.sh` (removed in the previous
commit) inside an lmsysorg/sglang container. With the SGLang-only conda path
gone, the whole workflow is dead.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore(clean-sglang): fix stale SGLang refs in docs/skills; tidy comments
docs/conf.py: point the "edit on GitHub" links at vllm-project/vime instead of
the inherited sgl-project.github.io repo. .claude/skills/*: update the dead
`slime/rollout/sglang_rollout.py` references to `vllm_rollout.py` (the real
default is slime.rollout.vllm_rollout.generate_rollout).
justfile: drop the redundant BASE_IMAGE override on release-cu129-arm64 (it
equalled the Dockerfile default; the multi-arch manifest already resolves
arm64). train{,_async}.py: drop stray "the" in the W&B comment.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(cleanup): target renamed vLLM subprocesses in pkill so VRAM is freed
vLLM's set_process_title() renames the VRAM-holding subprocesses
(VLLM::EngineCore, VLLM::Worker_TP*, vllm::router), so their cmdline no
longer contains "vllm serve". The previous `pkill -9 -f "vllm serve"`
matched only the launcher and left engine/worker children holding GPU
memory, leaking it into the next run — masked only by the indiscriminate
`pkill -9 python`, which is unsafe on colocate/shared nodes.
Match both the launcher and the renamed children with
`pkill -9 -f '[v]llm serve|VLL[M]::'`; the [v]/[M] bracket trick keeps the
pattern from matching pkill's own cmdline. This makes the broad python
kill unnecessary, so its already-commented-out lines are removed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(speculative): use method=mtp (not eagle) for embedded-MTP models
vLLM's SpeculativeConfig requires an explicit draft `model` for
method=eagle; with only num_speculative_tokens set it raises
"num_speculative_tokens was provided but without speculative model".
The migrated configs in scripts/examples/docs pass no model, so they must
use method=mtp, which reuses the target checkpoint's embedded MTP layer
(DeepSeek-R1, GLM-4.x-MoE, MiMo, Qwen3-Next/3.5).
The two docs examples that pass an explicit "model" are genuine eagle
usage and are left unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(vllm): launch each rollout engine with its ServerGroup's per-group TP
launch_server_process / _init_normal derived tensor-parallel size and
CUDA_VISIBLE_DEVICES from the global --rollout-num-gpus-per-engine, ignoring
the per-engine num_gpus_per_engine already carried on the VLLMEngine actor.
A ServerGroup configured with num_gpus_per_engine greater than the global
flag (e.g. tp=2) therefore launched as tp=1, while the NCCL weight-sync
rendezvous sized world_size from engine_gpu_counts (the per-group value).
The two disagreed: the trainer waited for a rank the under-sized engine
never started, so init_weight_transfer_engine hung for 300s
("3/4 clients joined") and the job failed.
Honor the per-engine num_gpus_per_engine at launch, falling back to the
global flag when unset (matches the SGLang path and PR #66's
_compute_server_args).
Verified on H200: tests/test_qwen2.5_0.5B_vllm_config_distributed now
launches engine0 tp=2 / engine1 tp=1, update_weights completes in 1.1s
(was a 301s timeout), and rollout+eval proceed.
AI assistance (Claude Code) was used for this change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(router-args): hybrid naming — vllm_ for ip/port, bare router_ for timeout
vllm-router's RouterArgs.from_cli_args only supports prefix "" or "router_"
(never "vllm_router_"), and excludes host/port from its CLI via
exclude_host_port=True. So:
- --vllm-router-ip / --vllm-router-port keep the vllm_ prefix: RouterArgs does
not own these CLI flags, vime does (populated via _start_router's manual
router_args.host/port assignment), so the vllm_ prefix is free and marks them
as vime-owned endpoint config.
- --router-request-timeout-secs goes bare (dest router_request_timeout_secs): it
is a genuine RouterArgs field, so it shares the --router-* namespace with
policy / cache_threshold / retries / … and flows through from_cli_args like
the other knobs.
- --vllm-router-policy keeps dest=router_policy (unchanged).
Also fixes conftest fixture to seed vllm_router_ip/port (was bare router_ip/port,
which never matched the vllm_engine reader) and updates README/README_zh prose.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
CalvinXKY
pushed a commit
that referenced
this pull request
May 30, 2026
* tests + CI: complete sglang→vllm rename across tests/ and .github/ Split from PR #18 (gcl/clean-sglang). One of 4 PRs splitting the original PR #18 by content area: docs (#38) / examples (#39) / **tests+CI** / core runtime. 42 files / +~750 / -~700. These are bundled in a single PR because the CI workflows reference test file names by string — splitting them would create a window where either tests are renamed but CI still points at the old names, or vice versa, breaking CI mid-roll. What this PR does: (A) tests/ (38 files): - Mechanical CLI-flag rename: --sglang-* → --vllm-* equivalents in all test scripts (matches the table now used in scripts/ and examples/). - Variable rename: SGLANG_ARGS → VLLM_ARGS where present. - 4 file renames (R086-R091, all >85% similarity): test_qwen2.5_0.5B_opd_sglang.py → test_qwen2.5_0.5B_opd_vllm.py test_qwen2.5_0.5B_sglang_config.py → test_qwen2.5_0.5B_vllm_config.py test_qwen2.5_0.5B_sglang_config_distributed.py → test_qwen2.5_0.5B_vllm_config_distributed.py test_sglang_config_mixed_offload.py → test_vllm_config_mixed_offload.py test_sglang_config_mixed_offload_ft.py → test_vllm_config_mixed_offload_ft.py tests/utils/test_sglang_config.py → tests/utils/test_vllm_config.py - 2 new tests for the IPC weight-transfer path landed in PR #18: tests/test_update_weight_from_tensor.py tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_tensor.py (These are PR #22 / colocate-IPC test coverage; the production code the slim PR #18 ships will rely on the same code from PR #22.) (B) .github/ (4 files): - workflows/conda-ci.yml: container image lmsysorg/sglang → vime (inferactinc/public:vime-vllm-cu129-latest). - workflows/pr-test.yml + pr-test.yml.j2 (template): * Container images (slimerl/slime[-test]:latest → vime image) on every job that ran on the sglang-era base. * e2e-test-sglang-config job → e2e-test-vllm-config job (renamed label `run-ci-sglang-config` → `run-ci-vllm-config`; matrix `test_file` entries updated to point at the renamed test files in (A)). * e2e-test-megatron + e2e-test-image matrices: `_opd_sglang.py` entries → `_opd_vllm.py`. - ISSUE_TEMPLATE/bug_report.yml: drop the "SGLang version (if relevant):" environment field, add "vLLM version:" and "vllm-router version:" lines. (PR #36 already changed "CUDA/ROCm version" → "CUDA version" earlier; that change is preserved.) Sgl residue intentionally kept (4 hits — all anti-regression assertions that prove sglang code paths are gone, not residual references to bring back): - tests/test_update_weight_from_tensor.py:753 — comment "The vLLM IPC implementation must NOT contain sglang-style Gloo gather code". - tests/unit/backends/vllm_utils/test_arguments.py:233-237 — three assertions that --sglang-router-ip, --sglang-router-port, and sglang_router_ip are NOT present in the argument parser. Tests + CI must land together; splitting them risks a window where the CI matrix references test files by names that don't exist yet (or no longer exist). After this lands, the test_file string in CI matches the test files on disk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Canlin Guo <canlinguosdu@gmail.com> * on_policy_distillation: port from SGLang to vLLM /v1/completions Follow-up on the test rename in this PR: test_qwen2.5_0.5B_opd_sglang.py → test_qwen2.5_0.5B_opd_vllm.py. The test only spawns a vLLM teacher and exercises the OPD pipeline; the real broken piece was slime/rollout/on_policy_distillation.py, which PR #18 left in SGLang request/response shape: request fields: "max_new_tokens": 0 (vLLM: "max_tokens") "return_logprob": True (sglang-only) "logprob_start_len": 0 (sglang-only) response parsing: reward["meta_info"]["input_token_logprobs"] (sglang shape) vLLM 0.21 supports the same workflow natively via `prompt_logprobs`: request to POST /v1/completions: { "model": <teacher>, "prompt_token_ids": sample.tokens, "max_tokens": 1, "temperature": 0, "prompt_logprobs": 1, "logprobs": 0, "skip_special_tokens": False, } response: response["choices"][0]["prompt_logprobs"] # list[dict[int, Logprob] | None] where Logprob is {"logprob": float, "rank": int, "decoded_token": str} References checked against vllm source: - reference/vllm/vllm/entrypoints/openai/completion/protocol.py:91 (request: prompt_logprobs: int | None) - reference/vllm/vllm/entrypoints/openai/completion/protocol.py:487 (response: prompt_logprobs: list[dict[int, Logprob] | None] | None) - reference/vllm/vllm/logprobs.py:13 (Logprob dataclass: logprob/rank/decoded_token) Implementation notes: 1. JSON serializes int dict keys as strings, so `_logprob_for_token` tries both `pos_entry.get(token_id)` and `pos_entry.get(str(token_id))`. 2. `pos_entry` is `None` at position 0 (no prior context) — handled explicitly. We also gracefully degrade if a token at position `i` is not in the top-1 logprob dict (falls back to 0.0, same as the prior sglang code would do). 3. The Logprob dataclass `decoded_token` field is unused; we only read `.logprob`. Both dict and `Logprob` shapes are accepted in case the server uses a flatter serialization toggle. 4. `args.opd_teacher_model` is the new model-name arg; falls back to `args.hf_checkpoint` if not set, mirroring how vime's other rollout paths derive the model name. Smoke-tested `_logprob_for_token` locally: - None entry → 0.0 - int key + dict value → logprob - str key (JSON shape) → logprob - missing token → 0.0 - flattened float value → float Also drops 3 lines from tests/unit/backends/vllm_utils/test_arguments.py: the `--sglang-router-ip`/`--sglang-router-port`/`sglang_router_ip` anti- regression assertions. Once the slim PR #18 lands and sglang is gone from the runtime, those assertions are vacuous; treating sglang as non-existent per the project policy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Canlin Guo <canlinguosdu@gmail.com> * tests: drop duplicate smoke test updates from PR40 * test(update_weight_from_tensor): drop stale _apply_monkey_patch_torch_reductions patch The inner ``with patch(f"{MODULE_PATH}._apply_monkey_patch_torch_reductions"):`` context in _run_update suppressed a helper call that PR #48 has since deleted from update_weight_from_tensor.py (commit 39bf899 on aoshen/align-ipc-rpc-with-slime). After that PR lands the patched attribute won't exist and this line raises AttributeError. Remove it now so the test survives PR #48 merge. The ``sglang_mod.monkey_patch_torch_reductions = MagicMock()`` stub on the fake sglang module is intentionally kept: on this branch the production code still imports it via ``from ..sglang import monkey_patch_torch_reductions`` (both update_weight_from_tensor._apply_monkey_patch_torch_reductions on PR #40's view of main, and hf_weight_iterator_direct.py at module level). Removing the stub here would break the test on PR #40 alone; it can be dropped in a follow-up once PR #48 finishes removing every import site. Tests: ``tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_tensor.py`` all 6 pass with this change applied to gcl/pr18-tests-ci HEAD. * tests: drop duplicate top-level test_update_weight_from_tensor.py The 786-line tests/test_update_weight_from_tensor.py is a stale rebase leftover from the original PR #18 branch — it predates the IPC test file PR #22 landed at the canonical unit-test path (tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_tensor.py) and predates PR #48's single-RPC weight-version contract. Comparing the two: * Both stub sys.modules / torch.distributed at module import time, so having two files compounds the test-isolation issue Gemini raised (PR #40 comment #1). * Coverage overlaps materially (e.g. test_ipc_init_called_on_first_update_only ≈ test_ipc_init_runs_once — same invariant, different wording). * The nested file is up-to-date with PR #48's RPC contract (update_weights_from_tensor.remote(**fields, weight_version=...)); the top-level file still uses the pre-#48 lifecycle shape and does not exercise the coordinator slot fields. * The nested path matches repo convention: tests/unit/ for mock-only unit tests, tests/ top level for e2e scripts. Closes Gemini comment #1 on PR #40. Gemini comment #2 (the same stub pattern in the surviving nested file) is a pre-existing issue from PR #22 / #48 and out of scope for this rename PR — to be addressed in a follow-up that converts _install_stubs() to an autouse module-scoped fixture with save/restore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(vllm_config): use real get_model_url default endpoint /inference/v1/generate get_model_url defaults to /inference/v1/generate (PR #18), not /v1/completions. Aligns this test with PR #18's test_vllm_config.py so the two PRs no longer conflict on this file and the assertion matches the actual runtime default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Drop on_policy_distillation.py from tests+CI PR (now owned by runtime PR #18) The OPD vLLM /v1/completions migration is a runtime change; it was folded into the core-runtime PR (#18). Restore this file to main here so the two PRs no longer overlap on it. #18 merges first, so this lands via #18. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Drop unit-test files now owned by runtime PR #18 test_vllm_config.py + the plugin_contracts tests are coupled to #18's runtime rename (they import vllm_config / vllm_rollout, which #18 creates). They live in #18; remove them here so the two PRs don't overlap. #18 merges first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: restore vLLM rollout args dropped during sglang→vllm rename The mechanical sglang→vllm rename dropped several rollout knobs instead of mapping them to their vLLM equivalents, weakening CI coverage (cuda-graph capture caps, speculative decoding, expert parallel). Restore them using the mapping established by the converted production scripts on main (run-glm4.7-30B-A3B.sh / run-glm5-744B-A40B.sh), verified against vLLM AsyncEngineArgs: --sglang-cuda-graph-max-bs N -> --vllm-max-cudagraph-capture-size N --sglang-cuda-graph-bs a b c -> --vllm-cudagraph-capture-sizes a b c --sglang-ep-size N -> --vllm-enable-expert-parallel --sglang-speculative-* (eagle) -> --vllm-speculative-config '{"method":"eagle","num_speculative_tokens":K}' Also: - glm4.7 pd: fix --vllm-max-num-seqs (was 8, taken from cuda-graph-max-bs; --sglang-max-running-requests was 16) and split out cuda-graph capture. - fix sglang→rollout mis-renames in temp-file prefixes (→ vllm_*). - test_vllm_config: rename test_update_weights_default_true → test_update_weights_defaults_to_none (it asserts `is None`). Dropped sglang flags with no vLLM equivalent (enable-dp-lm-head, moe-dense-tp-size, watchdog-timeout, mamba-scheduler-strategy, disaggregation-transfer-backend, enable-metrics) stay dropped; PD KV-transfer is driven by --prefill-num-servers + the --vllm-config prefill/decode topology. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(plugin_contracts): migrate from sglang_rollout to vllm_rollout The three plugin-contract tests still imported slime.rollout.sglang_rollout and called install_stubs(with_sglang_router=True), but _shared.install_stubs already dropped that parameter — so all three failed at collection (TypeError: unexpected keyword 'with_sglang_router'). Complete the migration: - install_stubs(with_sglang_router=True, ...) -> install_stubs(...) - import generate_and_rm / generate_rollout from slime.rollout.vllm_rollout - default rollout/eval path string -> slime.rollout.vllm_rollout.generate_rollout (matches runtime default at slime/utils/arguments.py:233) - FakeGenerateState: sglang_enable_deterministic_inference -> vllm_enable_deterministic_inference, with group_sampling_seeds defaulting to None and gated on the flag (mirrors the already-migrated tests/unit/rollout/test_vllm_rollout.py). All 34 plugin-contract cases pass (were 3 collection errors before). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(update_weight_from_tensor): drop stale slime…megatron_utils.sglang mock The test pre-registered a sys.modules mock for slime.backends.megatron_utils.sglang (monkey_patch_torch_reductions), left over from when update_weight_from_tensor imported it. The module under test no longer imports that module (its real deps are get_gloo_group / HfWeightIteratorBase / update_weight_from_distributed), so the mock is dead. Removing it makes tests/ and .github/ fully sglang-free. Test still passes (7/7). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Clean] Remove SGLang runtime code Rebuilt against current main so the PR contains only the SGLang runtime removal -- the docs / tests-ci / examples / scripts / docker portions were split into separate PRs that have since merged. - Delete dead SGLang server/runtime code: sglang_utils/{arguments,sglang_engine}.py, rollout/sglang_rollout.py, the megatron_utils/sglang.py re-export shim, and all docker/**/sglang.patch files. - Rename the rollout config module sglang_utils/sglang_config.py -> vllm_utils/vllm_config.py (SglangConfig -> VllmConfig, _resolve_sglang_config -> _resolve_vllm_config, --sglang-config -> --vllm-config); inline the GPU_MEMORY_TYPE_* constants in rollout.py. - Add megatron_utils/fp8_helpers.py for the UE8M0 fp8 helpers formerly re-exported through the sglang shim; repoint quantizer_fp8 to it. - Swap sglang_router -> vllm_router in http_utils/wandb_utils; drop the dead sglang-router dependency from requirements.txt. - Finish the SGLang->vLLM rename in the runtime so it is internally consistent and matches the tests landing in the tests/CI PR: * router args --router-* -> --vllm-router-* (vllm_router_ip/port/timeout); * get_model_url reads vllm_model_routers (aligning with rollout.py); * --opd-type sglang -> vllm; engine_overrides rename; * sglang_enable_deterministic_inference -> vllm_enable_deterministic_inference, wired to a real --vllm-enable-deterministic-inference flag (exports VLLM_BATCH_INVARIANT=1); * consistent_hash session-id routing uses vllm-router's x-session-id header; * drop dead trace helper build_sglang_meta_trace_attrs; de-SGLang comments/docstrings. - Rename test_sglang_config.py -> test_vllm_config.py and de-SGLang the plugin-contract tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Address review: finish de-SGLang + fold OPD/router-policy into runtime - naming: replace residual generic "rollout engine"/"engine"/"comm" wording with concrete vLLM (engine_overrides -> vllm_overrides; arguments help text; http_utils comments; rollout.py "inference workers"). sglang->vllm is correct, sglang->generic is not. - megatron_to_hf: drop the q_a_proj/kv_a_proj_with_mqa pairing + _cached_tensors global. That was sglang-only: sglang's loader torch.cat's both shards within a single load_weights call (needs them co-bucketed), whereas vLLM loads each shard independently via stacked_params_mapping into fused_qkv_a_proj. Also fix the misleading "merge into single fused name" comment. - docker/Dockerfile: remove now-dead sglang/sglang-router --no-deps stubs + the build-time `import sglang` smoke check (slime no longer imports sglang_router). - OPD: migrate on_policy_distillation.py teacher logprobs to vLLM /v1/completions (prompt_logprobs) instead of sglang return_logprob / meta_info.input_token_logprobs. - routing replay: register --vllm-router-policy (dest=router_policy) so the consistent_hash x-session-id session-affinity path is actually wired (was dead). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Review follow-ups: mirror slime vllm_config parsing + restore vLLM process cleanup - vllm_config.from_yaml: drop the needless `models_raw` intermediate and iterate `data["vllm"]` directly, restoring the "Accept both server_groups / legacy engine_groups" comment -- mirrors slime's sglang_config.from_yaml line-for-line. - command_utils.execute_train: re-add a process kill for leftover rollout engines as `pkill -9 -f "vllm serve"` (the old `pkill -9 sglang` was dropped with no vLLM equivalent), so stale engines don't hold GPUs/ports across runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Drop test changes from runtime PR; tests live in the tests+CI PR (#40) The plugin_contracts tests and the test_sglang_config -> test_vllm_config rename are coupled to the test/CI rename effort and are owned by #40. Restore them to main here so #18 is purely the SGLang runtime removal. #18 merges first; #40 rebases and re-lands the vLLM test versions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fp8_helpers: copy SGLang verbatim (fix import crash); pin vLLM deep_gemm env - fp8_helpers.py: replace the bespoke rewrite with SGLang's exact implementations of quant_weight_ue8m0 / transform_scale_ue8m0 and their DeepGEMM helpers (per_block_cast_to_fp8, ceil_to_ue8m0, ceil_div, ceil_align, the torch-impl packer). deep_gemm is imported lazily inside the functions (as SGLang does), so module import no longer requires deep_gemm. This fixes the module-level `NameError: _get_tma_aligned_size` that crashed `import megatron_to_hf` on any deep_gemm image, and drops the invented sf-stride fixup block that was not in upstream. Only should_deepgemm_weight_requant_ue8m0 stays vLLM-adapted (is_deep_gemm_e8m0_used) since SGLang's reads SGLang-internal deep_gemm_wrapper. - vllm_engine.launch_server_process: set VLLM_USE_DEEP_GEMM=1 + VLLM_DEEP_GEMM_WARMUP=relax explicitly (setdefault) alongside VLLM_BATCH_INVARIANT, replacing SGLang's removed deep_gemm precompile/warmup envs. All vLLM engine env now lives in the subprocess env builder (single source of truth). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fp8_helpers: revert to vLLM impl + fix the import NameError; add vLLM trace attrs - fp8_helpers.py: keep the vLLM-based implementation (uses vllm.utils.deep_gemm, consistent with the vLLM runtime) rather than the SGLang verbatim copy. Fix the module-level crash: the `try` block referenced `_get_tma_aligned_size` before it was bound (the "pre-imported with fallback" import was never written), which raised NameError whenever deep_gemm imported successfully -- and NameError is not caught by `except ImportError`, so `import megatron_to_hf` crashed on any deep_gemm image. Replace the bogus self-assignment with the real import: `from vllm.utils.deep_gemm import get_tma_aligned_size as _get_tma_aligned_size`. - trace_utils/vllm_rollout: add build_vllm_meta_trace_attrs and attach finish_reason + token usage to the vllm_inference_generate span (mirrors SGLang's build_sglang_meta_trace_attrs; vLLM responses lack the pd_* timing, which lives in vLLM's own OTLP traces). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(opd): score teacher via /inference/v1/generate with prompt_logprobs Move the vllm OPD teacher path off the OpenAI /v1/completions endpoint onto vime's native /inference/v1/generate (the same endpoint the rollout engines use), and fix three latent issues: 1. model field: /inference/v1/generate takes `model` as OPTIONAL. Stop defaulting to args.hf_checkpoint (the *student* name, which mis-names a teacher!=student server). Add --opd-teacher-model; send `model` only when set, otherwise omit it (single-model teacher servers use their loaded model). 2. multimodal: the old code sent image_data to a token-only endpoint, which is invalid. Raise NotImplementedError until the /v1/chat/completions/render -> /inference/v1/generate flow is wired (mirrors slime.rollout.vllm_rollout.generate). 3. logprob robustness: read top-level GenerateResponse.prompt_logprobs, assert it is present and length-aligned with token_ids, assert the per-sample tensor covers response_length, and raise (not silently return 0.0) on a missing token logprob. vLLM always includes the actual prompt token in prompt_logprobs, so a miss is a real error. Alignment is unchanged (plp[i] <-> tokens[i], skip pos 0, take [-response_length:]). Follow-up (separate, in the tests PR): the OPD e2e test must launch a teacher that exposes /inference/v1/generate and point --rm-url at it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(clean-sglang): purge SGLang from tools, train scripts, and build infra tools/: drop dead `args.sglang_enable_ep_moe` shim (read nowhere); reword profile/replay helpers to vLLM and map analyzer hints to vLLM flags (--enforce-eager, --gpu-memory-utilization). train{,_async}.py: comments SGLang -> vLLM. build infra: remove build_conda.sh (SGLang-only conda path); drop the GB300 sgl-kernel install from the Dockerfile; delete docker/npu_patch/ wholesale. docker base image: bump to vLLM v0.22.0. justfile ARM recipes now pin the real multi-arch vLLM base images instead of the dead SGLANG_IMAGE_TAG/ ENABLE_SGLANG_PATCH build-args -- cu129-arm64 -> v0.22.0-cu129-ubuntu2404 (CUDA 12.9), cu13-arm64 -> v0.22.0-ubuntu2404 (the default-CUDA tag is already CUDA 13.0) + ENABLE_CUDA_13=1. vLLM tags are multi-arch manifests, so docker selects the arm64 image automatically on an ARM host. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci(clean-sglang): drop conda-build workflow (ran deleted build_conda.sh on SGLang image) The single build-conda job ran `bash build_conda.sh` (removed in the previous commit) inside an lmsysorg/sglang container. With the SGLang-only conda path gone, the whole workflow is dead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(clean-sglang): fix stale SGLang refs in docs/skills; tidy comments docs/conf.py: point the "edit on GitHub" links at vllm-project/vime instead of the inherited sgl-project.github.io repo. .claude/skills/*: update the dead `slime/rollout/sglang_rollout.py` references to `vllm_rollout.py` (the real default is slime.rollout.vllm_rollout.generate_rollout). justfile: drop the redundant BASE_IMAGE override on release-cu129-arm64 (it equalled the Dockerfile default; the multi-arch manifest already resolves arm64). train{,_async}.py: drop stray "the" in the W&B comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tests): use method=mtp (not eagle) in vllm speculative config The migrated speculative configs pass no draft `model`, so method=eagle raises "num_speculative_tokens was provided but without speculative model" in vLLM's SpeculativeConfig. These models carry embedded MTP layers, so method=mtp is correct and unblocks the mimo MTP-only-grad test (#19). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cleanup): target renamed vLLM subprocesses in pkill so VRAM is freed vLLM's set_process_title() renames the VRAM-holding subprocesses (VLLM::EngineCore, VLLM::Worker_TP*, vllm::router), so their cmdline no longer contains "vllm serve". The previous `pkill -9 -f "vllm serve"` matched only the launcher and left engine/worker children holding GPU memory, leaking it into the next run — masked only by the indiscriminate `pkill -9 python`, which is unsafe on colocate/shared nodes. Match both the launcher and the renamed children with `pkill -9 -f '[v]llm serve|VLL[M]::'`; the [v]/[M] bracket trick keeps the pattern from matching pkill's own cmdline. This makes the broad python kill unnecessary, so its already-commented-out lines are removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speculative): use method=mtp (not eagle) for embedded-MTP models vLLM's SpeculativeConfig requires an explicit draft `model` for method=eagle; with only num_speculative_tokens set it raises "num_speculative_tokens was provided but without speculative model". The migrated configs in scripts/examples/docs pass no model, so they must use method=mtp, which reuses the target checkpoint's embedded MTP layer (DeepSeek-R1, GLM-4.x-MoE, MiMo, Qwen3-Next/3.5). The two docs examples that pass an explicit "model" are genuine eagle usage and are left unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(vllm): launch each rollout engine with its ServerGroup's per-group TP launch_server_process / _init_normal derived tensor-parallel size and CUDA_VISIBLE_DEVICES from the global --rollout-num-gpus-per-engine, ignoring the per-engine num_gpus_per_engine already carried on the VLLMEngine actor. A ServerGroup configured with num_gpus_per_engine greater than the global flag (e.g. tp=2) therefore launched as tp=1, while the NCCL weight-sync rendezvous sized world_size from engine_gpu_counts (the per-group value). The two disagreed: the trainer waited for a rank the under-sized engine never started, so init_weight_transfer_engine hung for 300s ("3/4 clients joined") and the job failed. Honor the per-engine num_gpus_per_engine at launch, falling back to the global flag when unset (matches the SGLang path and PR #66's _compute_server_args). Verified on H200: tests/test_qwen2.5_0.5B_vllm_config_distributed now launches engine0 tp=2 / engine1 tp=1, update_weights completes in 1.1s (was a 301s timeout), and rollout+eval proceed. AI assistance (Claude Code) was used for this change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ckpt): add --dist-ckpt-optim-fully-reshardable for PAO+offload save/load test_qwen3_4B_ckpt.py uses precision-aware optimizer + cpu-offload (HybridDeviceOptimizer). Under the default dp_reshardable (bucket-centric) optimizer sharding, save/load produce unequal-length param_state lists, so dist-ckpt load fails with "Cannot merge two lists with different lengths (81 and 79)". fully_reshardable is model-centric and immune to bucket-layout changes. Verified on the r3 image (Megatron-LM 0.16.0rc0 @ 1dcf0da): save+load both succeed, and source review confirms master_param / step / HybridDeviceOptimizer sync are handled on this path. This is the flag described in PR #50 that was never actually merged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(router-args): hybrid naming — vllm_ for ip/port, bare router_ for timeout vllm-router's RouterArgs.from_cli_args only supports prefix "" or "router_" (never "vllm_router_"), and excludes host/port from its CLI via exclude_host_port=True. So: - --vllm-router-ip / --vllm-router-port keep the vllm_ prefix: RouterArgs does not own these CLI flags, vime does (populated via _start_router's manual router_args.host/port assignment), so the vllm_ prefix is free and marks them as vime-owned endpoint config. - --router-request-timeout-secs goes bare (dest router_request_timeout_secs): it is a genuine RouterArgs field, so it shares the --router-* namespace with policy / cache_threshold / retries / … and flows through from_cli_args like the other knobs. - --vllm-router-policy keeps dest=router_policy (unchanged). Also fixes conftest fixture to seed vllm_router_ip/port (was bare router_ip/port, which never matched the vllm_engine reader) and updates README/README_zh prose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Canlin Guo <canlinguosdu@gmail.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
* [Clean] Remove SGLang runtime code
Rebuilt against current main so the PR contains only the SGLang runtime
removal -- the docs / tests-ci / examples / scripts / docker portions were
split into separate PRs that have since merged.
- Delete dead SGLang server/runtime code: sglang_utils/{arguments,sglang_engine}.py,
rollout/sglang_rollout.py, the megatron_utils/sglang.py re-export shim, and all
docker/**/sglang.patch files.
- Rename the rollout config module sglang_utils/sglang_config.py ->
vllm_utils/vllm_config.py (SglangConfig -> VllmConfig, _resolve_sglang_config ->
_resolve_vllm_config, --sglang-config -> --vllm-config); inline the
GPU_MEMORY_TYPE_* constants in rollout.py.
- Add megatron_utils/fp8_helpers.py for the UE8M0 fp8 helpers formerly re-exported
through the sglang shim; repoint quantizer_fp8 to it.
- Swap sglang_router -> vllm_router in http_utils/wandb_utils; drop the dead
sglang-router dependency from requirements.txt.
- Finish the SGLang->vLLM rename in the runtime so it is internally consistent and
matches the tests landing in the tests/CI PR:
* router args --router-* -> --vllm-router-* (vllm_router_ip/port/timeout);
* get_model_url reads vllm_model_routers (aligning with rollout.py);
* --opd-type sglang -> vllm; engine_overrides rename;
* sglang_enable_deterministic_inference -> vllm_enable_deterministic_inference,
wired to a real --vllm-enable-deterministic-inference flag (exports
VLLM_BATCH_INVARIANT=1);
* consistent_hash session-id routing uses vllm-router's x-session-id header;
* drop dead trace helper build_sglang_meta_trace_attrs; de-SGLang comments/docstrings.
- Rename test_sglang_config.py -> test_vllm_config.py and de-SGLang the
plugin-contract tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Address review: finish de-SGLang + fold OPD/router-policy into runtime
- naming: replace residual generic "rollout engine"/"engine"/"comm" wording
with concrete vLLM (engine_overrides -> vllm_overrides; arguments help text;
http_utils comments; rollout.py "inference workers"). sglang->vllm is correct,
sglang->generic is not.
- megatron_to_hf: drop the q_a_proj/kv_a_proj_with_mqa pairing + _cached_tensors
global. That was sglang-only: sglang's loader torch.cat's both shards within a
single load_weights call (needs them co-bucketed), whereas vLLM loads each shard
independently via stacked_params_mapping into fused_qkv_a_proj. Also fix the
misleading "merge into single fused name" comment.
- docker/Dockerfile: remove now-dead sglang/sglang-router --no-deps stubs + the
build-time `import sglang` smoke check (slime no longer imports sglang_router).
- OPD: migrate on_policy_distillation.py teacher logprobs to vLLM /v1/completions
(prompt_logprobs) instead of sglang return_logprob / meta_info.input_token_logprobs.
- routing replay: register --vllm-router-policy (dest=router_policy) so the
consistent_hash x-session-id session-affinity path is actually wired (was dead).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Review follow-ups: mirror slime vllm_config parsing + restore vLLM process cleanup
- vllm_config.from_yaml: drop the needless `models_raw` intermediate and iterate
`data["vllm"]` directly, restoring the "Accept both server_groups / legacy
engine_groups" comment -- mirrors slime's sglang_config.from_yaml line-for-line.
- command_utils.execute_train: re-add a process kill for leftover rollout engines
as `pkill -9 -f "vllm serve"` (the old `pkill -9 sglang` was dropped with no
vLLM equivalent), so stale engines don't hold GPUs/ports across runs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drop test changes from runtime PR; tests live in the tests+CI PR (#40)
The plugin_contracts tests and the test_sglang_config -> test_vllm_config rename
are coupled to the test/CI rename effort and are owned by #40. Restore them to
main here so #18 is purely the SGLang runtime removal. #18 merges first; #40
rebases and re-lands the vLLM test versions.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fp8_helpers: copy SGLang verbatim (fix import crash); pin vLLM deep_gemm env
- fp8_helpers.py: replace the bespoke rewrite with SGLang's exact implementations
of quant_weight_ue8m0 / transform_scale_ue8m0 and their DeepGEMM helpers
(per_block_cast_to_fp8, ceil_to_ue8m0, ceil_div, ceil_align, the torch-impl
packer). deep_gemm is imported lazily inside the functions (as SGLang does), so
module import no longer requires deep_gemm. This fixes the module-level
`NameError: _get_tma_aligned_size` that crashed `import megatron_to_hf` on any
deep_gemm image, and drops the invented sf-stride fixup block that was not in
upstream. Only should_deepgemm_weight_requant_ue8m0 stays vLLM-adapted
(is_deep_gemm_e8m0_used) since SGLang's reads SGLang-internal deep_gemm_wrapper.
- vllm_engine.launch_server_process: set VLLM_USE_DEEP_GEMM=1 +
VLLM_DEEP_GEMM_WARMUP=relax explicitly (setdefault) alongside VLLM_BATCH_INVARIANT,
replacing SGLang's removed deep_gemm precompile/warmup envs. All vLLM engine env
now lives in the subprocess env builder (single source of truth).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fp8_helpers: revert to vLLM impl + fix the import NameError; add vLLM trace attrs
- fp8_helpers.py: keep the vLLM-based implementation (uses vllm.utils.deep_gemm,
consistent with the vLLM runtime) rather than the SGLang verbatim copy. Fix the
module-level crash: the `try` block referenced `_get_tma_aligned_size` before it
was bound (the "pre-imported with fallback" import was never written), which
raised NameError whenever deep_gemm imported successfully -- and NameError is not
caught by `except ImportError`, so `import megatron_to_hf` crashed on any
deep_gemm image. Replace the bogus self-assignment with the real import:
`from vllm.utils.deep_gemm import get_tma_aligned_size as _get_tma_aligned_size`.
- trace_utils/vllm_rollout: add build_vllm_meta_trace_attrs and attach finish_reason
+ token usage to the vllm_inference_generate span (mirrors SGLang's
build_sglang_meta_trace_attrs; vLLM responses lack the pd_* timing, which lives
in vLLM's own OTLP traces).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(opd): score teacher via /inference/v1/generate with prompt_logprobs
Move the vllm OPD teacher path off the OpenAI /v1/completions endpoint onto
vime's native /inference/v1/generate (the same endpoint the rollout engines
use), and fix three latent issues:
1. model field: /inference/v1/generate takes `model` as OPTIONAL. Stop
defaulting to args.hf_checkpoint (the *student* name, which mis-names a
teacher!=student server). Add --opd-teacher-model; send `model` only when
set, otherwise omit it (single-model teacher servers use their loaded model).
2. multimodal: the old code sent image_data to a token-only endpoint, which is
invalid. Raise NotImplementedError until the
/v1/chat/completions/render -> /inference/v1/generate flow is wired (mirrors
slime.rollout.vllm_rollout.generate).
3. logprob robustness: read top-level GenerateResponse.prompt_logprobs, assert
it is present and length-aligned with token_ids, assert the per-sample tensor
covers response_length, and raise (not silently return 0.0) on a missing
token logprob. vLLM always includes the actual prompt token in
prompt_logprobs, so a miss is a real error.
Alignment is unchanged (plp[i] <-> tokens[i], skip pos 0, take [-response_length:]).
Follow-up (separate, in the tests PR): the OPD e2e test must launch a teacher
that exposes /inference/v1/generate and point --rm-url at it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore(clean-sglang): purge SGLang from tools, train scripts, and build infra
tools/: drop dead `args.sglang_enable_ep_moe` shim (read nowhere); reword
profile/replay helpers to vLLM and map analyzer hints to vLLM flags
(--enforce-eager, --gpu-memory-utilization). train{,_async}.py: comments
SGLang -> vLLM.
build infra: remove build_conda.sh (SGLang-only conda path); drop the GB300
sgl-kernel install from the Dockerfile; delete docker/npu_patch/ wholesale.
docker base image: bump to vLLM v0.22.0. justfile ARM recipes now pin the real
multi-arch vLLM base images instead of the dead SGLANG_IMAGE_TAG/
ENABLE_SGLANG_PATCH build-args -- cu129-arm64 -> v0.22.0-cu129-ubuntu2404
(CUDA 12.9), cu13-arm64 -> v0.22.0-ubuntu2404 (the default-CUDA tag is already
CUDA 13.0) + ENABLE_CUDA_13=1. vLLM tags are multi-arch manifests, so docker
selects the arm64 image automatically on an ARM host.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci(clean-sglang): drop conda-build workflow (ran deleted build_conda.sh on SGLang image)
The single build-conda job ran `bash build_conda.sh` (removed in the previous
commit) inside an lmsysorg/sglang container. With the SGLang-only conda path
gone, the whole workflow is dead.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore(clean-sglang): fix stale SGLang refs in docs/skills; tidy comments
docs/conf.py: point the "edit on GitHub" links at vllm-project/vime instead of
the inherited sgl-project.github.io repo. .claude/skills/*: update the dead
`slime/rollout/sglang_rollout.py` references to `vllm_rollout.py` (the real
default is slime.rollout.vllm_rollout.generate_rollout).
justfile: drop the redundant BASE_IMAGE override on release-cu129-arm64 (it
equalled the Dockerfile default; the multi-arch manifest already resolves
arm64). train{,_async}.py: drop stray "the" in the W&B comment.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(cleanup): target renamed vLLM subprocesses in pkill so VRAM is freed
vLLM's set_process_title() renames the VRAM-holding subprocesses
(VLLM::EngineCore, VLLM::Worker_TP*, vllm::router), so their cmdline no
longer contains "vllm serve". The previous `pkill -9 -f "vllm serve"`
matched only the launcher and left engine/worker children holding GPU
memory, leaking it into the next run — masked only by the indiscriminate
`pkill -9 python`, which is unsafe on colocate/shared nodes.
Match both the launcher and the renamed children with
`pkill -9 -f '[v]llm serve|VLL[M]::'`; the [v]/[M] bracket trick keeps the
pattern from matching pkill's own cmdline. This makes the broad python
kill unnecessary, so its already-commented-out lines are removed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(speculative): use method=mtp (not eagle) for embedded-MTP models
vLLM's SpeculativeConfig requires an explicit draft `model` for
method=eagle; with only num_speculative_tokens set it raises
"num_speculative_tokens was provided but without speculative model".
The migrated configs in scripts/examples/docs pass no model, so they must
use method=mtp, which reuses the target checkpoint's embedded MTP layer
(DeepSeek-R1, GLM-4.x-MoE, MiMo, Qwen3-Next/3.5).
The two docs examples that pass an explicit "model" are genuine eagle
usage and are left unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(vllm): launch each rollout engine with its ServerGroup's per-group TP
launch_server_process / _init_normal derived tensor-parallel size and
CUDA_VISIBLE_DEVICES from the global --rollout-num-gpus-per-engine, ignoring
the per-engine num_gpus_per_engine already carried on the VLLMEngine actor.
A ServerGroup configured with num_gpus_per_engine greater than the global
flag (e.g. tp=2) therefore launched as tp=1, while the NCCL weight-sync
rendezvous sized world_size from engine_gpu_counts (the per-group value).
The two disagreed: the trainer waited for a rank the under-sized engine
never started, so init_weight_transfer_engine hung for 300s
("3/4 clients joined") and the job failed.
Honor the per-engine num_gpus_per_engine at launch, falling back to the
global flag when unset (matches the SGLang path and PR #66's
_compute_server_args).
Verified on H200: tests/test_qwen2.5_0.5B_vllm_config_distributed now
launches engine0 tp=2 / engine1 tp=1, update_weights completes in 1.1s
(was a 301s timeout), and rollout+eval proceed.
AI assistance (Claude Code) was used for this change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(router-args): hybrid naming — vllm_ for ip/port, bare router_ for timeout
vllm-router's RouterArgs.from_cli_args only supports prefix "" or "router_"
(never "vllm_router_"), and excludes host/port from its CLI via
exclude_host_port=True. So:
- --vllm-router-ip / --vllm-router-port keep the vllm_ prefix: RouterArgs does
not own these CLI flags, vime does (populated via _start_router's manual
router_args.host/port assignment), so the vllm_ prefix is free and marks them
as vime-owned endpoint config.
- --router-request-timeout-secs goes bare (dest router_request_timeout_secs): it
is a genuine RouterArgs field, so it shares the --router-* namespace with
policy / cache_threshold / retries / … and flows through from_cli_args like
the other knobs.
- --vllm-router-policy keeps dest=router_policy (unchanged).
Also fixes conftest fixture to seed vllm_router_ip/port (was bare router_ip/port,
which never matched the vllm_engine reader) and updates README/README_zh prose.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: aoshen02 <aoshen@inferact.ai>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
momo609
pushed a commit
that referenced
this pull request
Jun 8, 2026
* tests + CI: complete sglang→vllm rename across tests/ and .github/ Split from PR #18 (gcl/clean-sglang). One of 4 PRs splitting the original PR #18 by content area: docs (#38) / examples (#39) / **tests+CI** / core runtime. 42 files / +~750 / -~700. These are bundled in a single PR because the CI workflows reference test file names by string — splitting them would create a window where either tests are renamed but CI still points at the old names, or vice versa, breaking CI mid-roll. What this PR does: (A) tests/ (38 files): - Mechanical CLI-flag rename: --sglang-* → --vllm-* equivalents in all test scripts (matches the table now used in scripts/ and examples/). - Variable rename: SGLANG_ARGS → VLLM_ARGS where present. - 4 file renames (R086-R091, all >85% similarity): test_qwen2.5_0.5B_opd_sglang.py → test_qwen2.5_0.5B_opd_vllm.py test_qwen2.5_0.5B_sglang_config.py → test_qwen2.5_0.5B_vllm_config.py test_qwen2.5_0.5B_sglang_config_distributed.py → test_qwen2.5_0.5B_vllm_config_distributed.py test_sglang_config_mixed_offload.py → test_vllm_config_mixed_offload.py test_sglang_config_mixed_offload_ft.py → test_vllm_config_mixed_offload_ft.py tests/utils/test_sglang_config.py → tests/utils/test_vllm_config.py - 2 new tests for the IPC weight-transfer path landed in PR #18: tests/test_update_weight_from_tensor.py tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_tensor.py (These are PR #22 / colocate-IPC test coverage; the production code the slim PR #18 ships will rely on the same code from PR #22.) (B) .github/ (4 files): - workflows/conda-ci.yml: container image lmsysorg/sglang → vime (inferactinc/public:vime-vllm-cu129-latest). - workflows/pr-test.yml + pr-test.yml.j2 (template): * Container images (slimerl/slime[-test]:latest → vime image) on every job that ran on the sglang-era base. * e2e-test-sglang-config job → e2e-test-vllm-config job (renamed label `run-ci-sglang-config` → `run-ci-vllm-config`; matrix `test_file` entries updated to point at the renamed test files in (A)). * e2e-test-megatron + e2e-test-image matrices: `_opd_sglang.py` entries → `_opd_vllm.py`. - ISSUE_TEMPLATE/bug_report.yml: drop the "SGLang version (if relevant):" environment field, add "vLLM version:" and "vllm-router version:" lines. (PR #36 already changed "CUDA/ROCm version" → "CUDA version" earlier; that change is preserved.) Sgl residue intentionally kept (4 hits — all anti-regression assertions that prove sglang code paths are gone, not residual references to bring back): - tests/test_update_weight_from_tensor.py:753 — comment "The vLLM IPC implementation must NOT contain sglang-style Gloo gather code". - tests/unit/backends/vllm_utils/test_arguments.py:233-237 — three assertions that --sglang-router-ip, --sglang-router-port, and sglang_router_ip are NOT present in the argument parser. Tests + CI must land together; splitting them risks a window where the CI matrix references test files by names that don't exist yet (or no longer exist). After this lands, the test_file string in CI matches the test files on disk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Canlin Guo <canlinguosdu@gmail.com> * on_policy_distillation: port from SGLang to vLLM /v1/completions Follow-up on the test rename in this PR: test_qwen2.5_0.5B_opd_sglang.py → test_qwen2.5_0.5B_opd_vllm.py. The test only spawns a vLLM teacher and exercises the OPD pipeline; the real broken piece was slime/rollout/on_policy_distillation.py, which PR #18 left in SGLang request/response shape: request fields: "max_new_tokens": 0 (vLLM: "max_tokens") "return_logprob": True (sglang-only) "logprob_start_len": 0 (sglang-only) response parsing: reward["meta_info"]["input_token_logprobs"] (sglang shape) vLLM 0.21 supports the same workflow natively via `prompt_logprobs`: request to POST /v1/completions: { "model": <teacher>, "prompt_token_ids": sample.tokens, "max_tokens": 1, "temperature": 0, "prompt_logprobs": 1, "logprobs": 0, "skip_special_tokens": False, } response: response["choices"][0]["prompt_logprobs"] # list[dict[int, Logprob] | None] where Logprob is {"logprob": float, "rank": int, "decoded_token": str} References checked against vllm source: - reference/vllm/vllm/entrypoints/openai/completion/protocol.py:91 (request: prompt_logprobs: int | None) - reference/vllm/vllm/entrypoints/openai/completion/protocol.py:487 (response: prompt_logprobs: list[dict[int, Logprob] | None] | None) - reference/vllm/vllm/logprobs.py:13 (Logprob dataclass: logprob/rank/decoded_token) Implementation notes: 1. JSON serializes int dict keys as strings, so `_logprob_for_token` tries both `pos_entry.get(token_id)` and `pos_entry.get(str(token_id))`. 2. `pos_entry` is `None` at position 0 (no prior context) — handled explicitly. We also gracefully degrade if a token at position `i` is not in the top-1 logprob dict (falls back to 0.0, same as the prior sglang code would do). 3. The Logprob dataclass `decoded_token` field is unused; we only read `.logprob`. Both dict and `Logprob` shapes are accepted in case the server uses a flatter serialization toggle. 4. `args.opd_teacher_model` is the new model-name arg; falls back to `args.hf_checkpoint` if not set, mirroring how vime's other rollout paths derive the model name. Smoke-tested `_logprob_for_token` locally: - None entry → 0.0 - int key + dict value → logprob - str key (JSON shape) → logprob - missing token → 0.0 - flattened float value → float Also drops 3 lines from tests/unit/backends/vllm_utils/test_arguments.py: the `--sglang-router-ip`/`--sglang-router-port`/`sglang_router_ip` anti- regression assertions. Once the slim PR #18 lands and sglang is gone from the runtime, those assertions are vacuous; treating sglang as non-existent per the project policy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Canlin Guo <canlinguosdu@gmail.com> * tests: drop duplicate smoke test updates from PR40 * test(update_weight_from_tensor): drop stale _apply_monkey_patch_torch_reductions patch The inner ``with patch(f"{MODULE_PATH}._apply_monkey_patch_torch_reductions"):`` context in _run_update suppressed a helper call that PR #48 has since deleted from update_weight_from_tensor.py (commit 39bf899 on aoshen/align-ipc-rpc-with-slime). After that PR lands the patched attribute won't exist and this line raises AttributeError. Remove it now so the test survives PR #48 merge. The ``sglang_mod.monkey_patch_torch_reductions = MagicMock()`` stub on the fake sglang module is intentionally kept: on this branch the production code still imports it via ``from ..sglang import monkey_patch_torch_reductions`` (both update_weight_from_tensor._apply_monkey_patch_torch_reductions on PR #40's view of main, and hf_weight_iterator_direct.py at module level). Removing the stub here would break the test on PR #40 alone; it can be dropped in a follow-up once PR #48 finishes removing every import site. Tests: ``tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_tensor.py`` all 6 pass with this change applied to gcl/pr18-tests-ci HEAD. * tests: drop duplicate top-level test_update_weight_from_tensor.py The 786-line tests/test_update_weight_from_tensor.py is a stale rebase leftover from the original PR #18 branch — it predates the IPC test file PR #22 landed at the canonical unit-test path (tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_tensor.py) and predates PR #48's single-RPC weight-version contract. Comparing the two: * Both stub sys.modules / torch.distributed at module import time, so having two files compounds the test-isolation issue Gemini raised (PR #40 comment #1). * Coverage overlaps materially (e.g. test_ipc_init_called_on_first_update_only ≈ test_ipc_init_runs_once — same invariant, different wording). * The nested file is up-to-date with PR #48's RPC contract (update_weights_from_tensor.remote(**fields, weight_version=...)); the top-level file still uses the pre-#48 lifecycle shape and does not exercise the coordinator slot fields. * The nested path matches repo convention: tests/unit/ for mock-only unit tests, tests/ top level for e2e scripts. Closes Gemini comment #1 on PR #40. Gemini comment #2 (the same stub pattern in the surviving nested file) is a pre-existing issue from PR #22 / #48 and out of scope for this rename PR — to be addressed in a follow-up that converts _install_stubs() to an autouse module-scoped fixture with save/restore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(vllm_config): use real get_model_url default endpoint /inference/v1/generate get_model_url defaults to /inference/v1/generate (PR #18), not /v1/completions. Aligns this test with PR #18's test_vllm_config.py so the two PRs no longer conflict on this file and the assertion matches the actual runtime default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Drop on_policy_distillation.py from tests+CI PR (now owned by runtime PR #18) The OPD vLLM /v1/completions migration is a runtime change; it was folded into the core-runtime PR (#18). Restore this file to main here so the two PRs no longer overlap on it. #18 merges first, so this lands via #18. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Drop unit-test files now owned by runtime PR #18 test_vllm_config.py + the plugin_contracts tests are coupled to #18's runtime rename (they import vllm_config / vllm_rollout, which #18 creates). They live in #18; remove them here so the two PRs don't overlap. #18 merges first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: restore vLLM rollout args dropped during sglang→vllm rename The mechanical sglang→vllm rename dropped several rollout knobs instead of mapping them to their vLLM equivalents, weakening CI coverage (cuda-graph capture caps, speculative decoding, expert parallel). Restore them using the mapping established by the converted production scripts on main (run-glm4.7-30B-A3B.sh / run-glm5-744B-A40B.sh), verified against vLLM AsyncEngineArgs: --sglang-cuda-graph-max-bs N -> --vllm-max-cudagraph-capture-size N --sglang-cuda-graph-bs a b c -> --vllm-cudagraph-capture-sizes a b c --sglang-ep-size N -> --vllm-enable-expert-parallel --sglang-speculative-* (eagle) -> --vllm-speculative-config '{"method":"eagle","num_speculative_tokens":K}' Also: - glm4.7 pd: fix --vllm-max-num-seqs (was 8, taken from cuda-graph-max-bs; --sglang-max-running-requests was 16) and split out cuda-graph capture. - fix sglang→rollout mis-renames in temp-file prefixes (→ vllm_*). - test_vllm_config: rename test_update_weights_default_true → test_update_weights_defaults_to_none (it asserts `is None`). Dropped sglang flags with no vLLM equivalent (enable-dp-lm-head, moe-dense-tp-size, watchdog-timeout, mamba-scheduler-strategy, disaggregation-transfer-backend, enable-metrics) stay dropped; PD KV-transfer is driven by --prefill-num-servers + the --vllm-config prefill/decode topology. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(plugin_contracts): migrate from sglang_rollout to vllm_rollout The three plugin-contract tests still imported slime.rollout.sglang_rollout and called install_stubs(with_sglang_router=True), but _shared.install_stubs already dropped that parameter — so all three failed at collection (TypeError: unexpected keyword 'with_sglang_router'). Complete the migration: - install_stubs(with_sglang_router=True, ...) -> install_stubs(...) - import generate_and_rm / generate_rollout from slime.rollout.vllm_rollout - default rollout/eval path string -> slime.rollout.vllm_rollout.generate_rollout (matches runtime default at slime/utils/arguments.py:233) - FakeGenerateState: sglang_enable_deterministic_inference -> vllm_enable_deterministic_inference, with group_sampling_seeds defaulting to None and gated on the flag (mirrors the already-migrated tests/unit/rollout/test_vllm_rollout.py). All 34 plugin-contract cases pass (were 3 collection errors before). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(update_weight_from_tensor): drop stale slime…megatron_utils.sglang mock The test pre-registered a sys.modules mock for slime.backends.megatron_utils.sglang (monkey_patch_torch_reductions), left over from when update_weight_from_tensor imported it. The module under test no longer imports that module (its real deps are get_gloo_group / HfWeightIteratorBase / update_weight_from_distributed), so the mock is dead. Removing it makes tests/ and .github/ fully sglang-free. Test still passes (7/7). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Clean] Remove SGLang runtime code Rebuilt against current main so the PR contains only the SGLang runtime removal -- the docs / tests-ci / examples / scripts / docker portions were split into separate PRs that have since merged. - Delete dead SGLang server/runtime code: sglang_utils/{arguments,sglang_engine}.py, rollout/sglang_rollout.py, the megatron_utils/sglang.py re-export shim, and all docker/**/sglang.patch files. - Rename the rollout config module sglang_utils/sglang_config.py -> vllm_utils/vllm_config.py (SglangConfig -> VllmConfig, _resolve_sglang_config -> _resolve_vllm_config, --sglang-config -> --vllm-config); inline the GPU_MEMORY_TYPE_* constants in rollout.py. - Add megatron_utils/fp8_helpers.py for the UE8M0 fp8 helpers formerly re-exported through the sglang shim; repoint quantizer_fp8 to it. - Swap sglang_router -> vllm_router in http_utils/wandb_utils; drop the dead sglang-router dependency from requirements.txt. - Finish the SGLang->vLLM rename in the runtime so it is internally consistent and matches the tests landing in the tests/CI PR: * router args --router-* -> --vllm-router-* (vllm_router_ip/port/timeout); * get_model_url reads vllm_model_routers (aligning with rollout.py); * --opd-type sglang -> vllm; engine_overrides rename; * sglang_enable_deterministic_inference -> vllm_enable_deterministic_inference, wired to a real --vllm-enable-deterministic-inference flag (exports VLLM_BATCH_INVARIANT=1); * consistent_hash session-id routing uses vllm-router's x-session-id header; * drop dead trace helper build_sglang_meta_trace_attrs; de-SGLang comments/docstrings. - Rename test_sglang_config.py -> test_vllm_config.py and de-SGLang the plugin-contract tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Address review: finish de-SGLang + fold OPD/router-policy into runtime - naming: replace residual generic "rollout engine"/"engine"/"comm" wording with concrete vLLM (engine_overrides -> vllm_overrides; arguments help text; http_utils comments; rollout.py "inference workers"). sglang->vllm is correct, sglang->generic is not. - megatron_to_hf: drop the q_a_proj/kv_a_proj_with_mqa pairing + _cached_tensors global. That was sglang-only: sglang's loader torch.cat's both shards within a single load_weights call (needs them co-bucketed), whereas vLLM loads each shard independently via stacked_params_mapping into fused_qkv_a_proj. Also fix the misleading "merge into single fused name" comment. - docker/Dockerfile: remove now-dead sglang/sglang-router --no-deps stubs + the build-time `import sglang` smoke check (slime no longer imports sglang_router). - OPD: migrate on_policy_distillation.py teacher logprobs to vLLM /v1/completions (prompt_logprobs) instead of sglang return_logprob / meta_info.input_token_logprobs. - routing replay: register --vllm-router-policy (dest=router_policy) so the consistent_hash x-session-id session-affinity path is actually wired (was dead). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Review follow-ups: mirror slime vllm_config parsing + restore vLLM process cleanup - vllm_config.from_yaml: drop the needless `models_raw` intermediate and iterate `data["vllm"]` directly, restoring the "Accept both server_groups / legacy engine_groups" comment -- mirrors slime's sglang_config.from_yaml line-for-line. - command_utils.execute_train: re-add a process kill for leftover rollout engines as `pkill -9 -f "vllm serve"` (the old `pkill -9 sglang` was dropped with no vLLM equivalent), so stale engines don't hold GPUs/ports across runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Drop test changes from runtime PR; tests live in the tests+CI PR (#40) The plugin_contracts tests and the test_sglang_config -> test_vllm_config rename are coupled to the test/CI rename effort and are owned by #40. Restore them to main here so #18 is purely the SGLang runtime removal. #18 merges first; #40 rebases and re-lands the vLLM test versions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fp8_helpers: copy SGLang verbatim (fix import crash); pin vLLM deep_gemm env - fp8_helpers.py: replace the bespoke rewrite with SGLang's exact implementations of quant_weight_ue8m0 / transform_scale_ue8m0 and their DeepGEMM helpers (per_block_cast_to_fp8, ceil_to_ue8m0, ceil_div, ceil_align, the torch-impl packer). deep_gemm is imported lazily inside the functions (as SGLang does), so module import no longer requires deep_gemm. This fixes the module-level `NameError: _get_tma_aligned_size` that crashed `import megatron_to_hf` on any deep_gemm image, and drops the invented sf-stride fixup block that was not in upstream. Only should_deepgemm_weight_requant_ue8m0 stays vLLM-adapted (is_deep_gemm_e8m0_used) since SGLang's reads SGLang-internal deep_gemm_wrapper. - vllm_engine.launch_server_process: set VLLM_USE_DEEP_GEMM=1 + VLLM_DEEP_GEMM_WARMUP=relax explicitly (setdefault) alongside VLLM_BATCH_INVARIANT, replacing SGLang's removed deep_gemm precompile/warmup envs. All vLLM engine env now lives in the subprocess env builder (single source of truth). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fp8_helpers: revert to vLLM impl + fix the import NameError; add vLLM trace attrs - fp8_helpers.py: keep the vLLM-based implementation (uses vllm.utils.deep_gemm, consistent with the vLLM runtime) rather than the SGLang verbatim copy. Fix the module-level crash: the `try` block referenced `_get_tma_aligned_size` before it was bound (the "pre-imported with fallback" import was never written), which raised NameError whenever deep_gemm imported successfully -- and NameError is not caught by `except ImportError`, so `import megatron_to_hf` crashed on any deep_gemm image. Replace the bogus self-assignment with the real import: `from vllm.utils.deep_gemm import get_tma_aligned_size as _get_tma_aligned_size`. - trace_utils/vllm_rollout: add build_vllm_meta_trace_attrs and attach finish_reason + token usage to the vllm_inference_generate span (mirrors SGLang's build_sglang_meta_trace_attrs; vLLM responses lack the pd_* timing, which lives in vLLM's own OTLP traces). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(opd): score teacher via /inference/v1/generate with prompt_logprobs Move the vllm OPD teacher path off the OpenAI /v1/completions endpoint onto vime's native /inference/v1/generate (the same endpoint the rollout engines use), and fix three latent issues: 1. model field: /inference/v1/generate takes `model` as OPTIONAL. Stop defaulting to args.hf_checkpoint (the *student* name, which mis-names a teacher!=student server). Add --opd-teacher-model; send `model` only when set, otherwise omit it (single-model teacher servers use their loaded model). 2. multimodal: the old code sent image_data to a token-only endpoint, which is invalid. Raise NotImplementedError until the /v1/chat/completions/render -> /inference/v1/generate flow is wired (mirrors slime.rollout.vllm_rollout.generate). 3. logprob robustness: read top-level GenerateResponse.prompt_logprobs, assert it is present and length-aligned with token_ids, assert the per-sample tensor covers response_length, and raise (not silently return 0.0) on a missing token logprob. vLLM always includes the actual prompt token in prompt_logprobs, so a miss is a real error. Alignment is unchanged (plp[i] <-> tokens[i], skip pos 0, take [-response_length:]). Follow-up (separate, in the tests PR): the OPD e2e test must launch a teacher that exposes /inference/v1/generate and point --rm-url at it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(clean-sglang): purge SGLang from tools, train scripts, and build infra tools/: drop dead `args.sglang_enable_ep_moe` shim (read nowhere); reword profile/replay helpers to vLLM and map analyzer hints to vLLM flags (--enforce-eager, --gpu-memory-utilization). train{,_async}.py: comments SGLang -> vLLM. build infra: remove build_conda.sh (SGLang-only conda path); drop the GB300 sgl-kernel install from the Dockerfile; delete docker/npu_patch/ wholesale. docker base image: bump to vLLM v0.22.0. justfile ARM recipes now pin the real multi-arch vLLM base images instead of the dead SGLANG_IMAGE_TAG/ ENABLE_SGLANG_PATCH build-args -- cu129-arm64 -> v0.22.0-cu129-ubuntu2404 (CUDA 12.9), cu13-arm64 -> v0.22.0-ubuntu2404 (the default-CUDA tag is already CUDA 13.0) + ENABLE_CUDA_13=1. vLLM tags are multi-arch manifests, so docker selects the arm64 image automatically on an ARM host. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci(clean-sglang): drop conda-build workflow (ran deleted build_conda.sh on SGLang image) The single build-conda job ran `bash build_conda.sh` (removed in the previous commit) inside an lmsysorg/sglang container. With the SGLang-only conda path gone, the whole workflow is dead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(clean-sglang): fix stale SGLang refs in docs/skills; tidy comments docs/conf.py: point the "edit on GitHub" links at vllm-project/vime instead of the inherited sgl-project.github.io repo. .claude/skills/*: update the dead `slime/rollout/sglang_rollout.py` references to `vllm_rollout.py` (the real default is slime.rollout.vllm_rollout.generate_rollout). justfile: drop the redundant BASE_IMAGE override on release-cu129-arm64 (it equalled the Dockerfile default; the multi-arch manifest already resolves arm64). train{,_async}.py: drop stray "the" in the W&B comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tests): use method=mtp (not eagle) in vllm speculative config The migrated speculative configs pass no draft `model`, so method=eagle raises "num_speculative_tokens was provided but without speculative model" in vLLM's SpeculativeConfig. These models carry embedded MTP layers, so method=mtp is correct and unblocks the mimo MTP-only-grad test (#19). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cleanup): target renamed vLLM subprocesses in pkill so VRAM is freed vLLM's set_process_title() renames the VRAM-holding subprocesses (VLLM::EngineCore, VLLM::Worker_TP*, vllm::router), so their cmdline no longer contains "vllm serve". The previous `pkill -9 -f "vllm serve"` matched only the launcher and left engine/worker children holding GPU memory, leaking it into the next run — masked only by the indiscriminate `pkill -9 python`, which is unsafe on colocate/shared nodes. Match both the launcher and the renamed children with `pkill -9 -f '[v]llm serve|VLL[M]::'`; the [v]/[M] bracket trick keeps the pattern from matching pkill's own cmdline. This makes the broad python kill unnecessary, so its already-commented-out lines are removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speculative): use method=mtp (not eagle) for embedded-MTP models vLLM's SpeculativeConfig requires an explicit draft `model` for method=eagle; with only num_speculative_tokens set it raises "num_speculative_tokens was provided but without speculative model". The migrated configs in scripts/examples/docs pass no model, so they must use method=mtp, which reuses the target checkpoint's embedded MTP layer (DeepSeek-R1, GLM-4.x-MoE, MiMo, Qwen3-Next/3.5). The two docs examples that pass an explicit "model" are genuine eagle usage and are left unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(vllm): launch each rollout engine with its ServerGroup's per-group TP launch_server_process / _init_normal derived tensor-parallel size and CUDA_VISIBLE_DEVICES from the global --rollout-num-gpus-per-engine, ignoring the per-engine num_gpus_per_engine already carried on the VLLMEngine actor. A ServerGroup configured with num_gpus_per_engine greater than the global flag (e.g. tp=2) therefore launched as tp=1, while the NCCL weight-sync rendezvous sized world_size from engine_gpu_counts (the per-group value). The two disagreed: the trainer waited for a rank the under-sized engine never started, so init_weight_transfer_engine hung for 300s ("3/4 clients joined") and the job failed. Honor the per-engine num_gpus_per_engine at launch, falling back to the global flag when unset (matches the SGLang path and PR #66's _compute_server_args). Verified on H200: tests/test_qwen2.5_0.5B_vllm_config_distributed now launches engine0 tp=2 / engine1 tp=1, update_weights completes in 1.1s (was a 301s timeout), and rollout+eval proceed. AI assistance (Claude Code) was used for this change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ckpt): add --dist-ckpt-optim-fully-reshardable for PAO+offload save/load test_qwen3_4B_ckpt.py uses precision-aware optimizer + cpu-offload (HybridDeviceOptimizer). Under the default dp_reshardable (bucket-centric) optimizer sharding, save/load produce unequal-length param_state lists, so dist-ckpt load fails with "Cannot merge two lists with different lengths (81 and 79)". fully_reshardable is model-centric and immune to bucket-layout changes. Verified on the r3 image (Megatron-LM 0.16.0rc0 @ 1dcf0da): save+load both succeed, and source review confirms master_param / step / HybridDeviceOptimizer sync are handled on this path. This is the flag described in PR #50 that was never actually merged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(router-args): hybrid naming — vllm_ for ip/port, bare router_ for timeout vllm-router's RouterArgs.from_cli_args only supports prefix "" or "router_" (never "vllm_router_"), and excludes host/port from its CLI via exclude_host_port=True. So: - --vllm-router-ip / --vllm-router-port keep the vllm_ prefix: RouterArgs does not own these CLI flags, vime does (populated via _start_router's manual router_args.host/port assignment), so the vllm_ prefix is free and marks them as vime-owned endpoint config. - --router-request-timeout-secs goes bare (dest router_request_timeout_secs): it is a genuine RouterArgs field, so it shares the --router-* namespace with policy / cache_threshold / retries / … and flows through from_cli_args like the other knobs. - --vllm-router-policy keeps dest=router_policy (unchanged). Also fixes conftest fixture to seed vllm_router_ip/port (was bare router_ip/port, which never matched the vllm_engine reader) and updates README/README_zh prose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Canlin Guo <canlinguosdu@gmail.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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--nnodes,--node-rank,--master-addr,--master-port, andmpbackends whennnodes > 1).Test Plan
python -m py_compile slime/backends/vllm_utils/vllm_engine.py slime/rollout/vllm_rollout.py slime/backends/megatron_utils/update_weight/update_weight_from_distributed.py tests/unit/backends/vllm_utils/test_vllm_engine.py tests/unit/rollout/test_vllm_rollout.py tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_distributed.pyPYTHONPATH=. ruff check slime/backends/vllm_utils/vllm_engine.py slime/backends/vllm_utils/arguments.py slime/rollout/vllm_rollout.py slime/backends/megatron_utils/update_weight/update_weight_from_distributed.py tests/unit/backends/vllm_utils/test_vllm_engine.py tests/unit/rollout/test_vllm_rollout.py tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_distributed.pyPYTHONPATH=. pytest -q tests/unit/backends/vllm_utils/test_vllm_engine.py tests/unit/backends/vllm_utils/test_arguments.py tests/unit/rollout/test_vllm_rollout.py->122 passed, 2 warnings117 passed6 passed19 passedtests/test_qwen2.5_0.5B_short.pywith stable vLLM image passed before latest review-fix roundtests/test_qwen2.5_0.5B_async_short.pywith stable vLLM image passed before latest review-fix roundtests/test_qwen2.5_0.5B_ppo_critic_only_short.pywith stable vLLM image passed before latest review-fix roundtests/test_qwen3.5_0.8B_gsm8k_short.pywith stable vLLM image passed before latest review-fix roundMulti-node end-to-end validation (Qwen3-30B-A3B, 2× H200 × 8)
11-config sweep on
Qwen3-30B-A3B(30B MoE, 128 experts, EP) onvime-vllm-r3:test(=inferactinc/public:vime-vllm-r3-latest). Covers single-host colocate, single-host disagg, cross-host colocate (the newnnodes > 1path), cross-host disagg, vLLM PP=2, and Megatron PP=2. Every config runs 2 rollout→IPC-weight-sync→train steps (not 1) so the sweep also catches multi-step issues (weight-version drift, grad_norm blowup, IPC state-machine reuse across updates). PASS = Megatron load → IPC weight transfer → vLLM rollout → train, for both steps, with boundedgrad_norm.Sweep matrix
rollout_num_gpus_per_enginenum_gpus_per_nodennodes_per_enginennodes == 1branch)nnodes > 1— one logical engine spans both hosts;--headlesson node_rank=1; mp backendsnnodes > 1+ multi-engine — each engine spans 2 logical nodesnnodes == 1per engine_get_vllm_tp_sizepp-divisor path)nnodes > 1nnodes > 1(cross-host)vLLM grabs
tp × pp × dpGPUs per engine, so the per-engine TP =engine_gpus // (pp × dp)(e.g. s11: 8//(1·2)=4, s12: 8//(1·4)=2). With--enable-expert-parallel, experts shard across the fulltp × dpworld while attention is replicated per DP rank — the standard "wide-EP" MoE serving layout.Results (2 steps each —
grad_normshown for step 0 / step 1)All single-host and cross-host topology configs (ref, s1–s10) PASS both steps.
grad_normstays bounded across step 0 → step 1 (single-host0.0 – 0.37; cross-host nnodes=2 colocate higher at0.49 – 1.30from the larger TP=8/16 world, but no NaN/inf/divergence).train_rollout_logprob_abs_diffholds ~0.04 – 0.05on single-host and engine-on-host configs, and0.12 – 0.22on the cross-host nnodes=2 colocate configs (s3/s4) where one engine's TP spans both hosts — expected, since cross-host all-reduce changes generation numerics slightly; it does not grow unboundedly.train/loss/pg_lossare ~1e-8/0because the ref checkpoint is loaded fresh, so there is no train/rollout policy gap on these early steps.Known issue 1: vLLM PP / DP CUDA-graph capture hangs (s8/s9/s11/s12/s13 run eager)
With
--vllm-pipeline-parallel-size 2or--vllm-data-parallel-size > 1, vLLM's CUDA-graph capture deadlocks for this MoE model in the colocate setup: all ranks pin 100 % GPU util and a cross-rank collective inside_capture_cudagraphstimes out (NCCL "last completed work: -1"; PP froze at ~2 % for >25 min). This is inside vLLM's multi-dim graph capture, orthogonal to the rollout topology this PR adds, so the PP/DP configs set--vllm-enforce-eagerto skip capture and still exercise the real parallel rollout path. With eager, all of them run rollout + IPC weight sync + train cleanly. Capturing under PP/DP is a vLLM-side follow-up, not a blocker for this PR.vLLM prefill/decode (PD) disaggregation — NIXL, implemented + 1P1D proven
PD disaggregation for the vLLM backend is now implemented (it was previously a reserved CLI stub —
_compute_server_argsdiscardedworker_type/bootstrap and emitted no KV-transfer flags). vLLM does PD via the NIXL KV connector: a prefill engine computes the prompt KV and a decode engine pulls it over a NIXL side channel, coordinating throughkv_transfer_paramscarried on the request/response. The stockvllm_routeronly implements SGLang-style PD (simultaneous dispatch + bootstrap room), so this PR adds a vLLM-native path:vllm_engine.py): whenworker_typeisprefill/decode, launch with--kv-transfer-config '{"kv_connector":"NixlConnector","kv_role":"kv_both","engine_id":...}'and a uniqueVLLM_NIXL_SIDE_CHANNEL_HOST/PORT.pd_proxy.py): speaks the surface slime's rollout already targets (POST /inference/v1/generate,/health, worker listing, broadcast control) and drives the two-step handshake — prefill withkv_transfer_params={do_remote_decode:true}+max_tokens=1, then decode with{do_remote_prefill:true, **prefill_params}. vLLM'sGenerateRequest/GenerateResponseboth carrykv_transfer_params, so the relay rides the existing endpoint.rollout.py):_start_routerlaunchespd_proxy(instead of the SGLangmini_lb) when a model has PD disaggregation and the vLLM backend is selected (opt-inSLIME_VLLM_PD_NIXL=1). The rest is already backend-generic —from_prefill_num_serversbuilds prefill/decodeServerGroups,start_enginescreatesVLLMEngineactors withworker_type, and each engine registers with the proxy viaPOST /workers.tests/multinode/vllm_disagg_serving_kv_transfer.patch): the disagg serving handler (vllm/entrypoints/serve/disagg/serving.py, in the vLLM image — not slime) acceptskv_transfer_paramsin itsGenerateRequestschema but never threaded it into the engine, so the NIXL handshake never engaged on/inference/v1/generate(it does on/v1/completions). The ~6-line fix injectsrequest.kv_transfer_paramsintoSamplingParams.extra_args(vLLM v1 reads it there).Proofs on 2× H200 (r3 image, Qwen3-0.6B):
tests/multinode/nixl_pd_proof.sh(raw/v1/completions, 1P1D): PASS — prefill returns NIXL handles (remote_engine_id,remote_block_ids, side-channelhost:port,remote_num_tokens), decode pulls KV and decodes" Paris. The capital of France is ...".tests/multinode/nixl_pd_proxy_test.sh(slimepd_proxyover/inference/v1/generate, the exact rollout path, with the serving patch): PASS — proxy returns HTTP 200 withkv_transfer_paramspopulated + correct decode output. Without the patch the same request returnsprefill returned no kv_transfer_params.So vLLM-native PD is implemented and proven end-to-end through the exact endpoint slime's rollout uses. The remaining step before merge is to land the serving fix in the vLLM image and run a full RL-loop PD smoke (
--prefill-num-servers+SLIME_VLLM_PD_NIXL=1).Known issue 2: intermittent CUDA illegal-memory-access on DP+EP offload
DP+EP is functional across single-host (s11 dp=2, s12 dp=4) and cross-host × nnodes=2 (s13 dp=2): vLLM launches
dpEngineCores (EngineCore_DP0/DP1/...) ontp×dpGPUs, each takes IPC weight updates (POST /update_weights200 across all DP API servers), and rollout + train complete both steps. Observed runs: s12 (dp=4) PASS first try; s11 (dp=2) PASS 2/3; s13 (cross-host dp=2) PASS on retry. The intermittent failures throwCUDA error: an illegal memory accessinside a DP worker's CUDA path — for s11 during the 2ndrelease_memory_occupation(sleep/offload between steps), for s13 during the 1st IPCcudaMemcpyweight transfer. It is non-deterministic (same config passes on retry) and lives in the vLLM data-parallel + sleep-mode/IPC path, not in the rollout-topology code this PR adds. Flagged for a vLLM-side follow-up; the slime-side enablement (per-enginetp = engine_gpus // (pp × dp)) is correct and committed (948d7fc).Parallelism dimensions swept
tp = engine_gpus // (pp × dp); vLLM DP+EP wide-expert layout (eager). See DP+EP known issue 2.tp × dpin s11/s12/s13nnodes_per_engine(PR #66 new axis)Code-path coverage in PR #66
s3ands4specifically exercise the newnnodes > 1branch inslime/backends/vllm_utils/vllm_engine.py:nnodes = max(1, gpus_per_engine // args.num_gpus_per_node)✅ (s3: 16/8=2,s4: 8/4=2)node_rank = rank % nnodes✅cmd.append("--headless")fornode_rank != 0✅ (head log:non-default args: {... 'nnodes': 2, 'tensor_parallel_size': 16, 'distributed_executor_backend': 'mp', ...}; worker rank receives--headless)cmd += ["--data-parallel-backend", "mp", "--distributed-executor-backend", "mp"]fornnodes > 1✅/server_info/ health-check /_register_worker_with_routerowned bynode_rank == 0only ✅POST /init_weight_transfer_engine→POST /start_weight_update→ N ×POST /update_weights(115 IPC chunks per update) →POST /finish_weight_update→POST /wake_up?tags=weights&kv_cache— verified via head-side vLLM access log ✅Test file + cross-host orchestration
Committed under
tests/multinode/(this PR) so reviewers can clone the branch and reproduce directly:tests/test_qwen3_30B_A3B_pr66_sweep.pyPR66_CONFIGenv vartests/multinode/head.sh/etc/hosts, starts ray head, runs testtests/multinode/worker.sh/etc/hosts, joins ray, waits on barriertests/multinode/launch_cross_host.sh(Run instructions, config matrix, and deployment requirements are inlined in this PR description — see the sections below.)
Snippet of the test file (full source in the PR diff):
Click to expand
test_qwen3_30B_A3B_pr66_sweep.pyHow to launch each config
ref,s1,s2,s6,s8,s10): launch one container on one H200 with the slime workspace mounted, setPR66_CONFIG=<id>, runpython tests/test_qwen3_30B_A3B_pr66_sweep.py. Slime starts its own ray head inside the container.s3,s4,s5,s7,s9): usetests/multinode/launch_cross_host.sh— it spawns one container on each host with--network host, builds the ray cluster externally, thenSLIME_SCRIPT_EXTERNAL_RAY=1 + MASTER_ADDR=<head_ip>makes slime reuse the cluster:The driver mounts
head.sh/worker.shinto each container; they handle the/etc/hostsrewrite, ray head/join, test execution, and teardown via NFS-shared barrier files. See the Cross-host deployment requirements section below for the full env-var reference.Compact script summaries (full source under
tests/multinode/)head.sh— head container entrypoint:pr66-worker.sh— runs inside the worker container (mounted at/root/worker.sh).launch-pr66-cross-host.sh— driver, run on the control machine. Spawns head + worker viadocker run --network hoston each host. (NFS-shared paths assumed.)Cross-host deployment requirements (discovered during validation)
These are operational / environment requirements, not PR-code changes. The PR's nnodes>1 code path is correct; these are what an integrator has to set up to actually run it cross-host on a stock Linux + Docker cluster:
--network hoston every container. Docker bridge networks don't span hosts; nnodes>1 needs cross-host TCP reachability for Ray + vLLM master_addr + Gloo full-mesh.127.0.1.1 <hostname>line in container/etc/hostswith<real_host_ip> <hostname>. Default Docker/etc/hostsmaps the container's hostname to127.0.1.1; Gloo's full-mesh peer init pulls each rank's local hostname-resolved IP and shares it with peers, so cross-host peers then try to connect to127.0.1.1and getConnection refused. Important:sed -idoes not work on the docker-managed/etc/hostsbind mount (it writes a renamed file that no longer maps to the bind target); usecat > /etc/hostsinstead — see scripts above.NCCL_SOCKET_IFNAME=<lan_iface>(e.g.ens7)GLOO_SOCKET_IFNAME=<lan_iface>NCCL_IB_DISABLE=1if IB interfaces are DOWN (otherwise NCCL retries IB and times out).SLIME_SCRIPT_EXTERNAL_RAY=1+MASTER_ADDR=<head_ip>in the head's env before runningexecute_train— tells slime to skip its ownray stop/ray start --headand reuse the already-built cluster.SLIME_HOST_IP=<real_host_ip>per container —slime/utils/http_utils.py:get_host_info()already honors this env var; setting it sidesteps any leftover127.0.1.1resolution inray._private.services.get_node_ip_address()paths.Image used:
inferactinc/public:vime-vllm-r3-lateston both hosts. vLLM nightly inside this image (v0.21.1rc1.dev38+gff712f644) containsvllm#39568(the RLHF IPC plumbing this PR talks to).Runtime observations
update_weights33–55 s (vs single-host ~30 s). The slowdown is the TCP-over-Ethernet fallback (IB not available on these particular H200s); semantics are correct.s6/s7)update_weightsare the slowest (50–55 s) because train and rollout GPUs are disjoint, so weight sync goes through NCCL bucket broadcast instead of in-process CUDA IPC.(TP, PP) = (1, 8)ckpt →(4, 1)/(8, 1)/ etc. at runtime) works across all configs with no manual intervention.train_rollout_logprob_abs_diff = 0.04 – 0.13across all configs (single-host ≈ 0.04, cross-host nnodes=2 ≈ 0.10–0.13). The slight uptick on nnodes=2 is consistent with the larger vLLM TP world doing more cross-host reductions during generation.