diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index c25e6347..c268610c 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -87,12 +87,15 @@ 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 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/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 3944c3d2..a7916c29 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -853,7 +853,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: @@ -1144,7 +1147,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"), + } 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 e27f414e..16ec94e9 100644 --- a/uv.lock +++ b/uv.lock @@ -2384,7 +2384,7 @@ wheels = [ [[package]] name = "policyengine-core" -version = "3.24.2" +version = "3.24.4" source = { editable = "." } dependencies = [ { name = "dpath" },