fix: remote transcription posts to /transcribe, not /align (stem-splitter#17)

lib/lyrics_transcribe.py

@@ -23,9 +23,18 @@
 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
-
-    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,
-        )
+        form["language"] = language
 
+    # 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()
 

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")