Repository navigation
fix: remote transcription posts to /transcribe, not /align (stem-splitter#17) - #959
Conversation
…tter#17)
transcribe_vocals_remote() POSTed the vocal stem to /align. That endpoint is FORCED ALIGNMENT —
"here are the lyrics, tell me when each word is sung" — and its `text` field is required. We have
no lyrics; transcribing them is the entire point. So the server rejected every request with a 422
from FastAPI's validation layer, before its handler ever ran, and remote transcription has never
worked for anyone.
It now posts to /transcribe (added in feedBack-demucs-server#14), which takes only the audio.
`language` moves from the query string to the FORM BODY, where the server actually reads it
(Form("")). As a query param it was silently ignored, so an explicit hint did nothing and
Whisper's auto-detection quietly decided instead — loading the wrong wav2vec2 aligner. It
"worked", it was just wrong, which is the failure mode that hides for months.
Error bodies are no longer cut at 300 chars. The body IS the diagnosis: a 422's JSON names the
field it rejected, a 500's traceback answers on its LAST line. Both got decapitated — which is
part of why this stayed invisible for so long. The message explaining the bug was inside the part
that got cut.
Nothing caught any of this because every test of this module tested the MAPPER, fed a hand-written
dict. The mapper was always fine. The request was never exercised, and the request was the bug.
tests/test_lyrics_transcribe_remote.py now pins it: the endpoint, the form field, the multipart
upload, the bearer token, an instrumental returning no lyrics rather than an error, and a 404
saying the server is too old. Verified they FAIL against /align + params.
Signed-off-by: topkoa <topkoa@gmail.com>
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRemote vocal transcription now posts to ChangesRemote transcription flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant transcribe_vocals_remote
participant TranscriptionServer
Client->>transcribe_vocals_remote: Submit vocals and optional language
transcribe_vocals_remote->>TranscriptionServer: POST multipart request to /transcribe
TranscriptionServer-->>transcribe_vocals_remote: Return segments or error body
transcribe_vocals_remote-->>Client: Return mapped lyrics or RuntimeError
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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)
tests/test_lyrics_transcribe_remote.py (1)
109-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new >4,000-character truncation path.
This fixture exceeds 300 characters but remains below
_MAX_ERR_BODY, so it verifies removal of the old cutoff without testing the new bound or truncation marker. Add a separate oversized-body case.Proposed test
+def test_a_very_large_server_error_is_bounded(vocals): + body = "x" * 5000 + with pytest.raises(RuntimeError) as exc: + _post_call(vocals, _Resp(status=500, text=body)) + assert "… [truncated, 5000 chars total]" in str(exc.value)🤖 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_lyrics_transcribe_remote.py` around lines 109 - 119, Add a separate test near test_a_server_error_surfaces_the_whole_body that supplies a response body longer than _MAX_ERR_BODY, then assert the raised RuntimeError contains the retained truncated content and the truncation marker while excluding content beyond the limit. Keep the existing test focused on preserving complete bodies below the new bound.
🤖 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 `@lib/lyrics_transcribe.py`:
- Around line 484-511: Update the public docstring of the transcription function
containing the `/transcribe` request to document the `/transcribe` endpoint
instead of the obsolete `/align` endpoint, keeping the documentation consistent
with the implementation.
---
Nitpick comments:
In `@tests/test_lyrics_transcribe_remote.py`:
- Around line 109-119: Add a separate test near
test_a_server_error_surfaces_the_whole_body that supplies a response body longer
than _MAX_ERR_BODY, then assert the raised RuntimeError contains the retained
truncated content and the truncation marker while excluding content beyond the
limit. Keep the existing test focused on preserving complete bodies below the
new bound.
🪄 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: fc31b7e3-4158-4252-95dc-1bc5c0ffca95
📒 Files selected for processing (2)
lib/lyrics_transcribe.pytests/test_lyrics_transcribe_remote.py
There was a problem hiding this comment.
Pull request overview
Fixes the remote WhisperX transcription client so it calls the correct server endpoint and sends parameters in the shape the server expects, and adds focused tests to prevent regressions in the HTTP request contract.
Changes:
- Switch remote transcription from
POST /align(forced-alignment) toPOST /transcribe(audio-only transcription). - Move the
languagehint from query params to multipart form data, and keep API key auth via Bearer token. - Improve error reporting by surfacing larger server error bodies and add a new test suite that asserts the exact request shape and key failure modes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/lyrics_transcribe.py |
Updates the remote transcription request (endpoint + form field) and improves server error body handling. |
tests/test_lyrics_transcribe_remote.py |
Adds request-level tests that pin endpoint selection, multipart upload shape, language handling, auth header, and error surfacing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… endpoint - _err_body() appended the truncation marker AFTER slicing to _MAX_ERR_BODY, so the result could exceed the cap it exists to enforce (4014 chars for a 4000 bound). A cap that is only a suggestion surprises exactly the callers who trust it — a log line, a job record persisted to disk and re-read on every load. The marker now fits inside the bound. It also stripped after measuring, so a short JSON body followed by kilobytes of trailing whitespace got truncated: real content cut to make room for blanks. Strip first, then measure. - The public docstring still advertised /align — the exact contract this PR exists to change, in the one place a reader would look for it. It now says what the function does and why, and that an older server answers 404. Found by Copilot and CodeRabbit on #959. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
_err_body() kept only the HEAD of an over-long body. On a traceback the last line is the diagnosis, and the docstring said exactly that while the code threw it away: a 4000-char window holding "Traceback (most recent call last)" and none of the exception is a window onto nothing. Same mistake as the 300-char cap it replaced, one level up — cutting off precisely the part the function exists to preserve. Head AND tail now, both inside the bound: two thirds head (what was being attempted), one third tail (what actually went wrong), with the marker between them. Verified the test FAILS against head-only truncation. Found by Copilot on #959. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
| with open(vocals_path, "rb") as f: | ||
| resp = requests.post( | ||
| f"{server_url}/align", | ||
| f"{server_url}/transcribe", | ||
| files={"file": (vocals_path.name, f, "audio/ogg")}, | ||
| params=params, | ||
| data=form or None, |
There was a problem hiding this comment.
Both good, and the first has teeth: the caller (_maybe_transcribe_lyrics) catches RuntimeError specifically, so one song's failed lyrics don't take down the batch around it. A DNS failure or a timeout coming out as requests.RequestException walks straight past that handler — turning "this song's lyrics failed" into "the whole batch died", which is a much worse day for someone converting an album.
Every exit is a RuntimeError now: transport errors, an unreadable stem, and non-200s. Tests cover connection failure, timeout, and a missing stem file.
The 404 explains itself too — a bare "404" sends someone hunting for a typo in their server URL, when the real answer is that their server predates /transcribe and only we are in a position to know that.
…r; 404 says why The docstring promised one failure mode — RuntimeError — and the caller (_maybe_transcribe_lyrics) catches exactly that so one song's failed lyrics don't take down the batch around it. But a DNS failure, a timeout, a reset connection or an unreadable stem escaped as requests.RequestException or OSError, walked straight past that handler, and turned "this song's lyrics failed" into "the whole batch died". A 404 now explains itself. Bare "404" sends someone hunting for a typo in their server URL; the real answer is that their server predates /transcribe, and we are the only ones in a position to know that. Found by Copilot on #959. Signed-off-by: topkoa <topkoa@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Client half of feedBack-plugin-stem-splitter#17. Pairs with feedBack-demucs-server#14, which adds the endpoint — merge that one first.
Remote transcription has never worked
transcribe_vocals_remote()POSTs the vocal stem to/align. That endpoint is forced alignment — "here are the lyrics, tell me when each word is sung" — and itstextfield is required. We have no lyrics; transcribing them is the entire point.So the server rejected every request with a 422 from FastAPI's validation layer, before its handler ever ran. Not a regression — this path has never worked for anybody.
Three fixes
POST to
/transcribe, which takes only the audio.languagemoves from the query string to the form body, where the server actually reads it (Form("")). As a query param it was silently ignored — so an explicit hint did nothing, Whisper's auto-detection quietly decided instead, and the wrong wav2vec2 aligner got loaded. It "worked", it was just wrong: the failure mode that hides for months.Error bodies are no longer cut at 300 chars. The body is the diagnosis — a 422's JSON names the field it rejected, a 500's traceback answers on its last line. Both got decapitated, which is part of why this stayed invisible: the message explaining the bug was inside the part that got cut.
Why nothing caught it
Every test of this module tested the mapper (
_whisperx_to_sloppak), fed a hand-written dict. The mapper was always fine. The request was never exercised, and the request was the bug.tests/test_lyrics_transcribe_remote.pynow pins the request itself: the endpoint, the form field, the multipart upload, the bearer token, an instrumental returning no lyrics rather than an error, and a 404 carrying "the server is too old" through instead of swallowing it.Verified the two key tests fail against the old
/align+paramscall.Compatibility
A server predating
/transcribeanswers 404, and the error now says so plainly rather than being truncated into noise.Lyrics suite green (25). The two
test_builtin_diagnostic_seedsymlink failures are pre-existing onmain— a Windows symlink-privilege artefact, unrelated to this change.Summary by CodeRabbit
New Features
Bug Fixes
Tests