Skip to content

feat(vllm): DP+EP and PD (prefill/decode) rollout disaggregation via the production vllm-router - #108

Closed
aoshen02 wants to merge 7 commits into
mainfrom
pr68-harden
Closed

feat(vllm): DP+EP and PD (prefill/decode) rollout disaggregation via the production vllm-router#108
aoshen02 wants to merge 7 commits into
mainfrom
pr68-harden

Conversation

@aoshen02

@aoshen02 aoshen02 commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Extends the multi-node rollout work (#68 / #85 / #89 lineage) to disaggregated rollout: data-parallel + expert-parallel engines, and prefill/decode (PD) split over NIXL, routed by the real vllm-router with static URLs (SkyRL-style) — not the MiniLB test stub.

Validated on 2-node GB200 (NVL72, arm64/cu13, 4 GPU/node): cross-node PD with tp4-per-engine (prefill on rack1-04, decode on rack1-05), VllmPDRouter starts from static prefill/decode URLs, NIXL handshake engages, rollout generates with 0× 502.

What's in here

  • DP+EP (f4ded47): wire tp = gpus // (pp*dp), EP across engines; fix the glm4v_moe fork eager-import that crashed engine construction. Probe + Qwen3-30B-A3B DP=2+EP e2e (a2fea2f).
  • PD disaggregation (281364d): prefill/decode engines over the vLLM NIXL KV connector; /sleep+/wake_up honor the tunable weight-transfer timeout instead of a hardcoded 30s (7a9f9e7); health-wait 300s→1200s for large DP/MoE startup (b366228). e2e for PD (f8ee60e, e1a87eb) and non-colocate DP=2 (3d2ea36).
  • Production router (tip, 2489dfe): route PD through VllmPDRouter (vllm_pd_disaggregation) with static prefill_urls/decode_urls — engines start first, the orchestrator collects each engine's (url, bootstrap_port)/url, then launches the router with those URLs. Engines pass router_ip=None so they skip /workers self-registration. MiniLB fallback removed. The hand-rolled pd_proxy stays opt-in via SLIME_VLLM_PD_NIXL=1, and now sends kv_transfer_params inside sampling_params.extra_args (where vLLM's disagg /inference/v1/generate actually reads it).
  • Hardening / observability: per-engine-TP + strict external-config check (67679bc, fix(vllm): per-engine TP + strict external config check (harden #68) #85), lifecycle reorder of vllm_engine.py (no logic change, 4c00a0c), [vllm-topo] logging (3fee27d, 66dda9d).

Scope / dependencies

🤖 Generated with Claude Code

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces multi-node rollout engine topology support for vLLM, including Data Parallelism (DP), Expert Parallelism (EP), and Prefill/Decode (PD) disaggregation via a new NIXL relay proxy. It also resolves an eager import crash in glm4v_moe.py and adds comprehensive end-to-end tests. The review feedback highlights critical improvements: reducing the startup timeout for headless workers to prevent a 20-minute block, moving the weight-sync group guard to cover non-packed paths, defensively handling None values in the proxy's JSON payloads, and registering a cleanup handler to close the aiohttp client session to avoid resource leaks.

if topology.node_rank == 0:
_wait_server_healthy(self._http_base(), process=self.process)
else:
_wait_worker_process_alive(self.process)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In multi-node setups, headless workers (node_rank > 0) do not expose an HTTP health endpoint, so they call _wait_worker_process_alive(self.process). Since _wait_worker_process_alive has a default timeout_s of 1200.0 (20 minutes) and loops until the timeout is reached if the process remains alive, this will cause every headless worker to block the init() method for 20 minutes on successful startup.

Since the head node (node_rank == 0) already waits for the entire cluster to be healthy via _wait_server_healthy, the headless workers only need to perform a short sanity check to ensure their subprocesses didn't immediately crash. Passing a short timeout (e.g., 15.0 seconds) to _wait_worker_process_alive will prevent this massive startup delay.

Suggested change
_wait_worker_process_alive(self.process)
_wait_worker_process_alive(self.process, timeout_s=15.0)

Comment on lines +229 to +235
if self._model_update_groups is None:
raise RuntimeError(
"[vllm-topo] _update_weights_vllm_packed reached with _model_update_groups=None "
f"(is_pp_src_rank={getattr(self, '_is_pp_src_rank', None)} group={getattr(self, '_group_name', None)}). "
"The weight-sync NCCL group was never connected; check engine_gpu_counts vs launched tp "
"in the [vllm-topo] server_args logs."
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The guard checking if self._model_update_groups is None is currently only implemented in _update_weights_vllm_packed. However, the non-packed path (_update_bucket_weights_from_distributed and _update_expert_bucket_weights_from_distributed) also runs on the PP source rank and calls update_weights_from_distributed using self._model_update_groups. If self._model_update_groups is None there, it will pass None to trainer_send_weights, leading to an opaque hang or crash.

To protect both the packed and non-packed paths, consider moving this guard to the very beginning of _sync_weights_to_rollout_engines so it executes once for the PP source rank before any weight synchronization begins.

Comment thread slime/backends/vllm_utils/pd_proxy.py Outdated
Comment on lines +102 to +104
pf_body = copy.deepcopy(body)
pf_body.setdefault("sampling_params", {})["max_tokens"] = 1
pf_body["sampling_params"].setdefault("extra_args", {})["kv_transfer_params"] = {"do_remote_decode": True}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using .setdefault() and direct subscripting on sampling_params and extra_args can raise a TypeError if the input JSON explicitly sets either of these fields to None (which is common in client payloads). Ensuring these fields are valid dictionaries before mutating them makes the proxy much more robust.

        pf_body = copy.deepcopy(body)
        sampling_params = pf_body.get("sampling_params")
        if not isinstance(sampling_params, dict):
            sampling_params = {}
            pf_body["sampling_params"] = sampling_params
        sampling_params["max_tokens"] = 1
        extra_args = sampling_params.get("extra_args")
        if not isinstance(extra_args, dict):
            extra_args = {}
            sampling_params["extra_args"] = extra_args
        extra_args["kv_transfer_params"] = {"do_remote_decode": True}

Comment thread slime/backends/vllm_utils/pd_proxy.py Outdated
Comment on lines +122 to +126
dc_body = copy.deepcopy(body)
dc_body.setdefault("sampling_params", {}).setdefault("extra_args", {})["kv_transfer_params"] = {
"do_remote_prefill": True,
**ktp,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Similar to the prefill step, defensively ensure that sampling_params and extra_args are valid dictionaries before setting kv_transfer_params to avoid potential TypeError exceptions when processing client payloads.

Suggested change
dc_body = copy.deepcopy(body)
dc_body.setdefault("sampling_params", {}).setdefault("extra_args", {})["kv_transfer_params"] = {
"do_remote_prefill": True,
**ktp,
}
dc_body = copy.deepcopy(body)
dc_sampling_params = dc_body.get("sampling_params")
if not isinstance(dc_sampling_params, dict):
dc_sampling_params = {}
dc_body["sampling_params"] = dc_sampling_params
dc_extra_args = dc_sampling_params.get("extra_args")
if not isinstance(dc_extra_args, dict):
dc_extra_args = {}
dc_sampling_params["extra_args"] = dc_extra_args
dc_extra_args["kv_transfer_params"] = {
"do_remote_prefill": True,
**ktp,
}

Comment thread slime/backends/vllm_utils/pd_proxy.py Outdated
Comment on lines +157 to +166
def build_app(self) -> web.Application:
app = web.Application(client_max_size=512 * 1024 * 1024)
app.router.add_post(GENERATE_PATH, self.handle_generate)
app.router.add_get("/health", self.handle_health)
app.router.add_post("/workers", self.add_worker)
app.router.add_get("/workers", self.list_workers)
app.router.add_delete("/workers/{url}", self.remove_worker)
for ctl in ("/abort_requests", "/resume", "/pause"):
app.router.add_post(ctl, self.handle_broadcast)
return app

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The aiohttp.ClientSession created in _ensure_session is never closed, which can lead to resource leaks and unclosed connector warnings when the application shuts down. Registering an on_cleanup signal handler to close the session ensures proper resource cleanup.

Suggested change
def build_app(self) -> web.Application:
app = web.Application(client_max_size=512 * 1024 * 1024)
app.router.add_post(GENERATE_PATH, self.handle_generate)
app.router.add_get("/health", self.handle_health)
app.router.add_post("/workers", self.add_worker)
app.router.add_get("/workers", self.list_workers)
app.router.add_delete("/workers/{url}", self.remove_worker)
for ctl in ("/abort_requests", "/resume", "/pause"):
app.router.add_post(ctl, self.handle_broadcast)
return app
async def cleanup(self, app: web.Application) -> None:
if self._session is not None and not self._session.closed:
await self._session.close()
def build_app(self) -> web.Application:
app = web.Application(client_max_size=512 * 1024 * 1024)
app.router.add_post(GENERATE_PATH, self.handle_generate)
app.router.add_get("/health", self.handle_health)
app.router.add_post("/workers", self.add_worker)
app.router.add_get("/workers", self.list_workers)
app.router.add_delete("/workers/{url}", self.remove_worker)
for ctl in ("/abort_requests", "/resume", "/pause"):
app.router.add_post(ctl, self.handle_broadcast)
app.on_cleanup.append(self.cleanup)
return app

@CalvinXKY

Copy link
Copy Markdown
Collaborator

When you commit a PR, please refer the CONTRIBUTING.md to modify your PR format.

aoshen02 added a commit that referenced this pull request Jun 3, 2026
…outer

Port of PR #108 onto vime/main (post-#127 topology + #105 slime/->vime/
rename). Carries the full feature set #108 accumulated, including the
DP+EP work formerly tracked by the now-closed #125.

Engine (vllm_engine.py):
- DP+EP sizing/launch: tp = gpus_per_engine // (pp * dp); EP via
  --enable-expert-parallel; single-node DP uses --data-parallel-backend mp.
- Conservative DP+EP sleep-mode defaults (no-async-scheduling,
  enforce-eager, distributed-timeout-seconds=1800; VLLM_ENGINE_READY_TIMEOUT_S
  extended for DP) to avoid cuMemcpyDtoDAsync segfaults, honouring user
  overrides.
- PD NIXL wiring: --kv-transfer-config NixlConnector (kv_both),
  VLLM_NIXL_SIDE_CHANNEL_HOST/PORT, disaggregation_bootstrap_port in
  _compute_server_args + [vllm-topo] server_args trace.

Router/rollout (rollout.py):
- _launch_static_pd_router: SkyRL-style static prefill/decode URLs for PD.
- _start_router is now non-PD only; dropped the has_pd_disaggregation param.
  (Fixes a latent NameError #108 left: the dropped param was still
  referenced in the body. PD config lives entirely in the static path.)
- _sanitize_vllm_router_args / _vllm_router_args_from_cli helpers
  (negative-int CLI sanitisation; disable_health_check when supported).
- start_rollout_servers rewired for use_static_pd_router + engine_router_ip.

Weight sync (update_weight_from_tensor.py +140, _from_distributed.py +11):
- packed / tensor_sizes params paired with update_weights_from_ipc_handles.

Tests: DP / PD / DP+EP-r3 integration tests; test_vllm_engine updates.

Excluded vs upstream #108:
- vime_plugins/megatron_bridge/glm4v_moe.py changes dropped (per request).
- Removed 3 #125-labelled engine tests + the wake_up weight-transfer-timeout
  assertion (user's staged change). NOTE: the underlying feature code is
  still live, so those tests would have passed — flagged for review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@aoshen02
aoshen02 changed the base branch from feature/multi_nodes to main June 3, 2026 08:44
aoshen02 added a commit that referenced this pull request Jun 3, 2026
…outer

Port of PR #108 onto vime/main (post-#127 topology + #105 slime/->vime/
rename). Carries the full feature set #108 accumulated, including the
DP+EP work formerly tracked by the now-closed #125.

Engine (vllm_engine.py):
- DP+EP sizing/launch: tp = gpus_per_engine // (pp * dp); EP via
  --enable-expert-parallel; single-node DP uses --data-parallel-backend mp.
- Conservative DP+EP sleep-mode defaults (no-async-scheduling,
  enforce-eager, distributed-timeout-seconds=1800; VLLM_ENGINE_READY_TIMEOUT_S
  extended for DP) to avoid cuMemcpyDtoDAsync segfaults, honouring user
  overrides.
- PD NIXL wiring: --kv-transfer-config NixlConnector (kv_both),
  VLLM_NIXL_SIDE_CHANNEL_HOST/PORT, disaggregation_bootstrap_port in
  _compute_server_args + [vllm-topo] server_args trace.

Router/rollout (rollout.py):
- _launch_static_pd_router: SkyRL-style static prefill/decode URLs for PD.
- _start_router is now non-PD only; dropped the has_pd_disaggregation param.
  (Fixes a latent NameError #108 left: the dropped param was still
  referenced in the body. PD config lives entirely in the static path.)
- _sanitize_vllm_router_args / _vllm_router_args_from_cli helpers
  (negative-int CLI sanitisation; disable_health_check when supported).
- start_rollout_servers rewired for use_static_pd_router + engine_router_ip.

Weight sync (update_weight_from_tensor.py +140, _from_distributed.py +11):
- packed / tensor_sizes params paired with update_weights_from_ipc_handles.

Tests: DP / PD / DP+EP-r3 integration tests; test_vllm_engine updates.

Excluded vs upstream #108:
- vime_plugins/megatron_bridge/glm4v_moe.py changes dropped (per request).
- Removed 3 #125-labelled engine tests + the wake_up weight-transfer-timeout
  assertion (user's staged change). NOTE: the underlying feature code is
  still live, so those tests would have passed — flagged for review.

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

Port of PR #108 onto vime/main (post-#127 topology + #105 slime/->vime/
rename). Carries the full feature set #108 accumulated, including the
DP+EP work formerly tracked by the now-closed #125.

Engine (vllm_engine.py):
- DP+EP sizing/launch: tp = gpus_per_engine // (pp * dp); EP via
  --enable-expert-parallel; single-node DP uses --data-parallel-backend mp.
- Conservative DP+EP sleep-mode defaults (no-async-scheduling,
  enforce-eager, distributed-timeout-seconds=1800; VLLM_ENGINE_READY_TIMEOUT_S
  extended for DP) to avoid cuMemcpyDtoDAsync segfaults, honouring user
  overrides.
- PD NIXL wiring: --kv-transfer-config NixlConnector (kv_both),
  VLLM_NIXL_SIDE_CHANNEL_HOST/PORT, disaggregation_bootstrap_port in
  _compute_server_args + [vllm-topo] server_args trace.

Router/rollout (rollout.py):
- _launch_static_pd_router: SkyRL-style static prefill/decode URLs for PD.
- _start_router is now non-PD only; dropped the has_pd_disaggregation param.
  (Fixes a latent NameError #108 left: the dropped param was still
  referenced in the body. PD config lives entirely in the static path.)
- _sanitize_vllm_router_args / _vllm_router_args_from_cli helpers
  (negative-int CLI sanitisation; disable_health_check when supported).
- start_rollout_servers rewired for use_static_pd_router + engine_router_ip.

Weight sync (update_weight_from_tensor.py +140, _from_distributed.py +11):
- packed / tensor_sizes params paired with update_weights_from_ipc_handles.

Tests: DP / PD / DP+EP-r3 integration tests; test_vllm_engine updates.

Excluded vs upstream #108:
- vime_plugins/megatron_bridge/glm4v_moe.py changes dropped (per request).
- Removed 3 #125-labelled engine tests + the wake_up weight-transfer-timeout
  assertion (user's staged change). NOTE: the underlying feature code is
  still live, so those tests would have passed — flagged for review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aoshen02 and others added 6 commits June 4, 2026 01:18
…is the real guard)

The colocate DP+EP + sleep/wake crash at update_weights (cuMemcpyDtoDAsync
segfault / cublas CUBLAS_STATUS_EXECUTION_FAILED -> illegal memory access) is
fixed at the root by FIX A (execute_dummy_batch skip while the engine is asleep
/ only partially woken / mid weight-update), not by forcing eager + sync
scheduling. Drop the PR #125 --enforce-eager / --no-async-scheduling injection
so cudagraph + async scheduling stay enabled; keep the longer
--distributed-timeout-seconds for the weight-sync window.

Verified on gb200 2-node 35B PD: iter0 weight-sync + rollout + train pass with
cudagraph FULL + async scheduling enabled.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_awake is the real guard)

_weight_update_active was never assigned anywhere in the tree — it was only
read (default False) in the execute_dummy_batch skip, so the branch was dead.
The guard that actually fires is `fully_awake`: vime's staged weight sync wakes
the engine weights-only (kv_cache still asleep), so fully_awake stays False for
the whole update window and the dummy forward is skipped. Remove the dead
`or _weight_update_active` clause and clarify the comment. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…usion, VLLM_ENGINE_READY_TIMEOUT_S)

- model_provider.py: remove provider.gradient_accumulation_fusion assignment — main already
  sets it via #101 (model_provider.py:99); #108's copy is a duplicate from a stale base.
- vllm_engine.py: remove the dp_size>1 VLLM_ENGINE_READY_TIMEOUT_S=1800 env (PR #125 carryover);
  the longer --distributed-timeout-seconds already covers the colocate readiness window.

No behavior change on the validated path. Kept the router pd_disaggregation/disable_circuit_breaker
hasattr shims — those fields exist in the installed vllm-router and were active in the green run.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… in validation

FIX B wrapped Worker.determine_available_memory to retry the "Error in memory profiling"
assert (init_free >= post_profile_free) that can trip when colocated megatron frees GPU
memory during the engine's profile_run. It never fired across the gb200 2-node validation
runs (8378/8380/8381/8383) — removing it is behavior-neutral on the validated path. The
guarded assert is a loud, startup-only, re-runnable failure (not silent), so dropping the
fragile monkeypatch on vLLM internals is an acceptable trade for a leaner diff. FIX A
(execute_dummy_batch skip) is kept.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…imal

#108 had gratuitously rewritten _start_router (changed `assert process.is_alive()` to an
if/raise, dropped the has_pd_disaggregation param + its vllm_pd_disaggregation/
disable_circuit_breaker block, reordered imports, churned the docstring). Revert all of that
— _start_router is now byte-identical to origin/main. The only rollout.py change vs main is
the genuinely-new PD path: _launch_static_pd_router (which also uses `assert process.is_alive()`
for consistency) + the use_static_pd_router dispatch in start_rollout_servers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…path)

Revert every #108 timeout change back to main — they were speculative defensive bumps the
validated gb200 2-node 35B run never exercised (measured: sleep 2.79s, startup ~171s, weight
update ~48s):
- drop the --distributed-timeout-seconds 1800 injection (no engine-arg override forced now)
- _wait_worker_process_alive default 1200 -> 300 (main)
- /sleep and /wake_up HTTP timeouts back to 30 (main); _weight_transfer_http_timeout stays
  used only where main already used it.

Behavior-neutral on the validated path; keeps the diff minimal.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@princepride

Copy link
Copy Markdown
Collaborator

Hi, how is the pr going, do you need my help? 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants