Skip to content

Recalculate after each modifier step on the built UK simulation - #718

Merged
juaristi22 merged 1 commit into
mainfrom
clear-uk-modifier-caches-717
Oct 25, 2026
Merged

juaristi22 merged 1 commit into
mainfrom
clear-uk-modifier-caches-717

Conversation

@juaristi22

Copy link
Copy Markdown
Contributor

Fixes #717

The UK runner applies three steps to the built policyengine_uk.Microsimulation: the policy's simulation modifier, the dynamic's parameter values (written after a policy modifier, as #575 kept), and the dynamic's simulation modifier. policyengine-uk keeps every value it calculates, so a value calculated before one of these steps kept its old value. Each step now ends with microsim.drop_computed_arrays(). That keeps every input, including values a modifier stores with set_input, and deletes everything else, which is calculated again when next read. It runs in finally, so a failed step drops too before the error ends the run.

The order (policy values, policy modifier, late dynamic values, dynamic modifier) and #575's date handling are unchanged. A run without a simulation modifier reaches none of the three steps, so it is unchanged. drop_computed_arrays is in policyengine-core from 3.33.0; the bundle pins 3.33.3.

What changes (one synthetic parent and child, 2027)

  • Dynamic values written after a policy modifier. The policy modifier calculates Child Benefit and net income; the dynamic adds £1,000 a year to Child Benefit. On main, the dynamic modifier and the output still read £1,438.95, not £2,438.95. Fixed on policyengine-uk 2.132.6 (pinned) and on the policyengine-uk#2673 head.
  • A policy modifier's own set_input, on the pinned policyengine-uk. A modifier that calculates household net income and then stores Child Benefit £1,000 higher left net income £1,000 short in the output. policyengine-uk#2673 fixes this inside policyengine-uk; this change also fixes it on 2.132.6.
  • Labour supply responses to elasticities a dynamic writes after a policy modifier. On the #2673 head, the unchanged wrapper keeps a response of £0 where the same run without the modifier gives −£28.23 a year (income effect only). With this change the two match. On 2.132.6 the response is £0 even without a modifier, a limitation inside policyengine-uk that this change does not touch.

Split with policyengine-uk#2673

With #2673 installed and the wrapper unchanged, both late-dynamic-value cases and the failure case still fail; only the set_input case passes. policyengine-uk does not watch the parameter tree (#2673 leaves that to the caller), so both changes are needed. Production use of the labour supply part also needs a policyengine-uk release with #2673 and a pin bump here; this PR does not change any pin.

Checks

  • New tests/test_uk_modifier_cache.py (the real model and one synthetic family, no mocks):

    • dynamic values reach a Child Benefit the policy modifier calculated, with and without a dynamic modifier reading it;
    • a value the policy modifier stores with set_input stays through every step, while net income it calculated before is refreshed;
    • a failed policy modifier stops the run, no later step runs, and the simulation keeps its inputs but none of the values it calculated.

    All four fail on main and pass with this change, on 2.132.6 and on the #2673 head.

  • test_population_route_applies_policy_before_dynamic: the recording stub now records each drop, so its eight policy/dynamic combinations pin where the drops happen.

  • No-op check: with modifiers that calculate a dozen outputs but change nothing, plus late dynamic values that change nothing, every output column for 25 people in 12 synthetic households is bitwise identical to a run without modifiers (75 columns on 2.132.6, 73 on the #2673 head).

  • Touched-surface suite (dated reforms, the new tests, budget reform contract, CGT responses, labour supply response, year files, extra variables, parametric and dict reforms, run records, execution receipts): 1,168 passed, 7 skipped (execution-receipt cases that need a token or another Python version), none failed.

  • Cost: none without a modifier. With one, what it calculated is calculated again. On a synthetic file of 50,000 people, a policy modifier that calculates net income before late dynamic values took about 2.4 s instead of 2.0 s; a modifier that reads Child Benefit and stores a new amount showed no difference.

  • ruff 0.17.2 format and check are clean; mypy reports nothing new in model.py.

Downstream

  • Only PolicyEngineUKLatest.run applies modifiers to a built simulation. The US route merges everything into the construction reform, and managed_microsimulation and calculate_household run no wrapper modifiers.
  • Runs without a simulation modifier reach none of the three steps and are unchanged. That covers the simulation API's economy runs and the other wrapper users that pass parameter values only.
  • Repos that pass modifiers to policyengine-uk's own Scenario don't go through this code, including uk-autumn-budget-dashboard-2026, which runs them through managed_microsimulation.
  • Three repos run modifiers through the wrapper's Policy or Dynamic: uk-cgt-reform (pins 6.10.1), uk-cgt-paper (4.22.3) and macro (below 5). They get this change only when they upgrade. Their modifiers edit parameters, swap formulas, store inputs with set_input, or register sim.baseline as a branch. All of that survives the drops, and none of them calculates values before a later step.
  • Saved outputs: ensure() reuses an output saved under the same id, and this change does not mark outputs saved before it as stale. Only a run whose modifier calculated values before a later step changed them gives a different result now; such outputs should be run again.

🤖 Generated with Claude Code

The UK runner applies the policy modifier, the dynamic's late parameter
values and the dynamic modifier to the built simulation. policyengine-uk
keeps every value it calculates, so a value calculated before one of these
steps kept its old value: late dynamic values missed values a policy
modifier had calculated, and values calculated before a modifier's
set_input did not include it. Each step now ends with
drop_computed_arrays(), which keeps every input (including those a
modifier stores with set_input) and drops the rest, in finally so a
failed step drops too before the error ends the run.

Fixes #717

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vahid-ahmadi

Copy link
Copy Markdown
Contributor

Review at 76312e48. Verdict: no must-fix findings. The fix is the right width and does what the body says. CI is green, so it is ready to leave draft once finding 1 is fixed or consciously accepted.

I ran everything below in a fresh clone and two fresh venvs: the pinned bundle (policyengine-uk 2.132.6, core 3.33.3) and the same with policyengine-uk at #2673's head 422885e9 (2.158.3, core 3.33.3). Our probes go through Simulation.run, so they test the real route at model.py:251-261.

Answers to what we raised

  • Where drop_computed_arrays comes from. It is a policyengine-core method, added in 3.33.0 (core CHANGELOG, 10 Oct 2026). It is not in #2673, and #2673 does not change it. So this PR does not depend on #2673 and there is no merge order between them. Both are needed: with #2673 installed and model.py at main, a policy modifier that reads marginal_tax_rate, followed by a late basic rate of 30%, still outputs a rate of 0.676 against 0.721 in a run built cold. With this PR it is 0.721, on both policyengine-uk versions.
  • Inputs kept, everything else recalculated. Through all three steps, these values set by a policy modifier after build stay: input-only variables (would_claim_tfc False, childcare_expenses £5,000); a formula variable with a set_input_dispatch_by_period handler (maintenance_loan_in_england_system, True to False); and a yearly set_input on a MONTH variable (ni_class_1_employee, £1,200 a year, which reads £100 a month before and after the drops). Core's docstring says values a custom set_input handler calculates are not inputs, so I checked that case specifically; it holds for these two. Household net income matches the cold run in every probe.
  • Late elasticities after a modifier (our #2667 point 5). Fixed when #2673 is also installed. Take a policy modifier that stores Child Benefit £1,000 higher and then reads household net income, followed by late elasticities. At main the response is £0 on both versions. At this head with #2673 it is −£28.23, the same as a Child Benefit parameter rise with the elasticities in the construction reform. A modifier that reads, sets and stops already gives −£28.23 at main with #2673, because #2673 handles values set with set_input itself. So this PR matters for values read after the last set_input, as the body says. On 2.132.6 the reference itself is £0, a policyengine-uk limitation and not this PR's.
  • finally. A failing step drops what it calculated, the error reaches the caller and no later step runs (the new failure test covers this; it passes). See nit 3.
  • Cost. On Microcosm 2024-25 (182,932 people, 2.132.6), one drop takes 8 ms for 861 arrays. A drop with nothing calculated since the last one takes 6 ms. Calculating six outputs again after a drop (household net income, income tax, UC, Child Benefit, Pension Credit, household benefits) takes 2.9 s, against 3.0 s cold. So the cost is calculating again what the modifiers calculated: at worst one extra output pass per modifier that calculates widely, and nothing for a run without a modifier. A narrower drop would need core to know which variables read which parameters, and it does not. The drop itself costs almost nothing, so I would not try. The drop also turns off macro_cache_read, but that is already False by default and neither the wrapper nor policyengine-uk sets it, so nothing is lost today.
  • Route UK dated reforms through policyengine-uk's statutory pipeline #575 order. Unchanged. The diff only wraps the three calls. The reform dict and late-values logic (model.py:220-246) are untouched, and the eight ordering cases now pin where each drop happens.
  • US. Unaffected. us/model.py:226-231 builds its simulation with everything in the construction reform and calls no wrapper modifiers. The diff touches only uk/model.py.

Findings

  1. should: say clearly when core is too old. Core is pinned ==3.33.3 only in the uk, models and dev extras; the base dependencies do not list it. policyengine-uk only asks for policyengine-core>=3.32.27 (2.132.6 and 2.158.3 both). The model-version check at common/model_version.py:137-147 warns about a policyengine-uk mismatch but does not check core. So an install without the extras, on core 3.32.x, imports fine and fails with AttributeError inside _apply_step only on a run with a modifier, after the modifier has run. A hasattr check that names core 3.33.0, or a core floor where the UK model is set up, would make that a clear error.
  2. should: track the labour supply part. The body says that part needs a policyengine-uk release with #2673 and a pin bump here. None of the four new tests covers it, because on 2.132.6 the response is £0 whatever the wrapper does. An issue for "pin bump + labour supply test (late elasticities after a policy modifier match the cold run)" would stop it being lost after #2673 is released.
  3. nit: finally can hide the step's error. If drop_computed_arrays itself raised, its error would replace the step's (Python keeps the first only as context). That is unlikely. On failure the run throws the simulation away anyway, so the drop matters only to a modifier that kept a reference to it. Fine as it is; one line in the docstring would explain why it is there.
  4. nit: the changelog fragment is long (about 190 words). The first two sentences and the #2673 note would do.

What I ran

  • New tests plus tests/test_uk_dated_reforms.py at this head: 21 passed on 2.132.6 and 21 passed with #2673. With model.py at main: 11 failed (the four new tests and the seven ordering cases with a modifier), 10 passed.
  • Full tests/ at this head on 2.132.6: 4,272 passed, 15 skipped, none failed.
  • CI at 76312e48: all 12 checks green (Lint, Mypy, changelog, release inputs, smoke imports and Test on 3.11 to 3.14).
  • Not read: #2673's own code in detail (reviewed separately), or the slower timings on the #2673 build, which I did not look into and do not put down to this PR.

🤖 Generated with Claude Code

@juaristi22

Copy link
Copy Markdown
Contributor Author

Thanks, Vahid. No code changes from this round; replies by number.

  1. Core older than 3.33.0: accepted as is. It needs an install outside the pinned extras that already has a core older than 3.33.0; a fresh install that resolves policyengine-uk's >=3.32.27 picks a current core. The failure is loud: the run raises an AttributeError naming drop_computed_arrays before any output exists, so it cannot give wrong numbers. The certified UK data also claims core 3.33.3. A check of the installed core against the bundle pin would belong in the shared model-version set-up for both countries, so it is not part of this PR.
  2. Labour supply part: tracked in Test the UK labour supply response to late elasticities once the pin includes policyengine-uk#2673 #719. It records the test to add when the UK pin moves to a policyengine-uk release with Preserve modifier inputs across direct callbacks and measurements policyengine-uk#2673 (your probe: a modifier that stores Child Benefit £1,000 higher and then reads net income, followed by late elasticities, must match the construction-reform run), and that the test must fail with model.py from before this PR. This PR closes Clear cached UK calculations between Policy and Dynamic modifiers #717, so the new issue keeps it from being lost.
  3. finally: kept, without a docstring line. It is there so that a modifier that kept a reference to the simulation never reads values from before the failed step, and so that every step ends the same way. If the drop itself raised, the step's error would still show in the traceback as the exception being handled.
  4. Changelog length: kept. It is the record people upgrading will read of what changes for runs with a modifier, including the labour supply caveat on 2.132.6.

Thanks too for the checks beyond ours: the input-only variables, the dispatch handler and the yearly input on a MONTH variable cover the case core's docstring leaves open.

🤖 Generated with Claude Code

@juaristi22
juaristi22 merged commit ba50a5f into main Oct 25, 2026
12 checks passed
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.

Clear cached UK calculations between Policy and Dynamic modifiers

2 participants