fix(mlx): pass the voice description the design model requires (#1405) - #1407
Merged
Conversation
`MLXAudioBackend.generate` assembles its kwargs by hand and forwards
`voice`, `ref_audio`, `ref_text` and `lang_code`. It never forwarded
`instruct` — while the comment immediately above the block asserted that it
did ("voice for Kokoro, ref_audio for CSM, instruct for Qwen3 — we pass
them all").
The curated `qwen3-tts` model IS the VoiceDesign variant, and mlx-audio
raises outright when that variant runs without an instruct. So the engine
could not produce audio under any input, and the reporter got a bare 400
quoting a library message that names an internal parameter and no action.
The comment is why this went unnoticed: it documented the behaviour the
code was missing.
Now the description is forwarded, and a design model without one fails with
something a user can act on instead of the library's wording.
Detection asks the model's own `tts_model_type` — the exact field mlx-audio
branches on, so it cannot drift from the library — and falls back to the id
convention only when a config does not expose it.
One pre-existing test had to change: it generated with the VoiceDesign
model and no description while asserting lang_code behaviour. That scenario
could never have worked against the real library; it passed only because
instruct was being dropped before it got there. It now supplies one, and
additionally asserts the forwarding.
5 new tests, 4 fail before. Backend suite: 4397 passed, 32 skipped.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesVoiceDesign support
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/services/tts_backend.py`:
- Line 1437: In the instruct handling near the kwargs construction, replace the
inline conditional assignment with a properly indented if block containing
kwargs["instruct"] = instruct, preserving the existing condition and behavior so
Ruff E701 is resolved.
In `@tests/test_mlx_audio_instruct_1405.py`:
- Line 25: Move the module-level services import into the backend fixture or
each test, resolving tts_backend at runtime so the regression suite uses the
current implementation despite sys.modules pollution. Keep only imports that are
safe at module scope in tests/test_mlx_audio_instruct_1405.py.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88250417-11d5-4c82-8ad8-ed1b47583d96
📒 Files selected for processing (4)
CHANGELOG.mdbackend/services/tts_backend.pytests/test_engines.pytests/test_mlx_audio_instruct_1405.py
CodeRabbit on #1407: a module-level `from services import tts_backend` keeps whatever object sys.modules held when the file was imported. Other suites rebind that name, so the binding can go stale and leave these tests exercising a different implementation than the one under test — passing alone and proving nothing in the full run. Resolve it in a fixture instead. Also split the one-line `if instruct:` now that it carries an elif.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1405.
Reported on macOS / M4 Pro with the
mlx-audioengine: generating returnedRoot cause
MLXAudioBackend.generateassembles its kwargs by hand:instructis never forwarded — while the comment immediately above that block asserts that it is:The curated
qwen3-ttsmodel is the VoiceDesign variant (Qwen3-TTS-12Hz-1.7B-VoiceDesign-4bit), andmlx_audio/tts/models/qwen3_tts/qwen3_tts.py:919raises when that variant runs without an instruct.So the engine could not produce audio under any input, and the user got a raw 400 quoting a library message that names an internal parameter and no action. The comment is why it went unnoticed — it documented behaviour the code was missing.
Fix
instructto the library;Detection asks the model's own
tts_model_type— the exact field mlx-audio branches on, so it cannot drift — and falls back to theVoiceDesignid convention only when a config doesn't expose the field.One pre-existing test changed
test_mlx_audio_generate_non_kokoro_model_ignores_kokoro_validationgenerated with the VoiceDesign model and no description while assertinglang_codebehaviour. That scenario could never have worked against the real library; it passed only becauseinstructwas dropped before reaching it. It now supplies one, and additionally asserts the forwarding — so the gap that hid this bug is itself covered.Tests
5 new, 4 fail before — including the core one, that the description reaches the model at all.
Backend suite: 4397 passed, 32 skipped.
The MLX audio backend now forwards voice descriptions as
instructfor Qwen3 VoiceDesign models and raises an actionable error when the description is missing. Model detection usestts_model_typewith a model-ID fallback, fixing speech generation on Apple Silicon. Tests cover forwarding, validation, and detection behavior; human review should verify compatibility with model configurations that omit or misreporttts_model_type.