Repository navigation
[Migrate] Move sampling parameter overrides from legacy dispatches to TTS adapters - #5272
Conversation
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@linyueqian please feel free to take a look, thanks |
| TEXT_EOS_TOKEN_ID, | ||
| ) | ||
|
|
||
| hf_config = self.engine_client.model_config.hf_config |
There was a problem hiding this comment.
Any Ming-TTS request with the normal non-empty sampling list will crash here: TTSModelAdapter.__init__ only sets self.ctx, so this raises AttributeError when _prepare_speech_generation() invokes the override. Please use self.ctx.server.engine_client (or self.ctx.engine_client) and add a Ming override regression test; the current 199-test suite does not exercise this path.
There was a problem hiding this comment.
fixed.
before ddc2296:
1 failed, 199 passed, 17 warnings in 1.34s
after:
200 passed, 17 warnings in 1.44s
There was a problem hiding this comment.
Verified at head — self.ctx.server is in place and the regression test now asserts stop_token_ids == [TEXT_EOS_TOKEN_ID] and max_tokens == 8 for max_new_tokens=7. Thanks.
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
linyueqian
left a comment
There was a problem hiding this comment.
Reviewed the full diff against the pre-PR dispatch code. The migration looks faithful: the adapter registry names cover exactly the deleted _SAMPLING_MAX_TOKENS_TTS_MODEL_TYPES set plus the ming_tts/glm_tts branches, models that never had overrides keep the identity default, and since the extra_params merge only writes extra_args, moving the cosyvoice3/GLM blocks after it is behavior-neutral as the description says. Nice to see 144 lines of dispatch leave serving_speech.py.
A few non-blocking comments, mostly cleanup:
- Three adapters still carry "stays in the orchestrator tail" NOTEs that this PR makes false; see the inline comment on
cosyvoice3.py. - Two more stale spots in
serving_speech.pyare outside the diff so I could not anchor comments there: the comment at line 452 still references the deleted_apply_cosyvoice3_dynamic_tokens, and the "Sampling overrides ... remain in the orchestrator tail" wording in the adapter-resolution comment (~line 505) and the pre-build comment (~line 2999) no longer holds after this PR. - Seven adapters now carry an identical
max_new_tokens-only override (fish, qwen3, voxtral, higgs v2/v3, indextts2, voxcpm2). A small shared helper on the base class (keeping the base default as identity so unmigrated adapters are unaffected) would remove the copy-paste. Fine to defer if you prefer strict move-only migration PRs. - Tiny style nit: in
cosyvoice3.py::apply_sampling_overridesthe two explanatory comment lines sit above the docstring; folding them into the docstring reads better.
LGTM once the stale comments are tidied up.
| @@ -50,3 +55,60 @@ async def build( | |||
| # NOTE: CosyVoice3 dynamic-token sampling stays in the orchestrator tail | |||
There was a problem hiding this comment.
This NOTE is now stale: the dynamic-token sampling moves into this adapter's apply_sampling_overrides in this very PR. Same for the equivalent NOTEs at glm_tts.py:50 and ming_tts.py:62. Worth deleting all three here so the next reader is not sent looking for logic in the orchestrator tail that no longer exists.
| ) | ||
| asyncio.run(ming_tts_server._prepare_speech_generation(request)) | ||
|
|
||
| assert ming_tts_server._tts_model_type == "ming_tts" |
There was a problem hiding this comment.
Suggestion: also assert the override outcome so a logic regression fails, not just a crash. The real apply_sampling_overrides runs in this test (only build is mocked), so you can grab ming_tts_server.engine_client.generate.call_args.kwargs["sampling_params_list"] and check stop_token_ids == [TEXT_EOS_TOKEN_ID], plus a variant with max_new_tokens=N asserting max_tokens == N + 1. Now that the cosyvoice3/GLM logic lives in adapters it is unit-testable in isolation for the first time, so direct tests for those two would also be a natural follow-up.
There was a problem hiding this comment.
added. And yes, unit tests for the cosyvoice3 and glm-tts adapters should be added as a follow up.
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
|
Updated to the latest main after Audex merge and resolved the resulting conflicts, reran the tests locally with 204 passed. @linyueqian, could you please take a look? |
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
|
Update: While reviewing To keep this PR focused, the update only covers the sampling-parameter logic already within its scope and does not change any capability-related behavior or state. Tests:
|
| } | ||
| ) | ||
| cosyvoice3_server._apply_cosyvoice3_dynamic_tokens = mocker.MagicMock(side_effect=lambda spl, req: spl) | ||
| cosyvoice3_server._adapter.apply_sampling_overrides = mocker.MagicMock( |
There was a problem hiding this comment.
[P2] Can we keep the real CosyVoice3Adapter.apply_sampling_overrides here and assert the resulting min_tokens/max_tokens? Replacing it with an identity mock means this test only proves dispatch reaches a stub, so a regression in the newly migrated dynamic-token logic (including max_new_tokens capping) would still pass. A focused adapter test or an assertion on generate(...).sampling_params_list would close this.
There was a problem hiding this comment.
Added the real-path sampling_params tests for GLM-TTS and CosyVoice3.This was previously suggested as a follow-up but didn't land.
pytest -v tests/entrypoints/openai_api/test_serving_speech.py — 205 passed
There was a problem hiding this comment.
Confirmed — the CosyVoice3/GLM tests now run the real apply_sampling_overrides (only prompt building and tokenizer internals are stubbed) and assert the resulting min_tokens/max_tokens. Closes this for me.
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
…pter models, remove related check/test/pre-commit hooks Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
|
@linyueqian, Update e0ee6a5: Removed Test: 353 passed |
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
|
Sorry — the conflict here is mine. #6008 landed while this was open and we both edited You deleted Resolution: keep both. Take main's One thing that may save you work: your Happy to push the resolution myself if you'd rather not deal with someone else's mess. CI on your current head was clean (build 13412, 22 jobs, 0 failures), so this is the only thing standing between it and a review. |
|
Conflicts resolved, @linyueqian. We’d better lower the threshold whenever the branch count drops during refactoring. |
|
Agreed, and thanks for resolving it. Lowering it as you go is still the right habit — it is just no longer something CI forces on you mid-refactor. #6008 changed the ratchet so a count below budget prints a reminder and exits 0; only an increase fails. That was the bug behind the two outages: the old check treated a dropped count as an error, so removing branches broke So: tighten it in the PR that does the removing when it is convenient, and if you forget, the next person sees the reminder instead of a red build. |
|
@linyueqian Conflicts resolved after IndexTTS 2.5 merge. All 359 tests passed. Can this PR move forward? |
linyueqian
left a comment
There was a problem hiding this comment.
Re-reviewed the full diff at 245d82ae against the pre-PR orchestrator tail. The migration is faithful. Three nits below, no blockers.
What I checked, since the diff grew a lot since my July pass (Audex helpers migrated, LegacyDetector removed, budget lowered to 20):
- Registry coverage is exact. The deleted
_SAMPLING_MAX_TOKENS_TTS_MODEL_TYPEShad 11 names; all 11 now resolve to adapters callingapply_max_new_tokens, plus the two special cases (ming_tts,glm_tts). Nothing gained or lost an override. - No inheritance hazard.
IndexTTS25Adapteris the only adapter that subclasses another registered adapter.MingFlashOmniTTSAdapter,MossTTS*,StepAudio2Adapter,CovoAudioAdapterandOmniVoiceAdapterall extendARTTSAdapterdirectly, so none of them silently inherits Ming'sstop_token_idsor amax_tokensoverride it did not have before. - The reordering is neutral. The
extra_paramsmerge writes onlytemperature/top_p/top_kandextra_args; the cosyvoice3 and GLM blocks touch onlymin_tokens/max_tokens/seed. Disjoint, so moving them after it cannot interact. - The Audex in-place
sampling_params_list[0] = ...is safe.serving_speech.py:2982builds a freshlist(...)andcoerce_param_message_typesmutates that copy in place, so the engine's shareddefault_sampling_params_listis never reindexed. Elements are still shared, which is why the injectors'deepcopyis load-bearing; that is preserved. - The CosyVoice3 consolidation is algebraically identical. Old:
min = min(max(1, L*min_ratio), max_new_tokens),max = max_new_tokens. New: the same, because themin_tokensread inside the newmax_new_tokensbranch is the value just assigned from the ratio. - The ratchet is honest.
python3.12 tools/pre_commit/check_tts_adapter.pyexits 0 at this head and the branch count is exactly 20, matchingMAX_MODEL_TYPE_BRANCHES, sotest_budgets_are_the_real_countsholds.
One more stale-doc item that is not anchorable to a diff hunk: tts_adapters/base.py:10-12 still says "the remaining models stay on the legacy path until individually migrated", which this PR is the one to make false.
Validation gap worth clearing before merge: there is no Buildkite build on 245d82ae. The only checks on this SHA are build (3.11), build (3.12), pre-commit, DCO and readthedocs, none of which run the five test files this PR changes. The ready label predates the latest push, so the label event that normally kicks Buildkite has not fired for this head. The 359-passed number is a local run only. Re-triggering CI would make the equivalence argument above test-backed rather than review-backed.
| params["duration_factor"] = [1.0 / speed] | ||
| return params | ||
|
|
||
| def apply_sampling_overrides( |
There was a problem hiding this comment.
[P3] This is byte-identical to the method added at IndexTTS2Adapter line 244, and IndexTTS25Adapter(IndexTTS2Adapter) already inherits it. Safe to delete the subclass copy.
| sampling_params_list[0].min_tokens = max(1, int(text_token_len * min_ratio)) | ||
| sampling_params_list[0].max_tokens = min(2048, int(text_token_len * max_ratio)) | ||
|
|
||
| if request.max_new_tokens is not None: |
There was a problem hiding this comment.
[P2] This branch is the only genuinely consolidated logic in the PR (everything else is a verbatim move), and it is the only one with no test.
test_prepare_speech_generation_cosyvoice3 exercises the ratio path only (min_tokens == 10, max_tokens == 2000, no max_new_tokens), and the fish_speech tests cover the shared apply_max_new_tokens helper, but nothing exercises CosyVoice3's own min_tokens = min(min_tokens, max_new_tokens) clamp.
A max_new_tokens=5 variant of the existing test closes it: with min_token_text_ratio=1 and text_token_len=10 the uncapped min_tokens is 10, so the assertion becomes min_tokens == 5 and max_tokens == 5.
There was a problem hiding this comment.
+1, the max_new_tokens=5 variant asserting min_tokens == 5 / max_tokens == 5 closes this for me.
| if not path.is_file(): | ||
| print(f"check_tts_adapter: expected file not found: {path}", file=sys.stderr) | ||
| return 1 | ||
| if not serving_path.is_file(): |
There was a problem hiding this comment.
[P3] Two leftovers from dropping the second counter:
- line 136 above still says "we always audit the same two files", now one.
.pre-commit-config.yaml:69still listsvllm_omni/entrypoints/openai/tts_adapters/__init__.pyin the hook'sfiles:pattern, so editing that file triggers a gate that no longer reads it.
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
|
@linyueqian PTAL, comments resolved, thanks! |
linyueqian
left a comment
There was a problem hiding this comment.
Round 3 on 740a5f61. All three items from my last pass are resolved, and I verified each rather than taking the diff at face value:
- The
files:regex droppingtts_adapters/__init__.pyis right.tools/pre_commit/check_tts_adapter.py:28only ever readsSERVING_SPEECH, so__init__.pywas never part of the audit surface. - Deleting
IndexTTS25Adapter.apply_sampling_overridesis behavior-preserving. The inheritedIndexTTS2Adapter.apply_sampling_overrides(indextts2.py:244-251) is identical,return apply_max_new_tokens(sampling_params_list, request). - The parametrized
test_prepare_speech_generation_cosyvoice3now covers both branches (None -> (10, 2000),5 -> (5, 5)), which was the gap.
No new findings. The migration still reads faithful to the legacy dispatch behavior.
One blocker left, and it is not the code: Buildkite has never run on this head. The status rollup carries GitHub Actions only (build 3.11/3.12, DCO, pre-commit, readthedocs), none of which execute tests. The ready label is on, but its last label event was 2026-08-10T20:14Z while the head commit landed 2026-08-12T17:06Z, and a fork PR needs a label event after the push to trigger a build. So the changed test in tests/entrypoints/openai_api/test_serving_speech.py has not actually executed anywhere.
Markers on that file are correct (core_model, cpu), so it will run once a build fires. I have toggled ready off and back on to kick one off. Happy to approve once it is green.
|
The latest |
… TTS adapters (vllm-project#5272) Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
PLEASE FILL IN THE PR DESCRIPTION HERE.
Purpose
M3 of #4855. This PR moves model-specific
sampling_params_listmutations fromserving_speechinto the corresponding TTS adapters throughapply_sampling_overrides. Removed model-specific dispatch logic inserving_speechwhile preserving the existing behavior for legacy dispatch paths.Compatibility Note
The legacy
glm_ttsandcosyvoice3paths previously applied theirsampling_params_listoverrides before mergingrequest.extra_params. With the unified adapter flow, the order is now:stream coercion -> extra_params -> apply_sampling_overrides -> seed, matches the contract documented in the adapter base class.These adapters do not read or modify
sampling_params_list[0].extra_args, so changing the order of these two independent operations does not change their behavior.For
cosyvoice3, the legacy flow applied the dynamic token limits in two steps:When
request.max_new_tokenswas provided, it then applied the request-level cap:The migrated
CosyVoice3Adapter.apply_sampling_overrides()consolidates these operations and preserves the previous behavior.Test Plan
pytest -v tests/entrypoints/openai_api/test_serving_speech.pypytest -v tests/worker/test_gpu_ar_model_runner.pyvLLM Version: 0.26.0
vLLM-Omni Commit: 4fc8892
Test Result
204 passed, 17 warnings in 1.44s23 passed, 16 warnings in 1.14sBEFORE 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)