Repository navigation
Conversation
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
This is a large PR (2075 LOC, 13 files). Please address the following before proceeding:
BLOCKING:
- [Test Coverage] — Test results not provided. The PR description says "Test Result: To be filled in after running GPU tests on test machine." Please run the GPU tests and paste the results inline before requesting review:
- CPU unit test results: ============================= test session starts ==============================
platform darwin -- Python 3.12.8, pytest-8.4.1, pluggy-1.5.0 -- /Users/hsliu/miniconda3/bin/python
cachedir: .pytest_cache
rootdir: /Users/hsliu/.openclaw/workspace/vllm-omni-skills
plugins: cov-5.0.0, mock-3.14.1, anyio-3.7.1, asyncio-1.0.0
asyncio: mode=Mode.STRICT, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collecting ... collected 0 items
- CPU unit test results: ============================= test session starts ==============================
============================ no tests ran in 0.00s =============================
-
GPU offline e2e:
-
GPU online e2e:
-
Reference parity vs upstream
-
Concurrency smoke test results
-
[Test Coverage] — Missing L3 test results for large PR. This PR is substantial (>1000 LOC and >10 files). Could you please run the L3 tests locally and paste the results here?
-
[Documentation] — CC BY-NC-ND 4.0 license restriction. The Vevo2 checkpoint has a non-commercial, no-derivatives license. While the PR calls this out, please add a clear migration path or note about what this means for production deployments:
- Commercial deployment requires contacting upstream authors
- Model weights cannot be modified or redistributed
- Consider adding this to a "License Considerations" section in the docs
|
Thanks a lot @hsliuustc0106 for the review. I will work on the changes |
|
Thanks @hsliuustc0106. I ran the GPU validation on an NVIDIA A10G (24 GB) and, in the process, found and fixed a real correctness bug. Summary below; the fix + test/doc hardening are pushed as TL;DRThe earlier "synthesis is just slow on A10G" symptom was actually a runaway-generation bug: a single short prompt produced 8638 s (2.4 h) of audio (207,320,160 samples) in ~99 min. Root-caused, fixed, and re-verified — output is now a correct 4.22 s (101,280 samples @ 24 kHz), and the L3 offline e2e test passes. Root causeThe AR scheduler keeps calling
Fix (
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
I did a dead-code-focused pass. I did not find true dead code in the new Python files: a narrow ruff pass for unused imports/locals/undefined names was clean, and vulture’s hits were framework hooks/signature args such as load_weights, compute_logits, embed_input_ids, and the registered adapter. I left a few actionable comments for issues that surfaced during the same pass.
|
please check #4539 as well |
… setup Address review feedback on vllm-project#3422: - modeling_vevo2: re-raise inference failures instead of yielding an empty sentinel; forward() drops the dead generator (marking the request finished) and propagates so a failed request fails loudly instead of returning silent audio. - end2end example: default --model to the init_vevo2_checkpoint.py-prepared ./ckpts/Vevo2 path (the bare RMSnow/Vevo2 repo cannot be loaded directly). - e2e tests: both suites skip clearly when VEVO2_MODEL_PATH points at no initialized checkpoint, instead of falling back to the unloadable repo id. - CI: the Vevo2 buildkite lane now initializes + exports VEVO2_MODEL_PATH. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… setup Address review feedback on vllm-project#3422: - modeling_vevo2: re-raise inference failures instead of yielding an empty sentinel; forward() drops the dead generator (marking the request finished) and propagates so a failed request fails loudly instead of returning silent audio. - end2end example: default --model to the init_vevo2_checkpoint.py-prepared ./ckpts/Vevo2 path (the bare RMSnow/Vevo2 repo cannot be loaded directly). - e2e tests: both suites skip clearly when VEVO2_MODEL_PATH points at no initialized checkpoint, instead of falling back to the unloadable repo id. - CI: the Vevo2 buildkite lane now initializes + exports VEVO2_MODEL_PATH. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
|
Hey @hsliuustc0106, @linyueqian I have addressed all of the comments do let me know if something is missing. |
hsliuustc0106
added
enhancement
ChatGPT Codex Connector
|
Both red checks addressed, and the branch is rebased onto current main (
There was a second failure you hadn't hit:
Merge state: #6138 coexists cleanly: Filed the two MOSS follow-ups you asked for: #6479 (the Leaving the |
linyueqian
added
ready
|
@yashkgp — I added 1. The failing step — caused by this PR, one-line fixExactly 1 of 19 steps failed: One test failed in the whole step: I reproduced it locally in 2.02 s, so there's no ambiguity: Cause.
Fix. Add Worth noting why neither of us caught it: it's a 2-second CPU test in a file neither of our local runs touched. Your 59-passed sweep and my 57-passed sweep were both Vevo2-scoped; 2. The Vevo2 lane did not run — the label came too lateI need to correct something I implied earlier. Adding The build was created at 20:42:58Z and I applied So the gap we discussed is still open: Suggested order
Everything I verified at |
Blocking fixes from the maintainer review on vllm-project#3422. 1. Stale output accessor (offline example + offline e2e suite) `OmniRequestOutput` has subclassed `RequestOutput` since vllm-project#5146, so the nested `.request_output` no longer exists and both files crashed with `AttributeError` *after* synthesizing correctly. They were the only two users left in the tree, and `tests/docs/test_example_output_accessors.py` -- a cpu/core_model guard -- was already failing on them, so this was red at L1, not only in the GPU lane. Each item yielded by `Omni.generate` is itself an `OmniRequestOutput`, so read `.outputs` off it directly. Note `OmniRequestOutput` is *not* iterable: the `moss_tts_nano` example cited in review iterates it and would raise `TypeError`, so this follows `gepard/end2end.py` instead, which has the same two-level indexing. 2. Online requests could not influence Vevo2's sampling, including `seed` Vevo2 samples inside Amphion's `inference_ar_and_fm`, reading its knobs from `additional_information`; the `SamplingParams` the dummy AR scheduler carries only drive the forced-EOS sampler. `Vevo2Adapter.build` never copied anything across, so `/v1/audio/speech` seeds were inert and `default_sampling_params.seed` was a dead setting. `build()` runs before the shared path writes `request.seed` onto `SamplingParams`, so the `moss_tts.py` precedent would have wired the deploy default rather than the request seed. Read `request.seed` directly and fall back to the deploy default; also forward top_k / top_p / temperature / flow_matching_steps from `extra_params`. 3. Seeded requests leaked global RNG state across a batch The save/restore `finally` sat after `yield chunk`, so it only ran when the generator was resumed on a later `forward()` step -- leaving the process RNG seeded while batch siblings ran, and never returning to the original state under nested save/restore. Seeding now happens in a `_seeded_rng` context manager scoped to the inference call alone, never spanning a yield. Also in this change: - `forward()` creates every generator for a step before advancing any, and `_create_stream_gen` validates eagerly, so one malformed row no longer aborts a step that already consumed its siblings' chunks. The remaining raise-on-inference-failure is documented against the missing per-row error channel in `OmniOutput`. - `init_vevo2_checkpoint.py` is idempotent and repairs in place, re-raises instead of warning on a failed placeholder write, fails loudly on a missing AR sub-checkpoint, and verifies every root artifact before reporting success. The CI lane gates the download on a sub-checkpoint the initialiser never writes and always runs the initialiser. - The `LlamaConfig` positional shim is a scoped context manager instead of a permanent process-wide patch, and rejects duplicate positional/keyword arguments. Dropped the `rope_scaling` backfill: it never fired, and on transformers>=5 `rope_scaling` is a write-through property over `rope_parameters`, so forcing it to None would clear the rope config. - Deploy YAML and recipe restate the memory budget subtractively, matching `worker/base.py`: the pipeline is subtracted from `gpu_memory_utilization`, not stacked on top of it. - Recipe verification numbers re-attributed to the paths that produced them, and the broken `#vevo2-license-considerations` anchor fixed. - `run_server.sh` and the `end2end.py` docstring document the one-time init step and use `hf download`. - Tests: streaming lifecycle (waveform -> sentinel -> EOS -> cleanup, and the runaway-generation guard), RNG hygiene, eager validation, adapter seed plumbing, shim scoping, online seed reproducibility, a 400-not-5xx assertion, output duration bounds, and an offline batch test that actually distinguishes isolation from reproducibility. 18 -> 33 CPU unit tests. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
5c5ad6f to
6ffdaab
Compare
|
@linyueqian — the The stage-key bug: fixed
Your point about why we both missed it is the right one, and it's the argument for the guard: a completeness check that only fires when a new adapter lands is invisible to any model-scoped sweep. Why no CI ran after that pushThe PR had a merge conflict, and that blocks workflow creation outright. The conflict was in the two TTS READMEs, and it had a trap worth flagging: my side carried SoulX-Singer content (6 refs in one file, 11 in the other), which main deleted in #6362. Resolving "in my favour" would have silently resurrected a model you'd just removed. I rebuilt both sections from main's file plus only what the feature commit actually adds — that commit contributes zero SoulX lines (they appear purely as diff context) — so the diff now has none. Current status at
|
Omni ReviewBot: no human activity for 14 days@yashkgp this pull request has had no human commit, comment or review since 2026-08-24. Per repository policy it may be closed if it stays inactive. 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. |
|
Hey @linyueqian |
|
Not yet, but it is close and the remaining items are mechanical. Three things stand between
So the sequence is: rebase and push, I re-label |
Adds Amphion's Vevo2 as a single-stage AR + flow-matching TTS model: model class, HF config registration, pipeline, model/pipeline registry entries and deploy config. - model_executor/models/vevo2: Vevo2ForCausalLM, Vevo2Config (registered with HF AutoConfig), and the pipeline wiring the AR LM + flow-matching decoder. Sub-models are cast to fp32; IPython is stubbed at import so headless envs work, and construction chdirs to the Amphion root. A small import-time shim maps Amphion's positional LlamaConfig(...) call to keyword args and backfills the legacy rope_theta/rope_scaling attributes, so the stack loads under transformers>=5 (no transformers<5 pin, which would conflict with requirements/common.txt and vLLM). - Frontend: Vevo2Adapter (tts_adapters/vevo2.py) plus _validate_vevo2_request / _build_vevo2_params on the speech server. Waveforms are emitted as delta chunks per yield, following the MOSS-TTS-Nano single-stage pattern. Empty text or a missing reference now raise (400 online, a loud failure offline) instead of returning a silent, successful-looking WAV. - Examples + docs: offline end2end, online client and run_server.sh, the one-time init_vevo2_checkpoint.py setup script (the published RMSnow/Vevo2 repo ships no root config.json), a speech_api.md entry, a recipes/RMSnow recipe, and a docs/models/supported_models.md row. - Tests + CI: unit + offline/online e2e suites (marked L4) and a Vevo2 buildkite lane that initializes the checkpoint and fails the step if the download or init fails, rather than skipping green. Vevo2 needs Amphion on PYTHONPATH (it is not published on PyPI); the deploy config's memory budget reflects the measured ~7.55 GiB pipeline peak. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
Blocking fixes from the maintainer review on vllm-project#3422. 1. Stale output accessor (offline example + offline e2e suite) `OmniRequestOutput` has subclassed `RequestOutput` since vllm-project#5146, so the nested `.request_output` no longer exists and both files crashed with `AttributeError` *after* synthesizing correctly. They were the only two users left in the tree, and `tests/docs/test_example_output_accessors.py` -- a cpu/core_model guard -- was already failing on them, so this was red at L1, not only in the GPU lane. Each item yielded by `Omni.generate` is itself an `OmniRequestOutput`, so read `.outputs` off it directly. Note `OmniRequestOutput` is *not* iterable: the `moss_tts_nano` example cited in review iterates it and would raise `TypeError`, so this follows `gepard/end2end.py` instead, which has the same two-level indexing. 2. Online requests could not influence Vevo2's sampling, including `seed` Vevo2 samples inside Amphion's `inference_ar_and_fm`, reading its knobs from `additional_information`; the `SamplingParams` the dummy AR scheduler carries only drive the forced-EOS sampler. `Vevo2Adapter.build` never copied anything across, so `/v1/audio/speech` seeds were inert and `default_sampling_params.seed` was a dead setting. `build()` runs before the shared path writes `request.seed` onto `SamplingParams`, so the `moss_tts.py` precedent would have wired the deploy default rather than the request seed. Read `request.seed` directly and fall back to the deploy default; also forward top_k / top_p / temperature / flow_matching_steps from `extra_params`. 3. Seeded requests leaked global RNG state across a batch The save/restore `finally` sat after `yield chunk`, so it only ran when the generator was resumed on a later `forward()` step -- leaving the process RNG seeded while batch siblings ran, and never returning to the original state under nested save/restore. Seeding now happens in a `_seeded_rng` context manager scoped to the inference call alone, never spanning a yield. Also in this change: - `forward()` creates every generator for a step before advancing any, and `_create_stream_gen` validates eagerly, so one malformed row no longer aborts a step that already consumed its siblings' chunks. The remaining raise-on-inference-failure is documented against the missing per-row error channel in `OmniOutput`. - `init_vevo2_checkpoint.py` is idempotent and repairs in place, re-raises instead of warning on a failed placeholder write, fails loudly on a missing AR sub-checkpoint, and verifies every root artifact before reporting success. The CI lane gates the download on a sub-checkpoint the initialiser never writes and always runs the initialiser. - The `LlamaConfig` positional shim is a scoped context manager instead of a permanent process-wide patch, and rejects duplicate positional/keyword arguments. Dropped the `rope_scaling` backfill: it never fired, and on transformers>=5 `rope_scaling` is a write-through property over `rope_parameters`, so forcing it to None would clear the rope config. - Deploy YAML and recipe restate the memory budget subtractively, matching `worker/base.py`: the pipeline is subtracted from `gpu_memory_utilization`, not stacked on top of it. - Recipe verification numbers re-attributed to the paths that produced them, and the broken `#vevo2-license-considerations` anchor fixed. - `run_server.sh` and the `end2end.py` docstring document the one-time init step and use `hf download`. - Tests: streaming lifecycle (waveform -> sentinel -> EOS -> cleanup, and the runaway-generation guard), RNG hygiene, eager validation, adapter seed plumbing, shim scoping, online seed reproducibility, a 400-not-5xx assertion, output duration bounds, and an offline batch test that actually distinguishes isolation from reproducibility. 18 -> 33 CPU unit tests. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
Verified the previous commit on a real A10G. Everything passed except the
new online seed test, which failed on the shared helper's transcript check
rather than on its own assertions:
AssertionError: Transcript doesn't match input: similarity=0.90,
transcript=' Determinism check for the online seat path.'
Whisper heard "seat" for "seed", landing exactly on the threshold. The
tests above this one already avoid hard-to-transcribe words for the same
reason (they sidestep the coined word "Vevo2"), and they pass
``transcript_escalation_model`` so a mishear escalates to a larger model
instead of failing; this test did neither.
Switch to plain dictionary words and add the escalation model. Re-ran on
the same box: 1 passed, and the seed assertions themselves -- byte-identical
audio for a repeated seed, different audio for a different seed -- now
actually execute, which is the coverage this test exists for.
Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
Re-measured every recipe figure on a single A10G 24 GB (g5.2xlarge), vLLM 0.27.1 / transformers 5.15.1 / torch 2.13.0+cu130, each one from the exact command it is printed under -- which is what @linyueqian asked for after I had wrongly presented their direct-Amphion figures as output of ``end2end.py``. T1, offline end2end.py: 7.26 s audio, Whisper WER 0.000, RMS 0.180, 30975 distinct sample values. T2, voice cloning by swapping --ref-audio: male reference 120.7 Hz -> output 121.4 Hz, female 231.8 Hz -> output 240.7 Hz. The measured reference F0s match the reviewer's independently-obtained values exactly, which cross-validates the method. T3, online: server resolves Vevo2ForCausalLM / Vevo2Adapter; the 400 and seed-reproducibility behaviours are pinned by tests rather than prose. Also records the suite results (offline 4 passed, online 5 passed, 59 CPU) and states the measurement tooling (whisper-small for WER, librosa.pyin for F0) so the numbers are reproducible. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
The Amphion pre-flight check was a bare YAML scalar containing a
colon-space inside its error message:
- test -d /opt/Amphion || { echo 'ERROR: /opt/Amphion not found; ...
YAML reads `key: value` there, so the list item parsed as a **dict**
rather than a string, and Buildkite's schema rejected the step:
$.steps[4].steps[5].commands[2] ... is not of type 'string'
Quoting the whole command makes it a single scalar again. Verified with
the real hook: `check_buildkite.py` reproduces the error above on the
unquoted form and exits 0 on the quoted one, and a sweep of every
`.buildkite/**/*.y[a]ml` for non-string `commands[]` entries now reports
none.
This was never a regression -- the line is byte-identical since the lane
was written. It only became visible when main gained the `check-buildkite`
hook, because Actions lints the PR merged with main. The guard therefore
never actually ran, so the "fails loudly if Amphion is missing" behaviour
it was supposed to provide starts working for the first time here.
Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
`check-torch-cuda-call` (new on main) flags the `torch.cuda.manual_seed_all(seed)` in `_seeded_rng`, and it is right to: the call is both redundant and CUDA-only. `torch.manual_seed` already reseeds every initialised accelerator -- `torch.random._manual_seed_impl` forwards to `torch.cuda.manual_seed_all`, `torch.mps.manual_seed`, `torch.xpu.manual_seed_all` and `_seed_custom_device` before seeding the default generator. So the explicit CUDA call added nothing on CUDA and pinned the path to CUDA everywhere else. The hook suggests `torch.Generator(device=...).manual_seed(...)`, which does not apply here: Amphion's `inference_ar_and_fm` samples from the global RNG and accepts no generator argument, which is why this manager seeds the process at all. Deleting the redundant line satisfies the hook's actual intent (no device-specific API) without changing behaviour. The save/restore pair is kept as-is -- `torch.manual_seed` perturbs device RNG, so the CUDA state still has to be restored for the seeding to stay invisible to sibling requests in the batch. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
`Simple · Engine&Entrypoints Test` went red on buildkite/vllm-omni #13967:
FAILED tests/entrypoints/openai_api/test_tts_detection.py::test_pipeline_stage_list_is_complete
AssertionError: stage keys claimed by adapters but absent from _PIPELINE_STAGES: ['vevo2']
`Vevo2Adapter` declares `stage_keys = frozenset({"vevo2"})`, so
`all_tts_stage_keys()` includes `vevo2`, but the `_PIPELINE_STAGES`
parametrization list was never extended. The guard exists precisely to catch
a new adapter whose stage key is detected but never exercised, so it is doing
its job -- `vevo2` just has to join the list.
Confirmed `vevo2` was the only missing key rather than patching blind:
diffing `all_tts_stage_keys()` (24 keys) against the list reports exactly
`['vevo2']`.
Verified:
- the failing test alone: 1 failed before, passes after
- the whole file: 114 passed
- the full CI step selection, `pytest tests/entrypoints tests/engine
-m 'core_model and cpu'`: 6 failed / 1991 passed, and the same selection on
this branch's base (3781214) gives an identical 6-failure set / 1985 passed
-- so zero regressions, and the six are pre-existing macOS-local failures
(platform-default stage-config resolution) that do not reproduce in CI.
Neither my Vevo2-scoped sweep nor the reviewer's caught this: the guard only
fires when a new adapter lands, which model-scoped testing cannot see.
Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
main finished moving per-model helpers out of serving_speech.py (vllm-project#6697, RFC vllm-project#4327), and non-streaming delta accumulation is now driven by OutputPolicy.accumulate_nonstreaming rather than a hardcoded model-type check. The rebase takes main's serving_speech.py unchanged, so: - Vevo2Adapter.validate()/build() now own what _validate_vevo2_request / _build_vevo2_params did on the server. - build() returns OutputPolicy(accumulate_nonstreaming=True), replacing the ("moss_tts_nano", "vevo2") entry in the old hardcoded check. Without it the non-streaming response keeps only the final, empty sentinel chunk. - _resolve_ref_audio now returns (wav, sr, cache_key); unpack all three. The new adapter tests mock the server with spec=OmniOpenAIServingSpeech, so a call into a server helper that no longer exists fails in the test. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
main gained the check-spdx-header and markdownlint-cli2 hooks (vllm-project#6273) after this branch's files were written. They are skipped in CI but run locally on changed files, so bring the Vevo2 files in line: add the vLLM-Omni SPDX copyright line, blank lines around headings and lists, compact table separators, a language on the recipe's result fence, and join adjacent blockquotes. No content changes. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
6ffdaab to
538c23f
Compare
|
@linyueqian: rebased onto current How the conflicts were resolved
The two new adapter tests mock the server with READMEs, and the trap. In both conflicts, the branch side was only the Vevo2 section: 30 and 69 lines, exactly what the feature commit adds. Main's new Breeze-TTS-2 section is kept, and Vevo2 goes after it. To rule out last round's trap properly, I compared the old PR patch (
Also:
|
| selection | this branch | clean main |
|---|---|---|
Vevo2 unit + test_tts_adapter.py + test_tts_detection.py + tests/docs/ |
227 passed (33 + 70 + 117 + 7) | n/a |
tests/entrypoints tests/engine -m 'core_model and cpu' (the Simple · Engine&Entrypoints selection) |
1 failed / 3399 passed | same 1 failed / 3391 passed |
tests/model_executor -m 'core_model and cpu' |
32 failed+errors / 2691 passed | same 32 / 2658 passed |
rest of tests/ -m 'core_model and cpu' |
224 failed+errors / 4078 passed | same 224 / 4075 passed |
The failure sets are identical on both sides, test by test, so none of them come from this PR. The one in the gate selection, test_legacy_lock_cannot_break_a_live_shared_holder, spawns /bin/true, which macOS does not have. The rest are local-environment failures present on clean main too. With VEVO2_MODEL_PATH set, -m "tts and cuda" --run-level full_model still collects all 9 Vevo2 e2e tests, so the lane is not empty.
SKIP=<workflow list> pre-commit run --all-files is clean. The local-only hooks are clean on the changed files, with two exceptions: mypy-3.10 flags torch.Tensor annotations in the Vevo2 unit tests (the torch = pytest.importorskip("torch") pattern that 8 other test files on main share), and shellcheck is not installed here. Both hooks are in the workflow's SKIP list.
Not run: the GPU e2e suites. I have no GPU here.
Before you cycle ready: the Vevo2 step will not be scheduled
Cycling the label gets the gate lane, but not TTS · Vevo2 · L4. That step lives in :card_index_dividers: E2E Tests, which is gated if: build.env("NON_CRITICAL") == "1". Reliability and Perf also accept weekly-test; this group does not. Checked two ways:
- Build 14483 ([Bugfix] Fix MiniMax Music 3 output validation #6879,
weekly-test, 08-31) uploaded the weekly pipeline and ran Reliability and Perf, but every E2E-group step wasignored, includingTTS · Gepard-1.0 · L4. - For this PR's diff,
upload_pipeline.py's own_render_test_pipelinekeeps that gated group plus two unrelated weekly jobs:Reliability Test · Invalid parameters · L4(matched viatts_adapters/in theqwen3_ttspath anchor) andPerf Test · Async Chunk · Randomon 2×H100 (it has no source filter).
So, as I read it, the Vevo2 lane needs either a manual build of this branch with NON_CRITICAL=1, which runs the whole E2E group (38 steps), or a lane placement a weekly-test PR can reach. I can push the second if you want it, but it is a lane-policy call, so I did not do it unasked. Either way, could you cycle ready for the gate lane?
The other recorded thread, the forward() raise, stays as we agreed on 08-22: raise plus the documented trade-off, since a per-request error channel is framework work.
Omni ReviewBot routing recordAssigned Direct under experiment |
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
This PR adds Vevo2 (Amphion AR + flow-matching TTS) as a single-stage Omni model that wraps upstream Vevo2InferencePipeline.inference_ar_and_fm instead of splitting AR/FM/vocoder into native stages. Online serving goes through a new Vevo2Adapter that requires ref_audio, copies request seed/sampling knobs into additional_information, and sets OutputPolicy(accumulate_nonstreaming=True) so non-streaming responses keep the waveform rather than the empty sentinel. Operators get a deploy YAML, checkpoint init helper, offline/online examples, recipe/docs license callouts, unit/e2e tests, and a dedicated weekly Buildkite step.
Change flow
flowchart TD
A["[EXISTING] /v1/audio/speech or Omni.generate"]:::existing --> B["[NEW] Vevo2Adapter validate/build<br/>seed + ref_audio → additional_information"]:::new
A --> C["[CHANGED] pipeline_registry + model registry<br/>model_type=vevo2"]:::changed
B --> D["[NEW] Vevo2ForCausalLM<br/>_seeded_rng + Amphion inference_ar_and_fm"]:::new
C --> D
D --> E["[EXISTING] waveform multimodal_output<br/>EOS via compute_logits"]:::existing
D --> F["[CHANGED] TTS · Vevo2 · L4 weekly step<br/>init checkpoint + e2e pytest"]:::changed
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
CI at
538c23faa96c(2026-09-30T23:10:51.799749+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Findings
- [P1] Vevo2 weekly lane is gated only on NON_CRITICAL, not weekly-test —
.buildkite/cuda/test-weekly.yml:113
TTS · Vevo2 · L4is nested under the:card_index_dividers: E2E Testsgroup whoseifis solelybuild.env("NON_CRITICAL") == "1"(test-weekly.yml:111-113). Sibling Reliability/Perf groups explicitly allowbuild.pull_request.labels includes "weekly-test", but this E2E group does not. PR discussion plans to merge after cycling theweekly-testlabel to get this lane’s first CI run; with the current gate, that label uploads the weekly pipeline but still skips the Vevo2 step on a PR. Move the step into a group whoseifincludesweekly-test(same pattern as Reliability/Perf), or broaden this group’s condition so the intended PR verification path can actually schedule the Amphion/init/e2e command.
Existing thread: #3422 (comment)
🤖 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!
Omni ReviewBot: finding feedback[p1] Vevo2 weekly lane is gated only on NON_CRITICAL, not weekly-test —
This finding appears in the bot's COMMENT review, but GitHub could not place it as an inline diff comment. If you are the PR author and disagree, react 👎 to this comment. The disagreement will be shown to the maintainer; it does not approve or merge the PR. |
Omni ReviewBot: no human activity for 13 days@yashkgp this pull request has had no human commit, comment or review since 2026-09-24. 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. |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Reviewed commit 538c23faa96c21b894955ef6ca2418e3fa7a0ca6.
Found two actionable issues in the Vevo2 integration and documented serving workflow.
[P2] Resolve checkpoint paths before changing the working directory
vllm_omni/model_executor/models/vevo2/modeling_vevo2.py:446
With the documented relative model path ./ckpts/Vevo2, the component paths constructed from self.model_path remain relative. Their existence checks run in the original directory, but the pipeline constructor receives them after this chdir to the Amphion checkout. They consequently resolve under Amphion instead of the directory containing the downloaded checkpoint, preventing model startup. Resolve the checkpoint root to an absolute path before building component paths and switching directories.
[P2] Match the client model name to the bundled server
examples/online_serving/text_to_speech/vevo2/openai_speech_client.py:67
The client defaults to sending model="vevo2", whereas run_server.sh serves ./ckpts/Vevo2 without a --served-model-name alias. The API server defaults its advertised names to args.model, and create_speech calls _check_model before generation. Thus the documented launcher/client combination returns a model-not-found error instead of audio. Add the matching served-model alias to the launcher or align the client's default with the server's advertised name.
Validation: Statically inspected the exact base-to-head diff, model implementation, configuration and registration, serving adapter and callers, worker request lifecycle, examples, CI configuration, and relevant unit and end-to-end tests. Rechecked findings against surrounding code. Runtime/GPU tests were not run; no repository code was executed.
|
Thank you for the Vevo2 integration and validation work. We have decided not to proceed with this proposal in its current form and are closing it as a maintainer decision. A future proposal could be reconsidered with a clear current integration need and supporting validation. |
5 participants
Purpose
Closes #3391. Integrates Vevo2 — Amphion's TASLP-accepted unified controllable AR + flow-matching framework for speech and singing voice generation — into vLLM-Omni.
cc @linyueqian per request in the issue thread.
MVP scope
async_chunk: false).Deferred to follow-up PRs (each tracked separately against #3391):
async_chunk: truestreamingar_model.generate())inference_ar_and_fm(Whisper LayerNorm, content-style tokenizer Linear). Re-measurement on an L40S showed no speed penalty from the cast (~1.7 to 2.6 s/request), so this is a ~1 GiB memory optimization rather than a speed one, and dropping it is a follow-up.Architecture choice — single-stage AR worker
The reference pipeline is invoked through a single Python call (
Vevo2InferencePipeline.inference_ar_and_fm). Rather than reimplement the AR + FM + Vocos stack as native vLLM stages — which would require vendoring ~13 Amphion files (no PyPI distribution) — this PR wraps the upstream pipeline in a single AR worker stage following the MOSS-TTS-Nano generator pattern: per-request_create_stream_genyields the full waveform once + an empty sentinel;compute_logits()flips EOS.This keeps the integration small and reviewable while leaving the door open for a Phase 2 PR that splits AR onto PagedAttention.
License posture
RMSnow/Vevo2checkpoint: CC BY-NC-ND 4.0 (non-commercial, no-derivatives).The license is called out explicitly in the deploy YAML, both example READMEs, the speech_api docs, the offline
end2end.pyheader, and the onlinerun_server.sh.Files
vllm_omni/model_executor/models/vevo2/configuration_vevo2.pyvllm_omni/model_executor/models/vevo2/modeling_vevo2.pyvllm_omni/model_executor/models/vevo2/pipeline.pyvllm_omni/deploy/vevo2.yamlvllm_omni/transformers_utils/configs/vevo2.pyvllm_omni/model_executor/models/registry.py,vllm_omni/config/pipeline_registry.pyvllm_omni/entrypoints/openai/serving_speech.pyexamples/offline_inference/text_to_speech/vevo2/{end2end.py,init_vevo2_checkpoint.py}+ README sectionexamples/online_serving/text_to_speech/vevo2/{run_server.sh,openai_speech_client.py}+ README sectiontests/model_executor/models/vevo2/test_vevo2_unit.pytests/e2e/offline_inference/test_vevo2.pytests/e2e/online_serving/test_vevo2.py.buildkite/test-merge.yml(Vevo2 E2E Testlane)docs/serving/speech_api.mdpyproject.toml(allowNDso typos hook stops "fixing" CC BY-NC-ND)Cross-cutting TTS-guide invariants
Verified per
docs/contributing/model/adding_tts_model.md§I1–I5:if x is Noneand defensively concatenatelist[Tensor].item()/.cpu()/.tolist()inside per-step loops_stream_genskeyed by request id withon_requests_finishedcleanupTest Plan
Local (CPU dev box)
pre-commit run --files <all changed files>passes (ruff, typos, suggestion, debug-statements, line-ending fixers)GPU (re-verified on an L40S 48 GB; transformers 5.14.1, vLLM 0.26.0)
Full run after the review fixes. The integration now executes end to end under transformers 5 via the import-time
LlamaConfigshim.Vevo2InferencePipelineloads (AR 508.93M, Flow-Matching 362.87M, CocoStyle 44.18M, CocoContentStyle 38.01M, Vocoder 255.04M)tests/e2e/offline_inference/test_vevo2.py): 3/3 passed (English, Chinese, batch isolation); real non-silent 24 kHz audio, ~2 s/request/v1/audio/speechgolden path (tests/e2e/online_serving/test_vevo2.py): 3/3 passed (English, Chinese, no-ref-text); Whisper transcripts match the input textref_audiorejected: server returns 400Iteration history during GPU validation
The integration uncovered several real upstream environment issues, each fixed and committed:
huggingface-cli downloaddeprecated in HF Hub 1.0+hf download(test-script only)config.jsonin upstream checkpoint →Could not determine model_typeinit_vevo2_checkpoint.pywrites onemodel_type=vevo2AutoConfigIPython.displayat module level (not in their reqs)llama_nar.pycallsLlamaConfig(0,256,1024,1,1)— transformers ≥5 forbids positional argstransformers<5pinpyworld/json5missing from upstreamrequirements.txtmodels/svc/vevosing/config/whisper_stats.ptchdirto Amphion root inload_weightsCannot find any model weights with <path>init_vevo2_checkpoint.pywrites placeholdermodel.safetensorsAutoTokenizer.from_pretrained(<root>)raisesKeyError: 'Vevo2Config'expected scalar type Float but found BFloat16ar_modelto fp32whisper_modelto fp32mat1 and mat2 must have the same dtypeEach commit fixes a real-environment issue any user reproducing the upstream pipeline would hit; the PR's setup-doc + import-shim story is materially more polished than the upstream baseline.
CI
Vevo2 E2E Testlane (advanced_model marker)Test Result
Re-ran everything end to end on a single NVIDIA A10G 24 GB (
g5.2xlarge), vLLM 0.27.1, transformers 5.15.1, torch 2.13.0+cu130, Amphion @ main onPYTHONPATH, checkpoint prepared with this PR'sinit_vevo2_checkpoint.py.tests/e2e/offline_inference/test_vevo2.pytests/e2e/online_serving/test_vevo2.pyend2end.py(documented command)--ref-audio)Vevo2InferencePipeline loaded, 290 weight shards, ~18.8 GiB GPUResolved architecture: Vevo2ForCausalLM/Resolved TTS serving adapter: Vevo2AdapterWhat this pins, specifically:
end2end.pyand the offline suite complete instead of raisingAttributeErrorat the consumer. The offline suite went 3 FAIL → 4 PASS (4, not 3, because the old batch test is now split into isolation + reproducibility).test_vevo2_same_seed_is_reproduciblepasses: two identical seeded/v1/audio/speechrequests return byte-identical audio, a different seed changes it. Previously the online path ignoredseedentirely.LlamaConfigshim — the real Amphion pipeline constructs under transformers 5.15.1 with the shim scoped to a context manager, so the scoping change did not regress the transformers-5 fix.init_vevo2_checkpoint.py— exercised on the real checkpoint: fresh init, idempotent re-run, and repair of the exact half-initialised state @linyueqian described.1.5xlength margin intest_vevo2_batch_isolation, which I had flagged as reasoned-not-measured, holds against real durations.Recipe verification numbers in
recipes/RMSnow/Vevo2.mdare now my own, from the documented commands on this hardware, replacing the figures I had wrongly presented as coming fromend2end.py. My measured reference F0s (120.7 / 231.8 Hz) match @linyueqian's exactly, which cross-validates the method.One honest note: the box needed
VLLM_USE_FLASHINFER_SAMPLER=0because the Deep Learning AMI I used ships the driver withoutnvcc, so flashinfer could not JIT its sampling kernel. That is an environment artifact of my test box, not a Vevo2 issue — the CI image has a toolchain.Adding the
weekly-testlabel would still be worth doing for a run on the project's own CI image.🤖 Generated with Claude Code