feat(perf): cache per-module builds and honor --rebuild/--ignore-cache#1004
Conversation
Per-module benchmarking built every submodule into a throwaway tempfile.TemporaryDirectory with no cache_key and no rebuild flag, so artifacts were rebuilt on every run and --rebuild/--ignore-cache were silently ignored. The single-model path already honored these flags via WinMLAutoModel. Mirror auto.py's cache logic in _perf_modules: build into the model's persistent cache dir keyed per submodule (cfg.generate_cache_key() folds in loader.module_path and traced I/O shapes, so sibling instances get distinct keys and coexist in one model dir), and forward rebuild. With --ignore-cache, fall back to a temp dir + forced rebuild. Closes #918
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Reviewed the per-module cache changes. The logic cleanly mirrors auto.py/the single-model path (use_cache = not ignore_cache, force_rebuild = rebuild or ignore_cache, persistent model dir + per-module cache_key, temp-dir fallback on --ignore-cache), and build_hf_model creates output_dir and scopes its rebuild-glob to {cache_key}_*.onnx, so sibling modules coexist and --rebuild won't wipe each other's artifacts. Ran tests/unit/commands/test_perf_module.py locally: 15 passed. Two notes inline — one behavioral nuance and one optional test-coverage suggestion; neither is blocking.
…mize isn't ignored skip_optimize / hack_max_optim_iterations are build_hf_model kwargs, not part of WinMLBuildConfig, so generate_cache_key() didn't reflect them. After a cached build, re-running with --no-optimize / --no-analyze / --max-optim-iterations silently reused the cached artifact in both the single-model (auto.py) and per-module (perf.py) paths. get_cache_key() now folds recognized build-control overrides into the key. Default builds keep their existing key (backward compatible); only non-default toggles get a distinct suffix. Applied at every call site (auto.py HF + ONNX, perf module, build single-model).
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Re-reviewed the new commit (53a8f26) — it directly resolves both points from my previous review:
- Optimize/analyze toggles now affect the cache key.
get_cache_keyfoldsskip_optimize/hack_max_optim_iterationsin, applied at all four call sites (auto.py HF + ONNX, perf module, build single-model). I checked key/build consistency: the same controls folded into the key are exactly what reachbuild_hf_model/build_onnx_model, so a later--no-optimizerun no longer silently reuses an optimized artifact. Nice that path.py's stdlib-only / zero-internal-imports contract is preserved (hashlib/json/typing are all stdlib). - Collision-free claim now has a real test.
test_sibling_instances_get_distinct_cache_keysuses two distinct config hashes (no longer mocked to a constant) and asserts distinctcache_keys reachbuild_hf_model;test_no_optimize_changes_cache_keyguards the flag→key coupling.
Verified locally: applied the PR head and ran tests/unit/cache/test_path.py + tests/unit/commands/test_perf_module.py → 33 passed. One optional, non-blocking note inline. LGTM otherwise.
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Approving. The follow-up commit cleanly resolves both points from my earlier reviews: optimize/analyze toggles now fold into the cache key at all four call sites (verified key/build consistency), and the collision-free claim has real test coverage. Verified locally — tests/unit/cache/test_path.py + tests/unit/commands/test_perf_module.py: 33 passed. The two inline notes are minor/optional and not blocking.
Summary
winml perf --modulebuilt every submodule into a throwawaytempfile.TemporaryDirectorywith nocache_keyand norebuildflag, so per-module artifacts were rebuilt on every run and--rebuild/--ignore-cachewere silently ignored. The single-model path already honored these flags viaWinMLAutoModel→auto.py.This mirrors
auto.py's cache logic in_perf_modules:~/.cache/winml/artifacts/<model_slug>/) and is reused on the next run.--rebuild: reuses the cache dir but overwrites artifacts.--ignore-cache: falls back to a temp dir + forced rebuild (prior behavior, now opt-in).Why per-module caching is collision-free
Each submodule config carries a unique
loader.module_path(plus its traced I/O shapes).cfg.generate_cache_key()hashes the full config, so every instance gets a distinct key — even shape-identical siblings — andbuild_hf_model'scache_keyfilename prefixing keeps each instance'smodel.onnxseparate within one shared model dir.Behavior change worth noting
Per-module artifacts now persist by default (more disk use), which is the intended consequence of matching the single-model /
auto.pybehavior.Tests
Added
TestPerfModuleCache(default → persistent dir +rebuild=False;--rebuild→rebuild=Truein cache dir;--ignore-cache→ temp dir +rebuild=True). Fulltests/unit/commands/test_perf_module.py: 15 passed.Closes #918
🤖 Generated with Claude Code