Repository navigation
Conversation
339e941 to
e38b0d1
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. |
|
This PR appears to belong to: docs/design/module/input_output_modality_contracts.md. Module owners: @Sy0307 @amy-why-3459 @Gaohan123 @alex-jw-brooks @RyanYun09, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
e38b0d1 to
71a13d8
Compare
|
The layering is right: one
The A3 table is Wrap once at resolve — |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
…solution (RFC vllm-project#4872 P2) extract_legacy_stage_metadata swallowed every resolve_processor failure and continued with custom_process_input_func=None, silently dropping the configured hook and falling back to _default_process_engine_inputs (downstream prompt = upstream token_ids). A bad path used to fail at import. - Remove the blanket try/except: ProcessorValidationError / ImportError / AttributeError now propagate (fail-fast), matching RFC P2 and the adding-omni text. M0 is a DeprecationWarning on C2/C3/C4, not a silent drop of the hook. - The worker-side mixin candidate chain (_load_custom_func) keeps its own skip-on-mismatch tolerance for *_full_payload probes; it is untouched. - Add fail-fast tests (bad module / missing attr / non-callable symbol). Reviewer: amy-why-3459 (Blocking 2) / hsliuustc0106 (blocking). Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…er resolve (RFC vllm-project#4872 P1) invoke_orchestrator_processor re-wrapped on every forward, so a legacy C2 shell like thinker2talker_token_only re-emitted the DeprecationWarning on each forward. wrap_orchestrator_processor is now memoized per callable: the signature probe and the warning happen once (at first resolution/invocation), then the cached C1-compatible wrapper is reused. - Add _WRAP_CACHE keyed by the callable; C1 callables still pass through unchanged (no warning). - Add test_dispatch_wrap_once.py locking: warning fires exactly once across repeated wraps / forwards, C1 passthrough warns never. Reviewer: amy-why-3459 (cleanup 4). Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…codec guard off import (RFC vllm-project#4872 P8b/P6) - qwen3_omni._compute_talker_prompt_ids_length is dead code after the P8b consolidation onto _common.compute_placeholder_prompt_len; delete it. - _assert_codec_token_ids_consistent is no longer run at module import (it pulled in transformers / the qwen3-tts config and broke light imports). It is now invoked explicitly at model-load startup in qwen3_omni_moe_talker.__init__, keeping the processor constants in sync with the HF configs the model actually runs with. Reviewer: amy-why-3459 (cleanup 5, 6). Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…llm-project#4872 M0) Wire the pure decision helper at startup: for every stage whose custom_process_input_func would never be invoked under the current async-chunk / sync wiring (mirroring the _route_output gate), log a warn-only hint so a configured-but-dead hook is not silently dropped. Defensive by design — fake pools / minimal clients never raise. Reviewer: amy-why-3459 (cleanup 7). Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…oken_only shell (RFC vllm-project#4872 P1) The snippet showed thinker2talker_token_only as the C1 (source_outputs, ctx) contract, but the function is still the four-argument legacy shell (prompt / requires_multimodal_data / streaming_context). Rewrite the snippet to document the real signature, and note that the orchestrator normalizes it once via wrap_orchestrator_processor with the C1 dual-entry builders (build_forward_placeholder / build_prewarm_placeholder) attached to the shell. Reviewer: amy-why-3459 (cleanup 8). Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…ibution Replace internal work-log phrasing (RFC proposal/phase numbers, local-dev shim wording, machine-specific skip notes, imperative/process-oriented instructions) with professional descriptions of what each test covers and why. Keep the RFC vllm-project#4872 reference as a top-level link for traceability. No functional changes: only comments and docstrings were edited. Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…rings for upstream contribution Remove internal work-log phrasing from module and function docstrings and inline comments across the stage-input processor implementation (RFC proposal/phase numbers such as P2/P3/P6/P8b, Phase 1a/1b/3, Step 1c, Part A, local-dev shim and macOS/CI environment notes, imperative/process-oriented wording). Rewrite as professional descriptions of what each helper does and why. Keep the RFC vllm-project#4872 reference as a top-level link for traceability. No functional changes: only comments and docstrings were edited. Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…y builder attachment (RFC vllm-project#4872) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…d four-positional dispatch (RFC vllm-project#4872 P1) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…ist normalization (RFC vllm-project#4872 P2) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…ts_model pipeline (RFC vllm-project#4872 P2) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…ature.bind (RFC vllm-project#4872 P1) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…roject#4872 P2) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…RFC vllm-project#4872 P3) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
… correct mixin thin-wrapper Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…ract_last 等 14 处重复定义
P0-A: _ensure_list → _common.ensure_list_* (audex/qwen2_5_omni/
qwen3_omni/cosyvoice3/step_audio2/dynin_omni 共 6 模块)
P0-B: _to_cpu_tensor → _common.to_cpu_tensor (auk/glm_tts 共 2 模块)
_to_token_id_list → _common.to_token_id_list (dynin_omni/
cosyvoice3 共 2 模块)
P0-C: _extract_last_frame/row → _common.extract_last_codec_frame
(fish_speech 替换 + higgs_audio_v2/v3 转发包装 共 3 模块,
audex 因数组索引差异保留)
P1-A: revert_delay/filter_real_code 新增参数化入口
(_common 内部函数保留, higgs_v3 调用方更新)
移除遗留 RFC/PR/Agent 注释 8 处 (7 文件)
激进精简注释 11 文件 (RFC/PR/阶段标记/LLM 乱码零残留)
修复 Code Review 3 项: ming_flash_omni ensure_list_strict,
step_audio2 ensure_list_preserve_none, auk ndim==2 校验
Net: 33 files, +256/-501 (-245 net)
Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…async_chunk_prewarm_prompt_len Rebase to 232dbc8 dropped three pieces of the RFC vllm-project#4872 P2/P3 work in stage_init_utils.py and one in orchestrator.py: - StageMetadata.prompt_transform_func field (dataclass) is back, so extract_legacy_stage_metadata can carry the resolved callable next to sync_process_input_func again. - extract_legacy_stage_metadata re-resolves prompt_transform_func from the stage config via importlib (same pattern as prompt_expand_func). - Remove the dead OmniInputPreprocessor import that the rebase reintroduced. - _build_prewarm_placeholder_input honors hf_config.async_chunk_prewarm_prompt_len so a talker stage whose engine positions must cover a speaker-prompt prefill can size its own prewarmed placeholder prompt. Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…pre-commit) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…uff F401 in CI pre-commit) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…-why-3459 review) Two new tests verify that a configured async_chunk_prewarm_prompt_len (e.g. the Nemotron deployment's 37-token speaker-prompt prefill) survives the inline-estimate fallback path: - test_prewarm_override_keeps_configured_length_on_builder_miss: stage WITHOUT a builder keeps the configured length. - test_prewarm_override_keeps_configured_length_on_builder_failure: builder that RAISES falls back to the inline estimate AND keeps the configured override. Also fixes FakePrewarmPool to be compatible with HEAD (adds get_bound_client and stage_client attributes). Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…text-cond semantics Address the two P1 findings in the latest review of PR vllm-project#6801: * P1 vllm-project#1: restore the `_processor_accepts_step_tokens` signature-cache initialization in `OmniChunkTransferAdapter.__init__`. It was dropped during the refactor, causing AttributeError on every async send and silently dropped chunks. * P1 vllm-project#2: preserve target-model config injection in the new dispatch contract. `OrchestratorInputContext` gains `target_model_config` and `next_stage_hf_config`, `StageEngineCoreClient.process_engine_inputs` fills them (getattr-guarded), and the legacy call-shape adapters inject them into processors that declare the matching keyword-only params (excluded from the c3 shape to avoid a `sampling_params` conflict). Also restore auk's wide-semantics `_to_cpu_tensor` (recursive list concat via torch.cat, numpy coercion via torch.as_tensor, ValueError on empty list), which was accidentally narrowed to `_common.to_cpu_tensor` by commit 0735430 and broke three TestTextCondExtraction tests. Adds regression tests for both P1 fixes and restores SIP full-suite to 594 passed / 0 failed. Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…e required keyword-only is_finished (amy-why-3459 review) - Apply async_chunk_prewarm_prompt_len override on the placeholder-builder success path, not only the inline fallback (P2 vllm-project#1). - Reject full-payload producers declaring a required keyword-acceptable is_finished that cannot bind the worker's best-effort kwarg (P2 vllm-project#2). - Narrow _apply_prewarm_prompt_len_override annotations from Any and restore legacy >1 guard semantics (precheck-pr code-quality sweep). - Add regression tests: builder success/miss/failure, len==1 edge, required keyword-only is_finished. Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…ure_list) Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
…re fish speech 1-D codec semantics (amy-why-3459 review) - _load_custom_func now resolves without expected_kind and gates on the mixin's structural payload-builder validator, so an explicitly configured hook with an arbitrary name is accepted based on its signature (pooling_output / multimodal_output + transfer_manager + request), never rejected by the registry's name-driven kind inference. - extract_last_codec_frame restores the legacy 1-D codec-frame semantics: a flat (1-D) input is returned directly without the validity/zero gate, matching Fish Speech / Qwen3-TTS behavior; only 2-D inputs go through audio_code_valid / any() validation. - Regression tests: fish speech 1-D golden cases (all-zero flat frame and flat frame + invalid validity flags still return the frame) and three connector-payload-builder acceptance tests covering full-payload (pooling_output), async-chunk (multimodal_output), and arbitrary-name hooks. Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
… docs Plan B (reviewer feedback on PR code bloat): drop the internal P1/P2/P3/P8b phase markers and 'vllm-project#6801' PR tags that leaked into test docstrings and design docs — they are development scaffolding, not durable documentation. - test_chunk_transfer_adapter.py: strip '(vllm-project#6801 P1 vllm-project#1)' from two docstrings - test_nemotron_voicechat.py: strip '(vllm-project#6801 P1 vllm-project#2)' and '(vllm-project#6801)' markers - test_orchestrator_stage_input_bridge.py: rewrite two '# P2/P3 deep-dive' section comments as plain prose - docs/design/feature/async_chunk.md: rename 'P8b dual entry' heading - docs/contributing/model/adding_omni_model.md: drop RFC vllm-project#4872 P8b/C1 tags Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
2b46774 to
7a77d83
Compare
amy-why-3459
left a comment
There was a problem hiding this comment.
Reviewed head 7a77d8318b793862e1bc0653e28501893ccf41fe. Requesting changes for the Voxtral codec-frame regression described inline.
Targeted validation: 1,130 tests passed, plus 3 subtests, covering stage input processors, the orchestrator stage-input bridge, chunk transfer adapter, connector mixin, and stage metadata. A separate CPU base/head reproduction executes the production compute_mm_logits() method with a simulated acoustic transformer and passes its output through the real worker payload builder: base produces an audio payload; head raises ValueError: unexpected audio_codes shape: (1, 1, 8). This is a function/payload reproduction, not model-weight or end-to-end inference validation.
Separate validation gap: please provide complete repository unit-test evidence tied to this SHA (and the updated head after fixing the regression), including the CPU UT shards defined in .buildkite/cuda/test-ready.yml and applicable required hardware/backend-specific UT jobs. Attach exact commands, environment/dependency versions, passed/failed/skipped/deselected/collection-error counts, and CI or complete-log links. Explain skips and unavailable jobs; if failures are pre-existing, include a same-environment base/head comparison. Existing complete CI evidence on the same revision is sufficient; targeted green tests alone do not establish full-suite coverage. Merge readiness remains unestablished until this evidence is supplied or maintainers explicitly disposition the coverage gap.
|
|
||
| if isinstance(multimodal_output, Mapping): | ||
| frame = _extract_last_frame(multimodal_output) | ||
| frame = _common.extract_last_codec_frame(multimodal_output, to_long=False) |
There was a problem hiding this comment.
[P1] Preserve Voxtral's three-dimensional codec frame shape
VoxtralTTSAudioGeneration.compute_mm_logits() constructs audio_list by splitting audio_codes.unsqueeze(1) along the batch dimension, so each per-request codec tensor has shape [1, 1, C]. The worker payload builder preserves this shape. The previous _extract_last_frame() flattened it successfully, but the new extract_last_codec_frame() accepts only 1-D and 2-D tensors and raises on this normal model output.
I reproduced this with the production model method (using a simulated acoustic transformer) and the actual build_mm_cpu() / build_omni_mm_payload() path: the per-request shape is (1, 1, 8), base emits the expected audio payload, and this head raises ValueError: unexpected audio_codes shape: (1, 1, 8) before buffering any frame. The transfer adapter catches the exception and drops the audio payload; completion can therefore forward only an empty terminal marker.
Please preserve Voxtral's frame-flattening semantics, or normalize this shape in its model-specific adapter without changing other models' last-row semantics. Add regression coverage using [1, 1, C] through payload construction, chunk emission, and final flush; the existing 1-D mock inputs do not cover the actual model output.
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
Purpose
This PR implements the
stage_input_processorsrefactor tracked by RFC #4872. Before this change the module suffered from:inspect.signatureprobe points were used to introspect processor callables, leaving the runtime contract implicit and fragile.stage_input_processorspackage exported no stable API.The change is layered:
_dispatch.py): introducesOrchestratorInputContextplus four Protocols (placeholder prompt builder, diffusion builder, and two producer contracts), a singleinvoke_orchestrator_processorentry point, legacy signature adapters (C0–C4), and aDeprecationWarning(M0) for legacy positional contracts._registry.py): centralizesresolve_processor/validate_processorwith loose/strictis_finishedrules,suffix ↔ kindmapping, and a dead-processor three-state gate; the three existing resolution points (stage_init_utils, the worker mixin, andchunk_transfer_adapter) are unified through it._common.py): consolidates 13 helper functions and deduplicates 10 modules by delegation, with a 77-case golden test suite locking the previously divergent semantics._constants.py): moves scattered constants into a single module, backed by a consistency test.build_forward/build_prewarmentries; the prewarm path now resolves through the registered builder and hardcoded token ids are removed.Test Plan
vLLM Version: local CPU regression on
0.28.0(arm64 wheel); repo CI (Dockerfile.ci/ Buildkite) also runs vLLM 0.28.0vLLM-Omni Commit: based on upstream
mainmerge-base232dbc82f(vLLM 0.28.0) + 31 commits:15513fdbb,68993b3bf,babc77dd4,d537e91bd,5b628fbe8,d7377885d,1460be497,d01562a83,b1a771801,3eafd2733,06fcb65ac,2054347f3,ee0aac3ca,48a4e7344,f44a02675,a690170bd,7c323bcb9,0a7387698,2e2953a97,249e6202f,1003b26c8,30b3cf087,81ba7d6a5,ed5608313,47a36dfb0,073543019,a36dbdb65,72852b558,ba23816c7,490e438be,ad81ac3ac; PR branch HEAD =ad81ac3ac31 commits span the original 25 (up to
29f17d4e) plus 4 rebase-damage repair commits + 2 CI hotfix commits + 2 latest review-round commits (490e438beregression tests for the prewarm override,ad81ac3acthe P1 fixes + AuK restore).CI status at
ad81ac3ac: DCO ✅ · build (3.11) ✅ · build (3.12) ✅ · pre-commit ✅ · docs/readthedocs ✅ (Buildkite pending)Tests executed (vLLM 0.28.0, arm64 CPU container, PR HEAD
ad81ac3ac):tests/model_executor/stage_input_processors/(full directory) — 594 passed / 0 failedtests/config/test_config_factory.py— 330 passed / 0 failedtests/engine/test_orchestrator_stage_input_bridge.py— 12 passed / 0 failedtests/engine/(CPU-marked) — 587 passed / 1 skipped / 2 deselected (GPU-only); this includes the 9 engine files that reference vLLM 0.28'sCoreEngineLaunch(previously uncollectable on 0.27.1) — 190 CPU cases passedtests/diffusion/files previously blocked on vLLM 0.28 (stage_diffusion_proc-dependent) — 80 passed, 1 pre-existing failure intest_diffusion_streaming_output(asyncio teardown race, not introduced by this PR)tests/model_executor/stage_input_processors/test_registry.py/test_placeholder_parity.py/test_constants_consistency.py— included in the 594 aboveThe 4 rebase-damage repair commits + 2 CI hotfix commits (
47a36dfb0,073543019,a36dbdb65,72852b558,ba23816c7) are scoped to engine/merge code and do not touch anystage_input_processorslogic or test files. The latest review-round commitad81ac3actouchesstage_input_processors/auk.py(AuK text-cond restore, see below) and lifts the SIP full suite to 594 passed / 0 failed under the same vLLM 0.28.0 environment.Test Result
CPU container (
linux/arm64, vLLM 0.28.0): all PR-relevant suites above ran with 0 failures. GPU/NCCL- and memory-sensitive cases (pi0, distributed comms, norm layers) are environment-limited locally and are validated by CI (Buildkite, vLLM 0.28.0).Latest Review Round — @amy-why-3459's two P1s + AuK restore (commit
ad81ac3ac)Commit
ad81ac3acresolves the two P1 compatibility regressions from @amy-why-3459's latest review (CHANGES_REQUESTED, review5255823592, 2026-09-19) and restores the AuK text-cond semantics. 6 files changed, +211/−8; DCO-signed (Signed-off-by: RyanYun09).P1 #1 — signature-cache initialization restored (
chunk_transfer_adapter.py)OmniChunkTransferAdapter.__init__re-initializesself._processor_accepts_step_tokens = {}beforesuper().__init__(model_config), so_accepts_new_token_ids()(invoked on every custom async-chunk producer send) no longer raisesAttributeError. Both a normal chunk payload and the final/segment-finished flush reach the connector; a production-path regression test was added through_send_single_request()(tests/distributed/omni_connectors/test_chunk_transfer_adapter.py).P1 #2 — target-model configuration injection preserved (
_dispatch.py/stage_engine_core_client.py)OrchestratorInputContextgainstarget_model_configandnext_stage_hf_config;StageEngineCoreClient.process_engine_inputsfills them (getattr-guarded);sampling_paramsis excluded from the C3 shape to avoid a duplicate-kwarg conflict.JoyAI
joyai_action_to_tts()now binds its keyword-onlytarget_model_config, and Nemotron'sthinker2talker_token_only()receivesnext_stage_hf_configas its fourth argument instead of being misread asstreaming_context— the native talker placeholder keeps its configuredtalker_init_len(e.g. 37). Regression tests added throughprocess_engine_inputs()(tests/distributed/omni_connectors/test_chunk_transfer_adapter.py,tests/model_executor/stage_input_processors/test_nemotron_voicechat.py).AuK text-cond restore (
auk.py)_to_cpu_tensorregains its AuK-wide semantics (recursive list →torch.cat(dim=0), numpy / array-like →torch.as_tensor, empty list →ValueError), fixing the accidental narrowing to_common.to_cpu_tensorintroduced by commit073543019; the threeTestTextCondExtractiontests pass again.Updated test evidence
tests/model_executor/stage_input_processors/(full directory) — 594 passed / 0 failedNPU E2E validation — refreshed on PR head
29f17d4ewith vLLM 0.28.0 (Ascend A3) ✅Supersedes the earlier
a0ecfe73refresh. The 6 review comments from @hsliuustc0106 are addressed in 6 commits (d3c24da5c7d8a9ac520deb6135eb88ea2e12e13929f17d4e); re-validated on the new head.vllm-ascend:v0.28.0-a3(CANN 9.1.0, vLLM 0.28.0 source-editable, vllm-ascend 0.20.2rc line, triton-ascend 3.2.2, torch 2.10.0+cpu / torch_npu 2.10.0.post4)rfc4872-stage-input-processors-refactor@29f17d4ea705qwen3_omni_moe.yaml(thinker TP2 / talker / code2wav),async_chunk=true, PIECEWISE cudagraphResult: 11/11 functional checks PASS — no functional regression vs the
a0ecfe73round and the v0.27.1 baseline.DeprecationWarning0 · dead-processor false positives 0 ·Signature.binderrors 0 · prewarm fallback warnings 0; recursive per-itemensure_listpath not exercised by this Qwen3-Omni pipeline (N/A on NPU, golden-locked by CPU tests)git diff a0ecfe73..29f17d4e -- vllm_omni/platforms/npu/worker/is empty)Shim attribution (not part of this PR): on this same branch, removing the 3
getattrshims reproduces the exact gap (AttributeError: 'NPUARModelRunner' object has no attribute 'calculate_kv_scales'at npu_ar_model_runner.py:718 → EngineDeadError → /health 503); re-applying them restores a fully passing service. The gap pre-exists on main's shared code: vLLM 0.28.0 base classes lack both attributes, the PR diff contains 0 touching changes to those lines, and the bare reference at :718 is byte-identical on main3d035bfa(merge-base). (main HEAD itself dies earlier at import time under this image; the PR branch carries its own vLLM 0.28.0 import-compat layer — compatibility better than main's.)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)