fix: correct Deepgram language validation to use full BCP-47 codes and model parameter #3266
+144
−13
Conversation
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
…d model parameter - Fix supports_language to only fall back to base ISO-639 code when no region is specified - Update is_supported_languages_impl to use the model parameter for validation - Update language_quality_live to check model-specific language support - Add nova-3-medical to the model fallback chain in best_for_languages - Add model override logic in transcribe-proxy for Deepgram when model doesn't support languages - Export DeepgramModel from owhisper-client for use in transcribe-proxy - Add tests for en-CA with nova-3-general (not supported) and nova-3-medical (supported) Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote-storybook canceled.
|
✅ Deploy Preview for hyprnote canceled.
|
✅ Deploy Preview for howto-fix-macos-audio-selection canceled.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
The should_override_deepgram_model function was using permissive OR logic that didn't match the strict logic in DeepgramModel::supports_language. This caused the proxy to not override the model for en-CA with nova-3-general even though the client correctly showed it as unsupported. Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+134
to
140
| if listen_params.model.is_none() || should_override { | ||
| if let Some(model) = | ||
| AdapterKind::from(provider).recommended_model_live(&listen_params.languages) | ||
| { | ||
| listen_params.model = Some(model.to_string()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Critical logic bug: When should_override is true (incompatible model/language combination detected) but recommended_model_live() returns None, the incompatible model remains in listen_params.model. This means an explicitly provided incompatible model (e.g., nova-3-general with en-CA) will still be used if no alternative is found, causing API failures.
Fix:
if listen_params.model.is_none() || should_override {
if let Some(model) =
AdapterKind::from(provider).recommended_model_live(&listen_params.languages)
{
listen_params.model = Some(model.to_string());
} else if should_override {
// Clear incompatible model when no alternative exists
listen_params.model = None;
}
}
Suggested change
| if listen_params.model.is_none() || should_override { | |
| if let Some(model) = | |
| AdapterKind::from(provider).recommended_model_live(&listen_params.languages) | |
| { | |
| listen_params.model = Some(model.to_string()); | |
| } | |
| } | |
| if listen_params.model.is_none() || should_override { | |
| if let Some(model) = | |
| AdapterKind::from(provider).recommended_model_live(&listen_params.languages) | |
| { | |
| listen_params.model = Some(model.to_string()); | |
| } else if should_override { | |
| // Clear incompatible model when no alternative exists | |
| listen_params.model = None; | |
| } | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
…matching Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
…d_languages_impl Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
…ttps://git-manager.devin.ai/proxy/github.com/fastrepl/hyprnote into devin/1768957161-fix-deepgram-language-validation
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
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.
Summary
Fixes incorrect language validation in the Deepgram adapter where:
modelparameter was being ignored (_model) in validation functionsen) instead of the full BCP-47 code (e.g.,en-CA)This caused users selecting
en-CAwithnova-3-generalto not see a warning, even though that combination doesn't work (nova-3-general doesn't supporten-CA, but nova-3-medical does).Key changes:
language_matches_supported_codes()helper that centralizes BCP-47 matching logic with proper fallback behaviorsupports_language()uses the helper and only falls back to base ISO-639 code when the language has no region specifiedNova3Medicalto the model fallback chain soen-CAauto-selectsnova-3-medicalis_supported_languages_implandlanguage_quality_livenow use the model parameter for validationshould_override_deepgram_model()to correct incompatible model/language combinationsDeepgramModelfromowhisper-clientfor use in transcribe-proxyUpdates since last revision
should_override_deepgram_modelnow usesparsed_model.supports_language(lang)directly for consistent validation logic between client and proxyReview & Testing Checklist for Human
en-CAshould NOT match models that only listenin their supported languages. This is intentional but may surprise users expecting fallback behavior.nova-3-generalwithen-CA, proxy should auto-override tonova-3-medical. Verify this doesn't cause unexpected behavior for users who explicitly chose a model.en-CAwithnova-3-medicalactually works, anden-CAwithnova-3-generalactually fails at the API levelen-US,en-GB,fr-CAthat ARE in the supported lists should still work correctlyen-CA+nova-3-general, the settings UI should now show a warningRecommended test plan:
en-CAas language andnova-3-generalas model → should show unsupported warningen-CAwith no model specified → should auto-selectnova-3-medicalen-USwithnova-3-general→ should work without warning["en", "es"]withnova-3-medical→ should show unsupported warningen-CAto verify proxy correctly routes tonova-3-medicalNotes