Skip to content

fix: carry source_split through validation, so split preservation actually runs - #26

Merged
Rebreda merged 2 commits into
mainfrom
fix/source-split-dropped-by-validate
Aug 24, 2026
Merged

fix: carry source_split through validation, so split preservation actually runs#26
Rebreda merged 2 commits into
mainfrom
fix/source-split-dropped-by-validate

Conversation

@Rebreda

@Rebreda Rebreda commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Split preservation shipped in 0.2.0 and never worked. My bug, and my verification was what let it through.

$ listenr build-dataset --manifest <mdc> --preserve-splits
ERROR: --preserve-splits was requested but no entry has a usable source_split.

on a manifest whose every record has source_split.

Cause

validate_entry rebuilds each record from a whitelist, and source_split was not on it. assign_splits runs on validated entries, so by the time the preserve logic looked, the field was always gone and the labelled count was always zero. Both branches were unreachable: --preserve-splits always errored, and the auto-detect default always fell through to reshuffling.

Reproduced on the first 50 records of the real manifest:

raw records with source_split: (50, 0)
after validate_entry:          (0, 50)

Why I did not catch it

My verification for #24 called assign_splits directly on raw manifest records, so it exercised a path no user could reach and printed a convincing result. The unit tests had the same gap: they passed dicts that still carried source_split. Everything agreed with everything else, and none of it went through the pipeline.

Fix

Carry the field through validation. Also write it to the CSV, so a built dataset records whether its splits are the corpus's own or a reshuffle. That distinction is the difference between a comparable result and a speaker-leaked one, and it was not previously recoverable from the output.

write_csv uses extrasaction="ignore" and the HF writer just gains a column that prepare_example already strips, so nothing downstream chokes.

Tests that would have caught it

Six end-to-end tests that write a manifest file to disk and run the real CLI, asserting the resulting split sizes. Against the 0.2.0 behaviour four of the six fail; with the fix all pass.

--- against shipped 0.2.0 behaviour ---
FAILED test_corpus_splits_survive_the_whole_pipeline
FAILED test_preserve_splits_flag_does_not_error_on_a_real_manifest
FAILED test_unlabelled_records_go_to_train
FAILED test_source_split_is_recorded_in_the_output
4 failed, 2 passed
--- fix restored ---
6 passed

Verification against the real corpus, through the full pipeline this time

validated 2194   corpus-labelled 2104   unlabelled 90
preserved -> train 1472, dev 385, test 337

dev and test match the corpus exactly. The 90 unlabelled go to train, so dev and test keep only the corpus's own speakers.

364 tests pass on 3.13, 342 passed and 12 skipped on 3.11.

Wants a 0.2.1 release once merged, since the headline fix of 0.2.0 is inert without it.

g added 2 commits August 23, 2026 22:57
Split preservation shipped in 0.2.0 and never worked. --preserve-splits
errored on every input, including corpora that plainly have the field, and
the auto-detect default always fell through to reshuffling.

validate_entry rebuilds each record from a whitelist and source_split was not
on it. assign_splits runs on validated entries, so by the time the preserve
logic looked, the field was always gone and the labelled count was always
zero. Both branches were unreachable.

My verification for #24 ran assign_splits against raw manifest records and
never went through validate_entry, so it demonstrated a code path that no
user could reach. The unit tests had the same gap: they fed dicts that still
carried source_split.

Fixed by carrying the field through, and added end-to-end tests that start
from a manifest file on disk and run the real CLI. Those fail against the
0.2.0 behaviour, four of six, and pass with the fix.

source_split is now also written to the CSV, so a built dataset records
whether its splits are the corpus's own or a reshuffle. That is the
difference between a comparable result and a speaker-leaked one, and it was
not previously recoverable from the output.

Verified against the real MDC manifest through the full pipeline: 2194
validated, 2104 corpus-labelled, 90 unlabelled, giving train 1472, dev 385,
test 337. dev and test match the corpus exactly.
A pass over every doc for content that was stale, or that narrated decisions
and corrections instead of telling the reader what to do.

Removed the deliberation. The Dockerfile carried a paragraph weighing a base
image bump we had not made. The gfx override section had accumulated three
layers of correction, arguing about what earlier advice implied. Neither
helps anyone running the tool. Both now state the current answer once: most
cards need no override on ROCm 7.2, including Strix Halo, and if you do need
one the value names the ISA you want to be treated as.

docs/releasing.md still described setting up a first release for a project
that did not exist on PyPI. It exists, and has shipped twice. The setup
section now records what is configured, and says plainly that TestPyPI has no
publisher so the rehearsal path will fail until one is added.

docs/recording.md showed startup output that the program does not produce: an
emoji and an em dash that are not in the code, and four missing lines.
Replaced with the real banner, captured from a run.

README gained a commands table. The full surface was only discoverable by
running listenr with no arguments, and three of the ten commands appeared
nowhere in the README at all. Three tests keep it from drifting: every
command must be listed, no invented command may appear, and every extra must
be named.

Also corrected the finetune command's own help, which still said Whisper
only, and two README table rows that predated Moonshine and the importers.

Removed em dashes throughout, matching the rest of the repo.

Verified: 367 tests on 3.13, 345 and 12 skipped on 3.11, 3.12 and a dev-only
install matching CI. Build and twine check pass, the README still renders for
PyPI with all seven images surviving the sanitiser, compose parses, actionlint
is clean, and every flag referenced anywhere in the docs exists in the CLI.
@Rebreda
Rebreda merged commit 7add9eb into main Aug 24, 2026
6 checks passed
@Rebreda
Rebreda deleted the fix/source-split-dropped-by-validate branch August 24, 2026 03:10
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