feat(api): report engine capabilities instead of declaring them twice - #1023
feat(api): report engine capabilities instead of declaring them twice#1023Lvigentini wants to merge 1 commit into
Conversation
`ModelConfig.supports_instruct` has been carried since the field was added and read nowhere -- the flag existed, was set deliberately on four configs, and nothing consulted it. The desktop app gates its instruct field on its own hard-coded `INSTRUCT_ENGINES` set instead. Two places declaring the same fact, which drift the first time an engine is added: a new engine that honours `instruct` needs a frontend edit before the field appears, even though the registry already says so. Adds `engine_supports_instruct()` alongside the existing `engine_needs_trim()` and `engine_retries_runaway()`, so the flag is finally read where it is declared, plus `engine_languages()` and `engine_model_sizes()` derived the same way. `GET /engines` exposes all of it, so clients can gate on capability rather than matching engine names. An engine whose variants disagree reports False. A request names an engine and the model size can change under it, so the conservative answer is the only one true for every variant. Also stops the silent drop. Base Qwen3-TTS accepts `instruct` and ignores it -- the config comment has said so all along. The app hides the field there, but API and MCP callers get no such cue and watch their delivery instructions vanish with nothing logged anywhere, which reads as the model refusing to follow them. `POST /generate` and `POST /generate/stream` now log a warning naming the engines that do honour it. No behaviour change to generation itself, and no change to which engines support what -- only to whether that is discoverable. 17 tests: the flag against the registry, the mixed-variant rule, derived languages and sizes, the endpoint against the helpers, and the warning including its negative cases (absent, empty, whitespace, capable engine). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe backend derives TTS engine capabilities from the registry, exposes them through ChangesTTS capabilities and instruction handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ModelConfig.supports_instructis set deliberately on four configs and read nowhere. The desktop app gates its instruct field on its own hard-coded set instead:Two places declaring the same fact. They drift the first time an engine is added — a new engine that honours
instructneeds a frontend edit before the field appears, even though the registry already says it works.Changes
engine_supports_instruct(), alongside the existingengine_needs_trim()andengine_retries_runaway(), so the flag is read where it is declared. Plusengine_languages()andengine_model_sizes()derived the same way.GET /enginesexposes the lot, so clients gate on capability rather than matching engine names:{"engines": [ {"engine": "qwen", "display_name": "Qwen TTS", "supports_instruct": false, "languages": ["zh","en","ja","ko","de","fr","ru","pt","es","it"], "model_sizes": ["1.7B","0.6B"], "has_model_sizes": true}, {"engine": "qwen_custom_voice", "supports_instruct": true, ...} ]}An engine whose variants disagree reports
False. A request names an engine and the model size can change under it, so the conservative answer is the only one true for every variant.No more silent drop. Base Qwen3-TTS accepts
instructand ignores it —backends/__init__.py:249has said so all along:The app hides the field there, so desktop users are fine. API and MCP callers get no such cue and watch their delivery instructions vanish with nothing logged anywhere — which reads as the model refusing to follow them rather than never receiving them.
POST /generateandPOST /generate/streamnow log a warning naming the engines that do honour it.Scope
No behaviour change to generation, and no change to which engines support what — only to whether that is discoverable. The frontend still uses its local set; swapping it to read
/enginesis a follow-up, and deliberately not bundled here so this stays backend-only and reviewable on its own.Worth noting this does not fix #579. That reporter is putting delivery cues inline in the text on base Qwen, which has no instruct field at all — a different problem.
Tests
17, in
backend/tests/test_engine_capabilities.py: the flag checked against the registry rather than restating it, the mixed-variant rule, derived languages and sizes, the endpoint checked against the helpers so the two can't diverge, and the warning including its negative cases — absent, empty, whitespace-only, and a capable engine.Branched off
main, independent of my other open PRs.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes