Repository navigation
Require policyengine-core 3.32.12, whose branches share cached arrays - #2054
Merged
Merged
Conversation
policyengine-core 3.32.12 (PolicyEngine/policyengine-core#556) makes Simulation.get_branch share the simulation's cached arrays with the new branch and copy each one only on first read, instead of deep-copying every array up front. Marginal tax rates, labour supply responses and the capital gains marginal tax rate all branch, so they stop paying for copies their branches never read. Raise the floor from 3.32.9 and relock (uv.lock moves core 3.32.9 -> 3.32.12; its entry for this package catches up from 2.102.3 to 2.104.7). The one behavioural difference core documents is a write in place into a cached array after branching, which a branch that has not read that array yet would now see. Extend the run-time half of the cached-array guard test to run the branching code paths (marginal tax rates, labour supply responses, capital gains realisation response) with every stored array read-only, so such a write fails the suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of #2054 found that freezing arrays only in InMemoryStorage.put misses arrays that reach a storage without it: the copy a branch makes on its first read of a shared array (core 3.32.12) and the copies Simulation.clone() makes. A write into one of those after a nested branch exists (the labour supply measurement under the "baseline" branch) passed the run-time guard on both core versions; with the fixture also freezing what InMemoryStorage.get returns it fails, and the unmutated file still passes on both. Shorten the changelog fragment to one sentence, as docs/engineering/skills/github-prs.md asks. Fixes #2109 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's #2115 relocked uv.lock; take it and move only policyengine-core, from 3.32.9 to 3.32.13 (core#578: a storage holds a shared-key set only while it shares an array, removing the ~1.3 MB per simulation that 3.32.12 added). The floor stays >=3.32.12. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Merged at reviewed head 0d279df (merge commit, Independent reviews (Subfleet, Codex lanes):
|
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.
Fixes #2109
What
policyengine-core>=3.32.9(set in Require policyengine-core 3.32.9 and stop exporting HF_TOKEN in CI #1898) to>=3.32.12, and locks 3.32.13. Theuv.lockdiff against main is the core version and hashes, the specifier, and the lock's ownpolicyengine-ukentry caught up with pyproject's version. uv also wanted to rewrite two equivalentcffimarkers underargon2-cffi-bindings; they are left as main has them, anduv lock --lockedpasses.policyengine_uk/tests/code_health/test_cached_arrays_not_written_in_place.py, from Copy cached arrays before writing in scenario modifiers #1988). They run the marginal tax rates (earnings and capital gains), the labour supply responses and the capital gains realisation response with every cached array made read-only. The fixture now freezes arrays as storage returns them as well as when it stores them, so a branch's first-read copies and the copiesSimulation.clone()makes are covered too.Why
policyengine-core 3.32.12 (released 2026-10-02) includes PolicyEngine/policyengine-core#556. Before it,
Simulation.get_branchdeep-copied every cached array into the new branch. Now the branch starts with read-only views of the simulation's arrays and copies each one only the first time it reads it.Simulation.clone()still copies everything. 3.32.13 (2026-10-03, PolicyEngine/policyengine-core#578) keeps that behaviour and stops giving every storage its own empty set of shared keys, which 3.32.12 had added (about 1.3 MB per simulation, per core's changelog). The lock takes 3.32.13; the floor is 3.32.12, the first release with shared branch arrays. This package branches simulations in:marginal_tax_rateandmarginal_tax_rate_wrt_employer_cost;marginal_tax_rate_on_capital_gainsand the capital gains realisation response;tax_credits_applicable_income.The floor rises, rather than only the lock, so that pip installs, which ignore
uv.lock, also get this. #2034's verification measured a labour-supply reform on the Enhanced FRS (2024, maximum resident set size under/usr/bin/time -l):The 3.32.10 and 3.32.11 releases in between allow pytest 9 and warn when a restricted Hugging Face download has no
HUGGING_FACE_TOKEN. Thepolicyenginepackage (6.2.1) pins exact versions of both core and this package in its extras, so the new floor does not change what it installs.Results unchanged
Core documents one behavioural difference. Code that writes in place into a simulation's cached array after branching (
x[mask] = 0,x += 1) now also changes what a branch reads, if the branch has not read that array yet. #1988 removed the writes of this kind that its static scan finds, and runs that scan in CI. The new run-time cases add a check on the branching paths themselves. Each of four in-place writes injected after aget_branchcall (marginal rate, capital gains marginal rate, labour supply, capital gains response) fails its case withValueError: output array is read-only. A fifth sits in the labour supplybaselinebranch, after its nested measurement branch exists. It writes into an array that branch got by a first-read copy (3.32.12) or a deep copy (3.32.9). The extended fixture catches it on both versions; the earlier freeze-on-store fixture missed it on both.Each comparison below is two real
Microsimulationruns on the same commit, in two environments that differ only in policyengine-core (3.32.9 and 3.32.13; same numpy 2.1.3, pandas 2.3.1, Python 3.13.9). They used a private copy of the Enhanced FRS 2024-25, whose hash is unchanged by every run. Every array kept (household, benefit unit and person level) must have the same dtype, shape and bytes, and every weighted total must be exactly equal. Arrays include:household_net_income,gov_spending,household_benefits,household_tax, HBAI income;HOUSEHOLD_BENEFIT_VARIABLES;marginal_tax_rateadult_1_pay_rise,adult_2_pay_risemarginal_tax_rate_wrt_employer_cost,marginal_tax_rate_on_capital_gainsadult_1_employer_cost_mtr,adult_2_employer_cost_mtrbaseline,lsr_measurementoriginally_split_income,split_incomeBaseline totals, identical on both versions (£bn):
household_net_incomegov_spendinghousehold_benefitshousehold_taxuniversal_creditstate_pensionpension_credithousing_benefitchild_benefitpipA CRC audit on 3.32.13 of the arrays stored through
InMemoryStorage.put, re-checked before each new branch was created and at exit, detected no change among the registered arrays still alive at those checkpoints: 12,707 arrays stored across 2 branches in themarginal_tax_raterun and 29,149 across 7 branches in the labour-supply run, 0 changed.The labour-supply run creates and reads its branches, but on main its responses are exactly zero in every year, on both versions: the baseline side of the measurement is a branch of the reform simulation (
simulation.get_branch("baseline")), which carries the reform's parameters, the pattern #1803 fixed for capital gains. That is a separate fix. The capital gains realisation response run is a branching path whose output does depend on its branches: −£0.87bn in 2025 to −£1.10bn in 2030, identical on both versions.Memory
Peak memory under
/usr/bin/time -lon the same Enhanced FRS copy, one process at a time. Max RSS is reported alongside peak memory footprint, because under memory pressure macOS compresses a process's older pages and max RSS under-reports. GB = 10^9 bytes.marginal_tax_ratemarginal_tax_rateThree repeats per 2024 row (
a / b / c), one run per 2025-2030 row; macOS memory pressure was normal (level 1) before and after every run. Both environments have every package byte-compiled, and instructions retired match within 2%, so both sides do the same work. (A first pass had only one environment compiled; the uncompiled side spent about 15% more instructions and 0.2 GB more on imports alone, so those numbers were discarded.) Wall time is not compared: the host is shared with other work.Tests
uv run --frozen --extra dev policyengine-core test policyengine_uk/tests/policy -c policyengine_uk: 1,381 passed with core 3.32.12 (head f383f6e), and 1,381 passed on main at the time (908da84, core 3.32.9).uv run --frozen --extra dev pytest policyengine_uk/tests/withHUGGING_FACE_TOKENset, so the microsimulation tests ran: 470 passed, 1 skipped (policyengine_bundlesis not installed), 2 xfailed (head f383f6e, core 3.32.12).uv run --frozen) and 35 passed with core 3.32.9, so on these paths core itself does not write into the arrays storage returns.get_branchinmarginal_tax_rate, inmarginal_tax_rate_on_capital_gains, in the labour supply response, and between the two capital gains measurements each fails the matching case (ValueError: output array is read-only; head f383f6e, core 3.32.12). The nested-branch write described above is missed by the freeze-on-store fixture and caught by the extended one, with core 3.32.13 and with 3.32.9.Invariants
axiom: n/a: dependency floor and tests only, no policy change.
🤖 Generated with Claude Code