From d49910c883f182fecbf3e05ad0fcfeb521e9f074 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 3 Oct 2026 09:11:02 -0400 Subject: [PATCH 1/2] Skip, don't fail, the module-release test once core stops leaking CI installs the newest policyengine-core. If a core release fixes the sys.modules leak (policyengine-core#520), the regression test's precondition would fail on every PE-UK PR until someone removed the workaround. It now skips with a note to remove release_dead_variable_modules and the test. Also correct the conftest comment: 15.8 GB was the CI runner's memory in use while the suite ran serially, not one process's size. Both from the review of #2115. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/tests/conftest.py | 8 ++++---- .../tests/test_variable_module_release.py | 17 +++++++++++------ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/policyengine_uk/tests/conftest.py b/policyengine_uk/tests/conftest.py index deee0cc756..aa66a2c098 100644 --- a/policyengine_uk/tests/conftest.py +++ b/policyengine_uk/tests/conftest.py @@ -36,10 +36,10 @@ def pytest_collection_modifyitems(config, items): # a module named "__" and registers it in # sys.modules, where it stays after the system is garbage collected. Every # Simulation builds its own system, so each one leaves about 1,000 modules -# (roughly 10 MB) behind, and the property tests build hundreds: one test -# process grew to 15.7 GB on a 16 GB CI runner. After each test, drop the -# modules of systems that no longer exist; live systems keep theirs. Remove -# this once policyengine-core stops leaking them +# (roughly 10 MB) behind, and the property tests build hundreds: running the +# suite serially used 15.8 GB of a 16 GB CI runner. After each test, drop +# the modules of systems that no longer exist; live systems keep theirs. +# Remove this once policyengine-core stops leaking them # (PolicyEngine/policyengine-core#520). _VARIABLE_MODULE_NAME = re.compile(r"(\d+)_-?\d+_") diff --git a/policyengine_uk/tests/test_variable_module_release.py b/policyengine_uk/tests/test_variable_module_release.py index 61d2047ccd..f3a94e2cfc 100644 --- a/policyengine_uk/tests/test_variable_module_release.py +++ b/policyengine_uk/tests/test_variable_module_release.py @@ -4,14 +4,17 @@ under the system's id and never removes them. The conftest's ``release_dead_variable_modules`` drops those of systems that no longer exist, which keeps a long test process's memory flat. If core stops leaking the -modules (PolicyEngine/policyengine-core#520), the precondition below fails -and the workaround can go. +modules (PolicyEngine/policyengine-core#520), the first test skips with a +note to remove the workaround, rather than failing every PR after a core +upgrade. """ import gc import sys from pathlib import Path +import pytest + from policyengine_uk import Simulation SITUATION = { @@ -41,10 +44,12 @@ def test_dead_systems_modules_are_released_and_live_ones_kept(request): dropped_owner = str(id(dropped.tax_benefit_system)) expected = dropped.calculate("income_tax", 2026) - assert _modules_of(dropped_owner), ( - "policyengine-core no longer registers variable modules under the " - "system's id; the conftest workaround may no longer be needed" - ) + if not _modules_of(dropped_owner): + pytest.skip( + "policyengine-core no longer registers variable modules under the " + "system's id (policyengine-core#520 fixed?); remove " + "release_dead_variable_modules from conftest.py and this test" + ) del dropped gc.collect() From a77930fa47c33b662e6a466fa16711bcc1e03b5d Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 3 Oct 2026 09:19:01 -0400 Subject: [PATCH 2/2] Fail, not skip, if core renames the modules it still leaks From the review of this PR: skipping whenever no module carried the system's id would also skip if core kept leaking but changed the name format, and then the conftest pattern would silently release nothing. The test now looks up the module core loaded a variable from: it skips only if that module is gone from sys.modules, and otherwise asserts the conftest pattern still recognises it. release_dead_variable_modules now counts only modules it actually removed, so the test's `released >= 1` fails when nothing is removed (checked by disabling the removal). The conftest comment now gives the serial run's memory as 99% of the runner's, since free -m reports MiB. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/tests/conftest.py | 6 +++--- .../tests/test_variable_module_release.py | 15 +++++++++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/policyengine_uk/tests/conftest.py b/policyengine_uk/tests/conftest.py index aa66a2c098..56e18f3671 100644 --- a/policyengine_uk/tests/conftest.py +++ b/policyengine_uk/tests/conftest.py @@ -37,7 +37,7 @@ def pytest_collection_modifyitems(config, items): # sys.modules, where it stays after the system is garbage collected. Every # Simulation builds its own system, so each one leaves about 1,000 modules # (roughly 10 MB) behind, and the property tests build hundreds: running the -# suite serially used 15.8 GB of a 16 GB CI runner. After each test, drop +# suite serially used 99% of a 16 GB CI runner's memory. After each test, drop # the modules of systems that no longer exist; live systems keep theirs. # Remove this once policyengine-core stops leaking them # (PolicyEngine/policyengine-core#520). @@ -66,8 +66,8 @@ def release_dead_variable_modules(max_owners: int = 8) -> int: for owner, names in owners.items(): if owner not in live: for name in names: - sys.modules.pop(name, None) - released += len(names) + if sys.modules.pop(name, None) is not None: + released += 1 return released diff --git a/policyengine_uk/tests/test_variable_module_release.py b/policyengine_uk/tests/test_variable_module_release.py index f3a94e2cfc..3d52036c73 100644 --- a/policyengine_uk/tests/test_variable_module_release.py +++ b/policyengine_uk/tests/test_variable_module_release.py @@ -6,7 +6,8 @@ which keeps a long test process's memory flat. If core stops leaking the modules (PolicyEngine/policyengine-core#520), the first test skips with a note to remove the workaround, rather than failing every PR after a core -upgrade. +upgrade. If core keeps leaking but names the modules differently, it fails, +because the release would then silently do nothing. """ import gc @@ -44,12 +45,18 @@ def test_dead_systems_modules_are_released_and_live_ones_kept(request): dropped_owner = str(id(dropped.tax_benefit_system)) expected = dropped.calculate("income_tax", 2026) - if not _modules_of(dropped_owner): + # The module core loaded the income_tax variable from, for this system. + module = type(dropped.tax_benefit_system.variables["income_tax"]).__module__ + if module not in sys.modules: pytest.skip( - "policyengine-core no longer registers variable modules under the " - "system's id (policyengine-core#520 fixed?); remove " + "policyengine-core no longer keeps variable modules in sys.modules " + "(policyengine-core#520 fixed?); remove " "release_dead_variable_modules from conftest.py and this test" ) + # Core still keeps them, so the conftest pattern must still recognise them, + # or the release would silently do nothing. + assert module.startswith(dropped_owner + "_"), module + assert _conftest(request)._VARIABLE_MODULE_NAME.match(module), module del dropped gc.collect()