[Refactor] Move model-specific capability metadata into TTS adapters - #6138
[Refactor] Move model-specific capability metadata into TTS adapters#6138sphinxkkkbc wants to merge 14 commits into
Conversation
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>
…c/vllm-omni into refactor/capability_migration
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
2e15746 to
4405826
Compare
…elper rather than class method Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
4405826 to
0c314cd
Compare
|
Partial review — I have gone through the ratchet change and traced the M4 shape, not the full 15-file diff yet. Flagging the part I did verify because it is the piece I own. Lowering So this removes 11 branches and re-tightens the budget to exactly the new count, which is the discipline the ratchet exists for and answers @hsliuustc0106's note on #5272 about lowering the threshold as the count drops. One thing worth knowing, since it bit this repo before: you do not need to keep that constant in lockstep on every future PR. My original version failed the build when This is M4 from #4855 and I have marked it as such in the RFC status table so nobody else starts it. Thanks for picking it up — I will come back with a full pass on the adapter metadata itself. |
|
This PR touches vllm_omni/entrypoints/, vllm_omni/model_executor/, tests/entrypoints/, tools/pre_commit/, vllm_omni/utils/ (15 files). Based on CODEOWNERS coverage of the changed files, the most-related reviewers appear to be: Could one of you take a look when you get a chance? Thanks! |
…/capability_migration
4859687 to
932b850
Compare
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
932b850 to
59ec7b2
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/entrypoints.md, docs/design/module/model_integration.md. Module owners: @alex-jw-brooks @linyueqian @NickCao @tzhouam @gcanlin @sphinxkkkbc, 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. |
…/capability_migration Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com> # Conflicts: # tests/entrypoints/openai_api/test_serving_speech.py # tools/pre_commit/check_tts_adapter.py # vllm_omni/entrypoints/openai/serving_speech.py # vllm_omni/entrypoints/openai/tts_adapters/glm_tts.py # vllm_omni/entrypoints/openai/tts_adapters/qwen3_tts.py # vllm_omni/entrypoints/openai/tts_adapters/voxcpm2.py # vllm_omni/entrypoints/openai/tts_adapters/voxtral.py
|
Marked this PR as ready for review and updated the description for clarity. @linyueqian PTAL, thanks! |
| except Exception: | ||
| pass | ||
| return None | ||
| await self._adapter.warmup() |
There was a problem hiding this comment.
Non-TTS deployments crash at startup here. omni_init_app_state constructs the speech server and calls warmup() unconditionally (api_server.py:1140), but _adapter is None whenever no TTS stage resolves — plain LLM serves, omni chat without a talker, VLA — so every such vllm serve --omni boot dies with AttributeError: 'NoneType' object has no attribute 'warmup'. The old code early-returned unless the model type was voxcpm2. Smallest fix: if self._adapter is None: return, ideally with a regression test that constructs the server with no TTS stage and awaits warmup().
There was a problem hiding this comment.
Verified at 5f09d11 (after the main merge): guard in place, test_warmup_with_no_adapter asserts the no-adapter path returns cleanly. Thanks!
| def _get_available_speakers(self) -> set[str]: | ||
| """Return all built-in, precomputed, and runtime-uploaded speakers.""" | ||
| available_speakers = set(self.uploaded_speakers) | ||
| if self._adapter is not None: |
There was a problem hiding this comment.
for_diffusion instances never get an _adapter attribute at all (the factory builds via cls.__new__ and only initializes the attributes it lists), so this is not None check itself raises AttributeError and takes down every voice-handling path on pure-diffusion TTS deployments: GET /v1/audio/voices (api_server.py:1485), create_speech with any voice set (line 3289, which runs before the _diffusion_mode dispatch), and _get_normalized_voice from _create_diffusion_speech (line 3132). The old code worked here because _init_speaker_storage initialized supported_speakers = set(). Fix: instance._adapter = None in for_diffusion — and a smoke test constructing this factory would be worth adding, since no test currently exercises it with the new capability layout.
There was a problem hiding this comment.
Verified — instance._adapter = None is in for_diffusion and test_diffusion_create_speech_with_unknown_voice exercises the voice path end to end (400 for unknown voices, matching the pre-refactor behavior).
| dim_err = self._validate_qwen_tts_speaker_embedding_dim(emb_dim) | ||
| if dim_err is not None: | ||
| raise ValueError(dim_err) | ||
| dim_err = self._adapter.validate_tts_embedding_dim(emb_dim) |
There was a problem hiding this comment.
Same unguarded deref on the request path: on a for_diffusion server (where _adapter is never set), POST /v1/audio/voices with speaker_embedding returns 500 with AttributeError instead of storing the voice — the old code accepted it, since _validate_qwen_tts_speaker_embedding_dim returned None for any non-qwen3 model type. Resolving via self._get_tts_adapter() (which already returns None in diffusion mode) and skipping the check when it's None restores parity. The test_app fixture papers over exactly this seam with a MagicMock adapter, so the endpoint tests can't catch it.
There was a problem hiding this comment.
Verified — the ternary guard restores the old accept-without-dim-check behavior, and test_api_server_upload_voice_with_no_adapter covers the route end to end (200). I also re-checked the remaining self._adapter.* sites you mention: the unguarded ones are qwen3/voxcpm2-only paths or wrapped by the prompt-length fallback.
hsliuustc0106
left a comment
There was a problem hiding this comment.
Requesting changes for two reachable crashes that share one root cause — the migration dropped the guards for servers without a resolved adapter:
warmup()awaitsself._adapterunconditionally whileomni_init_app_statecalls it on every AR-mode serve, so non-TTS deployments (plain LLM, omni chat without a talker, VLA) crash at startup.for_diffusionbuilds viacls.__new__and never sets_adapter, so_get_available_speakers()raises AttributeError and breaksGET /v1/audio/voices,create_speechwith avoiceset, and the embedding-upload path on pure-diffusion TTS deployments.
All three fixes are a few lines plus the two missing regression tests (no current test constructs a server without an adapter). Everything else verified clean: ratchet 20→9 recounted independently, per-adapter capability loaders match the old dispatch table exactly, and no stale references to the removed attributes. Happy to re-review once the guards land.
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
|
Addressed @hsliuustc0106's review comments in 04628c1. All three issues involved code paths where no TTS adapter is resolved, added the corresponding guards and regression tests. Also double-checked the remaining |
|
@linyueqian PTAL, thanks! |
linyueqian
left a comment
There was a problem hiding this comment.
Full pass on the adapter metadata, following up on my partial review of the ratchet. Reviewed at 5f09d11c. The migration shape is right, and I confirmed @hsliuustc0106's three findings are genuinely fixed at this head. One blocker in the new code.
[blocking] VoxCPM2 warmup raises NameError and kills server startup
vllm_omni/entrypoints/openai/tts_adapters/voxcpm2.py:97 constructs OpenAICreateSpeechRequest(...), but the only import of that name is inside if TYPE_CHECKING: at line 17, so it is never bound at runtime. The old code lived in serving_speech.py, which imports the symbol at runtime (line 41), so the move lost the binding.
The constructor call sits above the try on line 104, so nothing catches it:
api_server.py:528 await omni_init_app_state(...) # unguarded
api_server.py:1140 await state.openai_serving_speech.warmup()
serving_speech.py:525 await self._adapter.warmup()
voxcpm2.py:97 NameError: name 'OpenAICreateSpeechRequest' is not defined
Every vllm serve --omni against openbmb/VoxCPM2 fails to boot. Executed against this head with the adapter's dependencies stubbed:
module imported OK; 'OpenAICreateSpeechRequest' in module globals: False
RESULT: warmup() raised NameError: name 'OpenAICreateSpeechRequest' is not defined
Fix: move the import to module scope in voxcpm2.py.
Two things let this through, both cheap to close:
- No test constructs
VoxCPM2Adapteror calls itswarmup().test_warmup_with_no_adapteronly covers the_adapter is Noneearly return added last round. A test that awaitsVoxCPM2Adapter.warmup()with a stubbed_generate_audio_byteswould have caught it. - Ruff as configured cannot see it.
F821passes because ruff treats aTYPE_CHECKINGimport as binding the name.TC004catches it exactly, and[tool.ruff.lint] selectinpyproject.tomldoes not includeTC:
$ uvx ruff check --isolated --select TC004 vllm_omni/ tests/ examples/
TC004 Move import `...OpenAICreateSpeechRequest` out of type-checking block. Import is used for more than type hinting.
--> vllm_omni/entrypoints/openai/tts_adapters/voxcpm2.py:17:61
Found 1 error.
One hit repo-wide, and it is this bug. Adding "TC004" to the select list is zero-noise and closes the class permanently. Fine as a follow-up if you would rather keep this PR scoped.
[nit] Small subtraction pass on the incidental edits
serving_speech.py:964:len(set(self.uploaded_speakers))islen()of a throwaway copy. Waslen(self.uploaded_speakers).serving_speech.py:532: droppinglist(...)aroundself.uploaded_speakersremoves a defensive copy for no gain. Safe today because_speaker_cache.clear()does not touch the dict, but the copy was what made it safe by construction.voxcpm2.py:64: the newspeaker_infolocal is used once.ming_tts.py:193:_load_ming_tts_codec_frame_ratehas no return annotation and falls off the end returningNoneimplicitly. Every sibling hook is annotated-> float | None.serving_speech.py:100:_TTS_LANGUAGES = DEFAULT_TTS_LANGUAGEShas no production reader left. The only one istests/entrypoints/openai_api/test_serving_speech.py:42. Point the test atDEFAULT_TTS_LANGUAGESintts_adapters/base.pyand drop the alias plus the import on line 59.capabilities.py:31:str(speaker).lower()where the old code hadspeaker.lower(). For a non-string speaker key the old path raised into itsexceptand returnedset(); the new one coerces. Not reachable with any supported config, just noting the silent change.
Verified clean
- Ratchet: recounted with the checker's own AST pass rather than trusting the constant.
main20, this head 9. The description still says 27 to 16, which was true before the main merge. - Capability parity across all 20 registered adapter classes (
indextts2.pyandmoss_tts.pyeach define two), for supported speakers, precomputed speakers, supported languages, codec frame rate, andvalidate_tts_embedding_dim. Every model that had a branch in the old dispatch has an override; every model that fell through to theelsenow inherits the equivalent base hook. No model's resolved capability changes. - The uploaded-versus-built-in split is read-path equivalent.
_get_available_voices,_get_normalized_voice,_is_default_voice, and thevalidate()in qwen3_tts, voxcpm2, and voxtral all resolve to the same union as the oldsupported_speakers. load_capabilities()moving earlier in__init__is safe. Every loader reads onlyctx.engine_client(set bysuper().__init__) andctx.server; none touch_moss_variant,_tts_tokenizer,_tts_executor, or_max_instructions_length, which are assigned later.- Nothing else can newly raise out of
load_capabilities().voxtral.py:55readshf_config.audio_configoutside the helper'stry, which looked like a lost fallback, butVoxtralTTSConfig.__init__setsself.audio_config = audio_config or {}unconditionally (transformers_utils/configs/voxtral_tts.py:28), so it cannot raise. Qwen3's unguardedtalker_config.hidden_sizeis the same exposure as merge-base, and is now lazier. - No stale references to the removed
self.supported_speakers,self.precomputed_speakers,self.supported_languages, orself._codec_frame_rate.serving_chat.pykeeps its own independent_supported_speakersand is untouched. - The unguarded
self._adapter.capabilities.codec_frame_rateatserving_speech.py:604is fine. Both callers (:659via_estimate_prompt_len_async, and:2757) sit inside_build_qwen3_tts_request, which is only reached fromQwen3TTSAdapter.build.
Worth calling out as a fix, not just a refactor
Dropping self.supported_speakers.discard(voice_name_lower) fixes a latent bug rather than just tidying up. At merge-base, supported_speakers held built-ins and uploads in one set, and _evict_existing_upload/delete_voice (lines 1207 and 1476) discarded by name. Uploading a voice whose name collides with a built-in and then deleting it permanently removed the built-in for the life of the process; nothing rejects a colliding upload name. The new split makes that impossible. Might be worth a line in the description.
Validation gap
No Buildkite build exists on any commit of this PR. The timeline has no ready label event, so tests/entrypoints/openai_api/test_serving_speech.py has never run in CI here. The 366-passed in the description was a local run at 04628c1b, which predates the 5f09d11c main merge. Worth a green run on this head before merge, particularly since the blocker sits in a path the suite does not cover.
On coverage more broadly: TestTTSMethods does exercise the real loaders (test_load_supported_speakers builds a real server and asserts on _adapter.capabilities, including the FrozenInstanceError check), so the qwen3 and ming capability paths are genuinely covered. The gaps are specifically VoxCPM2Adapter.warmup() and VoxtralTTSAdapter._load_supported_speakers. Separately, the test_app fixture at line 250 swaps in a MagicMock adapter and its inline list_voices at line 278 reimplements the route against that mock instead of calling production _get_available_voices(), so the endpoint tests cannot see this seam. That predates the PR, but line 278 was updated to track the refactor rather than pointed at production.
Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
…c/vllm-omni into refactor/capability_migration Signed-off-by: boatman <109857087+sphinxkkkbc@users.noreply.github.com>
93e87a7 to
3da6236
Compare
PLEASE FILL IN THE PR DESCRIPTION HERE.
Purpose
M4 of #4855. Move model-specific capability metadata, such as supported languages, built-in speakers, and precomputed voice profiles, from the speech server to TTS adapters.
After this PR, the server only owns runtime state, such as voices uploaded through
/v1/audio/voices. Static, model-specific capabilities are resolved once during adapter initialization and stored in a frozenTTSCapabilitiessnapshot.Models that require specialized capability-loading behavior override the corresponding adapter hooks, while model-independent parsing logic is shared through stateless helpers.
Contract Change
Added a frozen
TTSCapabilitiesdataclass for initialization-time, model-specific capabilities:precomputed_speakerssupported_speakerssupported_languagescodec_frame_rateThese fields were previously stored directly on the server. They are now owned by the resolved TTS adapter and loaded as a single capability snapshot during server initialization. Common defaults, such as the fallback language set, are defined by the base adapter.
supported_speakersis stored as afrozenset. Note thatprecomputed_speakersremains a mapping of profile metadata and is not deeply immutable.Preserved runtime capabilities, such as voices uploaded through
/v1/audio/voices, on the server asself.uploaded_speakers.Added
_get_available_speakers()to expose the union of:Previously, uploaded speaker names were also copied into
self.supported_speakers. After this PR,supported_speakersrepresents only model-provided built-in speakers, whileuploaded_speakersrepresents runtime uploads. The collections are independently owned and are no longer kept in sync.As a result, mutations such as the following are no longer necessary and have been removed:
This also fixes a latent bug where deleting an uploaded voice whose name collides with a built-in voice would silently remove the built-in voice from
supported_speakers, because the set previously stored both built-in and uploaded voice names.Extracted shared capability-loading logic from
serving_speech.pyinto stateless helpers intts_adapters/capabilities.py.Why this change: In the early stage of the migration, base adapter previously invoked these helpers as fallbacks through
self.ctx.server._load_*(). Since the capability metadata itself is now adapter-owned, I chose to move the related stateless loaders out of the server as part of the same ownership migration.This extraction is a subjective organizational choice with no intended behavior change. Feedback is welcome.
Migrated model-specific capability and lifecycle logic to the corresponding adapters, including:
Removed model-type dispatch from the precomputed speaker-profile validation path.
Previously,
_load_precomputed_speakers()eventually calledload_validated_profile_tensors()inspeaker_cache.py, where validation behavior was selected through model-type branches.The validation contract is now explicit:
Each adapter supplies its own validation callback and any model-specific metadata it requires. For example, Qwen3-TTS provides the expected speaker embedding dimension derived from the loaded talker configuration.
Isolated speaker persistence in
test_serving_speech.pyby setting:This prevents tests, especially failed tests, from reading or leaving speaker artifacts in the developer's persistent local cache.
Test Plan
pytest -v \ tests/entrypoints/openai_api/test_serving_speech.py \ tests/worker/test_gpu_ar_model_runner.py \ tests/entrypoints/openai_api/test_tts_detection.pyvLLM Version:
0.27.0
vLLM-Omni Commit:
3da6236
Test Result
368 passed
BEFORE SUBMITTING: Read [CONTRIBUTING.md](https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md) and run the [precheck-pr skill](https://github.com/vllm-project/vllm-omni/blob/main/.claude/skills/precheck-pr/SKILL.md) with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)