fix: carry source_split through validation, so split preservation actually runs - #26
Merged
Merged
Conversation
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.
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
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.
Split preservation shipped in 0.2.0 and never worked. My bug, and my verification was what let it through.
on a manifest whose every record has
source_split.Cause
validate_entryrebuilds each record from a whitelist, andsource_splitwas not on it.assign_splitsruns 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-splitsalways errored, and the auto-detect default always fell through to reshuffling.Reproduced on the first 50 records of the real manifest:
Why I did not catch it
My verification for #24 called
assign_splitsdirectly 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 carriedsource_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_csvusesextrasaction="ignore"and the HF writer just gains a column thatprepare_examplealready 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.
Verification against the real corpus, through the full pipeline this time
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.