Fix: keep the uploaded file extension when transcribing#903
Open
e3o8o wants to merge 1 commit into
Open
Conversation
/transcribe wrote every upload to a temp file named .wav regardless of its real format. librosa picks its decoder from the extension, so any non-wav upload failed with "could not open/decode file" even though the format is one the app handles elsewhere. profiles.py already solves this for voice samples by keeping the uploaded extension when it is one of the audio types it accepts, and falling back to .wav otherwise. Same approach here, same set. The fallback means an unknown or missing extension behaves exactly as it does today.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe transcription endpoint adds an audio-extension whitelist and uses approved uploaded extensions for temporary files, defaulting to ChangesTranscription file handling
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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.
The bug
POST /transcribewrites every upload to a temp file named.wav, whatever the file actually is:load_audiocallslibrosa.load, which picks its decoder from the extension. So an.m4a(or.mp3,.flac,.ogg) upload is handed to the wav decoder and dies:The formats work everywhere else in the app, so this reads as "Voicebox cannot transcribe my recording" rather than "the temp file was misnamed". I hit it trying to transcribe an
.m4avoice memo to use as a cloning reference.The fix
profiles.pyalready solves exactly this for voice samples (lines 161-165): keep the uploaded extension when it is one of the audio types accepted, and fall back to.wavotherwise. This applies the same approach, with the same set, to/transcribe.The fallback matters: an unknown or missing extension still becomes
.wav, so today's behaviour is unchanged for anything that already worked, and an unexpected filename can never inject an arbitrary suffix.Verification
The strongest evidence is that your own code already proves it.
POST /profiles/{id}/sampleskeeps the uploaded extension,POST /transcribedoes not, and they shareload_audio. Same server, same file:.m4aposted to/profiles/{id}/samples→ 200, decoded and transcoded to wav.m4aposted to/transcribe→could not open/decode file/transcribe→ 200, transcribed fineSo the only difference between the path that works and the path that fails is the temp file's name.
I also checked the suffix selection across the cases that matter:
Sepa.m4a→.m4a,clip.MP3→.mp3(case-insensitive),noext→.wav,evil.exe→.wav, and empty/None filenames →.wav.Tested against v0.3.x on macOS (MLX). I could not run the patched server itself, since my system Python does not have the backend's dependencies and I was testing against the bundled
voicebox-serverbinary, so the end-to-end confirmation above is on the shipped build rather than this patch. The change is small enough to read, but worth saying plainly.One thing I deliberately did not do
That extension set now exists in four places, and three of them disagree:
routes/generations.py:25.opusroutes/profiles.py:161.opusservices/captures.py:75.aac, no.opusroutes/transcription.py(this PR)profiles.pyUnifying them would change which formats three unrelated endpoints accept, which felt like the wrong thing to smuggle into a bug fix. Happy to send that as a separate PR if you want it, or to point this one at a shared constant instead if you would rather it not add a fourth copy.
Unrelated, while I was in here
whisper-turboreliably 500s on MLX withThere is no Stream(gpu, 2) in current thread, and can take the process down;baseandsmallwork. I can reproduce it consistently but do not understand the MLX threading here well enough to propose a fix, so I have left it out rather than guess. Happy to file it as an issue with the reproduction if useful.Summary by CodeRabbit