Make target_compilation survivable across worker restarts (#299 prereq 1; implements #217) - #317
Merged
Merged
Conversation
Investigation only, no code changes yet. Diagnosed B's lost cache and the memory-bound; see PROGRESS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Design in PROGRESS.md: (1) --checkpoint-root decoupled from ephemeral out-dir (fixes B's lost cache), (2) #217 finer reform-vector cache key, (3) memory-bound analysis (reform loop already batched; base-sim is residual one-time peak, scoped out). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The predecessor replaced the inline checkpoint-path block in main() with a call to _resolve_checkpoint_paths(args, artifact_root=...) but died before defining the helper. Define it near the other checkpoint helpers as a pure function that reproduces the original default-under-<out>/artifacts semantics, with the new --checkpoint-root default override and unchanged --no-* / explicit override precedence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ouched files (#299) Fixes the two ruff findings in the salvaged test/driver code so `ruff check .` (the CI lint gate) passes: the synthetic kill sentinel gets the required Error suffix, and both touched files are ruff-formatted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 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
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.
Summary
Makes
tools/build_us_fiscal_refresh_release.pytarget_compilationsurvivable across worker restarts, so a preempted or restarted Build E run reuses already-completed JCT reform materializations instead of recomputing all of them. Two independent, code-confirmed fragilities let Build B silently lose its per-reform cache and forced full recomputation; this PR closes both, adds the finer reform-vector cache key from #217, and documents a rigorous peak-memory bound for Modal box sizing.Prerequisite 1 of #299 ("fix target_compilation OOM first"). Also implements #217 (reusable materialization checkpoints across calibration-only runs).
Root cause (code-confirmed)
Per-reform materialization checkpointing already existed and is robust in isolation:
_read_reform_income_tax_cache/_write_reform_income_tax_cachewrite an atomic (os.replace), sha256-verified, shape-checked JSON+.npyentry per reform; the reform loop reads at loop top, computes on miss, writes immediately after each reform, anddels + GCs the dense vector between reforms. A kill after 2 of 5 reforms leaves 2 valid entries and a restart should resume at reform 3.Build B still lost the whole cache because of two compounding, code-level fragilities:
Ephemeral out-dir binding (primary).
target_materialization_cache_dirandtarget_frame_checkpoint_pathdefaulted to<out>/artifacts/…. When Modal preempted B and auto-restarted into a fresh--out, the default cache path moved with it, orphaning the two completed entries on the dead ephemeral dir — so all reforms recomputed. The--target-materialization-cache-dir/--target-frame-checkpointoverrides existed but nothing forced a stable location; one missing flag (or out-dir templating) reproduces B exactly.Over-coarse reform-vector key (Make target materialization checkpoints reusable across calibration-only runs #217, compounding). The per-reform cache identity included
build_commit+target_registry_version. A restart that re-resolves HEAD to a newer commit, or any registry bump, invalidated all reform entries even though the reforms and PE-US were unchanged.What this PR changes
Fix A — durable checkpoint root, decoupled from the ephemeral out-dir.
New
--checkpoint-root PATH. When set, the per-reform materialization cache and the full target-frame checkpoint default to<checkpoint-root>/target_materialization_cacheand<checkpoint-root>/target_frame_checkpoint.h5instead of under<out>/artifacts. Explicit--target-materialization-cache-dir/--target-frame-checkpointoverrides and the--no-*disable flags keep their existing precedence. Point--checkpoint-rootat a persistent Modal volume that is stable across worker restarts and a fresh--outcan no longer orphan completed reforms. The path resolution is factored into a pure, unit-testable helper_resolve_checkpoint_paths(args, artifact_root=…).Fix B — finer reform-vector cache key (#217).
The per-reform income-tax cache identity now projects the build context onto only the inputs that determine a per-household reform estimate:
(base_dataset_sha256, policyengine_us_version, target_period, congressional_district_vintage_crosswalk_sha256)plus the reform's(measure, neutralized_variable)andn_households.build_commit,seed, andtarget_registry_versionare deliberately excluded (seeREFORM_VECTOR_CACHE_CONTEXT_KEYS). A restart at a newer commit, or a calibration-only / registry-only rerun, now reuses completed reforms; a change to base-H5, reform vector, PE-US version, period, or geography still invalidates (no stale poisoning). The cache schema version is bumped 1 → 2 so old coarse-key entries live under different filenames and never collide. The whole-frame checkpoint keeps its own separate, coarser identity (_target_frame_checkpoint_identity, unchanged) — safe because it already gates on target-surface / registry identity independently. Diagnostics still record the per-reform checkpoint key and hit/miss/write status for every reform viacompilation["target_materialization_cache"], satisfying #217 AC4.Satisfies #217 acceptance criteria 1–4:
test__given_only_build_commit_changed__then_reform_cache_is_reused.test__given_changed_frame_identity__then_stale_checkpoint_is_not_reused.cache_stats["entries"]flows into release calibration diagnostics.Fix C — memory bound (analysis; no base-sim rewrite). See below.
Fix D — TDD on a tiny synthetic frame. Four new tests over a 2-household / 3-tax-unit synthetic (fully faked PE-US, no data download):
test__given_kill_after_two_reforms__then_restart_only_recomputes_the_third— raise mid reform 3, assert exactly 2 durable entries on disk, restart, assert reforms 1–2 load from cache and only reform 3 recomputes (hits==2, misses==1, writes==1), and all three reform vectors are numerically correct.test__given_changed_reform_vector__then_stale_checkpoint_is_not_reused— same measure name, different neutralized variable ⇒ recompute (no stale reuse).test__given_changed_frame_identity__then_stale_checkpoint_is_not_reused— different base-H5 sha ⇒ recompute.test__given_only_build_commit_changed__then_reform_cache_is_reused— only build_commit/seed/registry change ⇒ pure cache hit, zero reform sims.Memory-bound analysis (for Modal box sizing)
The peak-memory question in prong C resolves per-component.
target_compilation= base-sim materialization + the household-batched reform loop, and it does not hold the calibration matrix (a later, separately-timed stage). Sizing the pool from the Build B/D failure diagnosis (224,026 households, ~2.5 persons/hh ⇒ ~0.5–0.6M persons; PE-US ships 5,648 variables, measured):Microsimulationover the full pool + full entity-table copy + top-level variable caches held to teardown): the PE-US holder cache dominates. Its hard ceiling — every one of the 5,648 PE-US variables cached simultaneously at person scale — is5,648 × 605k × 8 B ≈ 25 GiB, and that ceiling is physically unreachable (most variables are unrelated benefit programs thatincome_tax's closure never touches). Realistic footprint forincome_tax+ the ~dozen top-level vars is ~1,000–2,000 distinct arrays of mixed scale ≈ 5–9 GiB._invalidate_all_caches()+del+ GC) before the loop begins, so peaks do not stack. Each iteration builds oneMicrosimulationoverbatch_sizehouseholds; at the defaultbatch=5,000(~13.5k persons) the per-batch ceiling is< 0.6 GiB, plus the resident output tables (~0.25 GiB, constant). Only ≤1 reform's dense vector is held at a time.2 × n_targets × n_households × 8 B = 2 × 32,637 × 224,026 × 8 ≈ 109 GiB, which runs aftertarget_compilationin thecalibrate()stage. If D genuinely died insidetarget_compilation(reform 3 of 5), the cause was most likely a timeout on the single unresumed pass rather than a memory ceiling; if it died at calibration, box sizing must be driven by that 109 GiB matrix plus optimizer working set, not by the base sim.Box-sizing recommendation for Build E:
--checkpoint-root <persistent volume>, commit the volume after each reform so a preemption resumes at the next reform;--maximum-microsim-batch-size 1000(cheap insurance, negligible cost);target_compilationfits comfortably in ≤32 GiB, but size the box for the calibration stage (≥300 GiB nonpreemptible remains the safe envelope for the 109 GiB dense matrix + solver).Testing
uv run pytest(workspace, afteruv sync --all-packages): 1406 passed, 51 skipped, exit 0.uv run ruff check .: clean.Notes for the reviewer
@-mentions. Does not touch HF upload paths, the calibration solver, or the whole-frame checkpoint identity.Prerequisite 1 for #299. Implements #217.