feat: wire compile_model into modelmaker (explicit opt-in) + 4 review follow-ups - #37
Open
musicalplatypus wants to merge 31 commits into
Open
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.
Plan 1 (harden-compile-work-followups): fix the run_quant_train_only crash (radar + timeseries_classification), strengthen the 3 weak compile_model_if_enabled tests, fix radar's DataLoader worker leak. Plan 2 (wire-compile-model-into-modelmaker): close the gap where compile_model_if_enabled was wired into tinyml-tinyverse but never reachable from the actual tinyml-modelmaker product path for radar/vision/audio, unlike timeseries.
…-defaults Adds compile_model=0 to radar/params.py's training dict and calls apply_hardware_defaults from init_params, matching timeseries's existing pattern. Wires --compile-model into radar_base.py's train argv builder so the field actually reaches the trainer. Without this, --compile-model was unreachable from the modelmaker (product) path even though tinyml-tinyverse's radar_classification.train.main() already supports it.
radar/params.py defaulted training.lr_scheduler to 'constantlr', which isn't one of init_lr_scheduler's three real scheduler names (StepLR, CosineAnnealingLR, ExponentialLR) or its special 'none' value (which is what actually produces a genuinely constant LR). Any radar training run through modelmaker with default params crashed during optimizer setup. Found by an implementer subagent during manual E2E verification while wiring compile_model into radar's modelmaker layer (task-1-report-planE.md, spawned as a separate tracked item); every other module (timeseries, audio, vision) already defaults to a supported value.
…ehavioral assertions
…e-defaults Adds compile_model=0 to vision/params.py's training dict and calls apply_hardware_defaults from init_params, matching timeseries's and radar's (Task 1) existing pattern. Wires --compile-model into image_base.py's train argv builder so the field actually reaches the trainer. Without this, --compile-model was unreachable from the modelmaker (product) path even though tinyml-tinyverse's image_classification.train.main() already supports it.
create_data_loaders() sets persistent_workers=True whenever args.workers > 0 (the default of 8). image_classification/train.py and all four timeseries_* reference scripts already wrap their training body in try/finally to call shutdown_data_loaders() on exit. radar_classification/train.py's main() never did this -- moot while main_debug (dead code) was the live entrypoint, but a real worker-process leak now that main() is live. Wraps the training body (everything after create_data_loaders through the end of main()) in try/finally, matching image_classification's exact wrapping boundaries. No logic changes, only indentation plus the new try/finally lines.
…-defaults Adds compile_model=0 to audio/params.py's training dict and calls apply_hardware_defaults from init_params, matching timeseries's, radar's (Task 1), and vision's (Task 2) existing pattern. Wires --compile-model into audio_base.py's train argv builder so the field actually reaches the trainer. Without this, --compile-model was unreachable from the modelmaker (product) path even though tinyml-tinyverse's audio_classification.train.main() already supports it.
Add a shared call-order recorder to the radar/image/audio compile-wired tests so they assert move_model_to_device runs before compile_model_if_enabled. The prior version mocked move_model_to_device as a bare no-op with no observable side effect, so no assertion could distinguish compile running before vs. after the device move -- a gap an independent reviewer proved empirically by swapping the two calls in radar_classification/train.py and showing the test still passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nabled mock The mock's lambda only accepted 3 positional args, but train.py's main() now calls compile_model_if_enabled(model, args, logger, input_shape=...), matching the pattern already used in radar/image/audio classification. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
apply_hardware_defaults auto-raises compile_model to 1 when
torch.cuda.is_available() -- so test_init_params_carries_compile_model_field
in each of test_{radar,vision,audio}_compile_model_param.py asserted
compile_model == 0 unconditionally, which fails wherever CUDA is actually
present (including GX10, this project's own CUDA test machine). Found by
an independent whole-plan review, which proved it by simulating CUDA
availability and showing all three return 1.
Fixed by wrapping init_params() in
patch('torch.cuda.is_available', return_value=False), matching the
pattern the pre-existing test_hardware_defaults_integration.py already
uses for the same reason.
Also relocated test_radar_lr_scheduler_default.py from tinyml-tinyverse's
test suite to tinyml-modelmaker's -- it imports tinyml_modelmaker (the
wrong dependency direction for that package) and tests a modelmaker
params default, not tinyverse code.
Whole-plan review flagged this as a decision needing the repo owner's explicit call rather than silent inheritance from timeseries's pattern. Decision: keep auto-enable, for consistency with timeseries and because the measured regressions were on very short synthetic benchmarks where warmup dominates -- real runs should amortize it. Recorded with rationale and a documented fallback if that assumption doesn't hold.
…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.
musicalplatypus
changed the title
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 (addressed to an agent, not useful to a human reader of a merged PR) and normalized all checkboxes from - [ ] to - [x] across all 5 plan docs -- every task they describe is actually done; the unchecked boxes were misleading in a shipped record.
apply_hardware_defaults infers "what the user explicitly set" by checking isinstance(args[0], dict) -- when a caller passes a YAML file path instead of a dict (a normal, supported way to call init_params()), that check silently evaluates to "nothing was explicitly set," so an explicit compile_model: 0 / native_amp: false in the YAML gets overridden on any CUDA machine. This was independently reproduced against a sibling project's analysis of the same function (mmcli's ANALYSIS-cuda-auto- defaults.md, finding F-1). Remove the apply_hardware_defaults call from radar/vision/audio's init_params() -- timeseries, which predates this session's changes, is unaffected and keeps the auto-enable behavior. compile_model stays a reachable, explicit-opt-in field (default 0) with its --compile-model argv wiring fully intact for all three modules; only the CUDA auto-flip is removed. Adds a regression test per module proving compile_model stays 0 even when CUDA is mocked available, to guard against silently re-wiring apply_hardware_defaults back in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
musicalplatypus
changed the title
…tion Supersedes (without deleting) the 2026-08-14 "keep auto-enable" decision recorded earlier the same day, with the rationale for the reversal -- F-1 from a sibling project's independent analysis of apply_hardware_defaults, reproduced against radar/vision/audio in 7651819. Keeps the audit trail of both decisions rather than rewriting history. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 → #37 (this PR). Depends on #34, #35, #36 — please merge those first; this PR's diff includes their commits until it does.
Summary
Three independent reviews (plan-quality, code-correctness, and an independent re-derivation of the technical claims) of the
compile_model_if_enabledwiring in #35 surfaced four follow-up items, all closed here:run_quant_train_only: True(a supported radar config) crashed withAttributeErrorwhen no prior float-training call had populated a module-global dataset cache — identical latent bug intimeseries_classification, fixed in both.inspect.getsource+ substring check that would pass under several realistic regressions (the call deleted outright, or left present as a comment/dead branch; its return value discarded; the call reordered relative to another). Replaced with real behavioral tests that patchcompile_model_if_enabled, drive the realmain(), and assert on its call arguments and that its return value is actually used. One initial gap in the rewrite (reordering againstmove_model_to_devicespecifically wasn't caught) was itself caught by re-review and fixed.radar_classification/train.py'smain()never calledshutdown_data_loaders(), leaking persistent DataLoader worker processes — a real leak oncemain()became the live entrypoint (fix: radar_classification's run() dispatched to a dead debug function, not main() #34). Verified via an automated whitespace-normalized diff that the ~110-line reindent introduced zero logic changes beyond the fix itself.compile_model_if_enabledwas wired into thetinyml-tinyversescripts (feat: wire compile_model_if_enabled into radar, image, and audio classification #35) but never reachable from the actualtinyml-modelmakerproduct path for radar/vision/audio — onlytimeserieshad the params/argv wiring. Extendedcompile_model(field +--compile-modelargv wiring) to all three, each verified end-to-end through the real modelmakerrun()path, not just unit tests.Also fixed along the way: radar's default
lr_schedulervalue wasn't valid forinit_lr_scheduler(any default-params radar run through modelmaker crashed on optimizer setup).One unrelated one-line fix is bundled in — a pre-existing stale mock in
test_anomalydetection_train_device_crash.pythat didn't accept theinput_shapekwargcompile_model_if_enablednow passes (from #35). Found and fixed independently by someone else working in the same shared checkout while this PR's work was in progress.Policy decision (this is the part that changes behavior, not just fixes bugs)
Revised 2026-08-14. This PR originally also wired
apply_hardware_defaultsinto radar/vision/audio, socompile_modelwould auto-enable on CUDA the same way it already does fortimeseries. That auto-enable has since been reverted for these three modules.Why:
apply_hardware_defaultsinfers "what the user explicitly set" viaisinstance(args[0], dict). When a caller passes a YAML file path instead of a dict — a normal, supported way to callinit_params(), and how at least one downstream tool invokes it — that check silently evaluates to "nothing was explicitly set." An explicitcompile_model: 0/native_amp: falsein the YAML gets silently overridden on any CUDA machine. This was independently found and reproduced against this exact function by a sibling project auditing it from the consumer side, and it's a correctness bug serious enough (silent override of an explicit user setting) that the auto-enable isn't worth keeping untilapply_hardware_defaultscan correctly introspect YAML-path configs, not just dict configs.compile_modelremains a reachable, explicit-opt-in field (default0) with its--compile-modelargv wiring fully intact for radar/vision/audio — set it directly in your config to turn compilation on.timeseries, unaffected by this change (predates this PR stack), keeps its existing auto-enable-on-CUDA behavior.compile_modelis not currently exposed in any module's UI/descriptionsblock, so this is a YAML-only override either way.Full rationale and history recorded in
docs/superpowers/plans/2026-08-14-wire-compile-model-into-modelmaker.md's Decision section.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
tinyml-tinyverseandtinyml-modelmakersuites pass locally (only pre-existing, unrelated failures remain — confirmed by name and diff scope; CI failures above are the same known pre-existing ones)lr_schedulerbugfix has its own regression testcompile_modelstays0even with CUDA mocked available, to catch any future re-wiring ofapply_hardware_defaultsinto these modules