Repository navigation
Conversation
…nner The eight audio_mrv2_* and audio_async_* switches become tri-state config fields resolved once at model init; model_runner: v2 no longer needs the H200 profile's hf_overrides block. The external decode graph decision moves from the model state into the talker's __init__, so the MLP graph disarm and the mutual-exclusion assert run on every path; before, v2 with enforce_eager: true flipped the flag after init and skipped the disarm. A declarative mrv2_unsupported_sampling_params on StagePipelineConfig fails non-neutral unsupported default_sampling_params at startup and names the v1 opt-out. Signed-off-by: Mr.Dong <1042542469@qq.com>
The default deploy config selects model_runner: v2 with the H200-validated stage shapes scaled to 16 sequences, and NPU/XPU/ROCm/MUSA pin v1 through the platforms section with the shipped V1 stage-0 shape restored. The connector streaming geometry follows the codec graph windows: left context 8, no holdback, and an 8-frame first chunk where V1 shipped one frame. The e2e suite gains the V1 opt-out and H200 params plus streaming-abort and long-input tests; the dfx entry gains a c=4 latency point. Signed-off-by: Mr.Dong <1042542469@qq.com>
Add the Higgs-Audio V3 MRV2 section to the stage configuration guide, restructure the deploy profile README around the V2 default and the V1 opt-outs, and update the recipe's deploy notes. Signed-off-by: Mr.Dong <1042542469@qq.com>
Shorten over-verbose MRV2 comments in the talker, pipeline, and config; restore the deploy README's profile structure and the default yaml's connector comments, keeping the MRV2 notes proportional. Signed-off-by: Mr.Dong <1042542469@qq.com>
| key | ||
| for key in sorted(set(params) & set(ps.mrv2_unsupported_sampling_params)) | ||
| if key not in _MRV2_NEUTRAL_SAMPLING_VALUES or params[key] != _MRV2_NEUTRAL_SAMPLING_VALUES[key] | ||
| ] |
There was a problem hiding this comment.
Explicitly neutral defaults such as logprobs: null, prompt_logprobs: null, allowed_token_ids: null, bad_words: [], and logit_bias: {} are rejected at startup, although this PR’s request path accepts them. Complete the neutral-value handling or share normalized validation between startup and requests.
There was a problem hiding this comment.
Fixed in c195ebb: startup validation now passes None/[]/{} for those keys, matching the request path. Non-neutral values still fail with the v1 opt-out named.
Empty values (None, [], {}) for None/collection sampling keys now pass
startup validation like the request path does; scalar keys keep exact
engine-default matching. Addresses the review on the falsy-neutral keys.
Signed-off-by: Mr.Dong <1042542469@qq.com>
The streaming-abort case now passes voice=None explicitly: the raw SDK call requires the argument on this code path, and the profile default must not paper over it. The long-input case pins max_new_tokens to 1900 so every param generates under the same cap (the V1 opt-out profile ships a lower Stage-0 default), and lowers the byte floor to 1 MB: under dummy weights audio-eos timing is weight-dependent, so the floor proves the long input completes rather than hitting a fixed duration. Signed-off-by: Mr.Dong <1042542469@qq.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
resolve_mrv2_defaults silently resolved every explicitly set MRV2 switch to False under model_runner: v1, while the field comment claimed an explicit bool overrides. Under v1 an explicitly set switch now logs a warning naming the ignored switches, the comment and docstring state the real semantics (explicit bools override under v2 only), and a regression test covers the v1 warning plus v2 honoring the same values without one. Shipped profiles are unaffected: the V1 opt-outs set no switches and the H200 profile runs v2. Signed-off-by: Mr.Dong <1042542469@qq.com>
Every Higgs profile ships the same Stage-0 max_tokens cap (2048); the comment claimed the V1 opt-out shipped a lower one. State the actual reason for pinning max_new_tokens to 1900: normalize below the shared 2048 cap across profile params. Signed-off-by: Mr.Dong <1042542469@qq.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the detailed parity plan. Making the eight MRV2 switches tri-state and resolving them once into a frozen HiggsMrv2Options is a nice cleanup: the model code no longer reads raw config attributes, explicit False on the two structural flags fails loudly under v2, and v1 warns instead of silently ignoring set switches. Moving the external-decode-graph decision into the talker __init__ also closes the enforce_eager: true + v2 ordering gap you describe.
A few things worth a look:
-
Unrelated
.claude/skills/review-pr/*changes. Four files under.claude/skills/review-pr/("smallest fix direction" to "root-cause fix direction") are part of this diff. They don't relate to Higgs v3 / MRV2; could you split them into their own PR so this one stays reviewable as a model-runner change? -
Connector geometry is not restored for the V1 platform fallbacks. The
platforms:overrides put ROCm/NPU/XPU/MUSA back onmodel_runner: v1and restore the stage-0 V1 shape (eager, no async scheduling, prefix caching), and the comment says the fallback keeps "the shipped V1 shape". But the top-levelconnector_of_shared_memorychanges (codec_left_context_frames: 25 -> 8,codec_right_holdback_frames: 4 -> 0,initial_codec_chunk_frames: 1 -> 8) are not platform-scoped, so those V1 fallbacks now also stream with the V2 codec geometry. If that's intended, the comment should say so and the ROCm pool run should cover audio quality with it; if not, those keys need a per-platform restore (or a note ifplatforms:can't override connectors). -
model_runner: v1on the default file is an untested combination. Someone who keepshiggs_multimodal_qwen3.yamlbut overridesmodel_runner: v1(CLI or a copied file, which the header suggests) getsenforce_eager: false,async_scheduling: true,enable_prefix_caching: false,FLASH_ATTN, and the V2 codec geometry on the V1 runner. Since every MRV2 switch resolves off under v1, isasync_scheduling: truesupported by the V1 Higgs path? A startup check, or a test that boots the default yaml withmodel_runner: v1, would catch a regression here. -
Startup sampling check only covers deploy defaults.
_validate_mrv2_default_sampling_paramschecksdefault_sampling_paramsat deploy-merge time, andadd_requestrejects per-request values, which is good. One gap: the neutral-value table only has scalar entries, so a default likelogit_bias: {}orbad_words: []passes (fine), butlogprobs: 0would be rejected even though it requests no logprobs on most paths. If that's intentional, a comment would help; otherwise addlogprobs/prompt_logprobswith their neutral value to_MRV2_NEUTRAL_SAMPLING_VALUES. -
Minor: stage-0
compilation_config(cudagraph_mode: FULL, capture sizes up to 16) and stage-1hf_overrides(codec_graph_*) are inherited by every platform fallback. Withenforce_eager: trueand v1 they should be ignored, but a quick note that they're CUDA/V2-only would save the next reader from wondering.
The TTFA change for the first chunk (about 320 ms vs about 40 ms on the V1 profiles) is called out clearly in the description; I'll leave whether that's the right default to the maintainers tracking #9387.
b9cae81 to
8b48027
Compare
logprobs=0 is not neutral under the completions-API semantics vLLM implements: it still requests the sampled token's logprob, which the v2 sampler never produces. The per-request check in the Higgs model state rejects any explicit value for the same reason, so the startup table matches the request path; state that in the table comment. Signed-off-by: Mr.Dong <1042542469@qq.com>
Flipping model_runner back to v1 on the default yaml leaves async_scheduling: true in place (the v2 requirement), a combination no shipped V1 profile exercises. Warn at init that the V1 profiles run with async_scheduling: false; the platform fallbacks set it explicitly and stay silent. Signed-off-by: Mr.Dong <1042542469@qq.com>
The fallback comment claimed the shipped V1 shape without qualifying what the fallback still inherits: stage-0 compilation_config and the stage-1 codec graphs are CUDA/v2-only and inert under eager v1, and connector settings cannot be scoped per platform, so the fallbacks stream with the v2 chunk shape. Say so where the claim is made. Signed-off-by: Mr.Dong <1042542469@qq.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Took a look at the three new commits since my last pass (7e7c4242, c928ba1e, 16accccf). They're small and mostly documentation; a couple of notes:
higgs_audio_v3_talker.py– new v1 +async_schedulingwarning. Makes sense as a guardrail. Since it lives in the talker__init__, it will be emitted once per worker/rank (and again on every re-instantiation), so with TP > 1 the log gets noisy.logger.warning_once(...)(available on vLLM'sinit_loggerloggers) would keep the signal without the repetition. It might also help to say what the user should do (e.g. "setasync_scheduling: falseor usemodel_runner: v2") rather than only that the combination isn't exercised.stage_config.py–logprobs/prompt_logprobsrationale. The explanation for keeping them out of_MRV2_NEUTRAL_SAMPLING_VALUESis clear and matches the request-path behaviour fromc195ebb2. If there isn't one already, a tiny unit test asserting thatlogprobs: 0is still rejected at startup (whilelogprobs: nullpasses) would lock this intent in, so a future "helpful" addition of0to the neutral table gets caught.higgs_multimodal_qwen3.yaml– fallback comment. Good to call out that stage-0compilation_configand stage-1codec_graph_*are inert on the v1 fallbacks. Minor: the last sentence ("the fallbacks stream with the v2 chunk shape above") could mention whether that chunk shape has been sanity-checked on the V1 runner, since that's the one setting that does carry over.
No functional concerns with the rest of this increment.
The platforms merge only handled cuda_mps, model_runner and stages, so a connector key was silently ignored and a V1 fallback platform inherited the default file's connector geometry, which for Higgs v3 is coupled to the v2 codec CUDA-graph windows. Platform sections now accept a connectors mapping, deep-merged into the named connector and rejected when the name is not defined; the Higgs fallback platforms restore the shipped V1 streaming geometry. Signed-off-by: Mr.Dong <1042542469@qq.com>
Say the two facts (0 still requests a logprob; the request path rejects any explicit value) without the semantics lecture. Signed-off-by: Mr.Dong <1042542469@qq.com>
Signed-off-by: Mr.Dong <1042542469@qq.com>
Signed-off-by: Mr.Dong <1042542469@qq.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the follow-ups. I went through the four new commits since my last pass (346e325d, 0fbc9b38, 97ede951, 1278c535). The warning_once change, the clearer warning text, and the new logprobs: 0 startup test all look good. A few notes on the new per-platform connectors override:
_apply_platform_overridesand non-mapping overrides. The outerconnectorsvalue is checked to be a dict, but each per-connectoroverrideisn't. A YAML typo such asconnector_of_shared_memory: nullor a scalar would fail inside_get_recursively_merged_dictwith anAttributeError(.items()onNone), not a clearValueError. Adding anisinstance(override, dict)check next to the unknown-name check would keep the error messages consistent.- Interaction with
base_configoverlays._merge_platformsmergesstagesbystage_id, but every other platform key, now includingconnectors, is replaced shallowly ({**bp, **op}). If an overlay deploy file setsplatforms.<plat>.connectorsfor just one key, the base file's full V1 geometry restore for that platform is dropped, not merged. That may be fine for now since nothing inherits from this file yet, but it's worth either deep-mergingconnectorsthere too or noting the shallow behaviour. A related point: the platform restore runs after top-level connectors are merged, so an overlay that changes top-levelcodec_*frames would be silently overridden on the non-CUDA fallbacks by the hard-coded25/4/1. - YAML duplication. The same three-key connector block now appears four times (rocm/npu/xpu/musa). A YAML anchor (
&v1_connector_geometry/*v1_connector_geometry) would keep them from drifting apart if the V1 geometry ever changes. - Test coverage. The new assertions cover the restore and the unknown-name error. A small check that
_apply_platform_overridesleaves other connectorextrakeys intact (to prove it really is a deep merge, not a replace) would protect the merge semantics.
No functional concerns with the docs/comment trim or the talker warning change.
|
Thanks @amy-why-3459 — everything from both passes is in:
|
Reject a non-mapping per-connector override with a clear ValueError instead of an AttributeError from the merge helper, deep-merge connectors in _merge_platforms so a base_config overlay adds to (not replaces) a platform's connector restores, anchor the repeated V1 geometry block in the Higgs yaml, and cover the deep merge and the mapping check in tests. Signed-off-by: Mr.Dong <1042542469@qq.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. I looked only at the new commit since my last pass (9f92aa02). It covers the earlier notes well: the per-connector isinstance(override, dict) check gives a clear ValueError, _merge_platforms now deep-merges connectors by name through _merge_connectors, the YAML anchor removes the four copies, and the new tests check both the deep-merge semantics and the mapping error.
A few small notes:
- Clearing connectors from an overlay. Because
_merge_platformsnow always deep-merges, an overlay that writesplatforms.<plat>.connectors: null(or{}) keeps the base file's connector restores instead of removing them. That's probably the right default, but there's no longer a way for an overlay to opt out of the inherited V1 geometry for a platform. A short comment on_merge_platformssaying this is intended would help future readers. - Overlay that nulls a single connector. If an overlay sets
connector_of_shared_memory: nullunder a platform,_get_recursively_merged_dictreplaces the base mapping withNone, and the new check in_apply_platform_overridesthen raisesmust be a mapping. The error is clear, but it points at the merged result, not at the overlay file that caused it. Not a blocker, just worth knowing. - Anchor aliasing. npu, xpu, and musa now alias the rocm mapping. That's safe as long as the loader (
load_yaml_config+to_dict) returns independent containers per alias and nothing mutates the platformconnectorsdict in place._get_recursively_merged_dictcopies, so this looks fine today. A one-line assertion in the test thatrocmandxpurestores are distinct objects (or that mutating one doesn't affect the other) would guard against a future in-place edit. - Still open from the last pass. The non-CUDA restores still hard-code
25/4/1, so an overlay that changes the top-levelcodec_*frames is silently overridden on rocm/npu/xpu/musa. Fine to leave as is if that's intended, but a comment next to&v1_connector_geometrynoting it would make the precedence explicit.
…endence State in _merge_platforms that an overlay cannot clear a platform's inherited connector restores, note on the yaml anchor that the shipped V1 geometry wins over top-level codec_* on the fallback platforms, and assert that the anchor resolves to independent containers whose merged results cannot leak across platforms. Signed-off-by: Mr.Dong <1042542469@qq.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked only at the new commit since my last pass (46bcf19c). It picks up the remaining notes: the _merge_platforms docstring now states that an overlay can't clear inherited connector restores, the comment next to &v1_connector_geometry makes the precedence over top-level codec_* explicit, and the test now asserts that the anchor resolves to distinct containers.
One small note on the new test:
- The mutation check doesn't exercise aliasing.
rocmandxpucome from two separateload_deploy_config(deploy_path)calls, so they are independent objects no matter how the anchor is resolved, and the== 25assertion would pass even if the loader returned shared containers. Theraw[...] is not raw[...]line above it is the part that actually guards the anchor. If you want the mutation check to add coverage, mutate the singlerawdict instead (e.g. changeraw["platforms"]["rocm"]["connectors"]["connector_of_shared_memory"]["extra"]["codec_left_context_frames"]and assert thexpuentry is unchanged), so a shared container would show up as a leak. Not a blocker.
Nothing else from me on this delta.
Loading the config twice made the two restores independent objects by construction, so the mutation half of the check could not fail on shared alias containers. Mutate the rocm entry of a single parse and assert the xpu entry of the same parse is unchanged. Signed-off-by: Mr.Dong <1042542469@qq.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Looked only at the new commit since my last pass (bf5d3b0b). This resolves the note on the anchor test: the mutation now goes through a single resolve_deploy_yaml parse, so if the &v1_connector_geometry anchor ever resolved to a shared container, writing codec_left_context_frames = 99 on the rocm entry would show up on the xpu entry and the == 25 assertion would fail. Mutating raw is also safe for the rest of the test, since resolve_deploy_yaml re-reads the YAML on every call (no caching), so the later load_deploy_config(deploy_path) for cuda still sees the original values.
Nothing else from me on this delta.
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Higgs Audio v3 runs on Model Runner V2 by default and passes the #9387 parity bar.
2. What is the highest-leverage variable for that outcome?
Whether V2 matches V1 on quality and performance for this model, measured on the same vLLM build.
3. How will this work move that variable?
Upstream vLLM may drop Model Runner V1 (vllm-project/vllm#63373). Higgs v3 already had an MRV2 path, but it was reachable only through eight yaml-only flags in the H200 profile. This PR makes
model_runner: v2self-sufficient, flips the default profile, and compares V2 with V1.4. Whose customer or user needs does this address, and what are they?
People who serve
bosonai/higgs-audio-v3-tts-4band need to stay on a vLLM-Omni that drops V1. They need v3 to run on V2 the way it runs on V1, with a V1 opt-out while V1 still exists.Purpose
Upstream vLLM plans to remove Model Runner V1 (vllm-project/vllm#63373). Issue #9387 tracks the core TTS models. Its table lists Higgs Audio as a v3 opt-in profile with no v2-generation MRV2 path. This PR promotes v3's MRV2 path from the opt-in
higgs_multimodal_qwen3_mrv2_h200.yamlto the shipped default. The v2-generation port is staged separately and follows the codec-stage ruling in #9387.Before, the default and both perf profiles ran v1, and a bare
model_runner: v2on the default yaml raised at startup. After, the MRV2 feature switches self-enable from the runner selection, the default yaml selects v2, andhiggs_multimodal_qwen3_high_throughput.yamlandhiggs_multimodal_qwen3_low_latency.yamlremain v1 as the opt-out.Changes:
The eight
audio_mrv2_*andaudio_async_*switches become tri-state config fields that default to the model runner selection.model_runner: v2on the default yaml now works without the H200 profile'shf_overridesblock.The external decode graph decision moves from the model state into the talker's
__init__. This also fixes a latent case where v2 withenforce_eager: trueskipped the MLP graph disarm because the flag was set after init.deploy/higgs_multimodal_qwen3.yamlselects v2 with the H200-validated stage shapes scaled to 16 sequences. The connector streaming geometry follows the codec graph windows, so the first chunk is now one 8-frame window, about 320 ms, where V1 shipped about 40 ms. Steady-state cadence is unchanged.NPU, XPU, ROCm and MUSA pin v1 through the
platforms:section. XPU rejects v2 stages at deploy load, and NPU generation workers reject v2 at init.The v2 sampler supports temperature, top_p, top_k and seed. Other sampling parameters fail at startup when set non-neutrally in
default_sampling_params, and per request with a message naming the v1 opt-out. A follow-up issue will track penalties.Under
model_runner: v1, explicitly set MRV2 switches log a warning instead of being silently ignored, and the default profile'sasync_scheduling: true— a v2 requirement that no shipped V1 profile exercises — logs a warning naming the V1 profiles.The
platforms:section can scope connector settings; the Higgs fallback platforms restore the shipped V1 streaming geometry.Tests cover the runner matrix across platforms, the flipped profile shape, flag resolution, the startup fail-fast, and e2e under the default, the V1 opt-out and the H200 params, including a streaming abort and a long input. The dfx entry gains a c=4 latency point.
Docs:
docs/configuration/stage_configs.md,deploy/README_higgs_audio_v3.md, and the recipe.Test Plan
vLLM Version: 0.32.0
V1 is the parent commit with the old default yaml. V2 is this PR. Both run the same vLLM build.
seed-tts-text/en, one server per runner, same requests and seeds.tests/e2e/online_serving/test_higgs_audio_v3.pyunder the default and the V1 opt-out params. The ROCm pool runs the default yaml's plain-text param to exercise the v1 fallback.tests/dfx/reliability/invalid_param_test/test_invalid_audio_speech.pyboots the default yaml, so its 4xx expectations move to the v2 path with this PR and get re-validated in the same CI run.Test Result
Local CPU suites on this PR: 757 passed across
tests/model_executor/models/higgs_audio_v3/andtests/config/test_config_factory.py, and 15 passed across the worker, stage-input-processor and adapter tests.test_codec_snake_kernelfails the same way on main in this environment and is not touched here.Measured on 1x H20 96GB, matched A/B — same machine, same vllm 0.32.0 build, same datasets and seeds, no env overrides. V1 is the pre-flip checkout
4daeba8b4running its shipped default (eager); V2 is this PR atc195ebb(the commit after it touches tests only). The comparison is shipped default against shipped default. Each point below is one measurement per server run, two server runs per side; run-to-run spread is given where it matters.Quality — seed-tts-eval EN hard, 1088 voice_clone prompts at concurrency 4; WER by whisper-large-v3, speaker similarity by wavlm-base-plus, plus UTMOS. Zero failed evaluations in all four runs.
V1's own two WER runs differ by 0.0018, more than either cross-runner difference, so WER is within run-to-run variation in both runs. SIM and UTMOS repeat to the fourth decimal within a side, so the small cross-side differences are consistent rather than noise: V1 is higher on SIM by 0.0004–0.0006 and V2 is higher on UTMOS by 0.0005 — 0.03–0.07% of the value, inside the ±0.002 SIM band, with the floors (WER ≤ 0.05, SIM ≥ 0.75) holding on every run.
Performance — the dfx entry on seed-tts-text prompts; medians, min–max over the two runs per side:
median_audio_ttfp_ms)median_audio_rtf)Run-to-run spread on both sides stays within 3% on TTFA and 1.5% on RTF and throughput. TTFA reads V2's first chunk (8 codec frames, ~320 ms of audio) against V1's 1-frame chunk.
Memory — startup accounting, identical across both runs (default yaml, stage-0 util 0.6): weights 7.61 GiB on both sides; available KV 49.01 GiB on V1 and 48.73 GiB on V2 (−0.6%); consumed 7.81 vs 7.97 GiB; V2 adds a 0.06 GiB CUDA-graph pool. Under the dfx load matrix (util 0.3/0.2, c=1/4/16, 5 s sampling) peak device memory is 37.2 GiB on V1 and 31.1 GiB on V2.
Streaming behavior — the e2e suite under dummy weights, shared matrix: 43/43 on the V2 branch across
plain_text(V2 default),v1_optout_high_throughput(V1 escape hatch) andmrv2_h200(tuned profile); 13/13 on the v1 baseline checkout.Not run:
Not changed:
_high_throughput.yamland_low_latency.yaml.