feat: symusic consolidation + pianoroll UI + route 404 fix - #120
Conversation
slittycode
left a comment
There was a problem hiding this comment.
Verdict: COMMENT
Summary
Drops mido + pretty_midi in favor of symusic throughout, adds a symusic.Synthesizer backend behind ASA_SAMPLE_SYNTH_BACKEND, and ships the transcription pianoroll surface (GET /api/analysis-runs/{id}/transcription/pianoroll + canvas heatmap). All new pianoroll logic is tested; test_transcription_pianoroll.py and transcriptionPianorollClient.test.ts ran clean in this review. No phase boundary violations, no Phase 1 contract changes.
Findings
Should fix
_resolve_backend docstring contradicts the code — sample_synthesis.py:122-127
The docstring says:
The env var still wins when set, so an operator can explicitly pin a backend even while passing
allow_soundfont_backends=False
The code says:
if override == "sine" or not allow_soundfont_backends:
return "sine_fallback"allow_soundfont_backends=False triggers the same branch regardless of what the env var says — it returns sine_fallback even with ASA_SAMPLE_SYNTH_BACKEND=symusic. The test test_allow_soundfont_false_forces_sine confirms this is the intended behavior, so the code and test are correct. The docstring is not.
_build_score_from_plan claims to be shared with write_midi, but write_midi duplicates it — sample_synthesis.py:237-238
The docstring says:
Shared by the symusic synth and
write_midito keep the plan-to-Score translation in one place; future plan extensions only need updating here.
write_midi doesn't call _build_score_from_plan — it contains the identical 14-line Score-building block inline. The docstring is false, and it's the kind of false that actively causes a bug: someone editing _build_score_from_plan to add a bank-select event or change the TPQ will trust the comment, skip write_midi, and ship diverged MIDI output. Either write_midi should call _build_score_from_plan, or the docstring should be corrected to say the two functions are parallel and both need updating.
Test results
test_transcription_pianoroll.py: 38/38 passtranscriptionPianorollClient.test.ts: 8/8 passBackendResolutionTests/SymusicRenderPathTests/WriteMidiTests: couldn't run (full product venv not available in this review environment; PR claims 958 backend tests pass at tip)
Phase boundary check
Clean. The pianoroll module is a pure visualization layer — reads transcriptionDetail from the pitch-note stage, never touches or re-derives Phase 1 measurement values. The HTTP response cites Phase 1 bpm + timeSignature so chain-of-custody (invariant #2) is preserved on the surface.
Generated by Claude Code
Drops pretty_midi from the sample synthesis MIDI-write path; symusic becomes the single canonical MIDI library on the backend. The .mid file is a spec-conformant Standard MIDI file that DAWs parse identically, so the change is non-audible — the existing parity gate in tests/test_sample_synthesis.WriteMidiTests reads the symusic output back via pretty_midi and confirms the notes survive the round trip. Module docstring updated to call out the new writer + the parity contract for the next maintainer. 919 / 919 backend unittests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ymusic
Research-only path: polyphonic_evaluation.summarize_midi_file and
scripts/import_midi_to_ground_truth.py now parse MIDI via
symusic.Score(path).to("Second") instead of pretty_midi.PrettyMIDI.
Test fixture writers in test_polyphonic_evaluation.py and
test_phase1_evaluation_transcription.py likewise build their tiny .mid
inputs through symusic.
Drops three pretty_midi importers; only test_sample_synthesis.py keeps
pretty_midi as the deliberate parity reader for write_midi's output.
919 / 919 backend unittests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The consolidation plan called for adding symusic-beat as an observational fourth method in beat_evaluation.py. Probing symusic's API shows get_beats / get_downbeats operate on symbolic Score tempo maps, not on audio — symusic is not an audio-analysis library and has nothing to offer this harness, which takes audio paths. Recording the rationale in the file's header docstring so a future maintainer doesn't re-do the same investigation. The pre-registered beat_this vs kick_accent gate (ADOPT_MARGIN = 0.10) is unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nd select Sample synthesis now has three render backends — symusic (Prestosynth), FluidSynth (existing), and the sine fallback — picked by: - ASA_SAMPLE_SYNTH_BACKEND env var (auto|symusic|fluidsynth|sine), so operators have a code-free escape hatch if any backend regresses in the wild. Default "auto" prefers symusic when a soundfont is locatable, drops to FluidSynth if symusic fails, and falls through to sine when no soundfont is reachable. Unknown env values log a warning and treat the request as "auto". - _resolve_backend() captures the precedence in one place. The new BackendResolutionTests cover all combinations (8 tests) — pure logic, no synthesis, no soundfont required. Implementation details: - New _render_with_symusic_synth(plan, sf_path) renders via Synthesizer(sf_path).render(score, stereo=False), returning the same float32 mono buffer shape as the other two backends. - _build_score_from_plan extracted from write_midi so the symusic synth path and the MIDI emitter share one plan-to-Score translation. - Backend literal extended: "symusic" | "fluidsynth" | "sine_fallback". RenderResult.backend already flows into the citation manifest so audition samples carry which synth produced them. - Refuses to auto-download symusic's built-in MuseScore SF3 — the network call is hidden from the request path. Operators who want the built-in must point SONIC_ANALYZER_SOUNDFONT at it explicitly. - prefer_fluidsynth kwarg kept as the test escape hatch (semantics: "allow soundfont-based backends") so existing tests pass unchanged. 13/13 sample_synthesis tests pass. The pre-existing test_cleanup failure on this branch (unrelated MT3 WIP added a 5th startup task without updating the cleanup test's assertion) is documented in the working tree but is NOT caused by this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation
The campaign began in PR-A (transcription pianoroll module + dep) and now
ends with symusic as the only MIDI library on the backend.
Three load-bearing changes here:
1. analyze_rhythm: the melody MIDI exporter migrates from mido to symusic.
The old code manually built ticks_per_beat math + note_on/note_off
events; symusic takes a Score in seconds and dumps a spec-conformant
.mid. melodyDetail.midiFile (the field consumers read) is unchanged —
only the bytes inside the file differ, and DAWs parse them identically.
2. test_sample_synthesis.WriteMidiTests: the parity reader was pretty_midi.
It's now symusic itself — a weaker contract ("symusic reads its own
output"), but it still verifies the file is a valid Standard MIDI file
and that pitches, onsets, and durations survive the round trip. Added
onset/duration assertions to harden the contract slightly.
3. requirements.txt: pretty_midi and mido removed. mido was already
transitive-only (no direct imports outside this migration's targets),
and pretty_midi's importers were:
- sample_synthesis.py (PR-C)
- polyphonic_evaluation.py (PR-D)
- import_midi_to_ground_truth.py (PR-D)
- test_polyphonic_evaluation.py (PR-D)
- test_phase1_evaluation_transcription.py (PR-D)
- sample_synthesis::write_midi parity reader (this PR)
All migrated. ./venv/bin/pip uninstall confirmed nothing else broke.
The pre-existing test_cleanup failure (MT3 WIP added a 5th create_task on
startup without updating the test's assertion) is unrelated to this PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Issues called out and now fixed:
1. Conservative auto precedence — FluidSynth wins under ASA_SAMPLE_SYNTH_BACKEND=auto
when both the binding and a soundfont are reachable. Symusic only kicks
in under auto when FluidSynth is not importable. Cross-backend audio
parity was never measured, so silently flipping the engine for operators
with working FluidSynth setups was a regression risk. They opt into the
faster Prestosynth path explicitly with ASA_SAMPLE_SYNTH_BACKEND=symusic.
2. Kwarg name — render_clip's prefer_fluidsynth had been reinterpreted as
"allow soundfont-based backends" with the rename of FluidSynth out of
exclusive control. Renamed to allow_soundfont_backends across the call
chain (render_clip → generate_samples → generate_and_register_samples)
plus every test caller. No HTTP route signatures touched — purely
internal API. Future maintainers reading the kwarg see what it does.
3. Symusic render path coverage — _render_with_symusic_synth was pragma:
no-cover'd because no soundfont is reachable in CI. Added
SymusicRenderPathTests:
- test_render_with_symusic_synth_builds_score_and_returns_mono_float32:
mocks Synthesizer, verifies the Score is built correctly + the call
signature matches the documented contract.
- test_render_with_symusic_synth_reduces_stereo_to_mono: defensive
coverage for the 2D-buffer fallback.
- test_symusic_synth_produces_nonzero_audio_with_real_soundfont:
@skipUnless integration test that actually invokes Prestosynth when
SONIC_ANALYZER_SOUNDFONT (or a known location) provides one.
4. PR-G parity strengthening — WriteMidiTests now does a layered check:
library-independent (raw bytes must start with MThd and contain MTrk)
followed by within-library symusic round-trip. The byte-level check
catches the "symusic writes something only it can read" failure mode
that worried us when pretty_midi was dropped as the cross-library
reader.
5. Dead code — symusic_synth_available() (added in PR-F, never used)
removed.
6. BackendResolutionTests rewritten to lock down the new conservative
precedence: tests now patch _FLUIDSYNTH_IMPORTABLE explicitly so the
assertions don't silently re-pass under whatever happens to be
installed locally.
936 / 936 backend unittests pass (3 skipped: 2 pre-existing + 1 new
@skipUnless integration test).
PR-E (out of scope — symusic isn't an audio-analysis library) stays
dropped with the rationale in beat_evaluation.py's header docstring.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renders the new transcription pianoroll endpoint (eac9bed / PR-A.2) as a canvas heatmap below the existing Session Musician panel. Only mounts when transcriptionDetail has at least one note. - transcriptionPianorollClient.ts: typed transport for GET /api/analysis-runs/{run_id}/transcription/pianoroll with AbortSignal propagation. 8 vitest cases cover URL construction, camelCase query encoding, payload typing, and BackendClientError for 4xx/5xx (incl. TRANSCRIPTION_NOT_COMPLETED, NOT_AVAILABLE). - TranscriptionPianoroll.tsx: canvas heatmap mirroring the ChromaHeatmap pattern. Velocity → cyan-to-yellow ramp; lowest pitch at the bottom (Ableton convention); octave-C labels on the left. Cites mode + note count + Phase 1 BPM + time signature in the header so every cell traces back to a measurement (PURPOSE invariant #2). - TranscriptionPianorollBlock.tsx: fetch container with loading / error / success states. Surfaces the backend error code in the error view so testers can correlate to the server envelope. - AnalysisResults.tsx: mounts the block inside the existing section-musician-suite, gated on apiBaseUrl && runId && transcriptionDetail.noteCount > 0. 693 / 693 vitest UI tests pass (5 s). tsc --noEmit clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The pitch-note worker stores the transcriptionDetail dict directly as
the pitchNoteTranslation stage result (complete_pitch_note_attempt(
result=transcription_detail)), but the pianoroll route read
pn_result.get("transcriptionDetail") — a nested key that never exists
in production — so the endpoint returned 404 TRANSCRIPTION_NOT_AVAILABLE
for every real run. The route tests masked the bug by feeding a wrapped
{"transcriptionDetail": {...}} fixture that did not match the worker.
- server.py: read the stage result as the unwrapped transcriptionDetail,
with defensive tolerance for a legacy wrapper, and require a notes list
so a null/foreign dict still 404s.
- test_server.py: unwrap _transcription_result so the happy-path test
exercises the production shape (and would now catch a regression).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Off-path maintainer script for PR-F: before anyone flips the auto sample-synthesis default away from FluidSynth, this renders the same deterministic C-major triad ClipPlan through both backends on one SoundFont and compares RMS, peak, and spectral centroid against loose "audible parity" tolerances (1 dB / 2 dB / 200 Hz). Skips cleanly (exit 0) when pyfluidsynth or a SoundFont is unavailable, so it is safe to run anywhere; writes a JSON verdict to .runtime/parity/synth_parity.json. Passing parity does not itself flip the default — that stays a deliberate, separate change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drives the real upload→run→results flow with a mocked backend and
asserts the Session Musician pianoroll surface end to end:
- Success: the block mounts when the run snapshot's pitchNoteTranslation
result carries noteCount > 0, fetches the canonical sub-resource
GET /api/analysis-runs/{run_id}/transcription/pianoroll, and paints the
canvas heatmap with a citation that echoes Phase 1's BPM + time
signature (chain of custody on the surface).
- Error: a 409 TRANSCRIPTION_NOT_COMPLETED envelope surfaces visually
with the structured backend code attached for correlation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4b7696f to
2943a5f
Compare
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>
Summary
Consolidates the backend's symbolic-MIDI handling onto symusic, adds the transcription-pianoroll UI surface, and fixes a live 404 bug on the pianoroll route.
Fix: pianoroll route returns 404 for every real run (LIVE on main)
#117shipped the endpoint in its pre-fix state. The route readstranscriptionDetailas a nested key, but the pitch-note worker stores it unwrapped as the stage result — soGET /api/analysis-runs/{run_id}/transcription/pianoroll404s on every real run today. This PR reads it unwrapped (with legacy-wrapper tolerance + anotes-list guard) and unwraps the test fixture so the regression is actually caught.symusic consolidation (backend)
sample_synthesis.write_midito symusic; add asymusic.Synthesizeraudio path behindASA_SAMPLE_SYNTH_BACKEND(auto|symusic|fluidsynth|sine, FluidSynth-first underauto); migrate research MIDI parsers offpretty_midi; droppretty_midi+mido. Plus a FluidSynth-vs-symusic parity probe (research-only; skips cleanly without FluidSynth/a SoundFont).Transcription pianoroll UI
TranscriptionPianoroll+TranscriptionPianorollBlock) + typed client, mounted in the Session Musician suite; Playwright smoke coverage.Invariants
Additive only — Phase 1 measurements stay authoritative; the pianoroll is a derived view citing
bpm+timeSignature(PURPOSE.md #2).Testing (post-rebase)
Backend
sample_synthesis+ pianoroll-route tests 27 OK; frontendtscclean + pianoroll client unit tests 8 OK; route fix confirmed present onserver.py.🤖 Generated with Claude Code