Repository navigation
[Core] Performance fix for orchestrator bottleneck: separates inter-stage from client outputs - #4527
Conversation
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
Resolve orchestrator shutdown conflict: keep main's single teardown path in run() finally; monitor flush remains there. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 314f3f269a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { | ||
| "model_outputs", | ||
| "sr", | ||
| "audio", |
There was a problem hiding this comment.
Keep non-final audio outputs on the connector path
In the Voxtral TTS async pipeline, stage 0 (audio_generation) is non-final and its generator2tokenizer_async_chunk processor reads multimodal_output["audio"] to send acoustic-code chunks to stage 1. Classifying every top-level audio payload as client-facing makes partition_payload_list return no inter_stage_outputs for that stage; the scheduler now calls save_async only for inter_stage_output, so those chunks (and the finish marker path driven by them) never reach audio_tokenizer. This split needs to be stage/final-output aware, or avoid treating inter-stage audio keys as client-only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid catch for Voxtral: stage-0 "audio" is inter-stage, but partition treats it as client mm.
I’d rather not fix this with root-key heuristics in mm_outputs.py. Proposal: each stage declares at model output time what is inter-stage vs client-facing (e.g. codes.* / hidden_states.* vs final model_outputs/audio), and the runner just forwards the two channels.
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
hsliuustc0106
left a comment
There was a problem hiding this comment.
Performance fix for orchestrator bottleneck: separates inter-stage from client outputs.
hsliuustc0106
left a comment
There was a problem hiding this comment.
Thanks for the performance work here. I found a couple of blocking regressions in the new inter-stage/client split, plus one monitor coverage issue.
Summary:
save_asyncstill needs to run for terminal async-chunk markers even when the inter-stage payload isNone.- The new
inter_stage_outputsscheduler contract needs NPU producer parity or a compatibility fallback. - The orchestrator monitor should include replicas that register after startup.
| except (AttributeError, NotImplementedError, RuntimeError, ValueError): | ||
| return 0 | ||
|
|
||
| def replica_inflight_count(self, replica_id: int) -> int: |
There was a problem hiding this comment.
replica_inflight_count has no try/except, unlike its sibling replica_outputs_queue_size above. It's reached from _sample_replica_metrics -> _roll_window -> note_loop, which runs bare on the orchestration loop body (end of _orchestration_loop in orchestrator.py) -- outside the inner try/except. Since the monitor is meant to be safe to enable in production ("diagnostic only / no runtime overhead when disabled"), any future change to ReplicaInfo/StagePool shape would let --enable-orch-monitor kill the orchestration task. Worth wrapping the sampler call (or matching the sibling's defensive pattern) so observability can't take down the orchestrator.
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com>
|
check ci failures https://buildkite.com/vllm/vllm-omni/builds/11855/canvas |
|
seems the ci do not triggered again, could you please re-add the tag? @hsliuustc0106 |
… split PR #4527 ("separates inter-stage from client outputs") broke the full-payload inter-stage handoff, starving the downstream stage of every multi-stage TTS/Omni pipeline: the consumer stage waited on connector input that never arrived, tripping the VLLM_OMNI_INPUT_WAIT_TIMEOUT_S safety net (FINISHED_ERROR) and yielding empty audio (transcript='') or request timeouts (Qwen3-TTS Base/CustomVoice exit 124, IndexTTS2, Qwen3-Omni, ...). Root cause (no-async-chunk / full-payload mode) -- VERIFIED: In gpu_ar_model_runner / gpu_generation_model_runner, #4527 changed the send-side accumulation to read from `pooler_inter` / `inter_stage_outputs`, the partition result that is None when async_chunk is False. But `should_accumulate_full_payload_output()` returns False when async_chunk is True. So the two conditions are mutually exclusive -- the accumulate block never ran, and the producer stage sent nothing. Fix: accumulate the full per-request payload (`pooler_output` / `per_req_payloads`), which is exactly the pre-#4527 behavior for this branch. Sibling fix (async-chunk mode): make `partition_flat_payload` non-lossy so the inter-stage fanout keeps every key (a downstream stage can't starve on a value that shares a root with a client-facing output, e.g. model_outputs); the client payload stays trimmed to `_CLIENT_MM_ROOT_KEYS` (the #4527 perf goal). Updates test_mm_outputs_partition.py to the non-lossy contract + regression test. Verification (H200, Qwen3-TTS-12Hz-0.6B-Base, offline no_async_chunk): - buggy code: stage-1 "timed out waiting for chunk/input ... FINISHED_ERROR" (the exact CI signature, reproduced with a shortened timeout) - with this fix: test_text_to_audio_001 PASSED, real audio produced (184s) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tzhouam <tzhouam@connect.ust.hk>
Integrate upstream main 0899a1a (11 commits since 1b318d1) — orchestrator inter-stage/client output split (vllm-project#4527), diffusion request-level batching (vllm-project#4079), speech SSE default (vllm-project#4679), and assorted model/example migrations. Auto-merged with no conflicts. Signed-off-by: chickeyton <ngton2014@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tage from client outputs (vllm-project#4527) Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com> Co-authored-by: Chenguang ZHENG <645327136@qq.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: Gao Han <hgaoaf@connect.ust.hk> Signed-off-by: Wojciech Kutak <wkutak@nvidia.com>
…tage from client outputs (vllm-project#4527) Signed-off-by: yinpeiqi <yinpeiqi809@gmail.com> Co-authored-by: Chenguang ZHENG <645327136@qq.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: Gao Han <hgaoaf@connect.ust.hk>
PLEASE FILL IN THE PR DESCRIPTION HERE.
Purpose
Fixes #4469.
This PR does two things:
Separate inter-stage payloads from client-facing multimodal outputs
inter_stage_outputs: keys intended for stage-to-stage connector transportmultimodal_outputs: client-facing keys onlypartition_flat_payload().OmniARSchedulerforwards client-facing data throughOmniEngineCoreOutput.multimodal_output, whilechunk_transfer_adapter.save_async()now usesinter_stage_outputonly (no fallback to client-facingmm_output).Add an optional Orchestrator load monitor
--enable-orch-monitor./vllm_omni_orch_monitor_<MMDDHHMM>.jsonVLLM_OMNI_ORCH_MONITOR_PATHduration_s,loop_idle,loop_activeoutputs_queue_size,inflightPerformance
Benchmark setup:
Before this fix
TTFT stays very high across the sweep. This matches the orchestrator bottleneck described in #4469: client-facing multimodal payloads were also flowing through the connector path, increasing orchestrator poll and forwarding overhead.
In this setup, the system is very sensitive to backlog. Once the orchestrator or stage queues start piling up, latency spikes quickly and the run becomes unstable — there is little headroom to recover. Results before this fix were also hard to reproduce: repeated runs at the same settings often diverged sharply, so the chart above should be read as directional rather than a single stable baseline (some run-to-run variance is expected).
The sweep shown here uses 200 requests. When we extend the same workload to 1000 requests, the old path almost always breaks down under accumulated backlog. We did not include those longer runs here because they frequently fail or produce unusable latency profiles.
After this fix
TTFT drops significantly, especially at higher load. Separating inter-stage and client-facing outputs removes unnecessary connector traffic and reduces orchestrator work per poll. Behavior is much more stable across repeated runs, and the same workload scales more predictably when request count increases.
Orchestrator monitor validation
With
--enable-orch-monitor, post-run JSON and shutdown summary show the orchestrator is no longer the limiting factor:Two follow-up runs under load: poll-loop activity stays healthy and the orchestrator does not become saturated again.
Per-replica
outputs_queue_sizeremains low throughout the run. Metrics above were collected from the orchestrator monitor JSON/log output.Test Plan
vLLM Version:
0.23.0vLLM-Omni Commit:
a693ae67f6eec6df1d443581857a9c016850a0a2Test Result
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)