fix: speaker detection — gated pyannote license surfaces a docs deeplink (closes #78) - #110
Merged
Merged
Conversation
…ink (closes #78) Issue #78 ("Speaker detection fails — speakers blend together or aren't detected correctly") was the user-visible symptom of the dub pipeline silently falling back to the silence-gap heuristic in `backend/api/routers/dub_core.py::_diarize`. The heuristic alternates Speaker 1 ↔ Speaker 2 on >1.2s gaps only, so two real speakers with similar pacing get merged or swapped — and once the auto-clone step extracts a reference voice for the wrong label, downstream dubs make "person A speak like person B" (the reporter's exact phrasing). The structural cause is that pyannote-3.1 is gated on HuggingFace: a valid HF_TOKEN by itself isn't enough — the user must also click "Agree and access repository" on both pyannote/speaker-diarization-3.1 AND pyannote/segmentation-3.0. We can't fix that for the user, but we CAN make the failure actionable instead of silent. Changes: - `backend/services/model_manager.py`: `get_diarization_pipeline()` gains an opt-in `return_error=True` shape that returns `(pipeline | None, error_sentinel)`. Sentinels distinguish NO_TOKEN / PYANNOTE_LICENSE_REQUIRED / LOAD_FAILED. A new `_classify_diarization_error()` sniffs the exception's class name + message for 401/403/gated/"accept license" signals — kept as a string heuristic so it survives huggingface_hub major-version churn. Bare-`None` default return preserved for the legacy `_transcribe` call site at dub_core.py:781. - `backend/api/routers/dub_core.py::_diarize`: now emits a structured SSE warning `{detail, source, error_class, docs_url}` instead of plain `{detail, source}`. The new fields let the front-end render a "See docs" button that deeplinks directly to the `License acceptance flow` section of `docs/features/diarization.md` (landed in PR #94) — the page with the click-by-click instructions for fixing this exact failure mode. - `backend/core/error_docs_map.py` + `frontend/src/utils/errorDocsMap.ts`: add a 5th taxonomy class `PYANNOTE_LICENSE_REQUIRED` pointing at the diarization docs section. Distinct from `HF_AUTH_FAILED` (which is the more general "token missing or invalid" case). The TS `classifyError` heuristic also picks up pyannote / gated / "speaker diarization" keywords so a thrown error in the boundary routes to the right deeplink too. - `tests/backend/core/test_error_docs_map.py`: bump locked-keys set to 5 classes; add an explicit assertion that the new class points at the `license-acceptance-flow` anchor. - `frontend/src/utils/errorDocsMap.test.ts`: bump locked-keys set to 5 classes; add classifier tests for pyannote / gated / accept-license keyword routing. - `tests/test_diarization_error_class.py`: regression test (20 cases) covering `_classify_diarization_error`, the new `get_diarization_pipeline(return_error=True)` shape, backward- compatible bare-`None` return for the legacy call site, and the error_docs_map deeplink target. Uses sys.modules patching so pyannote / torch are never actually imported. HF token plumbing: unchanged. The new code continues to route through `token_resolver.resolve()` per the AUTH-01 contract — no new bare `os.environ.get("HF_TOKEN")` reads. Cross-platform: identical behaviour on macOS / Windows / Linux — the only platform-touching change is a docs URL string, which is opened via the existing `openExternal()` helper that already abstracts Tauri's `shell.open` on all three platforms. Verification: .venv/bin/python -m pytest tests/test_diarization_error_class.py \ tests/backend/core/test_error_docs_map.py -v # 20 passed in 0.03s bun run test src/utils/errorDocsMap.test.ts # 13 passed (1 test file) .venv/bin/python -m pytest tests/test_segmentation.py \ tests/test_dub_transcribe.py \ tests/backend/services/test_token_resolver.py \ tests/test_model_manager_preload.py # 40 passed, 10 xfailed (pre-existing), 1 xpassed Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…78) Companion to the fix in d6e6586. 20 test cases covering: - `_classify_diarization_error` — the string heuristic that buckets pyannote/HF exceptions into NO_TOKEN / LICENSE / LOAD sentinels. Pinned for 401/403/gated/accept-license/accept-user-conditions signals so it survives huggingface_hub major-version churn. - `get_diarization_pipeline(return_error=True)` — the new 2-tuple return shape that lets the dub pipeline's SSE warning carry an error_class. - Backward compatibility — the bare-`None` return on the default signature is preserved so dub_core.py:781's legacy `_transcribe` call site doesn't break. - The error_docs_map deeplink — the new PYANNOTE_LICENSE_REQUIRED class points at `docs/features/diarization.md#license-acceptance-flow`. Uses sys.modules patching for pyannote.audio.Pipeline + token_resolver so the real torch + pyannote + HF API are never imported. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a 5-class diarization error taxonomy, classifies pyannote failures into missing-token/license/load sentinels, returns sentinel-aware pipeline tuples, and emits structured SSE warning events with docs deeplinks; frontend taxonomy and comprehensive tests were updated to match. ChangesDiarization Error Handling & Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@backend/api/routers/dub_core.py`:
- Around line 588-600: The SSE warning currently always sets error_class =
"PYANNOTE_LICENSE_REQUIRED" which hides load failures; change the logic in the
diarization failure handling (the block that references err_sentinel,
DIARIZATION_ERR_LOAD, resolved, and detail) to set error_class conditionally: if
err_sentinel indicates a license problem (the original license sentinel), keep
"PYANNOTE_LICENSE_REQUIRED", otherwise set a distinct load error class such as
"DIARIZATION_LOAD_FAILED" (or use DIARIZATION_ERR_LOAD) so runtime/load failures
are preserved and propagated to clients; apply the same conditional branching
where similar code appears around the other occurrence (lines ~619-623).
In `@tests/test_diarization_error_class.py`:
- Around line 34-37: The loop that tries to remove modules uses a conditional on
getattr(sys.modules.get(mod_name), "__file__", None) which prevents removing
already-imported modules and allows state leakage; change the logic to
unconditionally remove the entries from sys.modules for each mod_name (i.e.,
call sys.modules.pop(mod_name, None) for "core.config" and
"services.model_manager") so the fixture truly forces a fresh import when the
test runs.
🪄 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: 16178252-f613-4167-a423-ae5c97c1584d
📒 Files selected for processing (7)
backend/api/routers/dub_core.pybackend/core/error_docs_map.pybackend/services/model_manager.pyfrontend/src/utils/errorDocsMap.test.tsfrontend/src/utils/errorDocsMap.tstests/backend/core/test_error_docs_map.pytests/test_diarization_error_class.py
Comment on lines
+588
to
+600
| # err_sentinel == DIARIZATION_ERR_LOAD (or unexpected None | ||
| # with a resolved token — historical safety net). | ||
| who = resolved.username or "(whoami suppressed)" | ||
| reason = ( | ||
| detail = ( | ||
| f"Speaker diarization model failed to load even though an HF " | ||
| f"token was found (source={resolved.source}, user={who}). " | ||
| f"Most common cause: the pyannote/speaker-diarization-3.1 " | ||
| f"license has not been accepted on HuggingFace by this " | ||
| f"account. See backend logs for the underlying error. " | ||
| f"Falling back to a silence-gap heuristic; rapid speaker " | ||
| f"turns may be merged." | ||
| f"Most common causes: the pyannote/speaker-diarization-3.1 " | ||
| f"license has not been accepted on HuggingFace, or there is " | ||
| f"a pyannote/torch version mismatch. See backend logs for " | ||
| f"the underlying error. Falling back to a silence-gap " | ||
| f"heuristic; rapid speaker turns may be merged." | ||
| ) | ||
| return assign_speakers_heuristic(all_segments), reason | ||
| error_class = "PYANNOTE_LICENSE_REQUIRED" |
Contributor
There was a problem hiding this comment.
Preserve distinct load-vs-license error classes in SSE warnings
Line 600 and Line 620-623 currently collapse load failures into PYANNOTE_LICENSE_REQUIRED, so LOAD_FAILED never reaches clients. That misclassifies runtime/load errors as license issues and breaks the intended taxonomy.
Suggested fix
- error_class = "PYANNOTE_LICENSE_REQUIRED"
+ error_class = "LOAD_FAILED"
@@
- error_class = (
- "PYANNOTE_LICENSE_REQUIRED"
- if err_class_post == DIARIZATION_ERR_LICENSE
- else "PYANNOTE_LICENSE_REQUIRED" # LOAD failures land here too
- )
+ error_class = (
+ "PYANNOTE_LICENSE_REQUIRED"
+ if err_class_post == DIARIZATION_ERR_LICENSE
+ else "LOAD_FAILED"
+ )Also applies to: 619-623
🤖 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 `@backend/api/routers/dub_core.py` around lines 588 - 600, The SSE warning
currently always sets error_class = "PYANNOTE_LICENSE_REQUIRED" which hides load
failures; change the logic in the diarization failure handling (the block that
references err_sentinel, DIARIZATION_ERR_LOAD, resolved, and detail) to set
error_class conditionally: if err_sentinel indicates a license problem (the
original license sentinel), keep "PYANNOTE_LICENSE_REQUIRED", otherwise set a
distinct load error class such as "DIARIZATION_LOAD_FAILED" (or use
DIARIZATION_ERR_LOAD) so runtime/load failures are preserved and propagated to
clients; apply the same conditional branching where similar code appears around
the other occurrence (lines ~619-623).
…les purge
The new `test_diarization_error_class.py` tests pass in isolation but fail
in the full suite — Wave 1's `fresh_resolver` fixture aggressively purges
all `services.*` and `core.*` modules from `sys.modules` mid-suite. When
this file's tests later did `from services import token_resolver` then
`monkeypatch.setattr(token_resolver, "resolve", ...)`, the local
`token_resolver` reference bound to a stale module identity. The function
under test does `from services import token_resolver` at call time, which
re-resolves through the (post-purge) `sys.modules['services.token_resolver']`
— a different object — so the monkeypatch was applied to one ID and the
function read from another.
Two fixes in this commit:
1. Don't pop `services.token_resolver` in this file's `model_manager`
fixture — the test body's import and the function's import must agree
on identity. Popping forces re-import that can create two distinct
modules.
2. Use the dotted-path form `monkeypatch.setattr("services.token_resolver.resolve", ...)`
instead of the object-attribute form. Pytest's dotted form re-resolves
the path through `sys.modules` at setattr time, so the binding is
always on the live module object regardless of which identity the test
imported earlier.
Verified: `pytest tests/ -q` → 442 passed, 0 failures (was 1 failed
before this commit on PR #110).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Closes #78. The reporter saw two real speakers in the source video get merged into one or get swapped ("person A speaks like person B"). The structural cause is that pyannote-3.1 is gated on HuggingFace — a valid HF_TOKEN by itself isn't enough; the user must also click "Agree and access repository" on both
pyannote/speaker-diarization-3.1ANDpyannote/segmentation-3.0. When that hasn't happened,dub_core._diarizesilently falls back to a 1.2s-silence-gap heuristic that alternatesSpeaker 1↔Speaker 2and (worse) feeds the wrong reference clip to the auto-speaker-clone step downstream.We can't accept the license for the user, but we can make the failure mode discoverable. The fix shape is structured-error-with-docs-deeplink, not a code path correction.
Repro
POST /dub/transcribe-stream/{job_id}with no HF token (or with a token whose account hasn't accepted the pyannote license). Before this PR: SSEwarningevent with plain{detail, source}, no actionable next step. After: SSEwarningcarries{detail, source, error_class, docs_url}wheredocs_urldeeplinks to the "License acceptance flow" section ofdocs/features/diarization.md(landed in PR #94).Root cause
services/model_manager.py::get_diarization_pipeline()caught every load exception identically and returned bareNone. The dub pipeline then couldn't distinguish "no token" from "license not accepted" from "torch/pyannote version mismatch", so the warning toast was the same for all three.warningevent the front-end consumes had noerror_class/docs_urlfields, so the existing error→docs map (backend/core/error_docs_map.pyfrom PR Phase 1 Wave 2: per-OS install docs + Settings UI + error→docs deeplinks #94) never got plumbed for the diarization path.classifyErrorheuristic treated any pyannote 401 as the genericHF_AUTH_FAILEDclass, which deeplinks to the HF-token-setup doc — useful, but not the page with the "click Agree on pyannote/speaker-diarization-3.1" instructions.Fix shape
Better error surface, per the issue brief. No code-path correction is possible — the license is the user's to accept.
backend/services/model_manager.py:get_diarization_pipeline()gains opt-inreturn_error=Trueshape returning(pipeline | None, error_sentinel). Sentinels:NO_TOKEN/PYANNOTE_LICENSE_REQUIRED/LOAD_FAILED. New_classify_diarization_error()sniffs the exception name + message for 401/403/gated/accept-license signals — string-based so it surviveshuggingface_hubmajor-version churn (theGatedRepoError/HfHubHTTPErrorsymbols aren't stable). Bare-Nonedefault is preserved for the legacy_transcribecall site atdub_core.py:781.backend/api/routers/dub_core.py::_diarize: emits structured SSE warning{detail, source, error_class, docs_url}with the new fields populated fromerror_docs_map.lookup(error_class).backend/core/error_docs_map.py+frontend/src/utils/errorDocsMap.ts: add 5th taxonomy classPYANNOTE_LICENSE_REQUIRED→docs/features/diarization.md#license-acceptance-flow. Distinct fromHF_AUTH_FAILED(which is the more general "token missing or invalid" case). TSclassifyErrorchecks pyannote / gated / accept-license keywords BEFORE the generic 401 branch so the more specific deeplink wins.HF token plumbing
Unchanged. All reads still go through
services/token_resolver.py::resolve()per AUTH-01.grep -n "os.environ.get.*HF_TOKEN" backend/services/model_manager.py backend/api/routers/dub_core.pyis still empty.Cross-platform
Identical behaviour on macOS / Windows / Linux — the only platform-touching change is a docs URL string opened via the existing
openExternal()helper that abstracts Tauri'sshell.open.Test plan
.venv/bin/python -m pytest tests/test_diarization_error_class.py tests/backend/core/test_error_docs_map.py -v— 20 passed in 0.02sbun run test src/utils/errorDocsMap.test.ts— 13 passed (1 file), including new classifier cases for pyannote / gated / accept-license keyword routing.venv/bin/python -m pytest tests/test_segmentation.py tests/test_dub_transcribe.py tests/backend/services/test_token_resolver.py tests/test_model_manager_preload.py— 40 passed, 10 xfailed (pre-existing), 1 xpassedPOST /dub/transcribe-stream/{job_id}against a multi-speaker clip with the pyannote license not accepted on the dev HF account emits the SSEwarningevent witherror_class=PYANNOTE_LICENSE_REQUIREDanddocs_urlpointing at the diarization docs section.Test coverage added
tests/test_diarization_error_class.py— 20 cases:_classify_diarization_error— 401 / 403 / gated repo / "accept license" / "accept user conditions" / named exception class →LICENSE; CUDA OOM / pickle weights-only →LOAD.get_diarization_pipeline(return_error=True)— NO_TOKEN / LICENSE / LOAD all return the right sentinel; bare-Nonelegacy shape preserved.error_docs_map—PYANNOTE_LICENSE_REQUIREDdeeplinks todocs/features/diarization.md#license-acceptance-flow; new class is in the locked taxonomy.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests