Skip to content

feat: add transcription pianoroll endpoint backed by symusic - #117

Merged
slittycode merged 1 commit into
mainfrom
feat/transcription-pianoroll
May 28, 2026
Merged

feat: add transcription pianoroll endpoint backed by symusic#117
slittycode merged 1 commit into
mainfrom
feat/transcription-pianoroll

Conversation

@slittycode

Copy link
Copy Markdown
Owner

Summary

Adds GET /api/analysis-runs/{run_id}/transcription/pianoroll — renders the pitch-note translation stage's transcriptionDetail as a velocity-encoded (pitch, time) uint8 matrix for a UI heatmap surface. Derived view; Phase 1 stays authoritative. Response cites bpm + timeSignature for chain of custody (PURPOSE invariant #2).

What changed

  1. New dependency: symusic==0.6.0 — cp311 wheels for darwin-arm64 + manylinux_x86_64, no compile step.
  2. transcription_pianoroll.py: pure module mapping Phase 1 transcription notes → symusic.Score → pianoroll matrix. 38 unit tests, ~2 ms total.
  3. server.py route handler: structured error codes — `TRANSCRIPTION_NOT_REQUESTED` (404), `TRANSCRIPTION_NOT_COMPLETED` (409), `TRANSCRIPTION_NOT_AVAILABLE` (404) — so the UI can pick the right affordance per state. 10 route tests cover the status mapping + validation.
  4. Docs: `ARCHITECTURE.md` + `CLAUDE.md` updated; `transcriptionDetail` location note added (stripped from `measurement.result`, lives in the `pitchNoteTranslation` stage instead).

Test plan

  • CI green on this branch (Frontend / Backend / Chromatic / Loudness WASM)
  • Backend suite passes locally: `cd apps/backend && ./venv/bin/python -m unittest discover -s tests` (after `pip install symusic==0.6.0` into the product venv)
  • Focused subset: `./venv/bin/python -m unittest tests.test_transcription_pianoroll tests.test_server`
  • Manual smoke: `GET /api/analysis-runs//transcription/pianoroll` against a finished run with a non-trivial transcription — confirm `(pitch_count × time_steps)` matrix returned and the four state-mapped status codes behave per spec on not-ready runs.

Provenance

This PR was filed by a different session than the one that wrote the code. The original commit (`eac9bed8`, co-authored by Claude Opus 4.7) landed directly on local main and was rebranched here to match the team's PR workflow. No code in the commit itself was modified during the rebranching.

🤖 Generated with Claude Code

Adds GET /api/analysis-runs/{run_id}/transcription/pianoroll, which
renders the pitch-note translation stage's transcriptionDetail as a
velocity-encoded (pitch, time) uint8 matrix for a UI heatmap surface.

- New symusic==0.6.0 dep (cp311 wheels for darwin-arm64 + manylinux).
- transcription_pianoroll.py: pure module mapping Phase 1 transcription
  notes → symusic.Score → pianoroll matrix. 38 unit tests, ~2 ms total.
- Route handler in server.py with structured error codes: distinguishes
  TRANSCRIPTION_NOT_REQUESTED (404), TRANSCRIPTION_NOT_COMPLETED (409),
  and TRANSCRIPTION_NOT_AVAILABLE (404) so the UI can pick the right
  affordance per state. 10 route tests cover the status mapping +
  validation.
- Derived view; Phase 1 stays authoritative. Response cites bpm +
  timeSignature for chain of custody (PURPOSE invariant #2).
- ARCHITECTURE.md + CLAUDE.md updated; transcriptionDetail location
  note added (stripped from measurement.result, lives in the
  pitchNoteTranslation stage instead).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@slittycode slittycode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES (posted as COMMENT — can't self-review)

Summary

Adds GET /api/analysis-runs/{run_id}/transcription/pianoroll, a derived-view endpoint that converts pitchNoteTranslation.result.transcriptionDetail.notes into a velocity-encoded (pitch, time) uint8 matrix via symusic. Phase boundary is clean — Phase 1 data is only read, never mutated; bpm and timeSignature are cited for chain of custody per invariant #2. 38 unit tests pass cleanly locally. One real issue: tpq has no upper bound, which creates an unbounded memory allocation path reachable by any API caller.

Findings

Should fix

Unbounded tpq causes potential OOM — transcription_pianoroll.py:202, server.py:2623

Both the module and the route handler validate only tpq < 1. There is no upper bound. The module docstring even acknowledges the risk: "larger matrices should be persisted through artifact_storage instead of inlining" — but there is no guard. At tpq=480 (General MIDI resolution), a 5-minute track at 120 BPM produces a matrix of ~88 × 288,000 cells; inlined as JSON that's ~75 MB per response. At tpq=4800 it's ~750 MB. At tpq=48000 it will OOM a typical server instance before the response is serialized.

Add a MAX_TPQ constant to transcription_pianoroll.py (suggest 480 — far more resolution than any UI heatmap needs) and reject above it with INVALID_TPQ in the route, same as the lower-bound check. Add one test that confirms tpq above the cap returns 400.

Minor

"queued" and "ready" pitchNoteTranslation statuses aren't individually tested — test_transcription_running_returns_409 only covers "running". The code (pn_status in ("ready", "queued", "running")) handles all three identically, so it's almost certainly correct, but a second parametrized case would be thorough. Not blocking on its own.

Test results

38/38 unit tests (tests.test_transcription_pianoroll) pass locally. Route tests (tests.test_server.TranscriptionPianorollRouteTests) require the full product venv; covered by CI — Backend check was in progress at review time, Frontend and Loudness WASM both green.

Phase boundary check

Clean. The module reads pitchNoteTranslation.result.transcriptionDetail and cites measurement.result.bpm + measurement.result.timeSignature without reassigning or recomputing any Phase 1 value. No Phase 1 contract fields removed, renamed, or semantically changed.


Generated by Claude Code

@slittycode slittycode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE (with one should-fix noted)

Summary

Adds GET /api/analysis-runs/{run_id}/transcription/pianoroll — a derived-view endpoint that renders the pitch-note translation stage's transcriptionDetail as a velocity-encoded (pitch, time) uint8 matrix via symusic. Phase 1 data is read-only throughout; the implementation is a clean read from Layer 2. 38 unit tests + 10 route tests; CI green across all three checks.

Findings

Should fix

tpq has no upper bound. transcription_pianoroll.py validates tpq >= 1 and the route mirrors that, but nothing caps the other end. A 3-minute clip at 200 BPM with tpq=1000 produces ~600k columns × 88 pitch rows = ~52 MB of inline JSON. The module docstring explicitly acknowledges that large matrices should go through artifact_storage instead of inlining, which means the author saw this — but left no guard in place. Add a hard cap (e.g. tpq <= 96, which already covers any musically useful resolution) and return INVALID_TPQ if exceeded, same as the lower-bound check. The inline-vs-artifact decision can come later, but the DoS vector should be closed now.

Worth considering

pn_status == None (stage absent from the snapshot entirely) falls through all the status checks and lands on TRANSCRIPTION_NOT_AVAILABLE. That error code implies the stage ran and produced nothing; the right code would be TRANSCRIPTION_NOT_REQUESTED or MEASUREMENT_NOT_COMPLETED depending on context. In practice the stage is always initialized in the snapshot so this path shouldn't trigger, but if the runtime ever returns a partial snapshot the caller gets a misleading error.

Header(None) values in _call_route — the comment explains Query(...) sentinel objects but the same applies to Header(None): calling the async function directly passes a FieldInfo object, not None, for x_asa_user_id/x_asa_user_email. It works because local-profile _resolve_route_user_context doesn't inspect those values. Worth keeping in mind if hosted-profile route tests are ever added that call the handler directly.

Test results

48 passes (38 unit + 10 route), 0 failures. CI: Frontend ✓, Backend ✓, Loudness WASM ✓.

Phase boundary check

Clean. transcription_detail is read from pitchNoteTranslation.result and never written back. Phase 1 bpm and timeSignature are consumed as citation metadata only — no re-derivation, no override. _velocity_from_confidence operates on Layer 2 note confidence, not any Phase 1 field. Phase 1 output schema unchanged.


Generated by Claude Code

@slittycode
slittycode merged commit 9185a25 into main May 28, 2026
3 checks passed
@slittycode
slittycode deleted the feat/transcription-pianoroll branch May 28, 2026 21:18
slittycode added a commit that referenced this pull request May 29, 2026
Resolves the PR #119 conflict with #117 (transcription pianoroll) and
#118 (docs refresh).

Sole conflict: apps/backend/requirements.txt — both branches added the
identical symusic==0.6.0 pin; only the preceding comment differed. Kept
main's explanatory comment and broadened it to name both consumers:
#117's transcription_pianoroll.py and this branch's sample_synthesis.py /
research MIDI parsers, which made symusic the canonical backend MIDI lib.

server.py and test_server.py auto-merged cleanly — #117's pianoroll
endpoint (~line 2584) and this branch's Live 12 catalogue gate (~line
1636) occupy disjoint regions. Verified both survive: catalogue-gate
import/call/error-handler and pianoroll import/endpoint all present.

Full backend suite on the merged tree: 999 pass, 1 skip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
slittycode added a commit that referenced this pull request May 29, 2026
Resolves the PR #119 conflict with #120 (symusic consolidation +
pianoroll UI + route 404 fix), which landed a superset of this branch's
symusic work.

Sole conflict: apps/backend/sample_synthesis.py. This branch carried the
initial write_midi→symusic migration (029fb92); #120 incorporated that
and built the symusic.Synthesizer backend, _resolve_backend precedence,
and _build_score_from_plan extraction on top. All three conflict regions
were a strict superset on #120's side (HEAD's third region contributed
nothing), so the file was resolved by taking origin/main wholesale —
no unique content lost.

Everything else auto-merged. Two post-merge corrections to keep this PR
scoped to its real payload:
  * requirements.txt reset to match origin/main exactly. The auto-merge
    correctly dropped pretty_midi + mido (per #120) and kept the single
    symusic==0.6.0 pin; a leftover broadened comment from the earlier
    #117 merge round was reverted to #120's wording, since this branch
    adds no dependency of its own (the catalogue/validator is stdlib-only).
  * server.py combines #120's pianoroll-route 404 fix (unwrapped
    transcriptionDetail read, server.py:2735) with this branch's Live 12
    catalogue gate — verified the unwrapped read is primary, so #117's
    nested-key bug did not survive.

This branch's remaining unique payload is the Live 12 catalogue + Phase 2
output validator (1a2b5ad + the warn-and-keep fix a2a7b97); the symusic
commits are now redundant and collapse to zero net diff against main.

Full backend suite on the merged tree: 1011 pass, 2 skip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant