Repository navigation
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@linyueqian @Sy0307 麻烦帮忙 review 一下这个 Qwen3-TTS PD disaggregation 的 PR,谢谢! |
1952659 to
e2b80fa
Compare
|
submit to main please |
linyueqian
left a comment
There was a problem hiding this comment.
Thanks for the substantial work here. The M-P-N-D topology detection, per-prefill bootstrap bookkeeping, and the port-collision validation in pd_utils are all solid, and the deploy YAMLs are thoroughly annotated. I ran the PR end to end on one 8x NVIDIA H20 (141GB) node (Qwen3-TTS-12Hz-0.6B-Base, seed-tts-eval en, 50 prompts per cell, vllm bench serve --omni, concurrency 1/8/16/32) and reviewed the runtime changes. Requesting changes based on what I measured.
1. [Blocking] This PR halves non-PD Qwen3-TTS throughput at concurrency
single topology, default qwen3_tts.yaml, 1 GPU, identical prompt set:
| c | base d4a869fe (req/s) |
this PR (req/s) | delta |
|---|---|---|---|
| 1 | 1.33 | 1.35 | ~0 |
| 8 | 3.83 | 2.13 | -44% |
| 16 | 4.49 | 2.17 | -52% |
| 32 | 4.45 | 2.15 | -52% |
Mean E2EL roughly doubles at c>=8 (1979 -> 3595 ms at c=8) and RTF at c=16 goes 0.72 -> 1.52, i.e. slower than real time. The cause is the _talker_mtp_forward change (see inline comment): reverting only that hunk on the PR head restores c=16 to 4.53 req/s. Every Qwen3-TTS deployment shares this code path, so this is a blocker independent of PD.
2. PD gains do not reproduce once the baseline is fair
Real 1p1d (3 GPUs: prefill + decode + code2wav; Mooncake handshake confirmed in logs and per-GPU memory residency):
| c | req/s | mean TTFP (ms) | mean E2EL (ms) |
|---|---|---|---|
| 1 | 1.11 | 272 | 899 |
| 8 | 2.50 | 289 | 3034 |
| 16 | 2.44 | 884 | 6145 |
| 32 | 2.43 | 1921 | 11111 |
And a tuned non-PD control on the base branch: talker and code2wav split across 2 GPUs, max_num_seqs: 128, gpu_memory_utilization: 0.6, max_num_batched_tokens: 4096 (the same caps the PD YAMLs use):
| c | req/s | mean TTFP (ms) |
|---|---|---|
| 1 | 1.45 | 148 |
| 8 | 6.18 | 344 |
| 16 | 7.32 | 712 |
| 32 | 8.78 | 677 |
Observations:
- 1p1d on 3 GPUs (2.4-2.5 req/s at c>=8) is slower than today's default single on 1 GPU (3.8-4.5 req/s), and about 3.6x slower than the 2-GPU non-PD control. The control also beats the absolute 1p6d number from the PR description (8.14 req/s on 8 GPUs) using 2 GPUs.
- The benchmark table in the description compares PD against a
singlebaseline that (a) runs on this branch, so it carries the regression from item 1, and (b) keepsmax_num_seqs: 10/gpu_mem 0.3/ 1 GPU while PD stages get 128-256 seqs / 0.6 / 3-8 GPUs. ThesingleTTFP blowup at c>=16 in that table is queueing at the max_num_seqs cap, not a fundamental limit. - The one real effect I can confirm: PD keeps TTFP flatter under load (884/1921 ms at c=16/32 vs 1244/3328 ms for default single). But the 2-GPU non-PD control is better still (712/677 ms), and at c=1 the KV-transfer hop makes PD TTFP worse than baseline (272 vs 158 ms).
Suggestion: rerun the sweep with the baseline on the base branch, caps aligned, and a same-GPU-count non-PD control (independent replicas or a split-stage deploy). If 1pNd still wins under those controls at some N, that is the number worth advertising.
3. Correctness verification is currently not runnable
tests/test_pd_disaggregation.pyis skipped at module level (see inline comment), so the stated test plan passes without executing an assertion.probe_1p1d.pyrequires an AR-only NDJSON deploy;start_pd.shreferencesqwen3_tts_pd_ar_only_3p1d.yamlwhich is not in the PR, and the shipped 1p1d yaml emits audio, which the probe rejects by design. The manual test plan cannot be executed as written.- I spot-checked audio with whisper-small on the self-describing check wavs: intelligible speech in both PD and non-PD cells, occasional garbled samples in both (temp 0.9 on a cloned voice), so inconclusive. Given the probe docstring itself warns about the code-predictor KV path, a runnable seeded/greedy bit-equality check should land with this PR.
4. Environment and packaging
mooncake-transfer-engine 0.3.8.post1 wheels require glibc >= 2.34 and link against CUDA 12 userspace. On a glibc 2.32 host with a cu13 torch stack the connector cannot load at all (libcudart.so.12 -> GLIBCXX_3.4.29 -> GLIBC_2.34 chain); I could only bring PD up inside an Ubuntu 22.04 container after also apt-installing libcurl4 and libibverbs1. None of this is documented and there is no requirements/extras entry for mooncake. Please add an optional dependency and a supported-environment note.
Also: as @hsliuustc0106 said, please retarget to main; DCO is failing as well.
5. Comment rewrites
Roughly 40 existing comments across the scheduler, orchestrator, transfer adapter, runner mixin, and patch.py are rewritten into short [yurain] tags, deleting explanations that document invariants (the wire-schema asymmetry note in chunk_transfer_adapter, the prune-protection rationale in the runner mixin, the LogitsProcessor invariant, the issue #2274 reference in qwen3_code_predictor). Please revert all comment-only hunks; that will also shrink the diff substantially.
Happy to re-run the same sweep on the same hardware once the perf hunk is addressed.
| # A torch.Generator is a single stream. Using one generator for a | ||
| # multi-row batch would make explicitly-seeded requests depend on | ||
| # other rows in the same scheduler step, so keep that path scalar. | ||
| if decode_batch_size > 1: |
There was a problem hiding this comment.
[Blocking] This makes every multi-row decode batch run the code predictor one request at a time, where the previous gate only did that for explicitly seeded requests without per-row generator support. Measured on one H20 (single topology, default yaml, identical prompts): c=8 3.83 -> 2.13 req/s, c=16 4.49 -> 2.17, c=32 4.45 -> 2.15. Reverting only this hunk on the PR head restores c=16 to 4.53. This path is shared by all Qwen3-TTS deployments, not just PD. If PD decode needs the scalar path for cross-request determinism, gate it on the PD decode role (e.g. kv_transfer_config.kv_role == "kv_consumer") or keep the previous generator-based condition.
| ) | ||
|
|
||
|
|
||
| _patch_mooncake_connector_for_pd() |
There was a problem hiding this comment.
[High] This applies the Mooncake monkey patch at vllm_omni import time in every process whenever mooncake happens to be installed, including non-PD deployments and unrelated models, and it mutates upstream vllm classes globally (request_finished, _send_blocks, register_kv_caches, the bootstrap server). _init_pd_state already applies the patch when a PD topology is actually detected, which is the right scope. Suggest removing this import-time call.
| return ret_k if ret_k != 0 else ret_v | ||
| except Exception: | ||
| pass | ||
| return original_send_blocks(self_worker, remote_session, src_ptrs, dst_ptrs, lengths) |
There was a problem hiding this comment.
[High] The K/V-split pointer arithmetic assumes a contiguous [2, num_blocks, ...] layout, and any exception in the try block silently falls back to the unpatched single call. If the layout assumption is ever wrong (different attention backend, MLA, a future vllm layout change), this transfers the wrong half of the cache and surfaces as corrupted audio instead of an error. Suggest validating the layout explicitly and raising on mismatch, and documenting which vllm version this pointer math was written against.
|
|
||
| from vllm_omni.entrypoints.pd_utils import PDDisaggregationMixin | ||
|
|
||
| pytestmark = pytest.mark.skip(reason="Temporarily skip PD entrypoint tests while PD config is being removed.") |
There was a problem hiding this comment.
[High] The whole module is skipped, so pytest tests/test_pd_disaggregation.py (the PR's test plan) passes without executing a single assertion. Either unskip the tests that apply to this implementation or drop the file. The docstring also references issue #1188 and a "v1908" migration that do not exist in this repo.
| merged_info[key] = sub | ||
| else: | ||
| merged_info[key] = value | ||
| request.additional_information = merged_info |
There was a problem hiding this comment.
[Medium] Behavior change for every async_chunk AR stage, not just PD: chunks previously replaced additional_information wholesale, now they are dict-merged one level deep, so a sub-key that disappears from a later chunk keeps its stale value. If the merge only needs to preserve omni_source_stage_id, consider carrying that single key instead of a general merge, or add a test pinning the intended semantics.
| if len(non_pd_decode) == 1: | ||
| return non_pd_decode[0] | ||
| raise RuntimeError( | ||
| f"[Orchestrator] Ambiguous downstream stages for stage-{src_stage_id}: {candidates}" |
There was a problem hiding this comment.
[Medium] This now raises for a stage with more than one non-PD downstream, where the old code always routed to stage_id + 1. Any existing fan-out pipeline that worked with numeric adjacency will now crash at forward time. Consider falling back to stage_id + 1 with a warning, or auditing the shipped configs to confirm none are affected.
| if hasattr(sp, "min_tokens"): | ||
| try: | ||
| sp.min_tokens = 1 | ||
| sp.min_tokens = 0 |
There was a problem hiding this comment.
[Medium] The removed comment said MooncakeConnector cancels the KV transfer unless finish_reason is "length"; min_tokens 1 -> 0 makes it possible for the prefill request to finish with reason "stop" at max_tokens=1. If the transfer still fires in that case, please note why (and keep the comment); if not, this reintroduces the bug the old value guarded against.
| "${SITE_PKG}/mooncake_transfer_engine-0.3.8.post1.dist-info" 2>/dev/null || true | ||
| fi | ||
| # 验证 mooncake import 正常(如果还失败就提前退出,省得跑 60 秒等 worker 死) | ||
| if ! python3 -c "from mooncake.engine import TransferEngine" 2>/dev/null; then |
There was a problem hiding this comment.
[Medium] Three issues in this script:
- This preflight false-negatives on pip-CUDA stacks: bare
python3 -c "from mooncake.engine import TransferEngine"fails on libcudart even when the real server (torch imported first) boots fine. Usepython3 -c "import torch; from mooncake.engine import TransferEngine". - The site-packages
mvself-heal above (with a sudo fallback) is not something a shipped example should do; pin the dependency properly instead. - Line 37:
DEFAULT_VLLM_OMNI_DIR="$(find_repo_root)"runs underset -ebefore theVLLM_OMNI_DIRoverride is honored, so the script dies silently (no output at all) when placed outside a repo checkout. Also thear_3p1dcase referencesqwen3_tts_pd_ar_only_3p1d.yaml, which is not in the PR.
| | YAML | Topology | GPUs | Use | | ||
| |------|----------|------|-----| | ||
| | [`qwen3_tts_pd_1p1d.yaml`](../../../vllm_omni/deploy/qwen3_tts_pd_1p1d.yaml) | 1 prefill + 1 decode + 1 code2wav | 3 | Smallest configuration; matches the legacy 1P1D path exactly. Use this for the [probe script](#1-correctness--latency-probe-1p1d) and as the K=1 regression baseline. | | ||
| | [`qwen3_tts_pd_mp1d.yaml`](../../../vllm_omni/deploy/qwen3_tts_pd_mp1d.yaml) | 2 prefill + 1 decode + 1 code2wav | 4 | Production M-P-1-D recipe with `pd_prefill_pick_strategy: round_robin`. | |
There was a problem hiding this comment.
[Low] This table, section 2, and the probe example (line 79) reference qwen3_tts_pd_mp1d.yaml and .codebuddy/plan/.../migration_inventory.md, neither of which exists in the PR; the troubleshooting section also references monkey_patch.py (now mooncake_pd_patch.py). Please reconcile the docs with what ships, and consider translating the shipped scripts' comments to English to match the rest of the repo.
|
Follow-up on the "1p1d alone undersells PD, the value shows at 1pNd" point: we ran the shipped
Mean stream underrun is sub-second for both at every point (baseline 0.18-0.61 s, 1p6d 0.17-0.43 s), so continuity does not change the ranking. A few observations:
Two additional issues observed while running PD, both reproducible and worth investigating independently of the benchmark discussion:
Caveats for the record: en locale (zh prompt-wavs were not available on these nodes), 50 prompts per client, one repetition per cell, PD c=1 cells excluded from the figure due to the post-boot stall artifact, and two of eight baseline clients at C=64 reported anomalously low throughput (client-side skew suspected; the conservative sum is reported). If there is a regime where the PD split itself (not decode replication) wins, the cleanest way to show it would be: baseline and PD on the same commit with the perf regression fixed, same GPU count on both sides, aligned max_num_seqs / gpu_memory_utilization / max_num_batched_tokens, and goodput under a stated TTFP+continuity SLO as the headline metric. Happy to re-run that matrix on the same hardware once the branch is updated. |
|
@yurain26 here are the 4-GPU and 5-GPU replicated baselines you asked for, same protocol and node as the 8x run above (base branch 4x single replicas (4 GPUs):
5x single replicas (5 GPUs):
Per-replica numbers are essentially identical across N=4/5/8 (about 1.35 / 2.35 / 3.39 / 3.83 req/s at per-instance c=1/2/4/8), so replication scales linearly on this node and you can extrapolate other N by multiplying. For reference against your PD topologies on the same hardware class: 1p3d measured 4.4 to 4.6 req/s peak (4-GPU variant with code2wav co-located on the prefill GPU; your original 5-GPU layout should land close to that since the prefill GPU is mostly idle either way). So the current gaps are roughly 3x to 3.5x at matched GPU count. Happy to re-run any of these against your tuned branch when it is ready; the drivers are reusable as-is. |
5b69761 to
5182916
Compare
| logger.error(f"Error updating model intermediate buffer: {e}") | ||
| # Decode additional_information payloads (dictionary) | ||
| if model_buffer_applied is not None: | ||
| self._restore_qwen3_tts_pd_sampler_state(req_id, model_buffer_applied) |
There was a problem hiding this comment.
[P2] Same contract issue on the decode side: _restore_qwen3_tts_pd_sampler_state has five raise paths (missing req_state, invalid payload, bad tensor, missing generator for non-greedy, set_state failure) and both call sites here sit outside the surrounding try/except blocks, so a validation failure propagates out of _update_states → execute_model → EngineDeadError, killing every in-flight request on the replica. Please wrap per-request and mark just that request failed.
| "pp_rank": int(getattr(self_worker, "pp_rank", 0)), | ||
| "addr": addr, | ||
| } | ||
| resp = requests.post(f"http://{host}:{port}/register", json=payload, timeout=5) |
There was a problem hiding this comment.
[P2] This synchronous requests.post(timeout=5) runs inside patched request_finished on the scheduler thread — an unreachable/slow bootstrap server stalls scheduling for up to 5s per finished request, and total failure is swallowed by the except below, so the first sign of trouble is decode pulls timing out at the 60s watchdog. Consider a background registration task with retry instead of inline registration.
| pull_deadlines.pop(req_id, None) | ||
| blocks = pull_block_ids.pop(req_id, None) | ||
| if blocks: | ||
| error_block_ids.update(blocks) |
There was a problem hiding this comment.
[P2] On expiry the request is reported finished and its blocks marked invalid so the scheduler can recompute — but the in-flight Mooncake pull task is not cancelled and keeps running. When the scheduler recycles those block ids for the recompute, a late-completing transfer can still be writing into them. Does the pull get cancelled (or the blocks fenced until the transfer settles) before the recompute can reuse them?
| fetch_finished_recving_reqs._vllm_omni_wrapped = True # type: ignore[attr-defined] | ||
| worker_cls.fetch_finished_recving_reqs = fetch_finished_recving_reqs # type: ignore[assignment] | ||
|
|
||
| if not hasattr(worker_cls, "get_block_ids_with_load_errors"): |
There was a problem hiding this comment.
[P3] The hasattr guard means _vllm_omni_load_error_block_ids is only drained when the pinned vLLM does not already define get_block_ids_with_load_errors. On a pin where upstream defines it, this entire error path silently stops reporting load errors to the scheduler — worth an explicit version check (or an assertion) given the PR also needs to move from the v0.25.0 base to the repo's current pin.
hsliuustc0106
left a comment
There was a problem hiding this comment.
First pass from me on the core PD/MD paths (mooncake patch, scheduler KV-reclaim, orchestrator handoff, runner restore). The design is ambitious and mostly holds together — decode double-submit is idempotent, MTP replicas get private graph pools with correct stream ordering, KV-reclaim has a dedicated test, and the per-request handoff payload is one-shot. Findings inline: per-request raises on the orchestrator output-dispatch path and the runner restore path take down the whole process instead of one request (P2 ×2), inline bootstrap registration stalls the scheduler thread (P2), the pull watchdog leaves the in-flight transfer running while its blocks get recycled (P2), and the hasattr-guarded error drain silently disables on pins that already define get_block_ids_with_load_errors (P3).
Also from the thread + checks: buildkite is red with PR-caused failures (per @linyueqian's 08-30 analysis), DCO is failing, and the branch sits on a v0.25.0 base that no longer matches the repo pin — a rebase onto current main is needed before the next CI read means anything.
|
@yurain26 gentle ping — this high-priority PR has seen no updates for 22 days and review feedback is waiting. Could you share a status or ETA when you get a chance? Thanks! |
Omni ReviewBot: CI is red on this head@yurain26 required checks failed on Please fix the failure and push again; this note is updated in place when the head goes green or moves. |
|
@yurain26 gentle ping — this high-priority PR has seen no updates for 25 days and review feedback is waiting. Could you share a status or ETA when you get a chance? Thanks! |
|
@yurain26 gentle ping — this high-priority PR has seen no updates for 28 days and review feedback is waiting. Could you share a status or ETA when you get a chance? Thanks! |
|
@yurain26 gentle ping — this high-priority PR has seen no updates for 32 days and review feedback is waiting. Could you share a status or ETA when you get a chance? Thanks! |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
Omni ReviewBot attempt recordReview attempt ended as failed (owner server and child both dead — reconciled; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
5 similar comments
Omni ReviewBot attempt recordReview attempt ended as failed (owner server and child both dead — reconciled; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (owner server and child both dead — reconciled; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (owner server and child both dead — reconciled; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (owner server and child both dead — reconciled; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (owner server and child both dead — reconciled; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
|
@yurain26 gentle ping — this high-priority PR has seen no updates for 35 days and review feedback is waiting. Could you share a status or ETA when you get a chance? Thanks! |
|
@yurain26 this PR is labeled Could you please take a look and push an update to get CI green? Once the checks pass we can proceed with review/merge. Thanks! |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
8 actionable finding(s).
CI at
f3b8ca9120bb(2026-10-08T15:32:27.738964+00:00): required check(s) blocking:pre-commit(missing),buildkite/vllm-omni(failed). Observed Buildkite:buildkite/vllm-omni-amd-ci(failed),buildkite/vllm-omni(failed),buildkite/vllm-omni-intel-ci(passed), and 1 more.
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 6 finding(s) below |
| Security | no finding reported |
| Docs / comments | 1 finding(s) below |
| Behavior / compatibility | 5 finding(s) below |
| Correctness | no finding reported |
Validated:
- [resolved] Orchestrator waits for pd_submit_ready not kv_ready (orchestrator.py:2565-2574) — residual: per-request RuntimeError still kills the output task (already raised).
- [resolved] Prefill min_tokens: _prepare_prefill_sampling_params sets min_tokens=1 when source min_tokens>0 (pd_utils.py:386-392); 1p1d YAML min_tokens:2 is rewritten. Residual: 1p3d greedy prefill omits min_tokens so original_min_tokens=0 would clear stops if those YAMLs ever loaded.
- [resolved] consume_new_request_additional_information is exercised by test_postprocess_scheduler_output (requests['new-ready'].additional_information is None). Residual: no unit test on the def itself.
- [resolved] OmniARScheduler.update_from_output:567 calls _drain_deferred_frees. Residual: test_omni_ar_scheduler_pd_decode_kv_reclaim.py uses a harness, not OmniARScheduler.update_from_output.
- [resolved] Existing threads: global Mooncake patch/get_required_kvcache_layout None; orchestrator/runner PD raises taking down the replica; bootstrap requests.post on scheduler thread; pull timeout vs in-flight write; pytestmark skip on test_pd_disaggregation.py.
- [resolved] one-shot additional_information drop: consume_new only when meta.pd_resume_token_id present; generic-ready kept (test_chunk_transfer_adapter). Residual: no unit test on consume_new itself, covered via postprocess.
8 actionable finding(s).
Verdict: REQUEST CHANGES
Findings
- [P1] PD
generateonly rewrites the picked prefill slot and slot 0 to a one-token S… —vllm_omni/entrypoints/async_omni.py
Evidence for PD `generate` only rewrites the picked prefill slot and slot 0 to a one-token S…
PD generate only rewrites the picked prefill slot and slot 0 to a one-token SP (async_omni.py:615-619); test_generate_routes_pd_request_to_selected_prefill_stage_and_slot_zero asserts the decode slot stays params[2]. Unchanged /v1/audio/speech copies the full YAML per-stage list (serving_speech.py:2982) and applies max_new_tokens, extra_params temperature/top_p/top_k, and request.seed only to sampling_params_list[0] (tts_adapters/base.py:68, serving_speech.py:3054-3072). _maybe_expand_sampling_params does not copy those onto D when the list is already length N. Orchestrator then builds D from sampling_params_list[decode_id] and does max_tokens-1 (orchestrator.py:2808,2658), so a speech request with max_new_tokens=8 prefills 1 token and decodes up to YAML 4095 (1p3d still temperature 0.9) instead of remaining 7. Before _prepare_prefill_sampling_params, copy the user talker max_tokens/min_tokens/temperature/top_p/top_k/seed onto every decode id (same in omni.py); take those fields from slot 0 when that is where speech serving wrote them. Add a CPU test that max_new_tokens=8 becomes D max_tokens=7 after _build_pd_decode_params.
Evidence: Trigger: /v1/audio/speech on this PR's Qwen3-TTS PD deploy with max_new_tokens=8 (or extra_params temperature/top_p/top_k). Adverse effect: P is forced to one token; D generates from YAML decode defaults (max_tokens: 4096 → 4095 after the P-token subtract; 1p3d temperature: 0.9) instead of remaining 7 / the user knobs. Proof: async_omni.py:615-619 prefill_sp = self._prepare_prefill_sampling_params(request_id, req_sp_list[p_id]) / req_sp_list[p_id] = prefill_sp / req_sp_list[0] = prefill_sp — D is not rewritten. tests/entrypoints/test_async_omni.py:214 assert submitted_params[0][2] is params[2] — the new test locks that in. Unchanged by this diff, present in the PR-time tree: vllm_omni/entrypoints/openai/serving_speech.py:2982 sampling_params_list = list(self.engine_client.default_sampling_params_list); vllm_omni/entrypoints/openai/tts_adapters/base.py:68 sampling_params_list[0].max_tokens = request.max_new_tokens; serving_speech.py:3054-3056 for name in ("temperature", "top_p", "top_k"): / setattr(sampling_params_list[0], name, value). vllm_omni/entrypoints/pd_utils.py:413-414 if len(sampling_params_list) != len(self.stage_configs) - 1: return sampling_params_list — full N-length YAML list skips expand-copy onto D. vllm_omni/engine/orchestrator.py:2808 params = req_state.sampling_params_list[next_logical] then 2658 remaining_max_tokens = int(sp.max_tokens) - 1. This diff's vllm_omni/deploy/qwen3_tts_pd_1p3d.yaml:118-121 temperature: 0.9 / max_tokens: 4096 on the decode stage.
Suggestion: p_id = self._pick_prefill_stage(request_id)
talker_sp = req_sp_list[p_id]
for d_id in self._get_pd_decode_ids():
if d_id < len(req_sp_list):
decode_sp = req_sp_list[d_id].clone()
decode_sp.max_tokens = talker_sp.max_tokens
decode_sp.min_tokens = getattr(talker_sp, "min_tokens", getattr(decode_sp, "min_tokens", 0))
decode_sp.temperature = talker_sp.temperature
decode_sp.top_p = talker_sp.top_p
decode_sp.top_k = talker_sp.top_k
decode_sp.seed = talker_sp.seed
req_sp_list[d_id] = decode_sp
prefill_sp = self._prepare_prefill_sampling_params(request_id, talker_sp)
req_sp_list[p_id] = prefill_sp
req_sp_list[0] = prefill_sp
Evidence for `_build_pd_decode_params` takes `prefill_stage_id` from the P→D submit (`orches…
_build_pd_decode_params takes prefill_stage_id from the P→D submit (orchestrator.py:2993) but never uses it; it always stamps _pd_bootstrap_addr / _pd_prefill_engine_id (P0-only from async_omni_engine.py:1030/1081) while the per-prefill maps sit unused. After 3P1D/2P2D load, a request that ran on P1/P2 still tells decode to query P0's bootstrap/engine_id. Index the maps with prefill_stage_id and fall back to the scalar only on miss.
Evidence: Trigger: this PR ships qwen3_tts_pd_3p1d.yaml with distinct producer bootstraps (vllm_omni/deploy/qwen3_tts_pd_3p1d.yaml:18 VLLM_MOONCAKE_BOOTSTRAP_PORT: 25201 vs :72 VLLM_MOONCAKE_BOOTSTRAP_PORT: 25202) and async_omni.py:613-614 if self._get_pd_decode_id() is not None and self._get_pd_prefill_ids(): / p_id = self._pick_prefill_stage(request_id) pins a request to P1/P2. At P→D submit orchestrator.py:2990-2993 params = self._build_pd_decode_params( / prefill_stage_id=src_stage_id, but the parameter is unused: orchestrator.py:2648 prefill_stage_id: int | None = None,. The body always stamps the P0 scalars: orchestrator.py:2685-2689 if self._pd_bootstrap_addr: / decode_kv_params["remote_bootstrap_addr"] = self._pd_bootstrap_addr / if self._pd_prefill_engine_id: / decode_kv_params["remote_engine_id"] = self._pd_prefill_engine_id. Those scalars are P0-only: async_omni_engine.py:1030 bootstrap_addr = bootstrap_addr_by_prefill.get(prefill_ids[0]) if prefill_ids else None and :1081 "prefill_engine_id": prefill_engine_id_by_prefill.get(prefill_ids[0]) if prefill_ids else None,. The maps are stored (orchestrator.py:569-571 self._pd_bootstrap_addr_by_prefill = dict(pd_config.get("bootstrap_addr_by_prefill") or {}) / self._pd_prefill_engine_id_by_prefill = dict(pd_config.get("prefill_engine_id_by_prefill") or {})) and never read. Overlay cannot repair bootstrap: producer fill only sets host/port (mooncake_pd_patch.py:164-167 kv_params.setdefault("remote_host", host) / kv_params.setdefault("remote_port", port)). Adverse effect: a request whose KV lives on P1 still tells decode to query P0's registry (http://127.0.0.1:25201 / P0 remote_engine_id) instead of P1's 25202.
Suggestion: bootstrap_addr = (
self._pd_bootstrap_addr_by_prefill.get(prefill_stage_id)
if prefill_stage_id is not None
else None
) or self._pd_bootstrap_addr
if bootstrap_addr:
decode_kv_params["remote_bootstrap_addr"] = bootstrap_addr
engine_id = (
self._pd_prefill_engine_id_by_prefill.get(prefill_stage_id)
if prefill_stage_id is not None
else None
) or self._pd_prefill_engine_id
if engine_id:
decode_kv_params["remote_engine_id"] = engine_id
Evidence for GPUARModelRunner.execute_model now freezes `export_pd_decode_state` at the KV s…
GPUARModelRunner.execute_model now freezes export_pd_decode_state at the KV snapshot (gpu_ar_model_runner.py:934-947) and attaches it on the empty/no-forward ACK via _attach_pd_states_to_kv_ack_output (:1041). Sibling NPUARModelRunner.execute_model (unchanged) still only copies kv_extracted_req_ids onto that output (npu_ar_model_runner.py:527-529) and never snapshots hidden_states.last / trailing_text. qwen3_tts_pd is registered globally with no CUDA gate, so the same PD YAML on NPU ACKs KV without that payload and orchestrator.py:2618-2623 raises Missing Qwen3-TTS decode state. Port the freeze+attach helpers (or share them on a common base), or explicitly gate the pipeline as CUDA-only.
Evidence: Trigger: serve this PR’s qwen3_tts_pd pipeline (e.g. vllm_omni/deploy/qwen3_tts_pd_1p1d.yaml, registered in pipeline_registry.py as "qwen3_tts_pd": QWEN3_TTS_PD_PIPELINE) on NPU. Adverse effect: prefill KV ACK has no hidden_states.last/trailing_text, so decode handoff crashes. This diff: vllm_omni/worker/gpu_ar_model_runner.py:934 export_pd_decode_state = callable(getattr(self.model, "export_pd_decode_state", None)); :935 if finished_reqs and export_pd_decode_state:; :946 self.model.export_pd_decode_state(self.model_intermediate_buffer.get(req_id, {})),; :1041 output = self._attach_pd_states_to_kv_ack_output(output, kv_ids). Unchanged by this diff, present in the PR-time tree: vllm_omni/platforms/npu/worker/npu_ar_model_runner.py:414 finished_reqs = getattr(scheduler_output, "finished_requests_needing_kv_transfer", {}); :415 if finished_reqs and hasattr(self.model, "get_kv_transfer_metadata"): (no export_pd_decode_state); :527-529 if kv_ids: / output = copy(output) / output.kv_extracted_req_ids = kv_ids. Also unchanged consumer: vllm_omni/engine/orchestrator.py:2618 if pd_decode_state is None: then :2621-2622 raise RuntimeError( / f"[Orchestrator][PD] Missing Qwen3-TTS decode state for req={req_id}; ".
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
|
|
||
| async_chunk: true | ||
|
|
||
| stage_args: |
There was a problem hiding this comment.
[P0] The new vllm_omni/deploy/qwen3_tts_pd_{1p3d,1p6d,2p2d,3p1d}.yaml files still…
Evidence and suggested fix
The new vllm_omni/deploy/qwen3_tts_pd_{1p3d,1p6d,2p2d,3p1d}.yaml files still declare topology under stage_args: (e.g. qwen3_tts_pd_1p3d.yaml:6); only qwen3_tts_pd_1p1d.yaml uses pipeline: qwen3_tts_pd + stages:. Unchanged load_deploy_config raises ValueError on that removed schema (stage_config.py:700; test_rejects_legacy_stage_args_schema), and vllm serve --deploy-config loads through that helper, so start_pd.sh 1p3d / default run_all_bench.sh TOPOS single 1p1d 3p1d 1p3d 1p6d cannot start those topologies. Renaming the key is not enough: QWEN3_TTS_PD_PIPELINE is a frozen 3-stage 1P1D graph and merge_pipeline_deploy iterates only pipeline.stages, so a 5-stage overlay would bind deploy stage 2 (decode) onto pipeline stage 2 (code2wav) and drop extra decode/code2wav stages. start_pd.sh also maps ar_3p1d to missing qwen3_tts_pd_ar_only_3p1d.yaml. Convert each topology to pipeline: + stages: with a matching PipelineConfig (or drop them from the scripts), add a load_deploy_config test for every shipped YAML, and remove 1p3d decode seed: 42 when converting.
Evidence: Trigger: start_pd.sh 1p3d (also 1p6d/2p2d/3p1d) and default run_all_bench.sh TOPOS include those tags; both pass --deploy-config at the YAML that starts with stage_args:. Adverse effect: serve never starts — loader raises before any worker comes up; ar_3p1d exits earlier on a missing file. qwen3_tts_pd_1p3d.yaml:6 stage_args: (same key at qwen3_tts_pd_1p6d.yaml:6, qwen3_tts_pd_2p2d.yaml:12, qwen3_tts_pd_3p1d.yaml:8; only qwen3_tts_pd_1p1d.yaml:31–33 uses pipeline: qwen3_tts_pd / stages:). Unchanged by this diff, present in the PR-time tree: stage_config.py:700–703 if "stage_args" in raw_dict: raise ValueError(f"Deploy config {path} uses the removed stage_args schema; ..."); tests/config/test_config_factory.py:1133–1138 test_rejects_legacy_stage_args_schema asserts that error; config_factory.py:352 return load_deploy_config(deploy_path) and omni_config.py:357 return load_deploy_config(resolved_path), str(resolved_path) are the --deploy-config consumers. start_pd.sh:57–62 maps 1p3d/1p6d/3p1d/2p2d/ar_3p1d onto those YAMLs (ar_3p1d → qwen3_tts_pd_ar_only_3p1d.yaml, which is not in the tree); start_pd.sh:179–182 vllm serve ... --deploy-config "$YAML"; run_all_bench.sh:49 TOPOS="${TOPOS:-single 1p1d 3p1d 1p3d 1p6d}". Naive rename still broken: pipeline.py:62–99 QWEN3_TTS_PD_PIPELINE is three stages (0 talker, 1 talker, 2 code2wav); stage_config.py:988 for ps in pipeline.stages: then ds = deploy_by_id.get(ps.stage_id) so a 5-stage 1p3d overlay (stages 1–3 decode, 4 code2wav) would bind deploy stage 2 onto pipeline code2wav and drop extras. qwen3_tts_pd_1p3d.yaml:122 seed: 42 is unique among the new PD YAMLs.
| done | ||
|
|
||
| # 3) 兜底:按 cmdline 杀残留的 vllm / stage / spawn worker | ||
| pkill -9 -f "vllm serve" 2>/dev/null || true |
There was a problem hiding this comment.
[P1] stop_pd.sh already kills pid-file and API-port trees, then host-wide `pkill -9…
Evidence and suggested fix
stop_pd.sh already kills pid-file and API-port trees, then host-wide pkill -9 -f of vllm serve, StageEngineCoreProc, and from multiprocessing.spawn (stop_pd.sh:62–64) and deletes same-user /dev/shm/{vllm*,mooncake*} plus /tmp/*.ipc (73–77). start_pd.sh:131 and default run_all_bench.sh (TOPOS includes 1p6d) invoke this on every start/topology switch, so unrelated vLLM/Omni jobs and any Python multiprocessing spawn child get SIGKILL. BOOTSTRAP_PORTS is only 25201–25206 (line 22) while 1p6d stage 6 sets VLLM_MOONCAKE_BOOTSTRAP_PORT: 25207 (qwen3_tts_pd_1p6d.yaml:407). Drop the three cmdline pkill fallbacks, scope shm/tmp cleanup to this run, and include 25207.
Evidence: Trigger: start_pd.sh:131 bash "$SCRIPT_DIR/stop_pd.sh" "$PORT" || true and run_all_bench.sh:49 TOPOS="${TOPOS:-single 1p1d 3p1d 1p3d 1p6d}" then run_all_bench.sh:204 bash "$SCRIPT_DIR/stop_pd.sh" "$PORT". Adverse effect 1 — host-wide SIGKILL of unrelated jobs: stop_pd.sh:62-64 pkill -9 -f "vllm serve" 2>/dev/null || true / pkill -9 -f "StageEngineCoreProc" 2>/dev/null || true / pkill -9 -f "from multiprocessing.spawn" 2>/dev/null || true (no pid/pgid/cwd filter; after pid-file and API-port tree kills at 45–59). Same-user wipe: stop_pd.sh:73-77 find /dev/shm ... -name 'vllm*' -o -name 'mooncake*' ... -user "$(whoami)" -delete and find /tmp -maxdepth 2 -name '*.ipc' ... -delete. Adverse effect 2 — 1p6d Mooncake port left bound: stop_pd.sh:22 BOOTSTRAP_PORTS=(25201 25202 25203 25204 25205 25206) while this PR’s last 1p6d decode is vllm_omni/deploy/qwen3_tts_pd_1p6d.yaml:407 VLLM_MOONCAKE_BOOTSTRAP_PORT: 25207 and :437 mooncake_bootstrap_port: 25207; fuser/wait never touch 25207, so the next 1p6d start can hit Address already in use.
| if remote_num_blocks <= 0 or not remote_block_lens: | ||
| remote_num_blocks = local_num_blocks | ||
| remote_block_lens = local_block_lens | ||
| layout_source = "local_fallback" |
There was a problem hiding this comment.
[P1] The new Mooncake _send_blocks wrapper (mooncake_pd_patch.py:551-554) on a m…
Evidence and suggested fix
The new Mooncake _send_blocks wrapper (mooncake_pd_patch.py:551-554) on a missing remote layout copies the producer’s num_blocks/block_len_per_layer as local_fallback and still split-sends V at (remote_num_blocks//2)*block_len (:565-587). Shipped 1p1d sets prefill gpu_memory_utilization: 0.5 and decode 0.6 (qwen3_tts_pd_1p1d.yaml:57,114; same mismatch on 1p3d/1p6d/2p2d), so P and D KV pools are not the same size. _export_consumer_layout can no-op/swallow errors and _cache_remote_layout only fills from meta/shm, so a miss (or first send before a successful consumer export) writes V into the wrong consumer region and decode continues on corrupted KV. Fail the send or skip the split when remote layout is missing; do not assume local==remote.
Evidence: Trigger: a Mooncake PD _send_blocks whose remote layout maps are empty (meta.num_blocks/block_lens absent and shm miss, or _export_consumer_layout no-op), on shipped 1p1d where P/D pools are sized differently. Adverse effect: V is sent at the producer’s half-pool offset into the consumer, so decode reads corrupted KV.
vllm_omni/distributed/kv_transfer/mooncake_pd_patch.py:551-554 if remote_num_blocks <= 0 or not remote_block_lens: / remote_num_blocks = local_num_blocks / remote_block_lens = local_block_lens / layout_source = "local_fallback"
vllm_omni/distributed/kv_transfer/mooncake_pd_patch.py:565-566 src_offset = (local_num_blocks // 2) * int(local_block_lens[0]) / dst_offset = (remote_num_blocks // 2) * int(remote_block_lens[0]) — after fallback this is the producer half-pool, then 581-587 still issues the V split send.
vllm_omni/deploy/qwen3_tts_pd_1p1d.yaml:57 gpu_memory_utilization: 0.5 (prefill/kv_producer) and :114 gpu_memory_utilization: 0.6 (decode/kv_consumer).
Suggestion: if remote_num_blocks <= 0 or not remote_block_lens:
raise RuntimeError(
f"Mooncake PD send lacks remote KV layout for session={session_key}; "
"refusing local_fallback because producer/consumer pool sizes may differ"
)
| HOST="${HOST:-127.0.0.1}" | ||
| PORT="${PORT:-8091}" | ||
| DATASET="${DATASET:-/root/datasets/seed-tts-eval}" | ||
| TOPOS="${TOPOS:-single 1p1d 3p1d 1p3d 1p6d}" |
There was a problem hiding this comment.
[P2] The PR body pins vLLM v0.25.0 and publishes §5 throughput deltas (+13.5% / +19%…
Evidence and suggested fix
The PR body pins vLLM v0.25.0 and publishes §5 throughput deltas (+13.5% / +19%) with no in-diff result JSON, while this head's PD YAMLs comment vLLM-Omni 0.26. Default run_all_bench.sh TOPOS still includes 3p1d/1p3d/1p6d, whose YAMLs use the removed stage_args schema and cannot boot through load_deploy_config. Unclaim the 0.25.0 pin and those figures until they match this head (in-diff JSON on a loader-valid topology, or an actual 0.25 pin).
Evidence: Trigger: PR #4934 body (not in the diff) still says **vLLM** v0.25.0 **commit** \8ccd02f`and@ c=32: throughput +13.5%, RTF −14%; @ c=64: throughput +19%, so a reader treats those as this-head results. Unmet requirement: no result JSON is in the diff, and this head cannot even start the default extra topologies. examples/online_serving/qwen3_tts_pd/benchmark/run_all_bench.sh:49 TOPOS="${TOPOS:-single 1p1d 3p1d 1p3d 1p6d}". vllm_omni/deploy/qwen3_tts_pd_1p1d.yaml:78 # vLLM-Omni 0.26 reads stage_config.final_output_type directly. vllm_omni/deploy/qwen3_tts_pd_1p3d.yaml:6 stage_args:(same key at qwen3_tts_pd_1p6d.yaml:6 and qwen3_tts_pd_3p1d.yaml:8). Unchanged by this diff, present in the PR-time tree: vllm_omni/config/stage_config.py:700-703if "stage_args" in raw_dict: raise ValueError(f"Deploy config {path} uses the removed `stage_args` schema; " "define topology in PipelineConfig and deployment overrides under `stages`.")—vllm serve --deploy-config` therefore cannot boot 3p1d/1p3d/1p6d on this head, so those default-TOPOS numbers cannot be this-head evidence.
| # max_tokens ("停不下来"). Collapsing to a single code_predictor makes decode | ||
| # MTP behave exactly like the single-node baseline. Uses ${VAR:-default} so a | ||
| # command-line export can still opt back in (e.g. NUM_REPLICAS=2). | ||
| export VLLM_OMNI_MTP_NUM_REPLICAS="${VLLM_OMNI_MTP_NUM_REPLICAS:-1}" |
There was a problem hiding this comment.
[P2] GitHub title is PD-only, but the PR body H1 and the MD disaggregation bulle…
Evidence and suggested fix
GitHub title is PD-only, but the PR body H1 and the MD disaggregation bullet present MTP replicas (e.g. 2× MTP : 1× AR, private CUDA-graph pools, crc32(req_id) pinning) as a shipped capability. On this head the PD launcher and runner both default VLLM_OMNI_MTP_NUM_REPLICAS to 1; start_pd.sh:161-168 disable replica>1 because extra replica state was implicated in never sampling codec-EOS (stop_token_ids=[2150]) and running to max_tokens; tests/ has no replica>1 / crc32-pinning coverage. Unclaim MD from the description until replica>1 is a documented opt-in with a test, or stop presenting 2× MTP as the shipped design.
Evidence: Trigger: a reader of PR #4934 body (not in the diff) is told MD is part of this change: **MD disaggregation**: inside the decode worker, the code predictor (MTP) is decoupled from the AR talker and replicated (e.g. 2× MTP : 1× AR). Each replica owns an independent CUDA stream and a private CUDA-graph pool; requests are pinned to the same MTP replica across decode steps via crc32(req_id) % num_replicas. Unmet requirement: that advertised replica>1 path is not what this head runs, and the launch script already records an adverse effect for enabling it. examples/online_serving/qwen3_tts_pd/benchmark/start_pd.sh:161-168 # MTP multi-replica sub-batch path is DISABLED by default (NUM_REPLICAS=1). / # The >1 path pins each request to a deepcopied code_predictor replica by / # crc32(req_id); under PD the extra replica state was implicated in requests / # that never sample the codec-EOS (stop_token_ids=[2150]) and run to / # max_tokens ("停不下来"). / export VLLM_OMNI_MTP_NUM_REPLICAS="${VLLM_OMNI_MTP_NUM_REPLICAS:-1}". vllm_omni/worker/gpu_model_runner.py:316 num_replicas = int(os.getenv("VLLM_OMNI_MTP_NUM_REPLICAS", "1") or "1") then :320-321 if num_replicas == 1: return — replica init never runs on the default. Repo-wide grep of tests/ for VLLM_OMNI_MTP_NUM_REPLICAS, _init_talker_mtp_replicas, and _mtp_replica_idx_for_req returned no test matches.
Omni ReviewBot: finding feedback[p1] PD See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: finding feedback[p1] The new Mooncake See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: finding feedback**[p1] stop_pd.sh already kills pid-file and API-port trees, then host-wide See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: finding feedback**[p1] See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: finding feedback[p0] The new See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
|
@yurain26 gentle ping — this high-priority PR has seen no updates for 35 days and review feedback is waiting. Could you share a status or ETA when you get a chance? Thanks! |
Omni ReviewBot: no human activity for 7 days@yurain26 this pull request has had no human commit, comment or review since 2026-10-09. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
|
@yurain26 TODO for current head Please resolve the current merge conflicts and give a fix plan for the existing Engine/Entrypoints failure report. The current head still has a failed CUDA status and DCO action required. Report a new commit plus local commands/environment/results for the routing, PD bookkeeping and structured engine-argument cases identified in the discussion. Maintainers can then validate current-head CI and decide when the matched throughput comparison can resume. Please push an update or reply with a plan/ETA by 2026-10-19 12:50 Asia/Shanghai (UTC+08:00). A reply, explained delay or hardware limitation is sufficient to keep this active; an unanswered author-owned blocker after that deadline may lead to closure after a fresh check. Waiting on maintainer review or CI does not count as author inactivity. Scope: readiness triage and existing-feedback follow-through; no new substantive code review or executed tests. Maintainers own project CI admission. |

Qwen3-TTS Prefill–Decode (PD) Disaggregation + Multi-token-prediction (MD) Disaggregation
1. Description
crc32(req_id) % num_replicas.vllm_omni/distributed/kv_transfer/mooncake_pd_patch.py; deploy configs 1P1D / 1P3D / 1P6D / 3P1D; example scripts and probe tools; orchestrator / scheduler / engine entrypoint changes; E2E testtests/test_pd_disaggregation.py.vLLM v0.25.0 commit
8ccd02f852. Dataset
seed-tts-eval/en(English short-sentence TTS), 100 prompts per concurrency level (seed=0, reproducible).3. Data sampling (deterministic 100 prompts)
The benchmark loader uses
random.Random(0).shuffle, soNUM_PROMPTS=100always selects the same 100 rows. You can pre-extract and persist them:Then use
DATASET=/root/datasets/seed-tts-100(only a symlink + a 100-rowmeta.lst; no re-download needed).4. Parameters (Single vs 1P1D, both 2 GPUs)
5. Performance (
seed-tts-evalen, 100 prompts/concurrency)6. Run flow
seed-tts-evalper §2 (git-lfs).seed-tts-100; or skip — withDATASET=/root/datasets/seed-tts-eval+NUM_PROMPTS=100the loader uses the samerandom.Random(0)seed and selects the same 100 rows, so results are identical.run_all_bench.shiterates over each topology (single / 1p1d / 3p1d / 1p3d / 1p6d): ①start_pd.shbrings up the service and waits for/health→ ② runsvllm bench serve --omni(openai-audio-speech backend, seed-tts dataset) perCONCURRENCIESlevel, saving the raw log /summary.txt/ JSON / one self-describing check wav per level → ③stop_pd.shtears down the service and frees GPUs; finally it aggregates all topologies into a comparison tableSUMMARY_ALL.txt(read this first).Env vars
MODEL / HOST / PORT / DATASET / VLLM_OMNI_DIR / RESULT_ROOTare all overridable;start_pd.shauto-detects the vllm-omni repo root.7. Test Plan
pytest tests/test_pd_disaggregation.py; manually deploy 1P1D viaqwen3_tts_pd_1p1d.yamland probe withprobe_1p1d.py. E2E tests pass;probe_1p1d.pyshows theaudio_codesstream is bit-identical to the single-process baseline, with PD median TTFB within 3× of the baseline.Base:
main