Repository navigation
Cache ADD and DIVIDE results only where a plain read returns them - #571
Merged
Merged
Conversation
calculate_add and calculate_divide stored their result at the requested period, where every later plain calculate of that variable and period found it. For a STOCK variable that is not its plain value (last month of the year, or the year's value for a month), so a monthly STOCK read 120 instead of 10 after an ADD and a yearly one 1 instead of 12 after a DIVIDE. The same happened over several periods of a variable's own unit (a plain read raises), for day variables, for integer or boolean FLOW results the cache truncates, and over an input stored at that period. Results are now cached only for the case _calculate itself routes to these options (a FLOW variable over a period of another unit), when the result has the variable's dtype and nothing is stored there yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 2, 2026
Draft
Review of 65b1656: refusing to replace any stored value also stopped an explicit ADD or DIVIDE from refreshing an aggregate cached before an input changed (set January to 12, ADD 2012, set January to 24, ADD 2012: a plain read of 2012 gave 12, where master gives 24). Only an input that a plain read finds there, on this branch, an ancestor or default, is now protected; a calculated value is replaced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
added a commit
that referenced
this pull request
Oct 3, 2026
…culate_add With policyengine-core#571, calculate_add no longer caches a sum over an input a plain read finds, so the input is kept and the test's premise (master's calculate_add overwriting it) did not hold. Storing the calculated value with put_in_cache replaces the input with or without #571. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
added a commit
that referenced
this pull request
Oct 3, 2026
…ly for read branches Review round 3: - Holder._set no longer drops a record entry when a calculated value is stored over an input. A calculate_add at a variable's own period stores the unchanged input and lost its record; a clone, which shares its source's record until #561, deleted the source's entries; and ETERNITY variables keep entries under several periods. The case the cleanup was for, calculate_add caching a sum over an input, is what #571 stops. - Master now gives a storage that shares nothing a frozenset for _shared (#578). The drop step deleted from it directly; it now leaves that to InMemoryStorage._stop_sharing_dropped_keys. - The helpers evicted calculate's fast cache for a sub-period stored under any branch name, including one the simulation does not read. They now evict only for branches it reads, as #576's holder-write property requires. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 6, 2026
Keep the cache-eligibility helper for ADD and DIVIDE and mark its accepted results derived, retaining master's input-only carry-over and uprating. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove the append-only input-record guard so explicit FLOW options can refresh derived aggregates after a target input is deleted. Keep visible inputs via Holder.put_in_cache(derived=True), including restored inputs and nearest-ancestor inputs. Preserve existing ordering properties and add focused refresh differential and provenance regressions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 9, 2026
MaxGhenis
marked this pull request as ready for review
October 9, 2026 06:31
Contributor
Author
|
US + core hub merge audit, core#571 at
|
MaxGhenis
added a commit
that referenced
this pull request
Oct 9, 2026
…ger period (#581) * Count only inputs as already set when a set_input helper splits a longer period set_input_divide_by_period and set_input_dispatch_by_period treated any value stored for a sub-period as already set, including values the simulation had calculated (a cached default or formula result). The same annual input then gave different months depending on what was calculated first: a divided input skipped calculated months and shared the rest among the others, or failed as inconsistent after the year had been read; a dispatched input reused a calculated month for every later month. The helpers now read the simulation's record of inputs (_user_input_keys): a recorded sub-period keeps its input (and, for dispatch, passes it on to the later sub-periods, as before), and a calculated one is replaced. After storing, they drop the variable's calculated values over overlapping periods (the sums calculate_add caches, the twelfths calculate_divide caches) under the input's branch and the branches it reads through, record what they store as inputs, and evict calculate's fast cache for those periods. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Evict calculate's fast cache for the periods a set_input helper drops calculate does not put sums or twelfths in its fast cache today, so this is defensive: a value held there for a period whose stored value the helper drops goes with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Restore dumped values as inputs; test nested branches and rolling years A dump does not record which values were inputs, and restore_simulation stored every value with put_in_cache, so a restored simulation had an empty input record and the helpers replaced restored inputs (review finding 1). Restore now stores each value as a recorded input, the rule #576 applies to dumps without an inputs.txt. Tests: a restored month keeps its value under a yearly input (divide and dispatch); an input on a nested branch drops the sum its parent branch calculated; an input stored for twelve months from March is kept. The order-independence property now creates up to two nested branches, with calculations at each level. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep the property test off disk file names Windows rejects On disk, a value calculated for a period like year:2013:2 is stored in a file named after the period, and Windows rejects ":" in file names (policyengine-core#526). The Windows CI jobs failed on such a read in the on-disk examples. In disk mode the property now skips those periods. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record which dumped values were inputs; forget an input a calculation replaces Review round 2: 1. Restoring every dumped value as an input froze calculated values through a later reform. The dumper now writes, next to each variable's arrays, the periods whose value was an input (inputs.txt), and restore records exactly those; a dump without the file restores every value as an input. This is policyengine-core#576's dumper change, taken byte for byte so the two PRs merge without conflict. 2. A value stored by put_in_cache over an input (say a calculate_add sum over an input set for twelve months) left the input's record entry in place, so the helpers and apply_reform kept treating the calculated value as an input. Holder._set now drops the entry, in both of its forms for twelve months from the first of a month, when it stores a value outside set_input. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test a calculated value replacing an input with put_in_cache, not calculate_add With policyengine-core#571, calculate_add no longer caches a sum over an input a plain read finds, so the input is kept and the test's premise (master's calculate_add overwriting it) did not hold. Storing the calculated value with put_in_cache replaces the input with or without #571. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Drop the record cleanup on calculated writes; evict the fast cache only for read branches Review round 3: - Holder._set no longer drops a record entry when a calculated value is stored over an input. A calculate_add at a variable's own period stores the unchanged input and lost its record; a clone, which shares its source's record until #561, deleted the source's entries; and ETERNITY variables keep entries under several periods. The case the cleanup was for, calculate_add caching a sum over an input, is what #571 stops. - Master now gives a storage that shares nothing a frozenset for _shared (#578). The drop step deleted from it directly; it now leaves that to InMemoryStorage._stop_sharing_dropped_keys. - The helpers evicted calculate's fast cache for a sub-period stored under any branch name, including one the simulation does not read. They now evict only for branches it reads, as #576's holder-write property requires. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.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
ADD and DIVIDE previously cached results where a later plain read could return a different value or raise. This change caches only compatible MONTH/FLOW → YEAR and YEAR/FLOW → MONTH results whose dtype matches the variable. Accepted writes use
derived=True, preserve the input visible to a plain read, and allow an existing derived aggregate to refresh after native inputs change. Found while reviewing #562 (soundness review, finding 5).STOCK entries, dtype-changing boolean/integer FLOW entries, and unsupported multi-period option entries no longer enter known-period lists, dumps, or computed-variable exports. Accepted aggregates remain derived and cannot become input-only carry-over or uprating sources. Automatic invalidation of dependent values immediately after
set_inputremains outside this change; refresh tests explicitly rerun the option.Master merge and combined behavior
Head:
77df88302596d0552a9b065a1ab112701b0fa1a8.Merged canonical
masteratb853f4989e973a25160698e28b8b3d57a580c654in merge commit693754220ce5b246676e6cc7ff5c32be095ff8ed. Git reported no textual conflicts. The merge preserves #558's holder storage directories and #561's per-tier supplied-input records, tier-aware cache invalidation/replay, recorded-input exports, and portable compound-period filenames.Ten new combined provenance regressions cover ADD and DIVIDE in memory and disk storage, including deleted target inputs and supplied disk inputs hidden by derived memory values. They verify that accepted option caches remain derived, create no supplied-input records, stay out of input-only dataframe exports, disappear during invalidation, and leave recorded native inputs available for replay. A refreshed memory aggregate can coexist with a recorded disk input; invalidation restores the supplied disk value for a later plain read.
Updated the helper docstring and regression comments to describe the merged input-record behavior, including deletion and dump restoration. Documentation review: no separate API documentation is required for this internal cache repair. Documentation impact: low; confidence: high. The existing towncrier fragment is retained.
Validation
Required focused files: 297 passed, 2 xfailed, no failures across 15 files. Each file ran separately in the foreground with
uv run pytest -q -p no:cacheprovider.The two expected failures cover the documented lifetime limit when a parent removes a storage directory that a forked child still uses.
Core A/B and country microsimulations remain deferred to the hub. Historical validation counts and source reviews are not presented as validation of this head; current-head CI and hub checks remain open.
A documentation build was not run: this merge updates the internal helper's docstring and test comments, while the API reference continues to generate from source.
Impact
Hub A/B microsimulation, PE-US, current head. The hub ran PolicyEngine-US main
0a8c3c2c43on the default dataset twice for each year. One run used Core masterb853f4989e, the merge base; the other used this PR's head77df883025. The runs went one at a time under the shared heavy-job lock.household_net_incomeincome_taxstate_income_taxhousehold_benefitsEvery compared array is identical. Run time was 177s against 173s (2025) and 177s against 185s (2026), with the same peak footprint on both cores (11,264 MB and 12,288 MB). PE-UK isn't microsimulated here. CI's "Test Core and country packages" jobs exercise it, and all 19 checks pass at this head.
Composition and hold
Merge order remains #576 → #561 → #571 → #581. The option-cache helper retains the eligibility and visible-input protection contract needed by #581. #581 was not modified and retains its separate d899 hold and dumper reconciliation with #576.
Max's d899 approved landing this PR third in that order, on the hub gates.
axiom: n/a: core engine caching, no policy encoding