Add --force-program flag to pin decoded notes onto one instrument - #178
Closed
mrkkucharski wants to merge 51 commits into
Closed
Add --force-program flag to pin decoded notes onto one instrument#178mrkkucharski wants to merge 51 commits into
mrkkucharski wants to merge 51 commits into
Conversation
download_model()'s destination path stripped the GCS object prefix (checkpoints/mt3/) and wrote straight into output_dir, but then unconditionally checked for output_dir/checkpoint_0 and raised RuntimeError -- the bucket's own objects have no checkpoint_0 prefix, so that check could never pass. Write each file under checkpoint_dir instead, so the on-disk layout actually matches every documented --checkpoint .../checkpoint_0 invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds the `rhythm` event type to the MT3 codec (vocabularies.py), threads it through note_sequences.py's encode/decode state machine, and solves the carrier problem -- NoteSequence.Note has no field for a role flag -- by grouping assign_instruments by (program, rhythm) and writing the canonical name into instrument_infos, which note_seq's own MIDI writer already keys track names on. Registers guitar_pilot_notes_ties against the current data/pilot corpus via a new converter (build_guitar_pilot_tfrecord.py) that reads MIDI directly with note_seq's standard reader and cross-checks it against the manifest. Makes metrics.py's F1 scoring (program, rhythm)-aware, consistent per granularity (flat collapses rhythm, midi_class/full retain it). Adds 6 round-trip tests covering every property in the Phase 0 plan. Runs both Phase 0 gate checks: the official 46M-param checkpoint restores into the extended codec with no vocab shape error and completes one real forward/backward/save/resume step (guitar_pilot_finetune_local.gin). The tiny-overfit check (guitar_pilot_gate_local.gin) climbed to F1=0.5 over 50k steps, short of the plan's 0.95 target -- a bounded exposure-bias gap, not a broken pipeline. Adds pyfluidsynth as a dependency: MT3's eval metrics hard-require it for audio summaries and fail outright without it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
modal_train.py + guitar_pilot_finetune_modal.gin: runs the pretrained,
full-size guitar_pilot fine-tune on a CUDA GPU against volumes mounted
at /workspace/{data,model,runs}, with train_steps as a per-invocation
relative step count (t5x.train's relative_steps overrides TRAIN_STEPS
unconditionally whenever set, so it can't be hardcoded).
model_download.rebuild_checkpoint_0_view: rebuilds the legacy checkpoint_0
symlink view from a model volume's flat files after mounting, since a
volume upload doesn't reliably carry relative symlinks across.
Transcriber._restore: switched off from_checkpoint_or_scratch, which only
reads the non-Orbax legacy layout, to create_checkpoint_manager_and_restore
with use_orbax=True -- the same path t5x.train's own resume uses, and the
only one that can read a checkpoint this fork's own training produces
(Orbax-native/OCDBT), not just the officially released one.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…le's checkpoint model_download.download_model now pulls the pinned checkpoint_1002000 step from mrkkucharski/mt3-guitar-pilot (public) at a fixed revision, via huggingface_hub.snapshot_download, and materializes it locally as checkpoint_0 -- the same on-disk contract every documented `--checkpoint .../checkpoint_0` invocation already expects. Replaces the GCS-based official-multitrack-v1 downloader entirely (no more Google dependency); rebuild_checkpoint_0_view is untouched since Modal training still needs it for the legacy pretrained layout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
validate/convert/train/resume already existed as single commands (reaper2mt3 check, build_guitar_pilot_tfrecord, modal run modal_train.py -- which is both train and resume, since it always resumes from whatever checkpoint sits in the model volume). evaluate and export did not: every fine-tune leg so far scored a checkpoint by transcribing and eyeballing note counts, and exported a reviewable REAPER project by running mt3-transcribe then midi2reaper build by hand. evaluate_checkpoint.py (mt3-evaluate) transcribes a dataset split with the real Transcriber and scores it against ground-truth corpus MIDI using the same (program, rhythm)-aware onset+offset F1 run_phase0_gate_eval.py already uses for the Phase 0 tiny-model gate. export_transcription.py (mt3-export) fuses mt3-transcribe and midi2reaper build into one command, bridging the two tools' separate uv-managed environments via subprocess (mirroring modal_train.py's own approach to t5x.train). Guards a real footgun: midi2reaper build exits 0 even when it writes nothing (REJECT or EXISTS-without---force), so this verifies the .RPP actually landed rather than trusting the exit code. Both registered as console scripts in pyproject.toml. Documented all six Phase 1 entry points together for the first time in README.md, plus the two new commands in mt3/README.md, and fixed a stale doc line describing the split-reassignment bug the prior commit's splits.json fix already closed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
train.gin hardcodes eval_period=5000, and t5x.train requires the checkpoint period, eval period, and GC period (0 here) to all be multiples of each other. Any save_period that does not divide 5000 -- 2000, for instance -- therefore fails fast with "Checkpoint period (N), eval period (5000), and GC period (0) must all be multiples of each other." Passing eval_period through alongside the save period keeps the two trivially compatible for any value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds gin/context_4s.gin, a thin overlay rebinding TASK_FEATURE_LENGTHS to
{'inputs': 512, 'targets': 1024}, to be loaded after an existing
guitar_pilot config. Kept as an overlay rather than an edit to the finetune
gin so a 2 s-vs-4 s ablation differs in exactly one causal factor; the 2 s
path is byte-identical.
The window is expressed only in frames (16 kHz / hop 128 = 125 fps, so 512
frames = 4.096 s), and both chunking paths in tasks.py -- select_random_chunk
for training and split_tokens_to_inputs_length for eval -- read
sequence_length['inputs'], so the rebinding reaches both with no other edits.
checkpoint_0 restores without positional surgery: t5x's T5Config has no
sequence-length-dependent field and network.py uses RelativePositionBiases
(a [num_buckets, num_heads] table read at the live input length), not
absolute position embeddings. The behavioural caveat is separate --
max_distance=128 saturates well inside a 512-frame window, so the weights
load but the model has not learned to use the extra context.
targets is deliberately left at 1024 rather than guessed upward. tasks.py
ends the training pipeline with handle_too_long(skip=skip_too_long) and
skip_too_long defaults to False, so an undersized targets RAISES rather than
dropping the example -- a dead paid GPU job. measure_target_lengths.py sizes
it beforehand by running the real pipeline with a deliberately high probe
targets and histogramming the true lengths.
Transcriber and mt3-transcribe gain input_length/--input-length so a 4 s
checkpoint can actually be run at inference; defaults are unchanged, and all
existing call sites pass checkpoint_path positionally.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
measure_target_lengths.py on guitar_pilot_notes_ties_vb1_train shows targets must grow with the window. At 512 frames one pass produced a maximum of 1533 tokens and overflowed the 1024 budget on 2 of 500 windows (0.4%); at 256 frames the same corpus peaks at 608-911 and fits. That 0.4% is not a quality tax, it is a crash. tasks.py ends the training pipeline with handle_too_long(skip=skip_too_long), skip_too_long defaults to False, and that branch raises rather than filtering -- so the overflow lands as a dead job at an unpredictable step on a paid GPU. 2048 clears the highest observed length by ~33%. It is explicitly not a proven bound: select_random_chunk draws different chunks each epoch, and two full-epoch passes returned maxima of 1249 and 1533, so no single pass bounds the tail. skip_too_long=True was rejected as the alternative because it trades the crash for silent bias -- the dropped windows are the densest passages. Note the cost, since it compounds with the window change: seqio pads every feature to its task_feature_length for XLA's static shapes, so the decoder computes at the full 2048 every step against a median content length of 187. 1024 -> 2048 quadruples decoder self-attention on top of the ~4x encoder attention from 256 -> 512. Also corrects the task name in the docs to the registered guitar_pilot_notes_ties_vb1_train. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restore and forward pass at 512/2048 verified by running them: Transcriber(checkpoint_0, input_length=512, target_length=2048) restores with no shape error and transcribes end to end, and the seqio task pipeline materialises a full 1040-window epoch at 512. The checkpoint-compatibility claim is now empirical rather than read off the source. A local training step could not be tested. python -m t5x.train dies in prepare_train_iter with "Can't find an output tensor for the output node: identity_RetVal [Op:MakeIterator]" -- but the identical command WITHOUT context_4s.gin fails in exactly the same place, so it is the known Apple-Silicon TensorFlow input-pipeline fault and not the window change. The task pipeline itself is fine; only t5x's clu iterator wrapper breaks. The first real 512 step has to happen on Modal. Warm inference timing over 30 s of audio, M4 CPU: 21.5 s at 256 (1.40x realtime, 1097 notes) versus 20.3 s at 512 (1.48x realtime, 460 notes). Whole-file throughput is roughly flat because halving the window count offsets the higher per-window cost -- but the 4 s figure flatters itself, since the un-adapted checkpoint predicts 460 notes instead of 1097 and the autoregressive decoder therefore runs far fewer steps. Re-measure after adaptation. Training gets no such reprieve: targets pad to the full 2048 every step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… dir _t5x_train_command gains context_4s, which appends gin/context_4s.gin after guitar_pilot_finetune_modal.gin (gin is last-write-wins, so the overlay only overrides 256/1024 from that position) and simultaneously redirects MODEL_DIR to guitar_pilot_finetune_64ex_it2_4s. Redirecting the directory is not a convenience. The 2 s MODEL_DIR is baked into the finetune gin, so without this a 4 s run would drop checkpoints into the 2 s run's directory and interleave two incompatible window lengths in one step sequence. Tying the redirect to the same flag that changes the window makes that collision unreachable rather than merely documented; --model-dir overrides the destination but cannot restore the 2 s default while --context-4s is set. The name follows the 2 s run's tag with a _4s suffix so the pair reads as one experiment. Both legs restore from checkpoint_0, so _4s denotes the window, not a continuation of it2. Verified: with context_4s the overlay is last in the gin chain and MODEL_DIR points at the _4s tag; without it the argv is byte-identical to before, with no MODEL_DIR binding at all. Preflight on an A10G passed: restore in 3.3 s, compile in 59 s, one optimizer step and a checkpoint save at 512/2048 with no OOM at BATCH_SIZE=1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Head-crop overlap (MT3_HEADCROP_OVERLAP_PLAN.md, Phase 1): a strided drop-in replacement for t5.data.preprocessors.split_tokens. At hop_tokens == window_tokens it is exactly equivalent to the existing non-overlapping split. At hop_tokens < window_tokens, windows overlap and the trailing (window - hop) frames of each window become lookahead context for the encoder; the existing max_decode_time cropping in metrics_utils.decode_and_combine_predictions already discards that context from the decoded output, so no downstream decode-path change is needed for this half of the plan. Window count is ceil(n / hop_tokens), independent of window_tokens -- verified against tf.signal.frame's own pad_end=True frame count, which depends only on frame_step. Each window is built via tf.signal.frame and then stripped back to its true per-window content length (mirrors t5.split_tokens' handling of its final, possibly-short segment) so a downstream feature converter pads it identically to today's baseline. Verified via ad hoc script (not checked into the repo) against 100+ (n, window, hop) configurations: exact coverage tiling of [0, n) with no gaps or overlaps, correct per-window content and orig_lengths, and byte-for-byte equivalence to a manual non-overlapping reference at hop == window. Existing test suite (71 passed, 4 skipped) is unaffected.
Head-crop overlap (MT3_HEADCROP_OVERLAP_PLAN.md, Phase 2). Transcriber gains a lookahead_frames constructor argument (default 0, reproducing the baseline exactly); it derives hop_frames = input_length - lookahead_frames and swaps the old t5.data.preprocessors .split_tokens_to_inputs_length call in _dataset() for the new preprocessors.split_tokens_strided from Phase 1, reading the window size from self.sequence_length['inputs'] rather than a constant so it stays correct at --input-length 512 on the 4 s branch. transcribe() keeps its existing NoteSequence-only return type -- evaluate_checkpoint.py and run_phase0_gate_eval.py call it directly and must not break. Diagnostics (est_invalid_events, est_dropped_events) and resolved geometry (window_frames, hop_frames, lookahead_seconds, cost_multiplier) are instead threaded through a new _transcribe_with_diagnostics() helper into TranscriptionResult, whose new fields default to values that leave export_transcription_test.py's _FakeTranscriber construction unaffected. cli.py adds --lookahead-frames and a mutually exclusive --lookahead-seconds convenience flag (converted at the fixed 125 frames/s the spectrogram front end uses). Verified end-to-end against the real checkpoint at model/mt3/checkpoint_0 on a 6.5 s clip: - lookahead_frames=0 reproduces the pre-change code path exactly -- output MIDI files are byte-identical (same MD5) between this branch and the original t5.split_tokens-based implementation (stashed and re-run for comparison). - lookahead_frames=128 (50% overlap at window 256) runs end-to-end and reports the expected geometry (hop_frames=128, cost_multiplier=2.0, lookahead_seconds=1.024). - --lookahead-seconds 1.024 produces an identical result to --lookahead-frames 128. - lookahead_frames >= input_length raises ValueError from both the direct constructor call and through cli.main(). Full test suite: 86 passed, 4 skipped (unchanged from before this commit).
Formalizes the ad hoc verification from the Phase 1/2 commits into a checked-in suite: exact equivalence to a non-overlapping reference at hop==window (the property Transcriber(lookahead_frames=0) depends on), coverage tiling with no gaps or overlaps across 18 (n, window, hop) geometries including window > n (every window short, not just the last) and hop not dividing window evenly, per-window content/length correctness, passthrough replication, and the two error paths. 7 passed, 1 skipped (tf.test.TestCase's own harness test).
…strided Wires lookahead_frames through Transcriber and the CLI for head-crop overlap inference. Smoke-tested against the it2/40k checkpoint; overlap did not show a clear quality improvement in informal checks, but the capability is now available behind lookahead_frames for future use and a proper F1-based ablation.
…description Introduces the value object the lookback feature's later plumbing will build on. lookback_frames=0 reproduces today's lookahead-only geometry exactly (the compatibility anchor for the whole feature). Transcriber accepts a lookback_frames keyword but raises NotImplementedError for non-zero values until the segmentation and decode paths are wired up. Part of the lookback feature (issue #1 of #1-#8).
Lookback: introduce a validated (lookback | keep | lookahead) window geometry
Adds _windowed_input_dataset(): left-pads audio by geometry.lookback_frames before framing so window 0 gets real silence rather than reused audio, keeps input_times in original-audio time throughout (the pad occupies negative timestamps rather than shifting the whole timeline), and drops the surplus trailing windows that left-padding would otherwise add (windows whose kept region starts at or past the end of the real audio). lookback_frames=0 is byte-identical to the pre-existing plain split_tokens_strided call -- the compatibility anchor this whole feature leans on. Transcriber._dataset now delegates to this helper; Transcriber itself still rejects non-zero lookback_frames until the decode side is wired up. Also broadens split_tokens_strided's docstring, which described only the trailing-overlap (lookahead) case; it now says overlap can lead, trail, or split across both ends, since the function itself has no opinion on that, only its callers do. Part of the lookback feature (issue #2 of #1-#8).
Adds min_time alongside the existing max_time, so a segment can crop both ends of its decoded output: events before min_time are suppressed (still run through decode_event_fn so sticky run-length state like program/velocity/rhythm carries over correctly, but withheld from state-mutating note effects via a state.suppress flag decode_events sets per event) rather than skipped outright, and events land in the half-open [min_time, max_time) interval rather than double-committing on a shared boundary. Also fixes a latent bug this surfaced: `if max_time and cur_time > max_time` treats max_time=0.0 as "no bound" due to Python truthiness, silently keeping events that should have been dropped. Now `max_time is not None`. decode_events now returns a 3-tuple (invalid, dropped, suppressed) instead of 2; updates every direct caller (metrics_utils.py's decode_and_combine_predictions, and the note_sequences_test.py / note_sequences_rhythm_test.py call sites) to match. No caller passes min_time yet, so suppressed_events is always 0 today and behavior is unchanged end to end -- decode_and_combine_predictions doesn't expose the new counter externally yet either. Part of the lookback feature (issue #3 of #1-#8).
…type hint decode_events counted an event as both invalid_events and suppressed_events when it fell before min_time and also raised ValueError from decode_event_fn -- suppressed_events now only increments after a successful decode, since an event that was never validly decoded isn't meaningfully "withheld by suppression". Also updates decode_tokens_fn's type annotation and docstring in metrics_utils.decode_and_combine_predictions, which still said Tuple[int, int] after decode_events grew a third return value.
Transcriber.__init__'s NotImplementedError guard comment said _dataset() doesn't honor a left-shifted window, but this PR makes it do exactly that via _windowed_input_dataset(). The real remaining gap is on the decode side (metrics_utils.decode_and_combine_predictions crops using only start_time, with no per-segment kept-region start yet).
The if/elif pair differed only in >= vs > and their max_time is not None guard, duplicated in a way a future edit could update one branch without the other. Minor cleanup from PR review; no behavior change (both branches were already individually correct and tested).
Lookback: shift encoder windows left and pad the start of the audio
Lookback: add a min_time head crop to decode_events
decode_note_event now checks state.suppress (set per-event by run_length_encoding.decode_events when min_time is given): a suppressed pitch/drum event returns immediately without touching active_pitches or note_sequence -- the previous window's kept region already committed it (or its closure) -- and a suppressed tie terminator clears is_tie_section and returns without ending any "untied" active notes. This matters because the tie section describes what's sounding at the WINDOW's first frame, lookback_frames before min_time -- not at the kept region's start, which is what active_pitches (carried over from the previous window's kept region ending exactly there) already reflects correctly. Left unhandled, this causes two failure modes: every held note gets chopped at the window boundary (the tie terminator ending it, since it wasn't declared tied at the wrong instant), and a tie section entry for an already-closed pitch raises "inactive pitch/program/rhythm in tie section". Sticky state (program/velocity/rhythm) still applies while suppressed, since decode_events routes suppressed events through decode_event_fn -- only note-emitting effects are withheld. Also adds a diagnostic: flush_note_decoding_state now logs a warning when force-closing a note held open longer than 30s, since discarding the tie section means a note's offset relies entirely on some later window actually decoding its note-off -- if none ever does, the note silently turns into one absurdly long note instead of visibly failing. lookback_frames=0 (min_time always None) is untouched: state.suppress starts False and decode_events never sets it, so every branch above takes its pre-existing path. Confirmed by the full existing note_sequences_test, note_sequences_rhythm_test, and metrics_utils_test suites passing unmodified, including the tie-across-boundary regression test. Part of the lookback feature (issue #4 of #1-#8).
Predictions may now optionally include 'min_decode_time' (this segment's own kept-region start, for a lookback-overlap caller) and, on the last segment only, 'max_decode_time' (an explicit cap, e.g. real audio duration, so a lookahead tail can't emit events into padding-only silence past the end of the file). Both are entirely optional and default to today's behavior: max_time for segment k is still derived from segment k+1's start_time when no min_decode_time is present, min_time defaults to None, and the last segment stays unbounded absent an explicit max_decode_time. decode_and_combine_predictions now returns a 4-tuple (adding total_suppressed_events, summed from decode_tokens_fn's third return value) instead of 3, and validates that predictions carrying min_decode_time agree with the start_time-based sort order it already uses -- the crop logic assumes segment k+1's min_decode_time is where segment k's kept region should end. event_predictions_to_ns surfaces the new total as 'est_suppressed_events'; metrics.py's per-example score table gains a matching 'Suppressed events' column. inference.py and scripts/run_phase0_gate_eval.py need no changes: neither unpacks decode_and_combine_predictions directly, only reads event_predictions_to_ns's dict, and an added key doesn't break that. Part of the lookback feature (issue #5 of #1-#8).
… value
next_pred.get('min_decode_time', next_pred['start_time']) only falls back
to start_time when the key is absent -- a caller that always includes
'min_decode_time' but sets it to None for a non-lookback segment left the
preceding segment's decode window unbounded, silently overlapping the
next segment's kept region.
…ogic run_length_encoding.decode_events now unconditionally assigns state.suppress = min_time is not None and cur_time < min_time (rather than only assigning it inside `if min_time is not None`), so a state reused across a min_time=X call followed by a min_time=None call can no longer carry a stale True forward and silently suppress every event in the second call. decode_note_onset_event (NoteOnsetEncodingSpec) gains the same suppress guard as decode_note_event: it has no active_pitches/tie-section state to naturally dedupe against, so without this it would unconditionally duplicate any onset re-declared in a window's suppressed prefix. decode_note_event's three near-identical `if state.suppress: return` guards (pitch, drum, and part of tie) collapse into one dispatch-level check for pitch/drum; tie keeps its own handling since it also needs to clear is_tie_section. Not changed, as accepted consequences of the suppressed-prefix design (each already covered by an existing test or comment, and reversing them would reintroduce the wrong-instant validation this feature exists to avoid): the monotonicity check and invariant checks (inactive-pitch note-off, already-tied, zero-velocity drum, duplicate tie-end) are intentionally skipped for suppressed events, since validating them against active_pitches/current_time as of the WINDOW's first frame -- not the kept region's start -- is exactly the false-positive source this feature exists to eliminate.
Lookback: reconcile the tie section, which describes the window start rather than the kept region
Lookback: carry per-segment crop bounds through decode_and_combine_predictions
Removes the NotImplementedError guard from #1: Transcriber now honors a non-zero lookback_frames end to end. Each prediction gets a 'min_decode_time' (its own kept-region start, quantized onto the same codec step grid as 'start_time' so a boundary event can't fall through the crack between two windows' crop bounds), and the chronologically-last prediction gets an explicit 'max_decode_time' capped at the real audio duration, so a lookahead tail can't decode events from the padding-only silence past the end of the file -- this applies regardless of geometry, not just when lookback is in use. Extracts the crop-bound math (_quantize, _min_decode_time, _cap_last_segment_tail) as pure module-level functions, directly unit-testable without a checkpoint, mirroring how _windowed_input_dataset was already split out in the windowing PR. TranscriptionResult gains est_suppressed_events, keep_frames, and lookback_seconds, alongside the existing hop_frames/lookahead_seconds/ cost_multiplier -- an ablation run's full geometry now travels with its output. The Transcriber docstring describes the three-part [lookback | keep | lookahead] window instead of only the lookahead half. lookback_frames=0 remains the compatibility anchor: min_decode_time then equals start_time exactly, which can never suppress anything (a segment's own cur_time is always >= its start_time by construction), so this is a no-op end to end -- confirmed by the full existing test suite passing unmodified alongside the new tests. Not yet covered here: a real-checkpoint smoke test across all four requested geometries, and byte-identical-to-main verification on a real WAV. Both need an actual checkpoint file, which this environment doesn't have; deferred to the end-to-end testing issue. Part of the lookback feature (issue #6 of #1-#8).
Mirrors the existing --lookahead-frames/--lookahead-seconds flags: its own mutually exclusive group, seconds truncated to whole frames at the fixed 125 frames/s. Both default to None -> lookback_frames=0, today's baseline. Validates the resolved geometry via WindowGeometry before Transcriber spends time loading a checkpoint, so an over-budget combination (lookback + lookahead >= input-length) is a clean argparse usage error naming all three numbers, not a stack trace from inside model construction. Prints the resolved window geometry alongside the note count in non-JSON output, so a mis-specified ablation run is obvious at a glance (e.g. "window 512f (4.10s) = 125f lookback + 250f keep + 137f lookahead, cost 2.05x"); the JSON output already carries it via TranscriptionResult.as_dict() from the Transcriber-wiring PR. Extracts _resolve_transcriber_kwargs() as a pure, import-free function (no mt3.transcription), so the CLI's own conversion logic is testable without paying for the heavy ML imports main() otherwise delays until after argument parsing. Part of the lookback feature (issue #7 of #1-#8).
_min_decode_time now returns None (not start_time) when geometry.lookback_frames is 0. Passing decode_events a non-None min_time -- even one that can never actually suppress anything, since a segment's own cur_time can never fall below its own start_time -- silently switches its max_time boundary test from the legacy inclusive `cur_time > max_time` to the half-open `cur_time >= max_time`. That drops an event landing exactly on a window boundary, which is common rather than rare once both sides are quantized onto the same codec step grid: a real regression in the plain baseline path this whole feature is supposed to leave untouched. _cap_last_segment_tail is now only applied when overlap is actually in use (lookback_frames or lookahead_frames nonzero), gated through a new _should_cap_last_segment_tail() helper. Capping the last segment's tail at the real audio duration is new behavior relative to the pre-overlap baseline (previously always unbounded) -- worth doing once overlap is in play, but not something the 0/0 compatibility anchor should silently pick up too. Both are now covered by direct unit tests instead of only being implicit in end-to-end behavior.
lookback_seconds and lookahead_seconds in transcribe_file() were computed via two different formulas for the same kind of quantity (one through geometry.seconds(), one a raw division). Both now go through geometry.seconds(), so they can't silently desync if self.geometry is ever rebuilt independently of self.lookahead_frames. More significantly: _quantize's `t - t % step` floors any value that is a decimal-exact multiple of the codec step down a WHOLE EXTRA STEP, because a step like 0.01 has no exact binary floating-point representation -- `5.0 % 0.01` evaluates to ~0.00999999999999990, not 0.0. This is a pre-existing bug (predates this whole feature; the formula is unchanged from the original pre-lookback code, just extracted into this named helper), latent because real start_time values derived from 125fps frame boundaries rarely land exactly on a 100-steps/s codec boundary -- but the end-to-end tests being written for issue #8 hit it immediately, since their synthetic audio uses clean decimal second boundaries for readability. Rewritten as floor(t * steps_per_second + epsilon) / steps_per_second, which is robust to this class of float noise while the epsilon (1e-9) is far too small to affect any genuine non-boundary value.
…ation _cap_last_segment_tail set max_decode_time to exactly audio_duration_seconds. Since the capped segment almost always also has its own min_decode_time (true whenever lookback_frames > 0), run_length_encoding.decode_events treats max_time as the EXCLUSIVE end of a half-open [min_time, max_time) interval -- correct when max_time is the *next* segment's own min_decode_time (that segment's min_time picks up anything at the exact boundary instead), but there is no next segment here to do that. A real note ending at exactly the last frame of the file -- an entirely ordinary case, not a contrived edge case -- was silently dropped instead of kept. Nudges the cap to audio_duration_seconds + 1e-6: far smaller than a single codec step (0.01s at the defaults), so it can't admit a genuinely hallucinated event from the padding tail, which would need at least one full step past audio_duration_seconds to be encoded at all. Surfaced by issue #8's end-to-end tests.
lookback_e2e_test.py: for a fixed reference NoteSequence, simulates a *perfect* multi-window transcription -- segmented exactly as _windowed_input_dataset would per WindowGeometry, but reporting the true events within each window's own observed range instead of running a real model -- and decodes it back through the actual production combine path (metrics_utils.event_predictions_to_ns, NoteEncodingWithTiesSpec, and the Transcriber crop-bound helpers). No checkpoint needed. Confirms, for all four requested geometries plus the 0/0 baseline: exact round-trip (note-for-note, not just as a deduplicated set -- an exact duplicate would otherwise hide inside set equality), no truncation or duplication of a note held across a window boundary, correct handling of a note starting at the very beginning of the recording (window 0's lookback-padding edge) and one ending at the very end (the last-window tail-cap edge), geometry invariance (all four produce byte-for-byte the same decoded output), and that the 0/0 baseline split across several windows matches decoding the whole recording as one unsplit window. Building this test surfaced two more real bugs beyond the two review already caught in the Transcriber-wiring PR (see that PR's latest commits): _quantize's floating-point modulo bug, and _cap_last_segment_tail's exact-boundary drop. Both are fixed there, not here -- this file only adds coverage. Also adds scripts/lookback_smoke_test.py: a documented, not-run-in-CI one-liner that runs one real WAV through all four geometries against a real checkpoint and reports note counts and decode diagnostics side by side, for the by-ear verification the synthetic tests structurally can't do (CLAUDE.md: F1 and listening quality can genuinely disagree). Part of the lookback feature (issue #8 of #1-#8) -- the last of the implementation issues (#9's ablation sweep and #10's training-geometry investigation are out of scope here, per the earlier scoping decision).
- Rename lookback_smoke_test.py -> lookback_smoke.py: pytest.ini collects every *_test.py file, so the old name made this a silent zero-test module in any repo-wide pytest run, and would have become a real collection failure the moment it gained any module-level code. - Add lookback-only and lookahead-only geometries to _TEST_GEOMETRIES: the module docstring claims the round-trip property holds "lookback, lookahead, both, or neither" but no existing case exercised exactly one in isolation. Surfaced a real remaining bug: for the lookback-only case (lookahead=0), the last window's nominal end lands exactly on the audio's real end, and event_start_indices' "strictly before this frame" convention excluded an event sitting exactly on that boundary -- slicing now goes one frame past the window's nominal end (a real window gets the same slack for free via tf.signal.frame's pad_end=True). - Correct an overclaiming comment: the held-note test's note does NOT cross a boundary under the baseline geometry (it fits entirely inside one window there); only true for the overlapping geometries. - lookback_smoke.py: only time transcribe_file(), not Transcriber's own checkpoint restore (previously dominated and hid the geometry-dependent cost the column exists to show); an explicit None-check instead of `or` for --input-length so an (invalid) explicit 0 isn't silently replaced by the default; wrap each geometry in try/except so one incompatible geometry doesn't discard every row already computed; add lookback-only and lookahead-only rows; corrected a docstring claim that didn't match what the code actually computes at input_length=512.
main()'s final diagnostics print read from the CLI-local WindowGeometry built only for pre-flight validation, instead of from `result` (TranscriptionResult), which already carries the same fields for exactly this purpose. The two were only numerically identical because both happened to derive from the same kwargs via the same constructor -- if Transcriber's own geometry resolution ever diverges from the CLI's pre-check, the printed diagnostics would silently describe a different geometry than what actually ran.
Lookback: wire lookback_frames through Transcriber and TranscriptionResult
Lookback: expose --lookback-frames / --lookback-seconds on the CLI
Lookback: end-to-end segmentation-invariance and baseline-equivalence tests
Lets single-instrument input skip the model's own program-change predictions entirely: NoteDecodingState.force_program pins current_program at init and makes decode_note_event ignore 'program' tokens (still consumed, so shift/timing stays in sync). As a side effect this also merges note fragments that would otherwise split across differing (wrong) program guesses, since active notes are keyed by (pitch, current_program, current_rhythm). Wired through Transcriber(force_program=...) and CLI --force-program [0-127], with range validation before any checkpoint is touched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
Opened against the wrong repo by mistake (gh defaulted to upstream instead of my fork). Closing — redoing this against mrkkucharski/mt3. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Author
|
this was my mistake. good you closed it. |
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
NoteDecodingStategets aforce_programfield: when set, it pinscurrent_programfrom init anddecode_note_eventignoresprogramtokens instead of updating state from them (still consumed, so shift/timing stays in sync). Propagated into the lookback shadow state too.Transcriber(force_program=...)builds an encoding spec that pins this on every fresh decoding state; range-validated to[0, 127]before any checkpoint is touched.TranscriptionResultnow carriesforce_programalongside the other run-config fields it already tracks.--force-program [0-127], threaded through and echoed in the human-readable summary output.Useful for single-instrument input where the model's own program-change predictions are unreliable/irrelevant — as a side effect it also merges note fragments that would otherwise split across differing (wrong) program guesses across a note's onset/offset, since active notes are keyed by
(pitch, current_program, current_rhythm).Test plan
note_sequences_test.py: new decode-level test showing a note whose onset/offset carry mismatched program tokens closes correctly and lands on the forced program.transcription_test.py:force_programrange validation raises before checkpoint load;_build_encoding_specunit-tested via a bare namespace stand-in forself.cli_test.py: flag parsing, range rejection, and pass-through intoTranscriberkwargs.153 passed, 20 skipped.🤖 Generated with Claude Code