Repository navigation
Conversation
Five test modules cached tax-benefit systems or simulations in module-level lru_caches that were never cleared, so a pytest worker kept them for the rest of its run. The two-child limit file kept one fresh system per parameter value drawn (13 on CI, +0.54 GiB RSS that stayed). - two-child limit and cliff property tests: build reforms over one shared baseline system (a reform clones only the parameters), keep the last two reforms (two-child) or the four mtr_adults values (cliff), and clear the caches in a module-scoped autouse fixture. - test_state_pension_age, test_state_pension_credit_qualifying_age and test_uc_work_related_groups_properties: clear their cached simulations in a module-scoped autouse fixture. - code_health/test_test_module_caches_released.py: every module-level functools cache a test module defines or imports must be cleared after the yield of a module-scoped autouse fixture. Test functions' source is unchanged, so Hypothesis's derandomized examples are the same. Fixes #2357 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The guard only saw a yield written directly in the fixture body, so a fixture that yields inside a with or try block (as reuse_country_model does) was reported as not releasing its caches. Count any cache_clear() after the fixture's first yield, and test decorator forms, that case and imports from other test files. Drop the leading "- " from the changelog fragment: towncrier's default template adds the bullet (#2162). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From the two reviews of #2358 (both approve, no blocking findings): - The guard now also sees async cached functions and `name = lru_cache(...)(function)`, follows aliased and relative imports from other test files, and requires a module to clear a cache it fills through a helper imported from another test file. It ignores a yield or cache_clear inside a function nested in the fixture, and only treats `lru_cache`/`cache` and `functools.*` as caches. Its docstring says what a static scan cannot see. - The two-child comment states why a reform is built only when the threshold changes instead of quoting a count that depends on the Hypothesis version. - The SPC comment says which module the import loads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
CI run cancelled to free runners (UK hub, 2026-10-11 23:30Z). The PolicyEngine org is at GitHub's limit of 60 concurrent jobs, with about 360 queued. Fleet ops asked the hub to let the Budget-critical #2305 take runners first. This PR's queued run had not started, so I cancelled it. Nothing is wrong with the PR. Please don't re-run it or push just to retrigger CI. The hub re-runs this run itself once #2305 has runners. The record is |
From the GPT-6.1 Sol re-review of #2358 at da5a9c8 (approve, nits only): - A cache made by assignment (`name = cache(function)`) is followed into the function it wraps, and `name = lru_cache(maxsize=1)` is not taken for a cache. - A cache imported under several names is released by clearing any of them. - The self-tests cover a relative import from a subdirectory, which only the import's level resolves. Seven mutations of the scan each fail a self-test. - The docstring and the SPC comment say the second module copy arises under pytest's default import mode, and the docstring lists re-exports through a third test file among what the scan does not see. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings the branch up to date and runs the checks main now requires (Test / Result, Lint). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Aggregate impact on certified data: passBase Checks: 0 broken by this change, 1 failing on both sides, 0 fixed. Moves flagged for review: 0 (thresholds in Notes
Moves flagged for reviewNone. Spending by benefit and service
Caseloads
Household net income by decileNo household's net income changes in any year. No change in any year: receipts by tax head; government balance, each component counted once; model totals; household income totals; population; poverty rates; people in poverty; inequality; mean household net income in each side's own deciles; standard reform costings (change in gov_balance); standard reforms: change in each reform's target. Checks failing on both base and head (1)
Counted-once checkEach of 84 components was raised by £1 per unit in 2026-27, and every head of its totals was read household by household: its own head must rise by exactly £1, no other tax head may move, no other benefit may rise, and the model's own total must move by what its heads moved. A head that breaks a rule is measured again twice: with every other head held at its baseline value, and with every other head held at its value after the first £1 while the component rises again. It fails where either still shows the response. 0 failed; 0 were skipped. Provenance
Dataset: Receipts by tax head: head levels
Spending by benefit and service: head levels
Caseloads: head levels
Government balance, each component counted once: head levels
Model totals: head levels
Household income totals: head levels
Population: head levels
Poverty rates: head levels
People in poverty: head levels
Inequality: head levels
Mean household net income in each side's own deciles: head levels
Standard reform costings (change in gov_balance): head levels
Standard reforms: change in each reform's target: head levels
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After the merges of main, test_every_test_module_cache_is_released found module-level functools caches that nothing clears in six test modules that gained them on main since this branch's base (c6d15c8): test_council_tax_uprating.py and test_water_uprating.py (_country_of_each_region), test_current_education_order.py (last_data_year, added after the hub's merge of fe0b99a), test_dated_reform_routes.py (current_law_leaf_values, later_first_dated_parameters, parameters_with_a_closed_span), test_fiscal_partition.py (partition, traced_reads, baseline_values, baseline_entity_values) and test_reform_inputs_survive_reset.py (base, a cached Simulation). Each module now clears them after the yield of a module-scoped autouse fixture, as the other five modules do. The guard is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Backlog F hand-off (UK hub build job, 2026-10-23) Head: Failure: after the main merges, Change: each module gets a module-scoped autouse fixture that calls
The code-health test is unchanged (no allowlist, no weakened scan). No production code changed. Tests at 5ce4095 (local, foreground,
Hypothesis shrinking was off for these runs, with the same examples. Ruff format and check pass. I did not run the full suite locally; CI runs it. The new releases were not memory-measured. Open: CI at this head and the hub's independent review. The PR is still a draft; I have not merged it or marked it ready. |
This branch has not been deployed
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
1 participant
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 #2357
What and why
Five test modules cached tax-benefit systems or simulations in module-level
functools.lru_caches that nothing cleared, so a pytest worker kept them for the rest of its run. Each cached system also keeps the last simulation built on it (Simulation.__init__setstax_benefit_system.simulation = self). On CI run 38165445604, the worker that rantest_two_child_limit_age_exemption_properties.pybuilt 13 systems, went from 1.10 to 1.65 GiB RSS during that file and stayed there.This PR changes test code only. It does not touch
conftest.py, and it branches frommain, independent of #2269, #2317 and #2331.Changes
tax_benefit_system(parameter, value)returnedoverride(CountryTaxBenefitSystem())from an unbounded cache: a new full system plus a reform for every threshold Hypothesis drew. It now builds one baseline system and builds each reform over it.Reform.__init__clones only the baseline's parameter tree and copies its variables dict, so a reform over the shared baseline keeps 20 MiB and takes 0.53 s of CPU, against 54 MiB and 1.69 s for a reform over its own fresh system (tracemalloc, Python 3.13.9, policyengine-core 3.33.2). The reform cache holds the last two reforms. Each example uses the repealed limit and then one threshold, so the repealed limit's reform is never evicted and a reform is built only when the threshold changes.mtr_adultsinst.integers(0, 3)can ask for.cache_clear()after itsyield: the two files above,test_state_pension_age.py(differential_simulation,situation_template),test_state_pension_credit_qualifying_age.py(one_person_units_templateand its own copy ofdifferential_simulation) andtest_uc_work_related_groups_properties.py(qualifying_age_couples).differential_simulationfrompolicyengine_uk.tests.test_state_pension_age. Pytest collects the files in this directory by bare name (no__init__.py), so that import is a second module object with its own cache:sys.modulesholds bothtest_state_pension_ageandpolicyengine_uk.tests.test_state_pension_age, with differentdifferential_simulationobjects. The two files never shared the simulation, and each now clears its own.code_health/test_test_module_caches_released.pyis a static check that every functools cache a test module defines, imports from another test file, or fills through a helper imported from another test file is cleared after theyieldof a module-scoped autouse fixture. It reports all five modules as they are onmain.Measured
Before is
mainat c6d15c8 in a clean worktree; after is this branch. Each pair ran at the same time on the same host (macOS 26.6.2, Apple M5 Max, Python 3.13.9, serial pytest), so both sides saw the same load. The host was heavily loaded, so compare within a pair and prefer CPU time to wall time. A pytest plugin outside the repo recorded, per module, wall and CPU time, RSS (sampled every 20 ms), tracemalloc, the systems and reforms built, and theTaxBenefitSystem,ReformandSimulationobjects alive after the module's teardown and agc.collect().The five files in one process, in collection order
test_cliff_evaluated_properties.pytest_state_pension_age.pytest_state_pension_credit_qualifying_age.pytest_two_child_limit_age_exemption_properties.pytest_uc_work_related_groups_properties.pysystem), 0 reforms and 0 simulations. After this PR that is the count after every one of the five modules.Python heap (tracemalloc), same five files
test_cliff_evaluated_properties.pytest_state_pension_age.pytest_state_pension_credit_qualifying_age.pytest_two_child_limit_age_exemption_properties.pytest_uc_work_related_groups_properties.pyHeap after the last file: 1,660 → 74 MiB.
Each property file alone in a fresh process, three runs
RSS before each file's first test was 255 to 260 MiB in all twelve runs.
Under coverage, as CI measures it
Cliff then two-child limit in one process with
--cov=policyengine_ukandCOVERAGE_CORE=sysmon(coverage 7.16.2 reports theSysMonitorcore with that setting): the two-child limit file took 15.2 → 5.9 s, peaked at 1,453 → 458 MiB RSS and kept +839 → +63 MiB; the cliff file took 6.0 → 4.1 s and kept +325 → +99 MiB.Time
The caches exist for speed, and this PR does not cost any. The two property files are faster because they build 1 system instead of 13 and 4. The other three files only gain a
cache_clear()at module end. Their CPU times moved by +5% (State Pension age), −1% (SPC) and −6% (UC work-related groups) in the paired run; three identical runs of the two-child limit file onmainranged from 10.8 to 12.7 s on this host, so those differences are within run-to-run variation.One cost to know about: CI runs
-n 4 --dist worksteal. If a worker runs a module's tests in two stretches with another module's tests between them, the fixture's teardown runs in between and the next call rebuilds the cache. Results do not change. Before this PR, a module split across workers already built its cache once per worker.Outcomes are unchanged
pytest --collect-only policyengine_uk/testsgives the same 5,111 test IDs before and after, plus the guard's 5 new tests.Simulation.calculateresult. With--hypothesis-seed=0, the 8,390 records from the five files are identical and in the same order before and after.maintoo, because they do not setderandomize=True. The two-child limit, cliff and UC work-related groups files do set it, and their 6,253 records matched without a seed.derandomize=Trueseeds from the test function's own source and signature. All 32 test functions in the five files, 16 of them Hypothesis tests, have byte-identical source tomain.Reviews
None of the three finished reviews completed a pytest run, because their sandboxes were read-only. The Sol re-review ran ruff, the guard's scan and four of its self-tests in memory. The pytest runs in this description are this session's.
Caches main added since
Merging main brought module-level caches into six more test modules. The guard caught every one, and each module now clears them after the
yieldof a module-scoped autouse fixture, as above. The guard itself is unchanged.test_council_tax_uprating.py_country_of_each_regiontest_water_uprating.py_country_of_each_regiontest_current_education_order.pylast_data_yeartest_dated_reform_routes.pycurrent_law_leaf_values,later_first_dated_parameters,parameters_with_a_closed_spantest_fiscal_partition.pypartition,traced_reads,baseline_values,baseline_entity_valuestest_reform_inputs_survive_reset.pybase(a cachedSimulation)These were not measured for memory. Most hold computed results (a region mapping, a year, parameter tables, traced reads, baseline values);
baseholds a simulation and its system.Not in this PR
SYSTEM = CountryTaxBenefitSystem(),_PROPERTY_SYSTEM = system.clone()). Every xdist worker imports every test module, so each worker holds them for the whole session: collecting the suite holds 315 MiB of heap and 8 systems, against 148 MiB and 2 systems with those six files ignored. Queued as a separate task.test_uc_work_related_groups_properties.pybuilds a new system for everySimulation(145 here), which is Follow-ups from #2160: qualifying-age docstring, UC work-related groups property-file speed, changelog fragment example #2162 item 2. Queued as a separate task.Test plan
ruff formatandruff checkon the six touched files (run on those files rather thanmake format, to leave other files alone).changelog.d/2357.fixed.md.main, and none on this branch.-n 4, Hypothesis shrinking off, same examples).axiom: n/a: test infrastructure
🤖 Generated with Claude Code
Reproducer:
policyengine_uk/tests/code_health/test_test_module_caches_released.py::test_every_test_module_cache_is_released,policyengine_uk/tests/code_health/test_test_module_caches_released.py::test_the_scan_finds_caches_and_releases,policyengine_uk/tests/code_health/test_test_module_caches_released.py::test_the_scan_follows_functions_to_the_caches_they_fill,policyengine_uk/tests/code_health/test_test_module_caches_released.py::test_the_scan_follows_imports_from_other_test_files,policyengine_uk/tests/code_health/test_test_module_caches_released.py::test_the_scan_resolves_this_directorys_imports