Skip to content

Use public Core APIs for UK parameter caches - #2495

Open
juaristi22 wants to merge 2 commits into
mainfrom
fix-2175-parameter-cache-apis
Open

juaristi22 wants to merge 2 commits into
mainfrom
fix-2175-parameter-cache-apis

Conversation

@juaristi22

@juaristi22 juaristi22 commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Refs #2175. This implements the parameter-cache part; the issue remains open.

UK parameter initialization and processing currently access Core's private cache fields. Delegate initialization, parameter replacement and dated-node invalidation to public Core operations. Keep the UK reset_parameter_caches entry point, live root and leaf identities, current Reform edits, baseline separation and failed-rebuild rollback. Policy values, formulas, inputs and dependencies remain unchanged.

The supplied-input migration still needs successful-write/provenance support and the separate PolicyEngine/policyengine-core#605 rebuild follow-up. #2265 and #2308 retain their supplied-input/tier scope. Selected parameter-operation source matches minimum Core 3.32.27, locked 3.33.2 and published 3.33.4, so this slice needs no dependency bump.

The eight cache lifecycle tests are behavior-preservation controls. This structural refactor claims no policy behavior change.

Validation: 72 qualified implementation cases, 74 qualified independent-review cases and 109 qualified cases in a separate combined #2490/#2485 tree. These overlap and include explicitly qualified unchanged cases; final runs freshly exercised the eight corrected cache cases in each context. The combined run exposed six minimal-fixture failures, corrected by retaining cloned UK/NI current and baseline trees while preserving every test expectation and setting. Independent current-head CODE/POLICY review completed with no findings or material gaps. Formatting, lint, source checks and the changed Markdown page's HTML build without execution passed. All 807 parameter YAML files remain unchanged.

Local runs used Python 3.13.14/Core 3.32.27/microdf 1.2.1. Full locked-environment, deployed API, private-data and population validation were outside these checks. The ownership gate's initial failure and all setup, fixture and introduced-dispatch failures remain in the evidence; this is a structural refactor with numerical preservation controls.

Preserve #2490's canonical NI traversal and #2485's construction/clone behavior when coordinating landing; the separate combined tree includes an explicit shared reset-hunk resolution. Draft #2092 also changes the processing file; its projection policy remains separate and unvalidated here.

Axiom: not applicable. This structural cache change preserves policy rules and values; no encoding or parity result is claimed.

@vahid-ahmadi vahid-ahmadi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine once the Reproducer line is corrected.

@juaristi22, the refactor matches Core 3.33.2. TaxBenefitSystem.__init__ copies the entities and links them as the removed code did. parameters_dir is set only after super().__init__, so Core does not load the parameter tree a second time. Core's modify_parameters replaces the tree and clears the system-level instant cache. In Core 3.33.2, only ParameterNode holds _at_instant_cache, so clearing nodes covers everything the old leaf loop touched. Import and construction time were no slower than main on my machine.

  1. should: the PR body names test_reset_refreshes_system_and_every_descendant_without_replacing_tree and test_reset_keeps_an_actual_reforms_current_edits_and_baseline_separate as reproducers. With policyengine_uk/tax_benefit_system.py reverted to main, all 8 tests in test_parameter_cache_apis.py still pass. So they are preservation guards, not reproducers. The changed fragment means check_fix_pr.py does not enforce this, but the body should not claim a reproducer. Either drop the line and say that no behaviour changes, or add a test that fails on main. For example, assert that production code no longer writes _parameters_at_instant_cache directly, which is #2175's first acceptance criterion.
  2. nit: tax_benefit_system.py:85-89. _install_parameters calls TaxBenefitSystem.modify_parameters unbound, to step past a Reform's delegation. That is fine, and the comment explains it. Note that Core's modify_parameters returns, rather than raises, a ValueError for a non-node. A wrong type would therefore pass silently here. Every caller passes a ParameterNode today.
  3. nit: reset_parameter_caches calls clear_parent_cache() on every node, and each call walks up to the root. That is O(nodes × depth). I measured about 2 ms, so this is not a problem now.

What I checked

  • Read the full diff, issue #2175, and Core 3.33.2's TaxBenefitSystem.__init__, modify_parameters and ParameterNode.clear_parent_cache.
  • pytest test_parameter_cache_apis.py: 8 passed on this head, and 8 passed with tax_benefit_system.py reverted to main.
  • pytest on the reform and parameter-processing tests (test_dated_reform_routes, test_from_reform_modifier, test_process_time_reforms, test_reform_inputs_survive_reset, test_reform_representations, test_fiscal_year_parameters, test_scenario_sum_timing): 357 passed on this head.
  • Timed import, construction and reset_parameter_caches() on main and on this head.
  • ruff check and ruff format --check on the changed Python files: clean.
  • Hosted CI on this head: all green.
  • Not checked: the minimum Core 3.32.27, since my environment has the locked 3.33.2; combined trees with #2490, #2485 or #2092.

🤖 Generated with Claude Code

@juaristi22

Copy link
Copy Markdown
Collaborator Author

Removed the Reproducer: claim and identified the eight cache lifecycle tests as behavior-preservation controls. This structural refactor claims no policy behavior change. Code, tests and the remaining validation and integration qualifications are unchanged.

@vahid-ahmadi vahid-ahmadi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The Reproducer point is fixed.

@juaristi22, thanks. The head is still a1320834c, so only the PR body changed since my last review.

Earlier points

  1. should (Reproducer claim): Fixed. The Reproducer: line has gone. The body now calls the eight cache tests behaviour-preservation controls, which matches what I found: they pass with tax_benefit_system.py reverted to main. I ran .github/check_fix_pr.py from this head on the current body. It passes and classes the PR as not a fix PR (a changed fragment and no bug label), so the reproducer rules do not apply. The hosted Fix evidence check re-ran when you edited the body (11:38 UTC) and passed.
  2. nit (unbound modify_parameters and the returned ValueError): not changed. That is fine, as every caller passes a ParameterNode.
  3. nit (clear_parent_cache walk cost): not changed. That is fine, at about 2 ms.

New findings
4. nit: this PR merges cleanly with main and with #2485. It has a text conflict in policyengine_uk/tax_benefit_system.py with #2490, and with draft #2092. git merge-tree shows this. The body already notes the shared reset hunk with #2490, so whichever PR lands second will need to rebase that hunk.

What I checked

  • The current PR body, and check_fix_pr.py run on it with this PR's file list.
  • Hosted CI on a1320834c: all green.
  • Merge with current main, #2485, #2490 and #2092.
  • Not re-run: the code has not changed, so last round's results still apply (8 cache tests, 357 reform and parameter tests, timing, ruff).

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
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.

2 participants