diff --git a/lib/lyrics_transcribe.py b/lib/lyrics_transcribe.py index 86e1051..8c15312 100644 --- a/lib/lyrics_transcribe.py +++ b/lib/lyrics_transcribe.py @@ -23,9 +23,18 @@ Engine selection Two transcription paths share a common output: * `transcribe_vocals_remote(path, server_url, ...)` — POST the vocal - stem to the `/align` endpoint on a feedBack-demucs-server (got-feedBack's - reference server already hosts WhisperX alongside Demucs at the same - URL). + stem to the `/transcribe` endpoint on a feedBack-demucs-server + (got-feedBack's reference server already hosts WhisperX alongside + Demucs at the same URL). + + It used to POST to `/align`, which is *forced alignment* — "here are + the lyrics, tell me when each word is sung". Its `text` field is + required and we have no lyrics (transcribing them is the point), so + the server answered 422 from FastAPI's validation layer before its + handler ran, and remote transcription never worked for anyone + (feedBack-plugin-stem-splitter#17). `/transcribe` takes only audio. + Requires feedBack-demucs-server ≥ the revision adding that endpoint; + an older server answers 404 and the error says so. * `transcribe_vocals_local(path, ...)` — load WhisperX in-process. Heavy (~3 GB of model weights for `large-v2` + the wav2vec2 aligner) and @@ -416,6 +425,38 @@ def transcribe_vocals_local( # ── Remote transcription ──────────────────────────────────────────────────── +_MAX_ERR_BODY = 4000 + + +def _err_body(resp) -> str: + """The server's error body, whole if it plausibly is one, and marked when it isn't. + + This was capped at 300 chars, which is enough for "Internal Server Error" and not much else. + The bodies carrying the most diagnosis are the long ones — a FastAPI validation body naming + the field it rejected, a 500 whose traceback answers on its LAST line — and those are exactly + the ones a 300-char cap decapitates. The cap survives so a server answering with a 2 MB HTML + error page can't dump a novel into a log line. + """ + # Strip FIRST, then measure: a body that is 300 chars of JSON and 3900 of trailing whitespace + # is not a long body, and truncating it would cut real content to make room for blanks. + text = (getattr(resp, "text", "") or "").strip() + if len(text) <= _MAX_ERR_BODY: + return text + + # Keep the HEAD **and the TAIL**. Head-only truncation throws away the exception line — and + # on a traceback the exception line is the answer. This docstring said as much while the code + # did the opposite: it cut off precisely the part it exists to preserve, which is the same + # mistake, one level up, as the 300-char cap it replaced. + # + # The marker sits inside the bound, not past it: otherwise _MAX_ERR_BODY is a suggestion, and + # the callers who trust it (a log line, a job record persisted to disk) are the ones surprised. + marker = f"\n… [truncated, {len(text)} chars total] …\n" + budget = max(0, _MAX_ERR_BODY - len(marker)) + head = budget * 2 // 3 # context: what was being attempted + tail = budget - head # verdict: what actually went wrong + return text[:head].rstrip() + marker + text[len(text) - tail:].lstrip() + + def transcribe_vocals_remote( vocals_path: Path, server_url: str, @@ -426,7 +467,17 @@ def transcribe_vocals_remote( min_word_score: float = 0.35, progress_cb: ProgressCB = None, ) -> list[dict]: - """POST the vocal stem to `{server_url}/align` and parse the response. + """POST the vocal stem to `{server_url}/transcribe` and parse the response. + + NOT `/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; producing them is the point. Posting there returned a + 422 from FastAPI's validation layer before the server's handler ran, so + remote transcription never worked at all + (feedBack-plugin-stem-splitter#17). + + Requires a feedBack-demucs-server carrying `/transcribe`; an older one + answers 404 and the raised error says so. Expects the server to respond with a JSON object carrying a `words` (or `segments`) field in WhisperX's native shape; `_whisperx_to_sloppak` @@ -454,21 +505,53 @@ def transcribe_vocals_remote( if api_key: headers["Authorization"] = f"Bearer {api_key}" - params: dict[str, str] = {} + # POST to /transcribe, not /align. + # + # /align is FORCED ALIGNMENT: "here are the lyrics, tell me when each word is sung". Its + # `text` field is required, and we have no lyrics — transcription is the whole point. So the + # server rejected every request with a 422 in FastAPI's validation layer, before its handler + # ever ran, and remote transcription has never worked for anyone. /transcribe answers the + # question we are actually asking and takes only the audio. + # (feedBack-plugin-stem-splitter#17; endpoint added in feedBack-demucs-server#14.) + # + # `language` goes in the FORM BODY, not the query string: the server reads it with + # Form(""), and a query param would be silently ignored — so an explicit language hint would + # do nothing and Whisper's auto-detection would quietly decide instead, which is exactly the + # kind of "it works but it's wrong" that hides for months. + form: dict[str, str] = {} if language: - params["language"] = language + form["language"] = language - with open(vocals_path, "rb") as f: - resp = requests.post( - f"{server_url}/align", - files={"file": (vocals_path.name, f, "audio/ogg")}, - params=params, - headers=headers or None, - timeout=timeout, + # Everything that can go wrong out here comes back as RuntimeError, which is what the + # docstring promises and what the caller catches. A DNS failure, a timeout, a reset + # connection or an unreadable stem file would otherwise surface as requests.RequestException + # or OSError and escape the one handler written to log-and-continue — turning "this song's + # lyrics failed" into "the whole batch died". + try: + with open(vocals_path, "rb") as f: + resp = requests.post( + f"{server_url}/transcribe", + files={"file": (vocals_path.name, f, "audio/ogg")}, + data=form or None, + headers=headers or None, + timeout=timeout, + ) + except requests.RequestException as e: + raise RuntimeError(f"could not reach the WhisperX server at {server_url}: {e}") from e + except OSError as e: + raise RuntimeError(f"could not read the vocal stem {vocals_path.name}: {e}") from e + + if resp.status_code == 404: + # The endpoint isn't there. Say what that means, because "404" on its own sends someone + # hunting for a typo in their URL when the real answer is that their server predates the + # feature. (feedBack-demucs-server#14 added /transcribe.) + raise RuntimeError( + f"the WhisperX server at {server_url} has no /transcribe endpoint (404) — it " + f"predates remote transcription support. Update the server, or use 'Check for " + f"update' if it is the plugin-managed one." ) - if resp.status_code != 200: - raise RuntimeError(f"WhisperX server error ({resp.status_code}): {resp.text[:300]}") + raise RuntimeError(f"WhisperX server error ({resp.status_code}): {_err_body(resp)}") data = resp.json() diff --git a/tests/test_lyrics_transcribe_remote.py b/tests/test_lyrics_transcribe_remote.py new file mode 100644 index 0000000..f826bc6 --- /dev/null +++ b/tests/test_lyrics_transcribe_remote.py @@ -0,0 +1,201 @@ +"""The remote transcription REQUEST — the thing that was never tested and never worked. + +`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". Its `text` field is required, +and 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 had never worked for anybody (feedBack-plugin-stem-splitter#17). + +Nothing caught it because 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. + +So these tests assert the request: which endpoint, and how `language` is carried. Both are +invisible to a mapper test, and both are wrong in ways that fail quietly rather than loudly. +""" +from pathlib import Path +from unittest import mock + +import pytest + +from lyrics_transcribe import transcribe_vocals_remote + +_ALIGNED = { + "segments": [{ + "start": 1.0, "end": 2.0, "text": "hello world", + "words": [ + {"word": "hello", "start": 1.0, "end": 1.4, "score": 0.9}, + {"word": "world", "start": 1.5, "end": 2.0, "score": 0.9}, + ], + }] +} + + +class _Resp: + def __init__(self, status=200, payload=None, text=""): + self.status_code = status + self._payload = payload if payload is not None else _ALIGNED + self.text = text + + def json(self): + return self._payload + + +@pytest.fixture +def vocals(tmp_path: Path) -> Path: + p = tmp_path / "vocals.ogg" + p.write_bytes(b"not really ogg, we never decode it here") + return p + + +def _post_call(vocals: Path, resp: _Resp, **kw): + with mock.patch("requests.post", return_value=resp) as post: + out = transcribe_vocals_remote(vocals, "http://server:7865", **kw) + return post.call_args, out + + +def test_it_posts_to_transcribe_not_align(vocals): + """THE regression. /align requires `text`; we have none, so it 422s every time.""" + call, out = _post_call(vocals, _Resp()) + + url = call.args[0] + assert url.endswith("/transcribe"), ( + f"posted to {url!r} — /align is forced alignment and its `text` field is required, so " + f"this request is rejected with a 422 before the server's handler ever runs" + ) + assert "/align" not in url + assert out, "a successful transcription must return syllables" + + +def test_the_language_hint_is_a_form_field_not_a_query_param(vocals): + """The server reads `language` with Form(""). Sent as a query param it is silently ignored — + so an explicit hint does nothing, Whisper's auto-detection quietly decides instead, and the + wrong wav2vec2 aligner gets loaded. It "works", it's just wrong: the failure mode that hides + for months.""" + call, _ = _post_call(vocals, _Resp(), language="es") + + assert (call.kwargs.get("data") or {}).get("language") == "es", ( + "the language hint must ride in the form body — the server reads Form('language'), and " + "a query param is dropped without a word" + ) + assert "language" not in (call.kwargs.get("params") or {}) + + +def test_no_language_sends_no_hint(vocals): + # Absent is not the empty string: "" would pin detection to a language named "". + call, _ = _post_call(vocals, _Resp()) + assert not (call.kwargs.get("data") or {}) + + +def test_the_file_is_sent_as_a_multipart_upload(vocals): + call, _ = _post_call(vocals, _Resp()) + files = call.kwargs.get("files") or {} + assert "file" in files, "the server reads File('file')" + assert files["file"][0] == "vocals.ogg" + + +def test_an_api_key_is_sent_as_a_bearer_token(vocals): + call, _ = _post_call(vocals, _Resp(), api_key="secret") + assert (call.kwargs.get("headers") or {})["Authorization"] == "Bearer secret" + + +def test_an_instrumental_is_an_answer_not_a_crash(vocals): + # The server returns 200 + no segments for a stem with no singing in it. That is a valid + # answer ("this song has no vocals"), and it must not read as a failure. + _, out = _post_call(vocals, _Resp(payload={"segments": [], "language": "en"})) + assert out == [] + + +def test_a_server_error_surfaces_the_whole_body(vocals): + """The error body IS the diagnosis. A 422's JSON names the field it rejected; a 500's + traceback answers on its last line. The old 300-char cap decapitated both — which is how + this bug stayed invisible: the message explaining it was inside the part that got cut.""" + tb = "Traceback (most recent call last):\n" + (" File x, line 1\n" * 40) + \ + "RuntimeError: CUDA out of memory" + assert len(tb) > 300 and "CUDA out of memory" not in tb[:300] + + with pytest.raises(RuntimeError) as exc: + _post_call(vocals, _Resp(status=500, text=tb)) + assert "CUDA out of memory" in str(exc.value) + + +def test_truncation_keeps_the_exception_line_not_just_the_header(): + """A traceback's ANSWER is its last line. Head-only truncation throws it away. + + This is the same mistake as the 300-char cap, one level up: cutting off precisely the part + the function exists to preserve. A 4000-char window that contains "Traceback (most recent + call last)" and none of the exception is a window onto nothing.""" + from lyrics_transcribe import _MAX_ERR_BODY, _err_body + + frames = "".join(f' File "/app/server.py", line {i}, in run\n step()\n' + for i in range(2000)) # far over the cap on its own + tb = "Traceback (most recent call last):\n" + frames + \ + "RuntimeError: CUDA out of memory. Tried to allocate 2.20 GiB" + + body = _err_body(_Resp(text=tb)) + assert len(body) <= _MAX_ERR_BODY + assert "CUDA out of memory" in body, ( + "the exception line is the diagnosis — a truncation that drops it keeps the part that " + "says work was happening and discards the part that says what went wrong" + ) + assert "Traceback (most recent call last)" in body, "the head is context worth keeping too" + assert "truncated" in body + + +def test_the_cap_is_a_bound_not_a_suggestion(): + """The truncation marker must fit INSIDE _MAX_ERR_BODY, not be appended past it. + + Otherwise the cap is advisory, and the callers who trust it — a log line, a job record + persisted to disk and re-read on every load — are the ones that get surprised.""" + from lyrics_transcribe import _MAX_ERR_BODY, _err_body + + body = _err_body(_Resp(text="x" * 500_000)) + assert len(body) <= _MAX_ERR_BODY, ( + f"body is {len(body)} chars, over the {_MAX_ERR_BODY} cap it claims to enforce" + ) + assert "truncated" in body and "500000" in body + + +def test_trailing_whitespace_is_not_content(): + # A 300-char JSON body followed by 3900 blanks is not a long body, and cutting real content + # to make room for whitespace would be a silly way to lose the diagnosis. + from lyrics_transcribe import _err_body + + payload = '{"detail":"nope"}' + assert _err_body(_Resp(text=payload + " " * 8000)) == payload + + +def test_a_404_explains_that_the_server_is_too_old(vocals): + """A bare "404" sends someone hunting for a typo in their URL. The real answer is that their + server predates the endpoint, and only we can know that.""" + with pytest.raises(RuntimeError) as exc: + _post_call(vocals, _Resp(status=404, text='{"detail":"Not Found"}')) + msg = str(exc.value) + assert "404" in msg + assert "/transcribe" in msg + assert "predates" in msg or "Update the server" in msg + + +class TestEverythingFailsAsRuntimeError: + """The docstring promises one failure mode: RuntimeError. The caller + (`_maybe_transcribe_lyrics`) catches exactly that so one song's failed lyrics don't take down + the batch around it. A transport error escaping as requests.RequestException walks straight + past that handler — turning "this song's lyrics failed" into "the whole batch died".""" + + def test_a_connection_failure(self, vocals): + import requests + with mock.patch("requests.post", + side_effect=requests.ConnectionError("name resolution failed")): + with pytest.raises(RuntimeError, match="could not reach"): + transcribe_vocals_remote(vocals, "http://nope:7865") + + def test_a_timeout(self, vocals): + import requests + with mock.patch("requests.post", side_effect=requests.Timeout("timed out")): + with pytest.raises(RuntimeError, match="could not reach"): + transcribe_vocals_remote(vocals, "http://server:7865") + + def test_an_unreadable_stem(self, tmp_path): + missing = tmp_path / "gone.ogg" # never created + with pytest.raises(RuntimeError, match="could not read"): + transcribe_vocals_remote(missing, "http://server:7865")