Skip to content

Cache ADD and DIVIDE results only where a plain read returns them - #571

Merged
MaxGhenis merged 6 commits into
masterfrom
fix-stock-option-caches
Oct 9, 2026
Merged

MaxGhenis merged 6 commits into
masterfrom
fix-stock-option-caches

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

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_input remains outside this change; refresh tests explicitly rerun the option.

Master merge and combined behavior

Head: 77df88302596d0552a9b065a1ab112701b0fa1a8.

Merged canonical master at b853f4989e973a25160698e28b8b3d57a580c654 in merge commit 693754220ce5b246676e6cc7ff5c32be095ff8ed. 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.

Coverage Files Passed Xfailed
#571 option regressions and properties, including ten new combined cases 3 36 0
#561 supplied-input and portable-filename files 8 186 0
#558 disk-storage and directory files 4 75 2

The two expected failures cover the documented lifetime limit when a parent removes a storage directory that a forked child still uses.

  • Complete core suite, run once in the foreground on this head: 2,090 passed, 1 skipped, 3 xfailed, no failures, in 1,236.12 seconds (20 minutes 36 seconds).
  • Ruff format and lint passed across 167 files.
  • Runtime validation uses CPython 3.13.15, pytest 9.1.1, and Hypothesis 6.168.3. The interrupted initial option-property attempt is excluded from these counts; the complete rerun passed both tests.

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 0a8c3c2c43 on the default dataset twice for each year. One run used Core master b853f4989e, the merge base; the other used this PR's head 77df883025. The runs went one at a time under the shared heavy-job lock.

Output 2025 change 2026 change Records changed
household_net_income 0 0 0
income_tax 0 0 0
state_income_tax 0 0 0
household_benefits 0 0 0
household and person-household IDs 0 0 0

Every 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

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>
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>
MaxGhenis and others added 2 commits October 7, 2026 16:58
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>
@MaxGhenis
MaxGhenis marked this pull request as ready for review October 9, 2026 06:31
@MaxGhenis
MaxGhenis merged commit 13e5806 into master Oct 9, 2026
19 checks passed
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

US + core hub merge audit, core#571 at 77df88302596d0552a9b065a1ab112701b0fa1a8 (squash): ADD and DIVIDE results are cached only where a plain read returns them.

@MaxGhenis
MaxGhenis deleted the fix-stock-option-caches branch October 9, 2026 07:00
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant