[v4] Drop legacy filter_field/filter_value scoping fields#290
Merged
[v4] Drop legacy filter_field/filter_value scoping fields#290
Conversation
Removes three unambiguously dead code paths and moves plotly out of
the core install so `import policyengine` doesn't pull the charting
stack. Changes are behavior-preserving for every downstream repo
surveyed (policyengine-api, policyengine-api-v2, policyengine-api-v2-alpha).
1. Delete `tax_benefit_models/{us,uk}.py` shim files. Python always
resolves the `us/`/`uk/` package directory first, so the .py files
were dead. Worse: both re-exported `general_policy_reform_analysis`
which is not defined anywhere — `from policyengine.tax_benefit_models.us
import general_policy_reform_analysis` raises ImportError at runtime.
2. Delete `_create_entity_output_model` + `PersonOutput` /
`BenunitOutput` / `HouseholdEntityOutput` in uk/analysis.py. Built
via pydantic.create_model at import time, referenced nowhere in
the codebase.
3. Delete `policyengine.core.DatasetVersion`. One optional field on
Dataset (never set by anything) and one core re-export. Nothing
reads it downstream.
4. Move `plotly>=5.0.0` from base dependencies to a `[plotting]`
optional extra. Only `policyengine.utils.plotting` uses plotly,
and nothing in src/ imports that module — only `examples/` do.
`plotting.py` now soft-imports with a clear install hint.
Downstream impact: none. Surveyed policyengine-api (pinned to a
pre-3.x API), policyengine-api-v2 (3.4.0), policyengine-api-v2-alpha
(3.1.15); none of them import the deleted symbols.
Tests: 216 passed locally across test_release_manifests,
test_trace_tro, test_results, test_household_impact, test_models,
test_us_regions, test_uk_regions, test_region,
test_manifest_version_mismatch, test_filtering, test_cache,
test_scoping_strategy.
Deferred (bigger refactors, follow-up PRs):
- filter_field/filter_value legacy path on Simulation (still wired
through Region construction; needs migration)
- calculate_household_impact → calculate_household rename (with
deprecation shim)
- Extract shared MicrosimulationModelVersion base (~600 LOC savings)
- Move release_manifest + trace_tro to policyengine/provenance/
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
utils/__init__.py eagerly imported COLORS from plotting.py, which now raises ImportError when plotly isn't installed. Every smoke-import job on PR #288 failed because plotting.py blocked at module load. Move COLORS + FONT_* constants to a new plotly-free utils/design.py; plotting.py re-exports them for backward compatibility and adds them to __all__. utils/__init__.py now pulls COLORS from design rather than plotting. Confirmed locally that pip uninstall plotly still lets 'import policyengine' + 'from policyengine.utils import COLORS' + 'from policyengine.core.release_manifest import get_release_manifest' all work cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the two-way scoping contract in favour of the single
ScopingStrategy path. The legacy fields were labeled "kept for
backward compatibility" but became dead wiring the moment every
caller started passing scoping_strategy explicitly.
Changes:
Simulation (core/simulation.py)
- Drop filter_field, filter_value fields.
- Drop _auto_construct_strategy model_validator that rewrote those
fields into a RowFilterStrategy.
Region (core/region.py)
- Drop filter_field, filter_value, requires_filter fields.
- Re-add requires_filter as a derived @Property: True iff
scoping_strategy is not None.
- Simplify get_dataset_regions / get_filter_regions to use
dataset_path / scoping_strategy directly.
Country models (tax_benefit_models/us/model.py, .../uk/model.py)
- Delete the `elif simulation.filter_field and simulation.filter_value:`
fallback branch in run() — unreachable because nobody sets those
fields anymore.
- Delete the _filter_dataset_by_household_variable private method —
only called from the elif branch. The underlying
utils.entity_utils.filter_dataset_by_household_variable helper
stays (it's what RowFilterStrategy.apply uses).
- Drop the now-unused import.
Region factories (countries/{us,uk}/regions.py)
- Stop setting requires_filter=True, filter_field=..., filter_value=...
alongside scoping_strategy. The scoping_strategy is already the
source of truth; the duplicate legacy triple was noise.
Tests
- test_filtering.py: drop TestSimulationFilterParameters (fields gone)
and TestUSFilterDatasetByHouseholdVariable /
TestUKFilterDatasetByHouseholdVariable (method gone; underlying
behaviour still covered by test_scoping_strategy.py
TestRowFilterStrategy). Keep the build_entity_relationships tests.
- test_scoping_strategy.py: drop three legacy-auto-construct tests,
replace one with a direct WeightReplacementStrategy round-trip
check.
- test_region.py, test_us_regions.py, test_uk_regions.py: assertions
move from `region.filter_field == "X"` to
`region.scoping_strategy.variable_name == "X"`.
- fixtures/region_fixtures.py: factories use
scoping_strategy=RowFilterStrategy(...) directly.
212 tests pass. Downstream impact: policyengine-api-v2-alpha uses the
legacy fields in ~14 call sites (grep confirmed); they migrate to
RowFilterStrategy in a paired PR. The v4 migration guide will call out
this single search-and-replace.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 19, 2026
4 tasks
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.
Stacked on #288. Second in the v4 PR chain. Pure breaking-change deletion.
What
Migration for callers
Downstream impact
One repo (`policyengine-api-v2-alpha`, pinned to `>=3.2.3`) uses the legacy fields at ~14 call sites. Will file a companion migration PR there when v4 cuts. Other two API repos are on the pre-3.x surface and unaffected.
Test plan
Queue after this
🤖 Generated with Claude Code