Repository navigation
feat: POST /transcribe — transcribe sung audio to word-level timed lyrics - #14
Conversation
…rics feedBack's "transcribe" path has been posting the vocals stem to /align, whose `text` form field is REQUIRED — so FastAPI rejected the request with a 422 before the handler ever ran. Every time, for everyone. Remote transcription never worked at all; the feature was wired to an endpoint that answers a different question, and there was no endpoint that answered the right one (got-feedBack/feedBack-plugin-stem-splitter#17). /align "here are the lyrics — when is each word sung?" (needs text) /transcribe "what are the lyrics, and when is each sung?" (needs only audio) Whisper hears the words; wav2vec2 then force-aligns Whisper's own transcript, which is why this isn't just asr.transcribe(): raw Whisper segments are line-ish and drift, and a karaoke chart built from them sits visibly late. Returns native whisperx.align() output, which is exactly what the client's existing mapper consumes — nothing downstream has to learn a new shape. An instrumental returns {"segments": []} with a 200. A stem with no singing in it is a valid answer, not a failed request, and a 400 there would make callers treat "this song has no vocals" as an error. The test drives a REAL FastAPI app, because the bug lived in the validation layer above the handler: a unit test calling transcribe_audio() directly would have passed while the endpoint 422'd — reproducing the exact blind spot that let this ship. Verified it fails against the unfixed server (404). Signed-off-by: topkoa <topkoa@gmail.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded ChangesLyrics transcription
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI
participant WhisperX
Client->>FastAPI: POST /transcribe with audio and optional language
FastAPI->>WhisperX: Transcribe audio
WhisperX-->>FastAPI: Segments and detected language
FastAPI->>WhisperX: Align retained segments
WhisperX-->>FastAPI: Word-level aligned segments
FastAPI-->>Client: Return segments and language
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server.py (1)
1220-1312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSignificant duplication with
align_lyrics(lines 775-1217).
transcribe_audiore-implements temp-file save/cleanup, the[a-z]{2,8}language-hint regex validation, therun_in_executordispatch, and theValueError→400 /Exception→500 mapping almost verbatim fromalign_lyrics. Consider extracting shared helpers (e.g._save_upload_to_tempfile(file),_normalize_language_hint(language), a small wrapper that runs a sync closure in the executor and maps its result to aJSONResponse) so both endpoints stay in sync as this logic evolves.🤖 Prompt for 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. In `@server.py` around lines 1220 - 1312, Extract the duplicated upload tempfile handling, language normalization/validation, executor dispatch, and exception-to-HTTP response mapping from align_lyrics and transcribe_audio into shared helpers. Update both endpoints to use those helpers while preserving their existing endpoint-specific transcription and alignment behavior, status codes, cleanup, and response shapes.
🤖 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 `@server.py`:
- Around line 1265-1290: Update transcribe_audio’s segment preparation before
whisperx.align to remove Whisper segments shorter than 0.2 seconds, matching the
filtering behavior used by align_lyrics. Preserve the existing no-segments
response after filtering, and pass only the filtered segments to alignment.
---
Nitpick comments:
In `@server.py`:
- Around line 1220-1312: Extract the duplicated upload tempfile handling,
language normalization/validation, executor dispatch, and exception-to-HTTP
response mapping from align_lyrics and transcribe_audio into shared helpers.
Update both endpoints to use those helpers while preserving their existing
endpoint-specific transcription and alignment behavior, status codes, cleanup,
and response shapes.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a41c4228-1b9a-489a-9b56-0740e6eff52b
📒 Files selected for processing (3)
README.mdserver.pytests/test_transcribe_endpoint.py
There was a problem hiding this comment.
Pull request overview
Adds a missing POST /transcribe endpoint to the FastAPI server so clients can upload vocals-only audio and receive WhisperX-style word-level timed lyrics without supplying any text (unlike /align, which is forced alignment and must require lyrics input). This fixes the contract mismatch where remote transcription was wired to /align and consistently failed validation.
Changes:
- Introduces
POST /transcribethat runs ASR then force-aligns the transcript to produce word-level timings in nativewhisperx.align()shape. - Adds an integration-style test that exercises FastAPI’s request validation layer (ensuring
/transcribedoes not 422 without lyrics, while/alignstill does). - Documents the new endpoint and expected response shape/behavior (including instrumental returning
{"segments": []}with 200).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
server.py |
Adds the new /transcribe endpoint alongside existing /align behavior. |
tests/test_transcribe_endpoint.py |
Adds a subprocess-based harness test that posts real multipart requests against the app. |
README.md |
Documents /transcribe usage, parameters, and return format. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… don't skip on a broken import - /transcribe passed Whisper's raw segments straight to wav2vec2, while /align filters sub-0.2s ones first and documents why: the aligner returns empty alignments for windows that short, so they degrade the output instead of adding to it. Whisper emits them on breaths, noise and stem bleed. The threshold is now one shared _MIN_SEGMENT_SECONDS — the two endpoints were carrying the same 0.2 independently, which is how they drift. A stem of nothing but sub-frame noise is now an instrumental (empty segments, 200), not a degraded align. - The temp file was only unlinked inside _do_transcribe()'s finally, which never runs if `await file.read()` or the write raises — a client that disconnects mid-upload, or a full disk, left the file behind. One orphan is nothing; one per failed upload on a server that runs for months is a disk quietly filling with nothing. - The test skipped on any ModuleNotFoundError. Every module the harness needs and doesn't stub — fastapi above all — is a hard requirement of server.py, so a missing one means the server cannot import, and skipping there would turn "the server is broken" into a green CI run. - README: the language hint is case-insensitive (the server lowercases before validating), so `EN` works. Documented, and pinned by a test. Found by CodeRabbit and Copilot on #14. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
… host's paths - TestClient fires FastAPI's startup event, which spawns the warmup thread — model loads and, on a cold cache, multi-GB downloads. In a test whose entire premise is "heavy ML is stubbed", that was the one thing still reaching for the network. skip_warmup is set before the client is built. - /transcribe returned bare str(e) on an unexpected failure, leaking the host's cache path, home directory and container layout to anyone who can reach the endpoint — and this server routinely sits on a LAN. The obvious fix (a flat "internal error") is wrong in the other direction: the caller has no access to these logs, and a message that reaches nobody who can act on it is exactly how the client bug that necessitated this endpoint stayed hidden for so long. So the full traceback goes to the operator's stdout, and the caller gets the exception type and message with absolute paths scrubbed. "CUDA out of memory" survives — it is the answer, and it is not a secret. "/app/cache/models--x/snapshots/…" does not. Found by Copilot on #14. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
… the comparison - _client_safe_error() promised "absolute paths scrubbed" and scrubbed exactly two roots. A scrubber that only knows about CACHE_DIR and $HOME is one tempfile.gettempdir() away from leaking anyway, and the docstring was writing cheques the code didn't honour. It now scrubs any absolute path, POSIX or Windows — while leaving URLs intact, because "could not reach https://huggingface.co/..." IS the diagnosis for a network failure and mangling it would be its own kind of lost message. Pinned by tests. - The upload was read whole into RAM before a byte reached disk. A lossless 4-minute vocals stem is ~40 MB, MAX_CONCURRENT of those is ~80 MB of pure waste, and nothing stopped a caller uploading 2 GB to see what happened. Streamed in 1 MB chunks. - _MIN_SEGMENT_SECONDS said "shorter than this are dropped" while the filter is a strict `>`, so a segment of exactly 0.2s is dropped too. Comment now matches the comparison. Found by Copilot on #14. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
`if language:` is true for " ", which then strips to "" and fails the [a-z]{2,8} pattern — so a
request that asked for nothing in particular came back as 400 "Invalid language code: ''".
Whitespace is not a language; it is the absence of one.
Normalize first, then decide whether a hint was given. /align had the identical bug, so the
normalizer is now shared: one _normalized_language() both endpoints call, which is also what
stops them drifting apart again.
Subtags like `en-US` are still rejected rather than truncated to `en` — silently reinterpreting a
request is how you end up aligning Portuguese audio with an English model and wondering why the
output is bad.
Found by Copilot on #14.
Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_transcribe_endpoint.py (1)
126-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider an explicit exact-boundary case for the segment-duration filter.
Test 3b/3c's "hm" segment (0.00–0.10) is either right at or safely below
_MIN_SEGMENT_SECONDS, but that isn't visible here since the threshold constant isn't shown in this file. Given the commit history explicitly called out confusion around the strict (>) vs. inclusive (>=) comparison for this filter, a segment whose duration is exactly_MIN_SEGMENT_SECONDS(asserted to be dropped) would make the regression test unambiguous and future-proof against that comparison flipping back to>=.🤖 Prompt for 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. In `@tests/test_transcribe_endpoint.py` around lines 126 - 151, Update the duration-filter regression coverage around the 3b/3c transcribe cases to include a segment whose duration is exactly _MIN_SEGMENT_SECONDS, using the production threshold symbol rather than an inferred value. Assert that this boundary segment is dropped before _whisperx.align and that an input containing only such a segment returns no output, preserving the intended strict-greater-than behavior.
🤖 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.
Nitpick comments:
In `@tests/test_transcribe_endpoint.py`:
- Around line 126-151: Update the duration-filter regression coverage around the
3b/3c transcribe cases to include a segment whose duration is exactly
_MIN_SEGMENT_SECONDS, using the production threshold symbol rather than an
inferred value. Assert that this boundary segment is dropped before
_whisperx.align and that an input containing only such a segment returns no
output, preserving the intended strict-greater-than behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9758cf19-ca13-4a69-a30a-7dd51ea504b6
📒 Files selected for processing (2)
server.pytests/test_transcribe_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server.py
…the event loop
- /align still chose detected_lang with `normalized_lang if language` — the RAW value, which is
truthy for " ". So a whitespace hint took the "caller supplied a hint" path with an EMPTY
normalized value, and asked _get_whisperx_aligner() for a model in the language named "".
I introduced this last round by sharing the normalizer and updating only one of the two call
sites. Both branch on the normalized value now.
The test I wrote for the blank hint drove /transcribe, which never had the bug — so it passed
against the broken code. The new one drives /align, and fails against it ("aligner asked for
''").
- The upload writes ran on the event loop. The heavy transcription is offloaded to an executor,
but a chunked write to a slow disk (a NAS, an overloaded container) stalls every other request
in the process while one upload dribbles in — including /health, which is what tells a client
the server is alive. The careful part was being undone by the careless one. Writes now go
through asyncio.to_thread.
Found by Copilot on #14.
Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
traceback.print_exc() defaults to stderr, while the summary line below it goes to stdout — so one error was split across two streams. Under `docker logs` those interleave by arrival, which means the trace and the line naming the failure end up apart, next to unrelated output, in the exact situation where an operator is trying to read them together. Everything else in this server logs with print(), so the traceback follows it. Found by Copilot on #14. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Fixes the server half of feedBack-plugin-stem-splitter#17.
There was no transcribe endpoint
feedBack's "transcribe" path posts the vocals stem to
/align— and/alignis forced alignment, whosetextform field is required. FastAPI rejects the request with a 422 before the handler ever runs. Every time, for everyone.So remote transcription has never worked. It isn't a regression: the feature was wired to an endpoint that answers a different question, and no endpoint answered the right one.
/aligntext)/transcribeWhat it does
Whisper hears the words; wav2vec2 then force-aligns Whisper's own transcript. That second pass is why this isn't just
asr.transcribe()— raw Whisper segments are line-ish and drift, and a karaoke chart built from them sits visibly late.Returns native
whisperx.align()output — the exact shape the client's existing mapper already consumes, so nothing downstream has to learn a new one.An instrumental returns
{"segments": []}with a 200. A stem with no singing in it is a valid answer, not a failed request; a 400 there would make callers treat "this song has no vocals" as an error.The test drives a real FastAPI app
Deliberately. The bug lives in the validation layer above the handler, so a unit test calling
transcribe_audio(...)directly would pass while the endpoint 422s — which is precisely the blind spot that let this ship. The test posts a real request with no lyrics in it, and also asserts/alignstill 422s withouttext(if that ever stopped being true, the original bug would become invisible rather than fixed).Verified it fails against the unfixed server:
HARNESS_FAIL:/transcribe returned 404.47 tests green.
Client side
The matching change to
transcribe_vocals_remote()in corefeedback(lib/lyrics_transcribe.py) is a separate PR — it currently posts to/alignand sendslanguageas a query param where the server reads a form field, so both need fixing together. This PR is the prerequisite.Summary by CodeRabbit
New Features
{"segments": []}and the detectedlanguage.Documentation
languagevalidation details (trim/lowercase; malformed subtags rejected).Bug Fixes
Tests
/transcribeand/align, including language hint rules, segment filtering, instrumental handling, and error behavior.