feat: optional MT3 polyphonic-transcription stage + interpretation grounding gate - #122
Conversation
Add MT3 (Magenta Multi-Task Multitrack) as a flag-gated, additive analysis stage that emits per-instrument MIDI. Opt-in per run via mt3_mode=enabled; absent/inert otherwise. MT3 never overrides Phase 1 measurements (PURPOSE.md invariant #1) — it is additive grounding only. Core stage: - mt3_transcription.py: lazy-imported MT3/JAX module (Mt3Result/Mt3Track, camelCase to_payload) so JAX never loads at server startup. - analysis_runtime.py: mt3_attempts table + lifecycle (create/reserve/ complete/fail), preferred-attempt pointer, snapshot, enqueue-on-measurement, recovery/interrupt/delete wiring. - server.py: _execute_mt3_attempt subprocess executor (artifact-ref MIDI), _mt3_worker_loop, per-run opt-in route, error classification. - requirements-mt3.txt: optional extra (JAX/t5x/note-seq), kept out of the product venv. Follow-ups: - F1: bidirectional Demucs stems handover — MT3 reuses pitch/note's stems when present, else writes its own for pitch/note to reuse (analyze.py). - F2: estimate endpoint prices the MT3 stage when requested (analyze_estimate.py + server.py _build_backend_estimate). - F3: Phase 2 grounding forwards mt3Result to the prompt and citation validator; phase2_system.txt declares OPTIONAL_MT3_TRANSCRIPTION_RESULT_JSON. - F4: frontend Mt3TranscriptionPanel + mt3Client (lazy per-track MIDI download), wired into AnalysisResults' Session Musician suite + StickyNav. Tests: mt3_transcription unit, runtime lifecycle, executor, estimate, citation-path, grounding, and frontend client. JSON_SCHEMA.md + CLAUDE.md document the stage and contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to 4624faf (optional MT3 stage), addressing audit findings. 1. mt3_mode validation was silently permissive: _coerce_mt3_mode coerced any unrecognized value to "off", so a client typo like "enable" quietly ran the analysis with MT3 disabled instead of erroring. The UnsupportedMt3ModeError, the MT3_MODE_UNSUPPORTED 400 catch, and the contract documented in analysisRunsClient.ts were all dead for HTTP clients. _coerce_mt3_mode now validates a real, non-empty string strictly (raising UnsupportedMt3ModeError) while still mapping the FastAPI Form(...) sentinel and empty values to "off". Added the matching catch to the estimate route and HTTP-path tests for both create + estimate. Mirrors pitch_note_mode's strict rejection. 2. requirements-mt3.txt + mt3_transcription.py docs now mandate a SEPARATE venv. The product venv pins numpy==2.4.3 / protobuf==7.34.0; JAX/t5x + note_seq pull older transitive versions that would break Essentia/torch on the request path. Dropped the "or install into the product venv" option and the stray error-message command that pointed there. 3. Fixed a stale measurement.ts comment claiming the frontend doesn't render MT3 — Mt3TranscriptionPanel mounts it from AnalysisResults. Additive-only and chain-of-custody invariants unchanged. Backend 997 OK, frontend lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Makes the committed MT3 stage reachable from the UI. Previously mt3_mode could only be set via an out-of-band curl: the panel + transport were wired, but no control ever set the mode. - VITE_ENABLE_MT3 flag (default OFF) gates the control only — the results panel still renders any MT3 result regardless of the flag. MT3 needs a separate venv + ~4GB weights, so it stays operator-gated (mirrors VITE_ENABLE_PHASE2_GEMINI). - App.tsx: mt3Requested state + a checkbox mirroring PITCH/NOTE TRANSLATION (native input so Playwright can drive it); mt3Mode threaded into estimate + run, computed as `mt3Requested && mt3ConfigEnabled` so a stale flag can't leak 'enabled'. - analyzer.ts: mt3Requested option + resolver; mt3Mode on createAnalysisRun (re-gated by the flag); isRunTerminal now waits for the mt3 stage (optional-chained, absent/not_requested == terminal) so an enabled MT3 result isn't missed when interpretation finishes first. - config.ts/vite-env.d.ts: enableMt3 + isMt3ConfigEnabled, mirroring the Phase 2 flag plumbing. - Tests: unit form-field coverage (create + estimate + off default), a pin for the flag-off re-gate, and a smoke test (opt-in sends mt3_mode=enabled + panel renders; flag-off hides the checkbox). Additive only; Phase 1 stays authoritative. Frontend-only — no backend change. Reviewed by 3 sequential Opus passes (correctness, invariants, regression); npm run verify green (smoke 48 passed, lint/unit/build clean). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Interpretation was a peer of the mt3 and pitch_note stages — all three became
reservable the moment measurement completed, so they raced. MT3 loads multi-GB
JAX weights (~30s + minutes of inference) and usually lost, leaving Gemini's
additive grounding (mt3Result / pitchNoteResult) None by the time interpretation
ran. So the F3 "Phase 2 grounding consumes MT3" plumbing rarely delivered.
Make interpretation wait for its grounding peers to settle:
- reserve_next_interpretation_attempt: add two correlated NOT EXISTS guards so a
queued interpretation attempt is ineligible while an mt3 OR pitch_note attempt
for the same run is in-flight ('queued'/'running'). Terminal states
(completed/failed/interrupted) unblock it, so a failure / interrupt / restart
recovery never deadlocks — interpretation then runs with whatever grounding is
available (possibly None), exactly as before. Reads no requested_*_mode: no
in-flight rows means the stage wasn't requested or already settled.
- _enqueue_requested_followups: create the interpretation attempt LAST, after
pitch_note and mt3. Each create commits in its own transaction; were
interpretation created first, the 0.25s-poll worker could reserve an ungrounded
attempt in the window before a grounding row commits (the mt3-on/pitch_note-off
case). Creating it last guarantees the grounding rows exist whenever it does.
- _execute_mt3_attempt / _execute_pitch_note_attempt: resolve get_source_artifact
INSIDE the try (it sat before it) and null-safe the except diagnostics. Pre-gate
this was harmless; with the gate, an attempt left 'running' by a pre-try raise
would block the run's interpretation indefinitely. Now any failure terminalizes,
keeping the no-deadlock guarantee airtight.
Pure scheduling — changes WHEN interpretation runs, never what any stage
produces. Phase 1 stays authoritative (PURPOSE.md #1); camelCase contract and
types.ts untouched; snapshot keeps emitting 'queued' while waiting.
Tests (+11): InterpretationGatingTests (mt3-blocks, pitch_note-blocks,
waits-for-both-then-unblocks, failed-unblocks, no-stages-immediate,
recovery-unblocks, enqueue-creates-interpretation-last); fix the existing
progress test to settle pitch_note before reserving interpretation; end-to-end
_build_phase2_prompt test proving a non-null mt3_result surfaces under
OPTIONAL_MT3_TRANSCRIPTION_RESULT_JSON; mt3 TimeoutExpired + get_source_artifact
failure terminalization tests (the no-deadlock linchpins). Full backend suite:
1008 passed, 3 skipped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
slittycode
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Adds MT3 as an optional, flag-gated, purely additive fourth stage behind mt3_mode=enabled per run — inert unless explicitly requested, heavy deps isolated to a separate venv, subprocess-isolated so JAX never enters the server process. The headline correctness change is the interpretation grounding gate: reserve_next_interpretation_attempt now blocks on in-flight MT3 and pitch-note rows, and _enqueue_requested_followups creates the interpretation row last so the gate can't be raced. Phase 1 contract is untouched. I cannot run the test suites in this environment (venv/node_modules not set up), but I read every new test file and the coverage of the gate logic is thorough: 8 scenarios including the no-deadlock and restart-recovery paths. The PR description's claim of 1008 backend / 704 frontend tests passing is consistent with the test structure I verified.
Findings
Should fix
server.py:1571 — the _Mt3UnavailableError handler accesses source_artifact["artifactId"] directly, not null-safe:
except _Mt3UnavailableError as exc:
runtime.fail_mt3_attempt(
attempt_id,
...
diagnostics={
"sourceArtifactId": source_artifact["artifactId"], # ← not null-safeThe generic except Exception handler four lines below correctly uses (source_artifact or {}).get("artifactId") (line 1588), with a comment explaining why. _Mt3UnavailableError can only be raised after subprocess.run() completes, which comes after get_source_artifact succeeds, so source_artifact is never None at that point — safe today by invariant. But the whole point of the get_source_artifact hardening commit was to make all except-handler paths null-safe. This is the one asymmetry and it's a one-line fix.
Worth considering
requirements-mt3.txt:45 — mt3 @ git+https://github.com/magenta/mt3.git is unpinned to a commit hash. The file comment acknowledges this explicitly, calls pinning "RECOMMENDED", and explains the checkpoint can't be verified automatically from inside this repo. For a library described as unmaintained upstream, this is a supply-chain risk for any deployment that actually runs pip install -r requirements-mt3.txt. Raising it only because "unmaintained" + "unpinned" is a combination that tends to bite later; the comment already tells operators to pin, so this is a policy call.
server.py:1381-1385 — _MT3_NOT_AVAILABLE_MARKERS is a hardcoded substring match against the exact error phrasing in mt3_transcription.py's Mt3NotAvailableError. The comment already flags this as fragile and says "update both sides together" if phrasing changes. No action needed, just confirming I saw it.
Test results
Could not execute. Venv and node_modules not installed in the review environment. Test structure verified manually for all new files:
test_mt3_transcription.py: unit tests forto_payload()camelCase contract,_resolve_sources()stems-first/full-mix fallback,discover_stems_dir(), typedMt3NotAvailableErrorpropagation, subprocess gate tests, slow integration test gated onRUN_SLOW_TESTS=1.test_analysis_runtime.py InterpretationGatingTests: 8 cases covering MT3-only block, pitch-note-only block, both-must-complete, fail-is-terminal, neither-requested-is-immediate, recovery-clears-stale-running, and the enqueue-order regression guard (monkey-patches create methods, asserts["pitch_note", "mt3", "interpretation"]call order).test_server.py Mt3ExecutorTests: happy path artifact-ref swap (midiB64 → midiArtifactId/midiSizeBytes),MT3_NOT_AVAILABLE→retryable: False, generic failure →retryable: True, timeout terminalization,get_source_artifactfailure terminalization, prompt grounding presence/absence.test_phase2_citation_paths.py Mt3CitationPathsTests: paths rejected withoutmt3_result, accepted with it, array-item paths (tracks.instrument,tracks.noteCount) validated, invented paths still flagged.
Phase boundary check
Clean. In detail:
complete_measurement()popstranscriptionfrom the raw analyzer output before persisting, so theASA_ENABLE_MT3=1direct-CLI path cannot leak MT3 data intostages.measurement._mt3_stage_snapshot()hardcodes"authoritative": False; the TypeScriptMt3StageSnapshottype enforcesauthoritative: falseas a literal (compile-time guard).- Phase 2 system prompt rule 5 explicitly prohibits MT3 from overriding BPM/key/loudness/duration/structure and prohibits it from voting against pitch-note translation data.
_validate_phase2_citation_pathsonly addstranscription.mt3.*paths to the allowed citation set whenmt3_resultis actually provided — fabricated MT3 citations without a real result are flagged.EXPECTED_TOP_LEVEL_KEYSintest_analyze.pycorrectly excludestranscription(the MT3 namespace lives onsnapshot.stages.mt3, not theanalyze.pyenvelope), with a comment explaining why.
Generated by Claude Code
Summary
Adds MT3 (Magenta Multi-Task Multitrack) as an optional, flag-gated, additive analysis stage that emits per-instrument MIDI, and makes the interpretation stage actually consume that grounding. MT3 never overrides Phase 1 measurements — it is additive context only (PURPOSE.md invariant #1). The whole feature is opt-in per run (
mt3_mode=enabled) and inert otherwise; its heavy JAX/t5x deps live in a separate optional venv and never touch the product install or the request path.4 commits, MT3-only diff (~33 files):
feat(mt3)— optional MT3 stage (F1–F4). Newmt3_transcription.py(lazy-imported so JAX never loads at startup);mt3_attemptstable + lifecycle/recovery/interrupt wiring inanalysis_runtime.py;_execute_mt3_attemptsubprocess executor +_mt3_worker_loop+ per-run opt-in route inserver.py;requirements-mt3.txtoptional extra. Follow-ups: bidirectional Demucs stems handover with pitch/note (F1), estimate prices the MT3 stage when requested (F2), Phase 2 grounding forwardsmt3Resultto the prompt + citation validator with a newOPTIONAL_MT3_TRANSCRIPTION_RESULT_JSONinput (F3), and a frontendMt3TranscriptionPanelwith lazy per-track MIDI download wired into the Session Musician suite (F4).fix(mt3)— strictmt3_modevalidation. An unknownmt3_modenow returns400 MT3_MODE_UNSUPPORTEDinstead of silently disabling MT3; install docs mandate a separate venv (JAX/t5x pull older numpy/protobuf that would break Essentia/torch on the request path).feat(ui)— flag-gated MT3 opt-in toggle.feat(backend)— interpretation grounding gate (the fix that makes F3 actually deliver).The interpretation grounding gate
mt3, pitch_note, and interpretation were peers — all reservable the moment measurement completed — so they raced. MT3 loads multi-GB weights and usually lost, leaving
mt3Result/pitchNoteResultNoneby the time Gemini ran. Now:reserve_next_interpretation_attemptblocks a queued interpretation while an mt3 or pitch_note attempt for the same run is in-flight (queued/running). Terminal states (completed/failed/interrupted) unblock it, so failure/interrupt/restart-recovery can never deadlock — interpretation then runs with whatever grounding is available, exactly as before._enqueue_requested_followupscreates the interpretation attempt last (after mt3 + pitch_note), closing the cross-transaction window where it could reserve ungrounded._execute_mt3_attempt/_execute_pitch_note_attemptresolveget_source_artifactinside thetry(it sat before it) and null-safe the except diagnostics, so a pre-subprocess raise terminalizes rather than leaving the attempt stuckrunning(which, under the gate, would block the run's interpretation indefinitely).Pure scheduling — changes when interpretation runs, never what any stage produces. camelCase contract and
types.tsuntouched; snapshot keeps emittingqueuedwhile waiting.Testing
unittest). New coverage: MT3 module/runtime/executor, estimate, citation-path + grounding, the interpretation-gating suite (incl. an enqueue-order regression guard), an end-to-end test proving a non-nullmt3_resultreaches the prompt, and mt3 timeout +get_source_artifact-failure terminalization tests (the no-deadlock linchpins).tscclean, build OK, 704 unit tests pass.Reviewer notes
requirements-mt3.txt(separate venv + out-of-band checkpoint download;apps/backend/models/is gitignored).mt3_mode=enabledper run.get_source_artifacthardening, now included).🤖 Generated with Claude Code