From 9d9c71197a19d1c58d763076c3ead214509266af Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 17 Apr 2026 20:03:07 -0400 Subject: [PATCH 1/3] Add downstream preflight smoke tests against published country models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: PR #474 landed the fix for a `_fast_cache` AttributeError that only surfaced in `policyengine-uk` / `policyengine-uk-data` because core's Microsimulation __init__ behavior changed subtly. Similarly, the breakdown-validator tightening in #449 silently broke `policyengine-us`'s `additional_tax_bracket` param until downstream pypkg integration tests tripped on it. The existing `SmokeTestForMultipleVersions` job already installs policyengine-us from PyPI, but the test it runs (`tests/smoke/test_us.py`) is gated by `RUN_SMOKE_TESTS=1` while CI sets `RUN_SMOKE_TESTS: "0"`, so it's always skipped. Add `tests/smoke/test_country_init.py` that simply instantiates `CountryTaxBenefitSystem()` for policyengine-us and policyengine-uk. No microdata required, no HF token required, runs in seconds. This catches any future core change that breaks country-model init — breakdown validator errors, missing private attrs, etc. — before it hits downstream. Also extends the workflow to install policyengine-uk alongside policyengine-us so both tests have something to exercise. Leaves the existing `test_us.py` / `RUN_SMOKE_TESTS` env-var gating alone — those tests need microdata credentials and are a separate decision. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/pr.yaml | 3 ++ changelog.d/add-downstream-preflight.added.md | 1 + tests/smoke/test_country_init.py | 53 +++++++++++++++++++ uv.lock | 2 +- 4 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 changelog.d/add-downstream-preflight.added.md create mode 100644 tests/smoke/test_country_init.py diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index c25e6347..5871b591 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -93,6 +93,9 @@ jobs: uv pip install --system policyengine-us fi shell: bash + - name: Install -uk package from PyPI + run: uv pip install --system policyengine-uk + shell: bash - name: Run smoke tests only run: python -m pytest -m smoke --reruns 2 --reruns-delay 5 -v -s env: diff --git a/changelog.d/add-downstream-preflight.added.md b/changelog.d/add-downstream-preflight.added.md new file mode 100644 index 00000000..73ee503a --- /dev/null +++ b/changelog.d/add-downstream-preflight.added.md @@ -0,0 +1 @@ +Add a downstream preflight smoke test (`tests/smoke/test_country_init.py`) that instantiates `CountryTaxBenefitSystem()` from the currently-published `policyengine-us` and `policyengine-uk` against the core under test. Catches the class of cross-repo regressions that the existing `RUN_SMOKE_TESTS`-gated microsimulation smoke test misses — breakdown-validator errors at parameter load, `_fast_cache`-init assumptions, and any other init-time changes that happen to break downstream country models. Also extends the smoke job in `.github/workflows/pr.yaml` to install `policyengine-uk` alongside `policyengine-us`. diff --git a/tests/smoke/test_country_init.py b/tests/smoke/test_country_init.py new file mode 100644 index 00000000..85123f1e --- /dev/null +++ b/tests/smoke/test_country_init.py @@ -0,0 +1,53 @@ +"""Preflight: instantiating the country tax-benefit systems must not +raise under the current `policyengine-core` under test. + +This catches a class of cross-repo regressions that full microsim +smoke tests don't: breakdown-validator errors at parameter load, +``_fast_cache``-attribute assumptions, and any other +system-initialisation changes core makes that happen to break +downstream country models. + +Unlike ``tests/smoke/test_us.py`` (which is gated by +``RUN_SMOKE_TESTS=1`` and needs real microdata credentials), these +tests only exercise ``CountryTaxBenefitSystem()`` — they don't need +a dataset, don't need HF tokens, and complete in seconds. They run +on every PR against the policyengine-core master branch via +``SmokeTestForMultipleVersions``. + +Each test is a soft-fail via ``skip`` if the corresponding country +package isn't installed — the workflow installs them explicitly, so +in CI this should always find the package. The skip just keeps +local test runs frictionless if you happen not to have both +policyengine-us and policyengine-uk on your machine. +""" + +from __future__ import annotations + +import importlib.util + +import pytest + + +@pytest.mark.smoke +def test_policyengine_us_tax_benefit_system_instantiates(): + if importlib.util.find_spec("policyengine_us") is None: + pytest.skip("policyengine-us is not installed") + # Importing this triggers the country package to load all + # parameters and all Variables, which is where breakdown-validator + # errors / _fast_cache-init bugs / etc. surface. + from policyengine_us import CountryTaxBenefitSystem + + system = CountryTaxBenefitSystem() + assert len(system.variables) > 0 + assert len(list(system.parameters.get_descendants())) > 0 + + +@pytest.mark.smoke +def test_policyengine_uk_tax_benefit_system_instantiates(): + if importlib.util.find_spec("policyengine_uk") is None: + pytest.skip("policyengine-uk is not installed") + from policyengine_uk import CountryTaxBenefitSystem + + system = CountryTaxBenefitSystem() + assert len(system.variables) > 0 + assert len(list(system.parameters.get_descendants())) > 0 diff --git a/uv.lock b/uv.lock index 92032733..0f6463ae 100644 --- a/uv.lock +++ b/uv.lock @@ -2384,7 +2384,7 @@ wheels = [ [[package]] name = "policyengine-core" -version = "3.24.1" +version = "3.24.3" source = { editable = "." } dependencies = [ { name = "dpath" }, From e20aa480c41d9d325cef4e1d636ab74ef96282f5 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 17 Apr 2026 20:07:25 -0400 Subject: [PATCH 2/3] Guard invalidated_caches mutation sites like _fast_cache Follow-up to PR #474's _fast_cache guards. The same pattern applied to the other private cache attribute on Simulation: country-package subclasses that override __init__ without calling super().__init__ can skip initialising self.invalidated_caches, so purge_cache_of_invalid_values and invalidate_cache_entry now fall back gracefully instead of raising AttributeError. purge_cache_of_invalid_values: early-return if invalidated_caches is missing. invalidate_cache_entry: lazily initialise invalidated_caches with the new entry if it's missing. Extends tests/core/test_fast_cache_guards.py with two new regression tests for the invalidated_caches path. Co-Authored-By: Claude Opus 4.7 (1M context) --- changelog.d/guard-invalidated-caches.fixed.md | 1 + policyengine_core/simulations/simulation.py | 11 +++- tests/core/test_fast_cache_guards.py | 57 ++++++++++++++----- 3 files changed, 52 insertions(+), 17 deletions(-) create mode 100644 changelog.d/guard-invalidated-caches.fixed.md diff --git a/changelog.d/guard-invalidated-caches.fixed.md b/changelog.d/guard-invalidated-caches.fixed.md new file mode 100644 index 00000000..c5c0b8e2 --- /dev/null +++ b/changelog.d/guard-invalidated-caches.fixed.md @@ -0,0 +1 @@ +Guard `invalidated_caches` mutation sites in `Simulation` against the attribute being missing, matching the pattern used for `_fast_cache` in #474. Country-package subclasses that override `__init__` without calling `super().__init__` can skip initialising this private attribute, so `purge_cache_of_invalid_values` and `invalidate_cache_entry` now fall back gracefully instead of raising `AttributeError`. diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index 17f60273..25ffe6cb 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -826,7 +826,10 @@ def purge_cache_of_invalid_values(self) -> None: if self.tracer.stack: return _fast_cache = getattr(self, "_fast_cache", None) - for _name, _period in self.invalidated_caches: + invalidated_caches = getattr(self, "invalidated_caches", None) + if invalidated_caches is None: + return + for _name, _period in invalidated_caches: holder = self.get_holder(_name) holder.delete_arrays(_period) if _fast_cache is not None: @@ -1117,7 +1120,11 @@ def _check_for_cycle(self, variable: str, period: Period) -> None: raise SpiralError(message, variable) def invalidate_cache_entry(self, variable: str, period: Period) -> None: - self.invalidated_caches.add((variable, period)) + invalidated_caches = getattr(self, "invalidated_caches", None) + if invalidated_caches is None: + self.invalidated_caches = {(variable, period)} + return + invalidated_caches.add((variable, period)) def invalidate_spiral_variables(self, variable: str) -> None: # Visit the stack, from the bottom (most recent) up; we know that we'll find diff --git a/tests/core/test_fast_cache_guards.py b/tests/core/test_fast_cache_guards.py index 455aa2bf..62762fca 100644 --- a/tests/core/test_fast_cache_guards.py +++ b/tests/core/test_fast_cache_guards.py @@ -1,19 +1,22 @@ """Regression tests: cache-manipulation methods on ``Simulation`` must not -crash when ``_fast_cache`` is missing. - -``Simulation.__init__`` sets ``self._fast_cache = {}`` as the first step of -initialisation. Country-package subclasses (e.g. ``policyengine_uk.Simulation``) -can legitimately override ``__init__`` without calling ``super().__init__`` -— instead they set the handful of attributes they need directly. In that -case ``_fast_cache`` never gets initialised, so any cache-mutation path -that assumes the attribute exists raised ``AttributeError`` during -``build_from_single_year_dataset`` / ``set_input`` / ``delete_arrays``. - -The defensive fix is to guard the bare ``.pop`` / ``.items`` / re-assign -sites the same way the read-side fast path in ``calculate()`` already does -— ``getattr(self, "_fast_cache", None)`` and skip the cache write when -it's ``None``. Core owns this protection so every downstream subclass -doesn't have to mirror the attribute. +crash when private cache attributes are missing. + +``Simulation.__init__`` sets ``self._fast_cache = {}`` and +``self.invalidated_caches = set()`` as part of initialisation. +Country-package subclasses (e.g. ``policyengine_uk.Simulation``) can +legitimately override ``__init__`` without calling ``super().__init__`` +— instead they set the handful of attributes they need directly. In +that case the skipped attribute never gets initialised, so any +cache-mutation path that assumes the attribute exists raised +``AttributeError`` during ``build_from_single_year_dataset`` / +``set_input`` / ``delete_arrays`` / ``purge_cache_of_invalid_values`` / +``invalidate_cache_entry``. + +The defensive fix is to guard the bare ``.pop`` / ``.items`` / ``.add`` +sites the same way the read-side fast path in ``calculate()`` already +does — ``getattr(self, "", None)`` and skip or lazily initialise +when it's missing. Core owns this protection so every downstream +subclass doesn't have to mirror the attributes. """ from __future__ import annotations @@ -74,3 +77,27 @@ def test_purge_cache_of_invalid_values_without_fast_cache_attribute(): sim.purge_cache_of_invalid_values() assert sim.invalidated_caches == set() + + +def test_purge_cache_of_invalid_values_without_invalidated_caches_attribute(): + sim = _bare_simulation() + sim.tracer = types.SimpleNamespace(stack=[]) + # No invalidated_caches, no _fast_cache — must not crash + sim.get_holder = lambda name: types.SimpleNamespace( + delete_arrays=lambda period: None + ) + + sim.purge_cache_of_invalid_values() + + +def test_invalidate_cache_entry_without_invalidated_caches_attribute(): + sim = _bare_simulation() + # No invalidated_caches — method should lazily initialise it + sim.invalidate_cache_entry("variable_name", "2024") + assert sim.invalidated_caches == {("variable_name", "2024")} + # Subsequent invalidations should accumulate + sim.invalidate_cache_entry("other_variable", "2025") + assert sim.invalidated_caches == { + ("variable_name", "2024"), + ("other_variable", "2025"), + } From 2b18b19d73e8fb9ce39d697284a0a8327cc25de6 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 17 Apr 2026 20:10:18 -0400 Subject: [PATCH 3/3] Add spm-calculator to --no-deps fallback install for py3.13+ policyengine-us 1.647.0 depends on spm-calculator (>=0.3.1), but the 3.13+ workflow branch uses --no-deps to dodge pytest-dependency build issues and manually reinstalls a subset. Add spm-calculator to that subset so the new preflight smoke test can instantiate CountryTaxBenefitSystem() on 3.13/3.14. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/pr.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index 5871b591..c268610c 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -87,7 +87,7 @@ jobs: uv pip install --system "tables>=3.11.0" uv pip install --system policyengine-us --no-deps # Install remaining dependencies manually (skip pytest-dependency which has build issues) - uv pip install --system click pathlib synthimpute tabulate + uv pip install --system click pathlib synthimpute tabulate spm-calculator uv pip install --system policyengine-us-data --no-deps else uv pip install --system policyengine-us