Disambiguate output stems so same-basename inputs don't overwrite (#7) - #81
Conversation
Clip names are derived from the input file stem plus a segment index, so two inputs with the same basename (a/computah.wav and b/computah.wav) wrote the same clip names and the second file silently overwrote the first. process() still summed stats["kept"] across both, so it reported more clips than landed on disk and training examples vanished with no error -- the exact hazard for the multi-folder and globbed workflows the script is built for. Fix at the source rather than per-caller: _unique_stems() assigns a distinct output stem per input file, in input order. A stem that is already unique is kept as-is, so the common single-folder case is unchanged; a collision gets the lowest numeric suffix not otherwise taken, and singletons are reserved first so a disambiguated "computah-1" can never land on a real "computah-1" input. Both the segmented and the background write paths route through it. process() now returns the count of files actually written (a set of paths) instead of the requested segment total, so the reported number equals the files on disk by construction. Covers the duplicate-basename case in test_prep_wake_samples.py for both the positive (segmented) and background paths, with the two inputs carrying different burst counts so an overwrite shows up as fewer files than clips. Mutation-checked: reverting only the source fails exactly the two new checks. Closes #7 wake-20260722T0805-774163
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daf365827b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
_unique_stems keeps in-run basenames distinct, but two collisions slip past it and silently drop a training example: a case-insensitive filesystem where a/Computah.wav and b/computah.wav resolve to one path, and a rerun into a populated out_dir. A casefold reservation key would fix the first but wrongly disambiguate distinct stems on the case-sensitive filesystem this runs on. Guarding the write instead is platform-correct for both and fails loud on any residual collision rather than clobbering.
The previous hard path.exists() guard broke the documented refresh: the docs tell people to rerun prep_wake_samples straight into samples/, and that now aborted on the first reused name and could leave a half-written dataset. The invariant is narrower than 'never overwrite': two inputs in one run must not clobber each other, but a prior run's leftover is a fine overwrite. _write_unique tracks the inodes written this run, so a case-insensitive FS collision (distinct-cased stems share an inode) still fails loud while a rerun refreshes cleanly.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d03b5437a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Suffixing every member of a colliding basename group meant a refresh that added a second computah.wav renamed the original too, stranding the last run's computah_###.wav. eval_wake_threshold reads every clip in the directory, so those orphans quietly trained and scored as duplicates. One member of each group now keeps the bare stem, picked by resolved path rather than input order: reordering the same files must not move the bare stem to a different one, or the strand comes right back. Reserving every input stem up front (not just singletons) keeps two overlapping groups from both claiming computah-1. Clips this run did not write now draw a warning naming the files and the fix, since deleting the user's dir would be too eager.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7763614e66
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The stale-clip scan reads the whole output directory, not just what this run wrote, because training and eval score every clip sitting there. It was written to mirror eval_wake_threshold._clips() and its comment said so, but it copied that function's body without its precondition: _clips() returns empty when the directory is absent, and the copy went straight to iterdir(). Only _write_clip() creates out_dir, so a run that produced no clips (a silent take, or every segment dropped by --min-dur or --max-dur) left it missing, and the scan raised FileNotFoundError after all the decode work was already spent. Skipping the scan beats creating the directory here. A missing directory holds no clips from an earlier run, and creating one would leave an empty directory behind for every run that produced nothing. The precondition now sits in _stale_clips(), next to the read it guards, because keeping the two apart is what let the copy lose it. Guarding on is_dir() alone would also swallow an --output that names an existing file, a real mistake worth naming rather than passing over in silence. That check runs before any audio is decoded, so one message covers it instead of whichever exception the run happened to reach first. #82 tracks what is still duplicated: the two modules keep separate AUDIO_EXTS sets, so this warning can name a file eval would never score. wake-20260722T1502-062c47
|
Progress on #7: cleared the open review thread on this PR. The zero-clip Filed #82 for the remaining duplication between wake-20260722T1502-062c47 |
1 participant
Problem
prep_wake_samples.pyderives each clip's name from the input file's stem, so two inputs that share a basename (a/computah.wavandb/computah.wav) wrote the same clip names and the second file silently overwrote the first.process()still summedstats["kept"]across both files, so it reported more clips than actually landed on disk. Any multi-folder or globbed recording workflow with duplicate stems lost training data with no error — issue #7.Fix
_unique_stems()assigns a distinct output stem per input file, in input order. A stem that is already unique is kept unchanged (the common single-folder case is untouched); a collision gets the lowest numeric suffix not otherwise taken. Singletons are reserved first, so a disambiguatedcomputah-1can never collide with a realcomputah-1input. Both the segmented and background write paths route through it.process()now returns the count of files actually written (tracked as a set of paths) rather than the requested segment total, so the reported number equals the files on disk by construction — the second acceptance criterion, held even if a future change reintroduced a name collision.Tests
test_duplicate_basename_no_overwritecovers both the positive (segmented) and background paths, with the two inputs carrying different burst counts so an overwrite shows up as fewer files than clips. Mutation-checked: reverting only the source fails exactly the two new checks and nothing else. Full suite 14/14, ruff format + check clean.Closes #7
wake-20260722T0805-774163