Repository navigation
Use public Core APIs for UK parameter caches - #2495
juaristi22 wants to merge 2 commits into
Conversation
vahid-ahmadi
left a comment
There was a problem hiding this comment.
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.
- should: the PR body names
test_reset_refreshes_system_and_every_descendant_without_replacing_treeandtest_reset_keeps_an_actual_reforms_current_edits_and_baseline_separateas reproducers. Withpolicyengine_uk/tax_benefit_system.pyreverted to main, all 8 tests intest_parameter_cache_apis.pystill pass. So they are preservation guards, not reproducers. Thechangedfragment meanscheck_fix_pr.pydoes 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_cachedirectly, which is #2175's first acceptance criterion. - nit:
tax_benefit_system.py:85-89._install_parameterscallsTaxBenefitSystem.modify_parametersunbound, to step past a Reform's delegation. That is fine, and the comment explains it. Note that Core'smodify_parametersreturns, rather than raises, aValueErrorfor a non-node. A wrong type would therefore pass silently here. Every caller passes aParameterNodetoday. - nit:
reset_parameter_cachescallsclear_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_parametersandParameterNode.clear_parent_cache. pytest test_parameter_cache_apis.py: 8 passed on this head, and 8 passed withtax_benefit_system.pyreverted to main.pyteston 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 checkandruff format --checkon 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
|
Removed the |
vahid-ahmadi
left a comment
There was a problem hiding this comment.
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
- 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 withtax_benefit_system.pyreverted to main. I ran.github/check_fix_pr.pyfrom this head on the current body. It passes and classes the PR as not a fix PR (achangedfragment 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. - nit (unbound
modify_parametersand the returnedValueError): not changed. That is fine, as every caller passes aParameterNode. - nit (
clear_parent_cachewalk 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.pyrun 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
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_cachesentry 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.