fix: remove dead --sample-rate flag from audio_classification scripts - #36
Open
musicalplatypus wants to merge 17 commits into
Open
fix: remove dead --sample-rate flag from audio_classification scripts#36musicalplatypus wants to merge 17 commits into
musicalplatypus wants to merge 17 commits into
Conversation
Post-fix (main() live per Task 1): CPU 0.621s/epoch, MPS 1.066s/epoch (1.72x slower) -- gap did not close vs pre-fix's 1.90x. Root cause: compile_model_if_enabled/apply_hardware_defaults are never called anywhere in radar_classification/train.py (only wired into the timeseries_* reference scripts), so torch.compile/AMP were off for all four runs regardless of which function run() dispatches to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Whole-plan review flagged that the Goal/Architecture sections still asserted the disproven premise (main() wired to compile_model_if_enabled) 130+ lines above the actual Results. Ledger and Results were already honest; this fixes the presentation for a top-down reader.
CUDA beats CPU 4.8x on GX10 even without compile (opposite of the Mac's MPS-loses-to-CPU picture). torch.compile's inductor backend failed to build on GX10 (Triton/gcc CUDA-codegen toolchain issue) and fell back to eager cleanly per the existing warmup-fallback mechanism -- so the CUDA compile question is still open there. CPU's aot_eager backend engaged successfully and gave a small (~3%) win, opposite in sign from the Mac's ~23% CPU loss for the same backend on the same model.
…ompile wiring CUDA still beats CPU on GX10 without compile, but only 1.3x here (vs radar's 4.8x) -- CNN_LENET5's conv/pool ops give GX10's CPU real work to do, unlike radar's pure linear stack. Same Triton/gcc inductor build failure reproduces identically to Task 1, confirming it's an environment-level GX10 toolchain issue, not model-specific. CPU aot_eager regressed ~104% here, a bigger relative hit than either machine saw for radar's smaller model.
…ompile wiring CUDA beats CPU 2.2x on GX10 without compile. GX10's CPU aot_eager is a clear net loss (+75%) for audio, flipping sign from the Mac's small CPU win -- the first case where the same backend/model combo disagrees between machines. Same Triton/gcc inductor build failure reproduces for the third module in a row, confirming it's systemic to this GX10 environment. Needed real torchaudio==2.9.0 (matching torch's version, unlike 2.11.0's ABI mismatch) plus a soundfile-backed load() patch in the benchmark driver only, since torchaudio's default torchcodec backend needs FFmpeg, which GX10 doesn't have installed.
Whole-plan review found the code across all 3 tasks clean but flagged several doc-only defects: a stale leftover paragraph contradicting the real GX10 section above it, a synthesis claim with the GX10 CPU win/loss grouping backwards, a false "first sign-flip" claim that contradicted Task 1's own recorded radar sign-flip, an imprecise safety argument for the audio torchaudio.load->soundfile patch, two contradicting ad-hoc mechanistic explanations for GPU-vs-CPU advantage, and one garbled/self-contradicting sentence. Fixed all of the above inline, replaced the synthesis with a data table (9 aot_eager measurements) and the stronger, better-supported summary the review recommended, and empirically closed the one open risk (checked the GX10 audio run's logged accuracy: 100%, confirming the soundfile patch didn't corrupt input). Added a progress ledger and spun out the two follow-up items the review flagged (sampling-rate/sample-rate naming collision, GX10 Triton/gcc inductor build failure) as separately tracked rather than left buried in a closed plan.
… modules python3.12-dev was missing on GX10 (no Python.h anywhere on the box), which is what Triton's cuda_utils.c build needed. Fixed on GX10 by the repo owner. Re-ran --compile-model 1/CUDA for radar, image, and audio: inductor now builds and engages with zero fallback warnings on all three. All three show large regressions at 30 epochs (radar +24%, image +407%, audio +373%), most likely one-time autotuning overhead dominating short runs rather than steady-state cost -- flagged as an observed number, not isolated from warmup in this pass.
Peer review caught that the code fix (a99f903) shipped without its own plan doc ever being committed -- it stayed untracked, breaking the convention plans 1 and 2 followed.
…eview Independent peer review found 4 statements left stale by an earlier correction pass (they still said the GX10 inductor question was "unmeasured"/"blocked" after the addendum had already resolved it), a garbled sentence, a noise-floor figure misapplied to discount a more rigorous result than the one it came from, and -- most importantly -- that the headline "aot_eager is a consistent net loss" conclusion didn't survive isolating one-time compile warmup from steady-state cost. Empirically re-checked with fresh benchmarks: image_classification's apparent CPU/MPS regressions (+124%/+29%) are mostly or entirely warmup dilution -- steady-state compile is actually ~37-50% faster on both devices once warmup is excluded. Rewrote Task 2's Results and the whole-plan synthesis to reflect this; the corrected picture is more mixed and run-length-dependent than either "consistent loss" or "consistent win," which is the honest conclusion the data supports.
4 tasks
…n doc An independent PR review caught a private Tailscale hostname, SSH key path, and username committed to a doc already live on open PRs against a public upstream repo. Genericized all references to the remote GX10 machine and local venv paths -- no change to the technical content, only removed identifying infrastructure details.
This was referenced Aug 14, 2026
Claimed main_debug() alone lacked compile_model_if_enabled/apply_hardware_defaults, implying main() already had it -- at this commit neither function does (that's added in a later change). Corrected to describe what's actually true at this point: main() is the one that CAN be extended with it, not that it already is.
Stripped the 'For agentic workers: REQUIRED SUB-SKILL...' header and normalized checkboxes from - [ ] to - [x] -- every task described is done.
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
2 participants
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.
This is part of a 4-PR stack: #34 → #35 → #36 (this PR) → #37. Depends on #34 and #35 — please merge those first; this PR's diff includes their commits until it does.
Summary
audio_classification/train.pyandtest_onnx.pyeach independently defined--sample-rate, butGoogleSpeechCommandsDatasetnever readsself.sample_rateanywhere — onlyself.sampling_rate, set from a different, shared flag (--sampling-rate, defined once in the base parser). Confirmed via repo-wide grep: no code anywhere readsself.sample_rateorargs.sample_rate.Practical impact was limited (the production
tinyml-modelmakerpath only ever passes--sampling-rate, which already works correctly), but--sample-ratewas a genuine trap for anyone reading or invoking these scripts directly — it looks like the flag that controls audio sample rate and silently does nothing.Changes
--sample-rateargparse definitions--sampling-rate(the flag that actually works) is untouchedA separate, real bug found while investigating this (not fixed here)
The base parser's generic
--sampling-rate(meant for radar/timeseries FFT preprocessing) is also whatGoogleSpeechCommandsDatasetreads for the actual audio sample rate — so a non-audio value passed there (e.g. something meaningful for radar's FFT sizing) producesn_fft=0and aRuntimeErrorin MFCC extraction. This didn't crash the supported modelmaker path (it always passes a real sample rate here), but it's a real, reproducible crash for anyone passing an unexpected--sampling-ratevalue to audio_classification directly. Happy to file this as a separate issue if useful — flagging here since it surfaced during this PR's investigation.CI
CI is red on all three OS jobs — pre-existing on
main, same root cause and same non-involvement as noted in #34; unchanged by this PR.Test plan
test_audio_sample_rate_dead_flag_removed.py, checks bothtrain.pyandtest_onnx.pytinyml-modelmaker) ever passed--sample-rate