Repository navigation
[Refactor] Output Processor Phase 2: separate multimodal output channel (#1601) - #2744
Conversation
|
PR #2744 - separate multimodal output channel (#1601) OVERALL: 2 BLOCKERS FOUND BLOCKER scan:
BLOCKING ISSUES:
Fix DCO and pre-commit gate failures, fill description, and provide test results. |
|
BLOCKER scan: This PR has multiple issues:
Please resolve all these issues before requesting review:
OVERALL: MULTIPLE ISSUES VERDICT: REQUEST_CHANGES |
|
Great, nice to see the phase 2 get started. If need any help, please inform me on Slack : ) |
604352d to
f0e9eb8
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
c2323c6 to
aa7349a
Compare
|
@tzhouam pls take a look. |
391450d to
4844050
Compare
tzhouam
left a comment
There was a problem hiding this comment.
please have a look.
added a review from gpt.
Address two reviewer concerns on PR vllm-project#2744: 1. Tensor-only wire boundary (Concern 1): - Add _ensure_tensor_values() in gpu_ar_model_runner that wraps scalars/lists into torch.Tensor and drops non-tensorizable values. - Applied in both AR and generation model runners after flatten_payload(). - Add msgpack roundtrip test confirming tensor-only payloads survive encode/decode and non-tensor values cause decode failure. - Add unit tests for _ensure_tensor_values boundary function. 2. No-detokenizer path regression (Concern 2): - Override process_outputs() in MultimodalOutputProcessor to route outputs from no-detokenizer stages (talker, code2wav) through a local handler instead of upstream which asserts detokenizer != None. - Override _new_completion_output() to build CompletionOutput directly when detokenizer is None, avoiding upstream assert. - Override _new_request_output() to build RequestOutput directly when logprobs_processor is None, avoiding upstream assert. - Guard stream_interval > 1 block to skip when detokenizer is None. - Add regression tests for detokenizer=None + multimodal_output, including stream_interval > 1 and FINAL_ONLY output kinds. Signed-off-by: Megha Agarwal <agarwalmegha1308@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… fix query_start_loc.cpu handling - Remove prompt_routed_experts parameter from OmniRequestState._new_request_output() since upstream RequestState._new_request_output() does not accept it, causing TypeError when calling super(). - Fix _build_multimodal_outputs() to handle callable query_start_loc.cpu (tensor method) consistently with sample_tokens(). - Update test assertions to use output.multimodal_outputs instead of output.pooler_output, matching the current code that puts hidden states in the multimodal_outputs channel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: meghaagr13 <divyanshsinghvi@gmail.com>
|
@tzhouam pls take a look |
|
looks fine to me, but please try to solve the CI failures. You may refer to this report: PR #2744 Buildkite CI Failure Debug Guide
1. Failure OverviewAll three Buildkite pipelines failed, with 12 failed jobs in total:
Key takeaway: CUDA, ROCm, and XPU all show the same error signatures. This is not a platform issue — it is two code bugs introduced by the PR. The visible symptoms are 2. Root Cause A:
|
| Path | Condition | Old code (pooler payload loop) handling |
|---|---|---|
| Sparse audio output | audio_sparse_output == True (VoxCPM2 and other TTS models) |
Uses sparse_mm_index per request; does not slice hidden_states |
| Partial downstream requests | len(downstream_req_ids) != len(req_ids_output_copy) (Qwen2.5-Omni thinker mixed batch) |
Uses per-request dict req_hidden_states_cpu |
| No hidden payload needed | needs_scheduled_hidden_payload == False |
Does not build hidden at all |
Meanwhile, _resolve_req_hidden_states() (~lines 870–885) does hidden_states_cpu[start:end] when combined_hidden_states is None (prefix cache disabled — CI uses enable_prefix_caching=False) → slicing None crashes.
The function signature declares hidden_states_cpu: torch.Tensor as non-optional, but call sites can pass None — the type annotation hides the bug.
Fix
Align _build_multimodal_outputs() with the old pooler-payload loop (~lines 1140–1260):
- Pass in
req_hidden_states_cpu(per-request dict),audio_sparse_output,sparse_mm_index, anddownstream_req_id_set. - Loop over
downstream_req_idsonly, not allreq_ids. - Hidden-state resolution priority:
combined_hidden_states[rid]→req_hidden_states_cpu[rid]→hidden_states_cpu[start:end]. When none apply (sparse audio path), omit the"hidden"key and only include mm payload. - Change
_resolve_req_hidden_states'shidden_states_cpuparameter totorch.Tensor | Noneand handleNoneexplicitly.
4. Local Reproduction
4.1 Check Out PR Code (Without Polluting Your Working Branch)
cd /rebase/vllm-omni
git fetch origin pull/2744/head:refs/remotes/pr/2744 # already done
git worktree add /tmp/pr2744 pr/2744 # isolate with worktree4.2 Fastest Repro (No GPU Model, Pure Logic)
Root cause A can be verified in one line (any environment with aligned vLLM):
python3 -c "from vllm.v1.request import Request; import inspect; \
print('num_cached_tokens' in inspect.getsource(Request.__init__))" # False → field removed4.3 GPU Repro (CI-Equivalent Image)
docker run --gpus all -it --rm -v /tmp/pr2744:/workdir -w /workdir \
$(grep -oP 'FROM \K\S+' docker/Dockerfile.ci | head -1) bash
pip install -e . --no-deps
# Root cause B — crashes in ~2 minutes:
pytest tests/e2e/offline_inference/test_voxcpm2.py::test_voxcpm2_zero_shot_001 -v
pytest tests/engine/test_async_omni_engine_abort.py::test_abort -v
# Root cause A — requires Qwen3-TTS weights:
pytest tests/e2e/online_serving/test_qwen3_tts_base.py -v -k async_chunk4.4 Verify the Fix
After applying fixes, run regression tests by impact area:
# Root cause A regression
pytest tests/e2e/online_serving/test_qwen3_tts_base.py tests/e2e/online_serving/test_qwen3_tts_customvoice.py -v
# Root cause B regression
pytest tests/e2e/offline_inference/test_voxcpm2.py tests/engine/test_async_omni_engine_abort.py -v
# PR unit tests should not regress
pytest tests/engine/test_ensure_tensor_values.py tests/engine/test_wire_multimodal_output.py tests/engine/test_output_processor.py -vOnce all pass, push and ask a maintainer to re-add the ready label to trigger Buildkite (that label was removed on 6/10).
5. How to Fetch Buildkite Logs Yourself
When CI fails, follow this workflow. Token is in /rebase/vllm-omni-rebase-agent/.env:
export BUILDKITE_API_TOKEN=<token>
# 1. Find build numbers for the three pipelines from the PR
gh pr view 2744 --repo vllm-project/vllm-omni --json statusCheckRollup
# 2. List failed jobs and their IDs
curl -s -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \
"https://api.buildkite.com/v2/organizations/vllm/pipelines/vllm-omni/builds/11139" \
| python3 -c "import json,sys; [print(j['state'],j['name'],j['id']) \
for j in json.load(sys.stdin)['jobs'] if j.get('state')=='failed']"
# 3. Download a single job log
curl -s -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \
"https://api.buildkite.com/v2/organizations/vllm/pipelines/vllm-omni/builds/11139/jobs/<JOB_ID>/log" \
| python3 -c "import json,sys; print(json.load(sys.stdin)['content'])" > job.log
# 4. Skip surface symptoms; go straight to the engine fatal traceback
grep -E "stage_engine_core_proc.py:188" job.log | tail -30Tip: OmniEngineDeadError, APIConnectionError, and StagePool _poll_stage_raw failed are all downstream symptoms. The real root cause is always at the bottom of the StageEngineCoreProc encountered a fatal error stack trace.
6. Appendix: Failed Test → Root Cause Mapping
| Test | Root Cause | Crashing Stage |
|---|---|---|
test_async_omni_engine_abort.py::test_abort |
B | Qwen2.5-Omni thinker (LLM_AR) |
test_qwen3_omni.py::test_* (4 cases) |
A | stage-2 code2wav (LLM_GENERATION) |
test_qwen3_omni_realtime_websocket.py::*async_chunk_smoke |
A | stage-2 code2wav |
test_voxcpm2.py::test_voxcpm2_zero_shot_001 etc. |
B | VoxCPM2 talker (LLM_AR, sparse audio output) |
test_voxcpm2_tts.py::test_text_to_audio_001 |
B | Same as above (online) |
test_qwen3_tts_base.py / test_qwen3_tts_customvoice.py |
A | code2wav (LLM_GENERATION) |
AMD mi325_1: *, Intel XPU Test |
A + B | Same as above (cross-platform, same signatures) |
|
@meghaagr13 Any help needed? |
Signed-off-by: Boao Shi <aoibosh@connect.hku.hk> Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # vllm_omni/engine/output_processor.py # vllm_omni/model_executor/stage_input_processors/cosyvoice3.py
…put builder Root cause A: remove dead num_cached_tokens access in omni_generation_scheduler.update_from_output(). vllm.v1.request.Request no longer has this field in vLLM 0.22 (stats moved to prefill_stats), crashing LLM_GENERATION stages (code2wav). Also drop the now-unused num_cached_tokens field from the _HashableRequest test mock. Root cause B: _build_multimodal_outputs() crashed with "'NoneType' object is not subscriptable" when hidden_states_cpu is None (sparse audio output, partial downstream batches, or no scheduled hidden payload). Rebuild the wire channel from the per-request payloads already assembled by the pooler-payload loop, which correctly handles prefix-cache merging, sparse audio, and partial downstream batches, and make _resolve_req_hidden_states None-safe. Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Cursor <cursoragent@cursor.com>
Update remaining async-chunk stage processors to accept the multimodal_output keyword used by OmniChunkTransferAdapter so emitted chunks are not dropped before reaching downstream stages. Add a Higgs-Audio-V3 regression test covering the adapter call shape that caused the online speech endpoint to return no audio. Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Cursor <cursoragent@cursor.com>
…nel (#1601) (#2744) Signed-off-by: Megha Agarwal <agarwalmegha1308@gmail.com> Signed-off-by: meghaagr13 <divyanshsinghvi@gmail.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: meghaagr13 <divyanshsinghvi@gmail.com> Co-authored-by: Boao Shi <aoibosh@connect.hku.hk> Co-authored-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Cursor <cursoragent@cursor.com>
…nel (#1601) (#2744) Signed-off-by: Megha Agarwal <agarwalmegha1308@gmail.com> Signed-off-by: meghaagr13 <divyanshsinghvi@gmail.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: meghaagr13 <divyanshsinghvi@gmail.com> Co-authored-by: Boao Shi <aoibosh@connect.hku.hk> Co-authored-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Cursor <cursoragent@cursor.com>
…nel (#1601) (#2744) Signed-off-by: Megha Agarwal <agarwalmegha1308@gmail.com> Signed-off-by: meghaagr13 <divyanshsinghvi@gmail.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: meghaagr13 <divyanshsinghvi@gmail.com> Co-authored-by: Boao Shi <aoibosh@connect.hku.hk> Co-authored-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Cursor <cursoragent@cursor.com>
…nel (#1601) (#2744) Signed-off-by: Megha Agarwal <agarwalmegha1308@gmail.com> Signed-off-by: meghaagr13 <divyanshsinghvi@gmail.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: meghaagr13 <divyanshsinghvi@gmail.com> Co-authored-by: Boao Shi <aoibosh@connect.hku.hk> Co-authored-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Cursor <cursoragent@cursor.com>
…oject#4421) The no_async_chunk (full-payload) voice-clone path leaks the reference audio into every output: the decoded WAV is [reference + target] instead of [target], so Whisper transcribes ref_text + input and similarity collapses (nightly L4 failure, ~0.27). Root cause: in the sync path the codec stream and its trim length travel on two channels. talker2code2wav_full_payload (worker) prepends ref_code to the codec and is the only side that sees it; talker2code2wav_token_only (orchestrator) emitted left_context_size, derived from the stage-0 RequestOutput multimodal_output. Since the separated multimodal-output channel landed (vllm-project#2744), the orchestrator copy no longer carries the talker codec, so token_only computes ref_frames=0 and emits left_context_size=0. Code2Wav then trims nothing and the prepended reference frames survive into the output. Fix: emit left_context_size from full_payload -- the authoritative producer that actually prepends ref_code -- co-locating the trim length with the prepend it describes, mirroring the async-chunk path which already ships left_context_size in-band. Verified on H20-class GPU (vLLM 0.23.0): code2wav ctx_frames 0 -> 101, output frames 139-146 -> 38-45, output duration ~12s -> ~3.5s (target only). Signed-off-by: Yueqian Lin <linyueqian@outlook.com>
…nel (vllm-project#1601) (vllm-project#2744) Signed-off-by: Megha Agarwal <agarwalmegha1308@gmail.com> Signed-off-by: meghaagr13 <divyanshsinghvi@gmail.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: meghaagr13 <divyanshsinghvi@gmail.com> Co-authored-by: Boao Shi <aoibosh@connect.hku.hk> Co-authored-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Cursor <cursoragent@cursor.com>
…nel (vllm-project#1601) (vllm-project#2744) Signed-off-by: Megha Agarwal <agarwalmegha1308@gmail.com> Signed-off-by: meghaagr13 <divyanshsinghvi@gmail.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: meghaagr13 <divyanshsinghvi@gmail.com> Co-authored-by: Boao Shi <aoibosh@connect.hku.hk> Co-authored-by: Taichang Zhou <tzhouam@connect.ust.hk> Co-authored-by: Cursor <cursoragent@cursor.com>
PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.
Purpose
Add multimodal output field instead of pooling output to avoid collisions in two. Resovles Phase 2 of #1601
Test Plan
python -m pytest tests/e2e/offline_inference/test_cosyvoice3.py -v
Test Result
tests/e2e/.../test_cosyvoice3.py::...[cosyvoice3.yaml] PASSED
tests/e2e/.../test_cosyvoice3.py::...[cosyvoice3_async_chunk.yaml] PASSED
================== 2 passed, 18 warnings in 167.05s ==================
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model. Please runmkdocs serveto sync the documentation editions to./docs.BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)