From 8500e5b1b86a127a66540ed608844d38802a721a Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:23:46 +0400 Subject: [PATCH 01/18] Accept breakdown=local_areas on the annual economy route `AnnualEconomyQuery` gains `breakdown: Literal["local_areas"] | None`, which asks for the UK constituency and local-authority impacts only. `calculation_options()` includes it only when it is set, so the options hash of every request without it is unchanged. The budget-window query still rejects it as an undeclared parameter, and the served `/specification` publishes it with its enum because it is generated from the same model. Tests written first. Before the change, 11 failed: the parser rejected `breakdown` as `extra_forbidden` (so the unknown-value tests saw `extra_forbidden` instead of `literal_error`, and a repeated value was not reported as repeated), the model had no `breakdown` attribute, and the specification had no `breakdown` parameter (KeyError). The budget-window rejection tests and the no-breakdown route test passed before the change; they record behaviour that must stay the same. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.added.md | 1 + docs/canonical-spm.md | 14 ++- policyengine_api/query_parameters.py | 15 +++ tests/unit/routes/test_economy_spm_query.py | 109 +++++++++++++++++++ tests/unit/test_openapi_spm.py | 9 ++ tests/unit/test_query_parameters.py | 68 ++++++++++++ 6 files changed, 212 insertions(+), 4 deletions(-) create mode 100644 changelog.d/uk-local-area-breakdown.added.md diff --git a/changelog.d/uk-local-area-breakdown.added.md b/changelog.d/uk-local-area-breakdown.added.md new file mode 100644 index 000000000..79cfbefc9 --- /dev/null +++ b/changelog.d/uk-local-area-breakdown.added.md @@ -0,0 +1 @@ +The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts as a calculation option with its own options hash and cached result. Requests without it keep their existing options hash. diff --git a/docs/canonical-spm.md b/docs/canonical-spm.md index 9794be4bb..07902bf2c 100644 --- a/docs/canonical-spm.md +++ b/docs/canonical-spm.md @@ -319,10 +319,16 @@ HTTP 400. An omitted `spm` inherits the certified default measurement, while a misspelled parameter is rejected. `region` is required; annual requests require `time_period`, while budget-window requests require `start_year` and `window_size` (1–75, ending no later than 2099). Years use four -digits. Optional fields are `version` (installed country model version) and -`target` (`general`, or annual-only `cliff`). Omitted `spm` inherits certified -bundle defaults. The served `/specification` generates these query declarations -from the same typed models as the Flask parser. +digits. Optional fields are `version` (installed country model version), +`target` (`general`, or annual-only `cliff`) and the annual-only `breakdown`. +Omitted `spm` inherits certified bundle defaults. The served `/specification` +generates these query declarations from the same typed models as the Flask +parser. + +`breakdown=local_areas` asks the annual route for the UK constituency and +local-authority impacts only. It is a calculation option, so the request has +its own options hash and its own cached result. Any other value returns HTTP +400, and requests without `breakdown` keep their existing options hash. Before cache access or submission, the API validates the selected worker application's `canonical-spm-v1` capability diff --git a/policyengine_api/query_parameters.py b/policyengine_api/query_parameters.py index 06e38cf01..dc99eeb11 100644 --- a/policyengine_api/query_parameters.py +++ b/policyengine_api/query_parameters.py @@ -168,6 +168,21 @@ class AnnualEconomyQuery(EconomyQuery): time_period: EconomyYear target: Literal["general", "cliff"] = "general" + breakdown: Literal["local_areas"] | None = Field( + default=None, + description=( + "local_areas computes only the UK constituency and local-authority " + "impacts, in a run separate from the national result. UK only, " + "with region uk or country/*, and not with target=cliff" + ), + ) + + def calculation_options(self) -> dict[str, Any]: + """Add ``breakdown`` only when requested, so other options hashes stay put.""" + options = super().calculation_options() + if self.breakdown is not None: + options["breakdown"] = self.breakdown + return options class BudgetWindowEconomyQuery(EconomyQuery): diff --git a/tests/unit/routes/test_economy_spm_query.py b/tests/unit/routes/test_economy_spm_query.py index 1667e4969..c9f34e3f4 100644 --- a/tests/unit/routes/test_economy_spm_query.py +++ b/tests/unit/routes/test_economy_spm_query.py @@ -289,3 +289,112 @@ def test_a_rejected_economy_query_parameter_quotes_no_validator_internals(econom message = response.get_json()["message"] assert not any(part in message for part in PYDANTIC_INTERNALS) dispatch.assert_not_called() + + +UK_ANNUAL_PATH = "/uk/economy/123/over/456" +UK_ANNUAL_QUERY = [("region", "uk"), ("time_period", "2026")] +UK_BUDGET_WINDOW_QUERY = [ + ("region", "uk"), + ("start_year", "2026"), + ("window_size", "2"), +] + + +@pytest.fixture +def uk_economy_http(monkeypatch): + """Return a real Flask client for the UK economy routes and their service mock. + + The mock stands in for both service methods, so a test sees exactly what + the route parsed, and no request reaches the cache or the simulation API. + """ + dispatch = Mock( + return_value=SimpleNamespace( + cache_status=None, + run_progress=None, + to_dict=lambda: { + "status": "computing", + "message": None, + "data": None, + "progress": 0, + "completed_years": [], + "computing_years": [], + "queued_years": [], + "error": None, + }, + ) + ) + for method in ("get_economic_impact", "get_budget_window_economic_impact"): + monkeypatch.setattr(economy_routes.economy_service, method, dispatch) + app = Flask(__name__) + app.config["TESTING"] = True + app.register_blueprint(economy_routes.economy_bp) + return app.test_client(), dispatch + + +def test_annual_route_passes_a_local_area_breakdown_to_the_service(uk_economy_http): + client, dispatch = uk_economy_http + + response = client.get( + UK_ANNUAL_PATH, + query_string=[*UK_ANNUAL_QUERY, ("breakdown", "local_areas")], + ) + + assert response.status_code == 200, response.get_json() + impact_request = dispatch.call_args.kwargs["impact_request"] + assert impact_request.options == {"breakdown": "local_areas"} + assert impact_request.region == "uk" + assert impact_request.target == "general" + + +def test_annual_route_without_breakdown_passes_no_breakdown_option(uk_economy_http): + client, dispatch = uk_economy_http + + response = client.get(UK_ANNUAL_PATH, query_string=UK_ANNUAL_QUERY) + + assert response.status_code == 200, response.get_json() + assert dispatch.call_args.kwargs["impact_request"].options == {} + + +def test_annual_route_rejects_a_repeated_breakdown(uk_economy_http): + client, dispatch = uk_economy_http + + response = client.get( + UK_ANNUAL_PATH, + query_string=[ + *UK_ANNUAL_QUERY, + ("breakdown", "local_areas"), + ("breakdown", "local_areas"), + ], + ) + + assert response.status_code == 400 + assert "'breakdown' must not be repeated" in response.get_json()["message"] + dispatch.assert_not_called() + + +def test_annual_route_rejects_an_unknown_breakdown(uk_economy_http): + client, dispatch = uk_economy_http + + response = client.get( + UK_ANNUAL_PATH, + query_string=[*UK_ANNUAL_QUERY, ("breakdown", "constituencies")], + ) + + assert response.status_code == 400 + message = response.get_json()["message"] + assert message.startswith("breakdown: Input should be 'local_areas'") + assert not any(part in message for part in PYDANTIC_INTERNALS) + dispatch.assert_not_called() + + +def test_budget_window_route_rejects_a_breakdown(uk_economy_http): + client, dispatch = uk_economy_http + + response = client.get( + UK_ANNUAL_PATH + "/budget-window", + query_string=[*UK_BUDGET_WINDOW_QUERY, ("breakdown", "local_areas")], + ) + + assert response.status_code == 400 + assert "breakdown" in response.get_json()["message"] + dispatch.assert_not_called() diff --git a/tests/unit/test_openapi_spm.py b/tests/unit/test_openapi_spm.py index 6bd504239..55deec651 100644 --- a/tests/unit/test_openapi_spm.py +++ b/tests/unit/test_openapi_spm.py @@ -48,6 +48,15 @@ def test_economy_query_and_receipt_schemas_match_public_http_contract(): assert parameters["target"]["schema"]["enum"] == ( ["general"] if suffix else ["general", "cliff"] ) + if suffix: + assert "breakdown" not in parameters + else: + breakdown = parameters["breakdown"] + assert breakdown["required"] is False + assert breakdown["schema"]["type"] == "string" + assert breakdown["schema"]["enum"] == ["local_areas"] + assert breakdown["schema"]["nullable"] is True + assert "constituency" in breakdown["description"] if suffix: assert parameters["window_size"]["schema"]["minimum"] == 1 assert parameters["window_size"]["schema"]["maximum"] == 75 diff --git a/tests/unit/test_query_parameters.py b/tests/unit/test_query_parameters.py index 97b2823eb..534641250 100644 --- a/tests/unit/test_query_parameters.py +++ b/tests/unit/test_query_parameters.py @@ -13,6 +13,8 @@ from policyengine_api.fastapi_routes.query_parameters import query_dependency from policyengine_api.query_parameters import ( + AnnualEconomyQuery, + BudgetWindowEconomyQuery, CountryQuery, DuplicateScalarQueryParameterError, HouseholdCollectionQuery, @@ -128,6 +130,72 @@ def test_duplicate_scalar_is_rejected_and_explicit_list_is_preserved() -> None: assert parsed.policy_ids == [first_id, second_id] +UK_ANNUAL_ECONOMY_QUERY = [("region", "uk"), ("time_period", "2026")] + + +def test_annual_economy_query_parses_a_local_area_breakdown() -> None: + parsed = parse_query_items( + AnnualEconomyQuery, + [*UK_ANNUAL_ECONOMY_QUERY, ("breakdown", "local_areas")], + ) + + assert parsed.breakdown == "local_areas" + assert parsed.calculation_options() == {"breakdown": "local_areas"} + + +def test_annual_economy_query_without_breakdown_keeps_its_options_unchanged() -> None: + """An omitted breakdown adds no option, so existing options hashes still match.""" + parsed = parse_query_items(AnnualEconomyQuery, UK_ANNUAL_ECONOMY_QUERY) + + assert parsed.breakdown is None + assert parsed.calculation_options() == {} + + +@pytest.mark.parametrize("value", ["", "constituencies", "LOCAL_AREAS", "true"]) +def test_annual_economy_query_rejects_an_unknown_breakdown(value: str) -> None: + with pytest.raises(ValidationError) as raised: + parse_query_items( + AnnualEconomyQuery, + [*UK_ANNUAL_ECONOMY_QUERY, ("breakdown", value)], + ) + + error = raised.value.errors()[0] + assert error["loc"] == ("breakdown",) + assert error["type"] == "literal_error" + + +def test_annual_economy_query_rejects_a_repeated_breakdown() -> None: + with pytest.raises( + DuplicateScalarQueryParameterError, + match=r"breakdown.*must not be repeated", + ): + parse_query_items( + AnnualEconomyQuery, + [ + *UK_ANNUAL_ECONOMY_QUERY, + ("breakdown", "local_areas"), + ("breakdown", "local_areas"), + ], + ) + + +def test_budget_window_economy_query_rejects_a_breakdown() -> None: + with pytest.raises(ValidationError) as raised: + parse_query_items( + BudgetWindowEconomyQuery, + [ + ("region", "uk"), + ("start_year", "2026"), + ("window_size", "2"), + ("breakdown", "local_areas"), + ], + ) + + error = raised.value.errors()[0] + assert error["loc"] == ("breakdown",) + assert error["type"] == "extra_forbidden" + + def test_multidict_adapter_preserves_the_canonical_contract() -> None: policy_id = uuid4() parsed = parse_multidict_query( From 24b1bed5f473898f060aeb8c3a4428e3b2b8d2f4 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:49:22 +0400 Subject: [PATCH 02/18] Validate breakdown=local_areas and send it to the simulation API `_build_economic_impact_setup_options` now rejects a breakdown before any cache read or worker capability check unless the request is for the UK, with region `uk` or `country/*`, and not `target=cliff`. The route returns those ValueErrors as 400. `SimulationOptions` gains `breakdown`, and `_setup_sim_options` fills it from the request options. The field is excluded from the payload when it is None, so national and budget-window submissions are unchanged. The options hash already includes `breakdown` (previous commit), so a breakdown request never matches a national lookup pattern, or the other way round. When the simulation API answers a breakdown submission with 400 or 422, the annual path now returns an error result, which the route sends as 502 with the simulation API's reason (each field of a 422 body is named). Before, the HTTPStatusError reached the generic handler as a 500. Nothing is stored, so a later request submits again once the simulation API can run breakdowns. Other statuses, and every request without a breakdown, still raise as before. `_build_budget_window_submission_error_message` is renamed `_build_submission_error_message`, since both paths now use it. The served specification and docs/canonical-spm.md describe the breakdown result and the 502. Tests written first. Before the change, 21 failed for the expected reasons: six unsupported UK requests did not raise ValueError; the two US requests passed the missing check and failed later on the SPM capability check; three route requests returned 200 instead of 400; the payload had no `breakdown` (KeyError, and `[None]` instead of `['local_areas']`); and 400 and 422 refusals propagated as HTTPStatusError. Tests that pass before the change pin unchanged behaviour: national payloads carry no `breakdown`, the two options hashes never match each other's lookup pattern, other refusal statuses still raise, and the HTTP client forwards only non-null fields. One test, for the unknown-value check that the query parser already makes unreachable, was added after the code. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.added.md | 2 +- docs/canonical-spm.md | 11 +- policyengine_api/openapi_spec.yaml | 17 +- policyengine_api/services/economy_service.py | 113 ++++++- .../test_simulation_gateway_contract.py | 81 +++++ tests/fixtures/libs/simulation_entrypoint.py | 32 ++ tests/unit/libs/test_simulation_entrypoint.py | 73 +++++ tests/unit/routes/test_economy_spm_query.py | 112 +++++++ tests/unit/services/test_economy_service.py | 306 +++++++++++++++++- .../test_economy_service_request_plumbing.py | 134 ++++++++ 10 files changed, 864 insertions(+), 17 deletions(-) diff --git a/changelog.d/uk-local-area-breakdown.added.md b/changelog.d/uk-local-area-breakdown.added.md index 79cfbefc9..fcced178f 100644 --- a/changelog.d/uk-local-area-breakdown.added.md +++ b/changelog.d/uk-local-area-breakdown.added.md @@ -1 +1 @@ -The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts as a calculation option with its own options hash and cached result. Requests without it keep their existing options hash. +The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or `country/*`, and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. diff --git a/docs/canonical-spm.md b/docs/canonical-spm.md index 07902bf2c..8874966e9 100644 --- a/docs/canonical-spm.md +++ b/docs/canonical-spm.md @@ -328,7 +328,16 @@ parser. `breakdown=local_areas` asks the annual route for the UK constituency and local-authority impacts only. It is a calculation option, so the request has its own options hash and its own cached result. Any other value returns HTTP -400, and requests without `breakdown` keep their existing options hash. +400, and requests without `breakdown` keep their existing options hash. It is +accepted only for the UK, with `region` `uk` or a UK country (`country/*`), +and not with `target=cliff`; other requests return HTTP 400 before anything is +submitted. The API sends `"breakdown": "local_areas"` to the simulation API, +which computes the impacts in a run separate from the national result, and +returns its result (`kind: "local_areas"`) like any other economy result. If +the simulation API refuses the request with HTTP 400 or 422, because the +deployed simulation API or the .py release it routes to cannot run breakdowns, +the route returns HTTP 502 with the simulation API's reason and stores +nothing, so a later request tries again. Before cache access or submission, the API validates the selected worker application's `canonical-spm-v1` capability diff --git a/policyengine_api/openapi_spec.yaml b/policyengine_api/openapi_spec.yaml index 7925f6935..1749afb3d 100644 --- a/policyengine_api/openapi_spec.yaml +++ b/policyengine_api/openapi_spec.yaml @@ -879,7 +879,7 @@ paths: # Query parameters are generated from policyengine_api.query_parameters. responses: 200: - description: Annual economic impact, progress, or persisted error state. Canonical completed results include actual baseline/reform receipts containing the selected SPM configuration. While computing, the optional stage fields come from the simulation entrypoint and appear only when its worker reports stages. + description: Annual economic impact, progress, or persisted error state. Canonical completed results include actual baseline/reform receipts containing the selected SPM configuration. While computing, the optional stage fields come from the simulation entrypoint and appear only when its worker reports stages. With breakdown=local_areas, a completed result is the simulation API's local-area breakdown instead, with kind local_areas, the constituency_impact and local_authority_impact lists, and the dataset and version fields. content: application/json: schema: @@ -945,6 +945,21 @@ paths: type: string message: type: string + 502: + description: The calculation failed, or the simulation API refused a breakdown=local_areas request it cannot run. The message gives the reason. + content: + application/json: + schema: + type: object + properties: + status: + type: string + enum: [error] + message: + type: string + result: + type: object + nullable: true /{country_id}/economy/{policy_id}/over/{baseline_policy_id}/budget-window: get: summary: Calculate budget-window economic impacts diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index e0edab60a..4a69c4d93 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -105,6 +105,12 @@ class ImpactStatus(Enum): BUDGET_WINDOW_MAX_YEARS = budget_window_utils.BUDGET_WINDOW_MAX_YEARS BUDGET_WINDOW_MAX_END_YEAR = budget_window_utils.BUDGET_WINDOW_MAX_END_YEAR BUDGET_WINDOW_SUBMISSION_VALIDATION_ERROR_STATUS_CODES = {400, 422} +LOCAL_AREA_BREAKDOWN = "local_areas" +# Statuses with which the simulation API refuses a breakdown it cannot run: +# 400 from a gateway whose routed .py release does not declare breakdowns, and +# 422 from an entry deployment that does not know the field. +LOCAL_AREA_BREAKDOWN_REFUSAL_STATUS_CODES = frozenset({400, 422}) +UK_COUNTRY_REGION_PREFIX = "country/" BUDGET_WINDOW_CLAIM_ATTEMPTS = 3 REFORM_IMPACT_START_CLAIM_ATTEMPTS = 3 # A budget-window progress message names this many computing years and counts @@ -148,6 +154,10 @@ class SimulationOptions(BaseModel): region: str model_version: str | None = None policyengine_version: str | None = None + # Left out of the payload unless set, so national submissions are unchanged. + breakdown: Literal["local_areas"] | None = Field( + default=None, exclude_if=lambda value: value is None + ) class EconomicImpactSetupOptions(BaseModel): @@ -685,7 +695,7 @@ def _submit_claimed_budget_window_batch( in BUDGET_WINDOW_SUBMISSION_VALIDATION_ERROR_STATUS_CODES ): failed_result = BudgetWindowEconomicImpactResult.failed( - self._build_budget_window_submission_error_message(error), + self._build_submission_error_message(error), queued_years=budget_window_setup.years, cache_status="miss", ) @@ -844,9 +854,7 @@ def _start_budget_window_batch( return self._simulation_gateway.run_budget_window_batch(sim_params) - def _build_budget_window_submission_error_message( - self, error: httpx.HTTPStatusError - ) -> str: + def _build_submission_error_message(self, error: httpx.HTTPStatusError) -> str: try: response_json = error.response.json() except ValueError: @@ -991,6 +999,7 @@ def _build_economic_impact_setup_options( if impact_request.dataset != "default": msg = "Custom datasets are not supported for economy calculations" raise ValueError(msg) + self._validate_breakdown(impact_request) resolved_spm = validate_worker_spm( country_id, options.get("spm"), @@ -1041,6 +1050,93 @@ def _build_economic_impact_setup_options( } ) + @staticmethod + def _validate_breakdown(impact_request: EconomicImpactRequest) -> None: + """Reject a local-area breakdown the simulation API has no run for. + + The breakdown covers the whole UK or one UK country, so it needs region + ``uk`` or ``country/*``: a constituency or local authority is already a + single area. It computes no cliff impacts, so ``target=cliff`` is + refused too. Raise ValueError, which the route returns as 400. + """ + breakdown = impact_request.options.get("breakdown") + if breakdown is None: + return + if breakdown != LOCAL_AREA_BREAKDOWN: + msg = f"Unknown breakdown: {breakdown!r}" + raise ValueError(msg) + if impact_request.country_id != "uk": + msg = "breakdown=local_areas is only available for UK economy calculations" + raise ValueError(msg) + region = impact_request.region + country_name = region.removeprefix(UK_COUNTRY_REGION_PREFIX) + is_uk_country = ( + country_name != region and bool(country_name) and "/" not in country_name + ) + if region != "uk" and not is_uk_country: + msg = ( + "breakdown=local_areas needs region uk or a UK country " + f"(country/...), not {region!r}" + ) + raise ValueError(msg) + if impact_request.target == "cliff": + msg = "breakdown=local_areas cannot be combined with target=cliff" + raise ValueError(msg) + + def _breakdown_refusal( + self, + error: httpx.HTTPStatusError, + *, + breakdown: str | None, + ) -> EconomicImpactResult | None: + """Return an error result when the simulation API refuses a breakdown. + + This API checks a breakdown request before submitting it, so a 400 or + 422 means the deployed simulation API, or the .py release it routes to, + cannot run breakdowns yet. The route returns the result as 502 with the + simulation API's reason. Nothing is stored, so a later request submits + again and succeeds once the simulation API can run it. + + Return None for any other status, and for every request without a + breakdown, so the caller re-raises those errors unchanged. + """ + status_code = error.response.status_code + if ( + breakdown is None + or status_code not in LOCAL_AREA_BREAKDOWN_REFUSAL_STATUS_CODES + ): + return None + return EconomicImpactResult.error( + message=( + "The simulation API refused the local-area breakdown request " + f"(HTTP {status_code}): {self._refusal_reason(error)}" + ) + ) + + def _refusal_reason(self, error: httpx.HTTPStatusError) -> str: + """Return the reason in a refusal, naming each field of a 422 body.""" + try: + body = error.response.json() + except ValueError: + body = None + details = body.get("detail") if isinstance(body, dict) else None + if isinstance(details, list): + reasons = [ + ": ".join( + part + for part in ( + ".".join(str(item) for item in detail.get("loc", [])), + str(detail.get("msg", "")), + ) + if part + ) + for detail in details + if isinstance(detail, dict) + ] + if any(reasons): + return "; ".join(reason for reason in reasons if reason) + return self._build_submission_error_message(error) + @observability_runtime.span( ECONOMY_ANNUAL_STAGES.name(Stage.ECONOMY_RESOLVE_CACHED_OR_NEW) ) @@ -1670,7 +1766,13 @@ def _handle_create_impact( if sim_params.get("time_period") is not None: sim_params["time_period"] = str(sim_params["time_period"]) - entrypoint_execution = self._simulation_gateway.run(sim_params) + try: + entrypoint_execution = self._simulation_gateway.run(sim_params) + except httpx.HTTPStatusError as error: + refusal = self._breakdown_refusal(error, breakdown=sim_config.breakdown) + if refusal is None: + raise + return refusal try: execution_id = self._simulation_gateway.get_execution_id( entrypoint_execution @@ -1723,6 +1825,7 @@ def _setup_sim_options( ), "model_version": setup_options.model_version, "policyengine_version": setup_options.policyengine_version, + "breakdown": setup_options.options.get("breakdown"), } ) diff --git a/tests/contract/test_simulation_gateway_contract.py b/tests/contract/test_simulation_gateway_contract.py index 1b4bc817f..217037170 100644 --- a/tests/contract/test_simulation_gateway_contract.py +++ b/tests/contract/test_simulation_gateway_contract.py @@ -1,3 +1,4 @@ +import json from unittest.mock import MagicMock import httpx @@ -5,11 +6,16 @@ from policyengine_api.libs import simulation_entrypoint from policyengine_api.libs.simulation_entrypoint import SimulationAPIModal +from policyengine_api.services.economy_service import ( + EconomicImpactSetupOptions, + EconomyService, +) from tests.fixtures.libs.simulation_entrypoint import ( MOCK_BATCH_JOB_ID, MOCK_BATCH_POLL_RESPONSE_COMPLETE, MOCK_BATCH_SUBMIT_RESPONSE_SUCCESS, MOCK_HEALTH_RESPONSE, + MOCK_LOCAL_AREA_BREAKDOWN_RESULT, MOCK_MODAL_JOB_ID, MOCK_POLL_RESPONSE_COMPLETE, MOCK_RESOLVED_APP_NAME, @@ -159,3 +165,78 @@ def test_gateway_versions_and_health_contract(monkeypatch): assert bundle_app_name == MOCK_RESOLVED_APP_NAME assert bundle_version == "1.729.0" assert client.health_check() is True + + +def _uk_submission(options: dict) -> dict: + """Return the annual UK payload the economy service builds for these options.""" + setup_options = EconomicImpactSetupOptions( + submission_claim_id="00000000-0000-4000-8000-000000000002", + country_id="uk", + reform_policy_id=2, + baseline_policy_id=1, + region="uk", + dataset="default", + time_period="2026", + options=options, + api_version="1.0", + target="general", + model_version="2.132.6", + policyengine_version="6.10.0", + ) + return ( + EconomyService() + ._setup_sim_options( + setup_options, + reform_policy={"gov.hmrc.income_tax.allowances.personal_allowance": {}}, + baseline_policy={}, + time_period="2026", + ) + .model_dump(mode="json") + ) + + +@pytest.mark.parametrize( + ("options", "expected_breakdown", "result"), + [ + ( + {"breakdown": "local_areas"}, + "local_areas", + MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + ), + ({}, None, MOCK_POLL_RESPONSE_COMPLETE["result"]), + ], + ids=["local-area-breakdown", "national"], +) +def test_gateway_uk_comparison_sends_breakdown_only_when_requested( + monkeypatch, options, expected_breakdown, result +): + _clear_gateway_auth_env(monkeypatch) + monkeypatch.setenv("OLD_SIMULATION_GATEWAY_URL", "https://simulation.test") + submitted_bodies = [] + + def respond(request: httpx.Request) -> httpx.Response: + if (request.method, request.url.path) == ( + "POST", + "/simulate/economy/comparison", + ): + submitted_bodies.append(json.loads(request.content)) + return _response(202, MOCK_SUBMIT_RESPONSE_SUCCESS) + assert (request.method, request.url.path) == ( + "GET", + f"/jobs/{MOCK_MODAL_JOB_ID}", + ) + return _response(200, {**MOCK_POLL_RESPONSE_COMPLETE, "result": result}) + + client = SimulationAPIModal() + client.client = httpx.Client( + base_url=client.base_url, transport=httpx.MockTransport(respond) + ) + + execution = client.run(_uk_submission(options)) + completed = client.get_execution_by_id(execution.job_id) + + assert [body.get("breakdown") for body in submitted_bodies] == [expected_breakdown] + assert ("breakdown" in submitted_bodies[0]) is (expected_breakdown is not None) + assert submitted_bodies[0]["region"] == "uk" + assert submitted_bodies[0]["include_cliffs"] is False + assert completed.result == result diff --git a/tests/fixtures/libs/simulation_entrypoint.py b/tests/fixtures/libs/simulation_entrypoint.py index 06fa98f25..11a6c4d50 100644 --- a/tests/fixtures/libs/simulation_entrypoint.py +++ b/tests/fixtures/libs/simulation_entrypoint.py @@ -51,6 +51,38 @@ "inequality_impact": {"baseline": 0.45, "reform": 0.42}, **worker_result_fields(years=["2025"]), } +# The simulation API's LocalAreaBreakdownOutput for a breakdown=local_areas run. +MOCK_LOCAL_AREA_BREAKDOWN_RESULT = { + "kind": "local_areas", + "model_version": "2.132.6", + "policyengine_version": "6.10.0", + "dataset": "microcosm_uk_2024_25_dense", + "data_version": "microcosm-uk-2024-25-dense-test-release", + "constituency_impact": [ + { + "constituency_code": "E14001063", + "constituency_name": "Aldershot", + "x": 10, + "y": 20, + "average_household_income_change": 120.0, + "relative_household_income_change": 0.003, + "population": 41000.0, + "disclosure_tier": "reported", + } + ], + "local_authority_impact": [ + { + "local_authority_code": "E07000092", + "local_authority_name": "Rushmoor", + "x": 3, + "y": 7, + "average_household_income_change": 110.0, + "relative_household_income_change": 0.0028, + "population": 39000.0, + "disclosure_tier": "approximate", + } + ], +} MOCK_POLICYENGINE_BUNDLE = { "model_version": "1.459.0", "policyengine_version": "3.4.0", diff --git a/tests/unit/libs/test_simulation_entrypoint.py b/tests/unit/libs/test_simulation_entrypoint.py index 2b4986f1d..5ab7267f5 100644 --- a/tests/unit/libs/test_simulation_entrypoint.py +++ b/tests/unit/libs/test_simulation_entrypoint.py @@ -719,6 +719,79 @@ def test__given_api_v1_default_bundle_payload__then_posts_gateway_contract_body( assert "model_version" not in posted_payload assert "data_version" not in posted_payload + @pytest.mark.usefixtures("mock_modal_logger") + @pytest.mark.parametrize( + ("breakdown", "expected_gateway_keys"), + [ + ( + "local_areas", + { + "country", + "scope", + "reform", + "baseline", + "time_period", + "region", + "include_cliffs", + "version", + "policyengine_version", + "breakdown", + "_metadata", + "_telemetry", + }, + ), + ( + None, + { + "country", + "scope", + "reform", + "baseline", + "time_period", + "region", + "include_cliffs", + "version", + "policyengine_version", + "_metadata", + "_telemetry", + }, + ), + ], + ids=["local-area-breakdown", "national"], + ) + def test__given_uk_payload__then_posts_breakdown_only_when_set( + self, + mock_httpx_client, + breakdown, + expected_gateway_keys, + ): + mock_httpx_client.post.return_value = create_mock_httpx_response( + status_code=202, + json_data=MOCK_SUBMIT_RESPONSE_SUCCESS, + ) + payload = { + "country": "uk", + "scope": "macro", + "reform": {}, + "baseline": {}, + "time_period": "2026", + "region": "country/scotland", + "include_cliffs": False, + "model_version": "2.132.6", + "policyengine_version": "6.10.0", + "spm": None, + "breakdown": breakdown, + "_metadata": {"dataset": "default"}, + "_telemetry": {"capture_mode": "disabled"}, + } + api = SimulationAPIModal() + + api.run(payload) + + posted_payload = mock_httpx_client.post.call_args.kwargs["json"] + assert set(posted_payload) == expected_gateway_keys + assert posted_payload.get("breakdown") == breakdown + @pytest.mark.usefixtures("mock_modal_logger") def test__given_http_error__then_raises_exception( self, diff --git a/tests/unit/routes/test_economy_spm_query.py b/tests/unit/routes/test_economy_spm_query.py index c9f34e3f4..a20ac7e30 100644 --- a/tests/unit/routes/test_economy_spm_query.py +++ b/tests/unit/routes/test_economy_spm_query.py @@ -4,12 +4,18 @@ from types import SimpleNamespace from unittest.mock import Mock +import httpx import pytest from flask import Flask from policyengine_api import spm from policyengine_api.routes import economy_routes +from policyengine_api.runtime_cache.core import CacheNamespace +from policyengine_api.runtime_cache.fake import InMemoryCacheBackend +from policyengine_api.runtime_cache.reform_impacts import ReformImpactCache +from policyengine_api.services.budget_window_cache import BudgetWindowCache from policyengine_api.services.economy_service import EconomyService +from policyengine_api.services.reform_impacts_service import ReformImpactsService @pytest.fixture(params=[False, True], ids=["annual", "budget-window"]) @@ -398,3 +404,109 @@ def test_budget_window_route_rejects_a_breakdown(uk_economy_http): assert response.status_code == 400 assert "breakdown" in response.get_json()["message"] dispatch.assert_not_called() + + +def _rejection(status_code: int, payload: dict) -> httpx.HTTPStatusError: + request = httpx.Request( + "POST", "https://simulation.test/simulate/economy/comparison" + ) + return httpx.HTTPStatusError( + f"Client error '{status_code}'", + request=request, + response=httpx.Response(status_code, json=payload, request=request), + ) + + +@pytest.fixture +def economy_service_http(monkeypatch): + """Return a Flask client whose economy routes use a real economy service. + + The service keeps its caches in memory and talks to a mocked simulation + API, so a request runs every check and the submission itself. + """ + namespace = CacheNamespace("test", "api") + gateway = Mock() + gateway.resolve_app_name.return_value = ("policyengine-simulation-test", "2.0.0") + gateway.get_execution_id.return_value = "submitted-job" + policies = Mock() + policies.get_policy_json.return_value = {} + service = EconomyService( + policy_service_=policies, + reform_impacts_service_=ReformImpactsService( + ReformImpactCache(InMemoryCacheBackend(), namespace) + ), + budget_window_cache_=BudgetWindowCache(InMemoryCacheBackend(), namespace), + simulation_entrypoint_=gateway, + ) + monkeypatch.setattr(economy_routes, "economy_service", service) + app = Flask(__name__) + app.config["TESTING"] = True + app.register_blueprint(economy_routes.economy_bp) + return app.test_client(), gateway + + +@pytest.mark.parametrize( + ("path", "query", "message"), + [ + ( + "/us/economy/123/over/456", + [("region", "us"), ("time_period", "2026")], + "only available for UK", + ), + ( + UK_ANNUAL_PATH, + [("region", "constituency/Aldershot"), ("time_period", "2026")], + "region uk or a UK country", + ), + ( + UK_ANNUAL_PATH, + [("region", "local_authority/E07000092"), ("time_period", "2026")], + "region uk or a UK country", + ), + ( + UK_ANNUAL_PATH, + [*UK_ANNUAL_QUERY, ("target", "cliff")], + "target=cliff", + ), + ], + ids=["us", "constituency", "local-authority", "cliff"], +) +def test_annual_route_rejects_an_unsupported_breakdown_with_400( + economy_service_http, path, query, message +): + client, gateway = economy_service_http + + response = client.get(path, query_string=[*query, ("breakdown", "local_areas")]) + + assert response.status_code == 400 + assert message in response.get_json()["message"] + gateway.run.assert_not_called() + + +def test_annual_route_returns_502_when_the_simulation_api_refuses_a_breakdown( + economy_service_http, +): + client, gateway = economy_service_http + gateway.run.side_effect = _rejection( + 400, {"detail": "policyengine 6.7.5 does not support local-area breakdowns"} + ) + + response = client.get( + UK_ANNUAL_PATH, + query_string=[ + ("region", "country/scotland"), + ("time_period", "2026"), + ("breakdown", "local_areas"), + ], + ) + + assert response.status_code == 502 + assert response.get_json() == { + "status": "error", + "message": ( + "The simulation API refused the local-area breakdown request " + "(HTTP 400): policyengine 6.7.5 does not support local-area breakdowns" + ), + "result": None, + } + assert gateway.run.call_args.args[0]["breakdown"] == "local_areas" diff --git a/tests/unit/services/test_economy_service.py b/tests/unit/services/test_economy_service.py index d4029050b..1daefe2ce 100644 --- a/tests/unit/services/test_economy_service.py +++ b/tests/unit/services/test_economy_service.py @@ -84,6 +84,25 @@ def setup_options_for(country_id: str, region: str) -> EconomicImpactSetupOption ) +def uk_breakdown_request( + *, + country_id: str = "uk", + region: str = "uk", + target: Literal["general", "cliff"] = "general", +) -> EconomicImpactRequest: + """Return an annual request for the UK local-area breakdown.""" + return EconomicImpactRequest( + country_id=country_id, + policy_id=MOCK_POLICY_ID, + baseline_policy_id=MOCK_BASELINE_POLICY_ID, + region=region, + dataset="default", + options={"breakdown": "local_areas"}, + api_version=MOCK_API_VERSION, + target=target, + ) + + class EconomyServiceHarness(NamedTuple): """The service under test, its call parameters and the mocks it talks to.""" @@ -1282,6 +1301,169 @@ def test__given_uk_request__preserves_model_version_in_bundle( sim_params = mock_simulation_entrypoint.run.call_args[0][0] assert sim_params["_metadata"]["model_version"] == "2.7.8" + @pytest.mark.usefixtures( + "mock_policyengine_version", + "mock_policy_service", + "mock_logger", + "mock_datetime", + "mock_submission_claim_id", + "mock_reform_impacts_service", + ) + def test__given_local_area_breakdown__submits_breakdown_to_simulation_api( + self, + economy_service, + mock_country_package_versions, + mock_simulation_entrypoint, + ): + mock_country_package_versions["uk"] = "2.7.8" + + result = economy_service.get_economic_impact( + uk_breakdown_request(), time_period=MOCK_TIME_PERIOD + ) + + assert result.status == ImpactStatus.COMPUTING + sim_params = mock_simulation_entrypoint.run.call_args.args[0] + assert sim_params["breakdown"] == "local_areas" + assert sim_params["country"] == "uk" + assert sim_params["region"] == "uk" + assert sim_params["include_cliffs"] is False + + @pytest.mark.usefixtures( + "mock_policyengine_version", + "mock_policy_service", + "mock_logger", + "mock_datetime", + "mock_submission_claim_id", + ) + @pytest.mark.parametrize("status_code", [400, 422]) + def test__given_simulation_api_refuses_breakdown__returns_error_without_storing( + self, + economy_service, + mock_country_package_versions, + mock_reform_impacts_service, + mock_simulation_entrypoint, + status_code, + ): + mock_country_package_versions["uk"] = "2.7.8" + mock_simulation_entrypoint.run.side_effect = make_http_status_error( + status_code, + {"detail": "policyengine 6.7.5 does not support local-area breakdowns"}, + ) + + result = economy_service.get_economic_impact( + uk_breakdown_request(), time_period=MOCK_TIME_PERIOD + ) + + assert result.status == ImpactStatus.ERROR + assert result.data is None + assert result.message == ( + "The simulation API refused the local-area breakdown request " + f"(HTTP {status_code}): " + "policyengine 6.7.5 does not support local-area breakdowns" + ) + # Nothing is stored, so a later request submits again once the + # simulation API can run the breakdown. + mock_reform_impacts_service.set_reform_impact.assert_not_called() + mock_reform_impacts_service.set_error_reform_impact.assert_not_called() + claim = mock_reform_impacts_service.claim_reform_impact_start + mock_reform_impacts_service.release_reform_impact_start.assert_called_once_with( + **claim.call_args.kwargs + ) + + @pytest.mark.usefixtures( + "mock_policyengine_version", + "mock_policy_service", + "mock_logger", + "mock_datetime", + "mock_submission_claim_id", + "mock_reform_impacts_service", + ) + def test__given_request_validation_errors_for_breakdown__names_each_field( + self, + economy_service, + mock_country_package_versions, + mock_simulation_entrypoint, + ): + mock_country_package_versions["uk"] = "2.7.8" + mock_simulation_entrypoint.run.side_effect = make_http_status_error( + 422, + { + "detail": [ + { + "type": "extra_forbidden", + "loc": ["body", "breakdown"], + "msg": "Extra inputs are not permitted", + "input": "local_areas", + } + ] + }, + ) + + result = economy_service.get_economic_impact( + uk_breakdown_request(), time_period=MOCK_TIME_PERIOD + ) + + assert result.status == ImpactStatus.ERROR + assert result.message == ( + "The simulation API refused the local-area breakdown request " + "(HTTP 422): body.breakdown: Extra inputs are not permitted" + ) + + @pytest.mark.usefixtures( + "mock_policyengine_version", + "mock_policy_service", + "mock_logger", + "mock_datetime", + "mock_submission_claim_id", + ) + @pytest.mark.parametrize("status_code", [401, 403, 429, 500, 503]) + def test__given_other_simulation_api_failure_for_breakdown__raises( + self, + economy_service, + mock_country_package_versions, + mock_reform_impacts_service, + mock_simulation_entrypoint, + status_code, + ): + mock_country_package_versions["uk"] = "2.7.8" + mock_simulation_entrypoint.run.side_effect = make_http_status_error( + status_code, {"detail": "gateway unavailable"} + ) + + with pytest.raises(httpx.HTTPStatusError): + economy_service.get_economic_impact( + uk_breakdown_request(), time_period=MOCK_TIME_PERIOD + ) + + mock_reform_impacts_service.release_reform_impact_start.assert_called_once() + + @pytest.mark.usefixtures( + "mock_policyengine_version", + "mock_policy_service", + "mock_logger", + "mock_datetime", + "mock_submission_claim_id", + ) + def test__given_simulation_api_refuses_national_request__raises( + self, + economy_service, + mock_country_package_versions, + mock_reform_impacts_service, + mock_simulation_entrypoint, + ): + mock_country_package_versions["uk"] = "2.7.8" + mock_simulation_entrypoint.run.side_effect = make_http_status_error( + 400, {"detail": "invalid request"} + ) + + with pytest.raises(httpx.HTTPStatusError): + economy_service.get_economic_impact( + replace(uk_breakdown_request(), options={}), + time_period=MOCK_TIME_PERIOD, + ) + + mock_reform_impacts_service.release_reform_impact_start.assert_called_once() + class TestGetBudgetWindowEconomicImpact: @pytest.fixture def economy_service( @@ -1864,9 +2046,7 @@ def test__given_modal_validation_json_error__extracts_message( ): error = make_http_status_error(400, payload) - message = economy_service._build_budget_window_submission_error_message( - error - ) + message = economy_service._build_submission_error_message(error) assert message == expected_message @@ -1875,9 +2055,7 @@ def test__given_modal_validation_plain_text_error__extracts_response_text( ): error = make_http_status_error(400, text="plain validation failed") - message = economy_service._build_budget_window_submission_error_message( - error - ) + message = economy_service._build_submission_error_message(error) assert message == "plain validation failed" @@ -1886,9 +2064,7 @@ def test__given_modal_validation_empty_error__falls_back_to_exception_text( ): error = make_http_status_error(400, text="") - message = economy_service._build_budget_window_submission_error_message( - error - ) + message = economy_service._build_submission_error_message(error) assert message == str(error) @@ -2750,6 +2926,39 @@ def test__given_congressional_district__returns_correct_sim_options( assert sim_options["region"] == "congressional_district/CA-37" assert "data" not in sim_options + def test__given_local_area_breakdown__sends_breakdown(self): + setup_options = setup_options_for("uk", "country/scotland") + setup_options.options = {"breakdown": "local_areas"} + + sim_options = ( + EconomyService() + ._setup_sim_options( + setup_options, + reform_policy=self.test_reform_policy, + baseline_policy=self.test_current_law_baseline_policy, + time_period=2026, + ) + .model_dump(mode="json") + ) + + assert sim_options["breakdown"] == "local_areas" + assert sim_options["region"] == "country/scotland" + + @pytest.mark.parametrize("mode", ["python", "json"]) + def test__given_no_breakdown__sends_no_breakdown_field(self, mode): + sim_options = ( + EconomyService() + ._setup_sim_options( + setup_options_for("uk", "uk"), + reform_policy=self.test_reform_policy, + baseline_policy=self.test_current_law_baseline_policy, + time_period=2026, + ) + .model_dump(mode=mode) + ) + + assert "breakdown" not in sim_options + class TestSetupRegion: """Tests for _setup_region method. @@ -2863,6 +3072,85 @@ def test__given_custom_dataset__rejects_it(self, dataset): time_period=MOCK_TIME_PERIOD, ) + class TestLocalAreaBreakdownSelection: + @pytest.mark.parametrize( + "region", + [ + "uk", + "country/england", + "country/scotland", + "country/wales", + "country/ni", + ], + ) + def test__given_uk_or_uk_country__accepts_breakdown(self, region): + gateway = MagicMock() + + setup_options = EconomyService( + simulation_entrypoint_=gateway + )._build_economic_impact_setup_options( + uk_breakdown_request(region=region), time_period=MOCK_TIME_PERIOD + ) + + assert setup_options.options == {"breakdown": "local_areas"} + assert setup_options.region == region + assert setup_options.options_hash.startswith( + "[breakdown=local_areas&dataset=default&" + ) + + @pytest.mark.parametrize( + ("country_id", "region", "target", "message"), + [ + ("us", "us", "general", "only available for UK"), + ("us", "state/ca", "general", "only available for UK"), + ( + "uk", + "constituency/Aldershot", + "general", + "region uk or a UK country", + ), + ( + "uk", + "local_authority/E07000092", + "general", + "region uk or a UK country", + ), + ("uk", "scotland", "general", "region uk or a UK country"), + ("uk", "country/", "general", "region uk or a UK country"), + ("uk", "uk", "cliff", "target=cliff"), + ("uk", "country/wales", "cliff", "target=cliff"), + ], + ) + def test__given_unsupported_request__rejects_breakdown( + self, country_id, region, target, message + ): + gateway = MagicMock() + + with pytest.raises(ValueError, match=message): + EconomyService( + simulation_entrypoint_=gateway + )._build_economic_impact_setup_options( + uk_breakdown_request( + country_id=country_id, region=region, target=target + ), + time_period=MOCK_TIME_PERIOD, + ) + + # Rejected before the worker's SPM capability is fetched. + gateway.get_spm_capability.assert_not_called() + + def test__given_unknown_breakdown_option__rejects_it(self): + request = replace( + uk_breakdown_request(), options={"breakdown": "constituencies"} + ) + + with pytest.raises(ValueError, match="Unknown breakdown: 'constituencies'"): + EconomyService( + simulation_entrypoint_=MagicMock() + )._build_economic_impact_setup_options( + request, time_period=MOCK_TIME_PERIOD + ) + class TestValidateUsRegion: """Tests for the _validate_us_region method.""" diff --git a/tests/unit/services/test_economy_service_request_plumbing.py b/tests/unit/services/test_economy_service_request_plumbing.py index e595f3ce9..77f9ea6c5 100644 --- a/tests/unit/services/test_economy_service_request_plumbing.py +++ b/tests/unit/services/test_economy_service_request_plumbing.py @@ -17,6 +17,7 @@ ReformImpactCache, ReformImpactIdentity, ReformImpactScope, + _like_matches, reform_impact_id, ) from policyengine_api.services.budget_window_cache import BudgetWindowCache @@ -385,6 +386,139 @@ def test_budget_window_poll_reports_running_years( ) +UK_BUNDLE_HASH_FIELDS = ( + f"dataset=default&model_version={MOCK_MODEL_VERSION}" + f"&policyengine_version={MOCK_POLICYENGINE_VERSION}" +) +UK_NATIONAL_LOOKUP_HASH = f"[{UK_BUNDLE_HASH_FIELDS}]" +UK_BREAKDOWN_LOOKUP_HASH = f"[breakdown=local_areas&{UK_BUNDLE_HASH_FIELDS}]" + + +def _stored_options_hash(lookup_hash: str) -> str: + """Return the hash a submission stores once its worker app is resolved.""" + return f"{lookup_hash[:-1]}&runtime_app_name={MOCK_RESOLVED_APP_NAME}]" + + +@pytest.fixture +def uk_package_versions(mock_country_package_versions): + """Give the UK the mocked country model version the US fixtures use.""" + mock_country_package_versions["uk"] = MOCK_MODEL_VERSION + + +def _uk_request(options: dict, region: str = "uk") -> EconomicImpactRequest: + return EconomicImpactRequest( + country_id="uk", + policy_id=MOCK_POLICY_ID, + baseline_policy_id=MOCK_BASELINE_POLICY_ID, + region=region, + dataset="default", + options=options, + api_version=MOCK_API_VERSION, + ) + + +def _submitted_payload_fields(payload: dict) -> dict: + """Return a submitted payload without its metadata and telemetry.""" + return { + key: value + for key, value in payload.items() + if key not in {"_metadata", "_telemetry"} + } + + +@pytest.mark.usefixtures("uk_package_versions") +def test_uk_breakdown_submission_has_its_own_hash_and_sends_breakdown( + economy_service, mock_simulation_entrypoint +): + result = economy_service.get_economic_impact( + _uk_request({"breakdown": "local_areas"}), time_period="2026" + ) + + assert result == EconomicImpactResult.computing() + payload = mock_simulation_entrypoint.run.call_args.args[0] + assert _submitted_payload_fields(payload) == { + "spm": None, + "country": "uk", + "scope": "macro", + "reform": {"sample_param": {"2024-01-01.2100-12-31": 15}}, + "baseline": {}, + "time_period": "2026", + "include_cliffs": False, + "region": "uk", + "model_version": MOCK_MODEL_VERSION, + "policyengine_version": MOCK_POLICYENGINE_VERSION, + "breakdown": "local_areas", + } + stored = economy_service._reform_impacts.get_recent_reform_impacts(5) + assert [impact.options_hash for impact in stored] == [ + _stored_options_hash(UK_BREAKDOWN_LOOKUP_HASH) + ] + assert stored[0].options_json == {"breakdown": "local_areas"} + + +@pytest.mark.usefixtures("uk_package_versions") +def test_uk_national_submission_sends_no_breakdown_and_keeps_its_hash( + economy_service, mock_simulation_entrypoint +): + economy_service.get_economic_impact(_uk_request({}), time_period="2026") + + payload = mock_simulation_entrypoint.run.call_args.args[0] + assert "breakdown" not in payload + stored = economy_service._reform_impacts.get_recent_reform_impacts(5) + assert [impact.options_hash for impact in stored] == [ + _stored_options_hash(UK_NATIONAL_LOOKUP_HASH) + ] + + +@pytest.mark.usefixtures("uk_package_versions") +def test_national_and_breakdown_hashes_never_match_each_others_lookup( + economy_service, +): + national, breakdown = ( + economy_service._build_economic_impact_setup_options( + _uk_request(options), time_period="2026" + ).options_hash + for options in ({}, {"breakdown": "local_areas"}) + ) + assert (national, breakdown) == ( + UK_NATIONAL_LOOKUP_HASH, + UK_BREAKDOWN_LOOKUP_HASH, + ) + + for lookup_hash, other_hash in ((national, breakdown), (breakdown, national)): + pattern = economy_service._build_options_hash_lookup_pattern(lookup_hash) + assert _like_matches(_stored_options_hash(lookup_hash), pattern) + assert not _like_matches(other_hash, pattern) + assert not _like_matches(_stored_options_hash(other_hash), pattern) + + +@pytest.mark.usefixtures("uk_package_versions") +def test_a_breakdown_request_never_reuses_the_national_job( + economy_service, mock_simulation_entrypoint +): + mock_simulation_entrypoint.get_execution_id.side_effect = [ + "national-job", + "breakdown-job", + ] + economy_service.get_economic_impact(_uk_request({}), time_period="2026") + economy_service.get_economic_impact( + _uk_request({"breakdown": "local_areas"}), time_period="2026" + ) + # A repeated breakdown request polls the breakdown job it already started. + economy_service.get_economic_impact( + _uk_request({"breakdown": "local_areas"}), time_period="2026" + ) + + submitted = [ + call.args[0].get("breakdown") + for call in mock_simulation_entrypoint.run.call_args_list + ] + assert submitted == [None, "local_areas"] + mock_simulation_entrypoint.get_execution_by_id.assert_called_once_with( + "breakdown-job" + ) + + @pytest.mark.parametrize( ("bundle_fields", "expected"), [ From 60477d5233b86dbe5f9c660362b678c1c68e98a9 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:53:07 +0400 Subject: [PATCH 03/18] Send Northern Ireland to the simulation API as country/northern_ireland The Public API's UK metadata names Northern Ireland `country/ni` (`country.py`), but .py's UK region registry, in both 6.7.5 and 6.10.0 (`policyengine/countries/uk/regions.py`), builds the country regions as `country/{england,scotland,wales,northern_ireland}` and looks codes up exactly, with no aliases. The simulation executor's region normaliser leaves `country/ni` as it is, so a Northern Ireland request named a region the registry does not have. `_setup_region` now maps `country/ni` to `country/northern_ireland` for the UK. Only the simulation payload changes: the cache scope, the options hash and the stored record keep `country/ni`, so existing Northern Ireland cache entries are still found. Other UK regions are unchanged. Tests written first. Before the change, 2 failed: `_setup_region("uk", "country/ni")` and the built payload both returned `country/ni` instead of `country/northern_ireland`. The tests that other UK regions pass through unchanged passed before and after. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.fixed.md | 1 + policyengine_api/services/economy_service.py | 10 ++++-- tests/unit/services/test_economy_service.py | 37 ++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 changelog.d/uk-local-area-breakdown.fixed.md diff --git a/changelog.d/uk-local-area-breakdown.fixed.md b/changelog.d/uk-local-area-breakdown.fixed.md new file mode 100644 index 000000000..d7c7b1534 --- /dev/null +++ b/changelog.d/uk-local-area-breakdown.fixed.md @@ -0,0 +1 @@ +UK economy requests for Northern Ireland (`country/ni` in the Public API metadata) now send `country/northern_ireland`, the name .py's UK region registry uses, to the simulation API. The cached result keeps the `country/ni` region. diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 4a69c4d93..3905ee205 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -111,6 +111,8 @@ class ImpactStatus(Enum): # 422 from an entry deployment that does not know the field. LOCAL_AREA_BREAKDOWN_REFUSAL_STATUS_CODES = frozenset({400, 422}) UK_COUNTRY_REGION_PREFIX = "country/" +# UK regions whose public metadata name differs from .py's region registry code. +UK_REGISTRY_REGION_CODES = {"country/ni": "country/northern_ireland"} BUDGET_WINDOW_CLAIM_ATTEMPTS = 3 REFORM_IMPACT_START_CLAIM_ATTEMPTS = 3 # A budget-window progress message names this many computing years and counts @@ -1956,15 +1958,19 @@ def _with_policyengine_bundle( return response def _setup_region(self, country_id: str, region: str) -> str: - """Validate the region for the given country. + """Validate the region and return the code the simulation API expects. Assumes region has already been normalized (e.g., "ca" -> "state/ca"). - Raises ValueError for invalid regions. + Raises ValueError for invalid regions. The UK metadata names Northern + Ireland ``country/ni``, but .py's UK region registry names it + ``country/northern_ireland``, so the payload uses the registry's name. """ # For US regions, validate (skip validation for national "us") if country_id == "us" and region != "us": self._validate_us_region(region) + if country_id == "uk": + return UK_REGISTRY_REGION_CODES.get(region, region) return region def _validate_us_region(self, region: str) -> None: diff --git a/tests/unit/services/test_economy_service.py b/tests/unit/services/test_economy_service.py index 1daefe2ce..f1c42ce45 100644 --- a/tests/unit/services/test_economy_service.py +++ b/tests/unit/services/test_economy_service.py @@ -2959,6 +2959,22 @@ def test__given_no_breakdown__sends_no_breakdown_field(self, mode): assert "breakdown" not in sim_options + def test__given_uk_northern_ireland__sends_registry_code_but_keeps_scope( + self, + ): + setup_options = setup_options_for("uk", "country/ni") + + sim_options = EconomyService()._setup_sim_options( + setup_options, + reform_policy=self.test_reform_policy, + baseline_policy=self.test_current_law_baseline_policy, + time_period=2026, + ) + + assert sim_options.region == "country/northern_ireland" + # The cache scope keeps the public region name. + assert setup_options.region == "country/ni" + class TestSetupRegion: """Tests for _setup_region method. @@ -2983,6 +2999,27 @@ def test__given_us_national__returns_us(self): result = service._setup_region("us", "us") assert result == "us" + def test__given_uk_northern_ireland__returns_the_region_registry_code(self): + # API v1's metadata publishes country/ni; .py's UK region registry + # names the region country/northern_ireland. + service = EconomyService() + result = service._setup_region("uk", "country/ni") + assert result == "country/northern_ireland" + + @pytest.mark.parametrize( + "region", + [ + "uk", + "country/england", + "country/scotland", + "country/wales", + "country/northern_ireland", + "constituency/Aldershot", + ], + ) + def test__given_other_uk_region__returns_unchanged(self, region): + assert EconomyService()._setup_region("uk", region) == region + def test__given_prefixed_state_tx__returns_unchanged(self): service = EconomyService() result = service._setup_region("us", "state/tx") From 1af1a684671ec569bcefb4e2fd6925c3b73a87d9 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Wed, 21 Oct 2026 18:13:47 +0000 Subject: [PATCH 04/18] Update PolicyEngine bundle to 6.10.0 (cherry picked from commit f7552f3bfd2a0b499296433c5dda6d8d8a0a8275) --- ...date-policyengine-bundle-6.10.0.changed.md | 1 + docker/Dockerfile | 2 +- pyproject.toml | 2 +- uv.lock | 20 +++++++++---------- 4 files changed, 13 insertions(+), 12 deletions(-) create mode 100644 changelog.d/update-policyengine-bundle-6.10.0.changed.md diff --git a/changelog.d/update-policyengine-bundle-6.10.0.changed.md b/changelog.d/update-policyengine-bundle-6.10.0.changed.md new file mode 100644 index 000000000..981170394 --- /dev/null +++ b/changelog.d/update-policyengine-bundle-6.10.0.changed.md @@ -0,0 +1 @@ +Update the PolicyEngine bundle to 6.10.0. diff --git a/docker/Dockerfile b/docker/Dockerfile index f4f15fc5c..5dfd6ee5f 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -1,4 +1,4 @@ FROM python:3.12 # Match the API bundle in pyproject.toml; the bundle updater changes both pins. # The models extra pins the complete certified runtime package set. -RUN pip install "policyengine[models]==6.7.5" ipython +RUN pip install "policyengine[models]==6.10.0" ipython diff --git a/pyproject.toml b/pyproject.toml index ed101271e..6ffc73f85 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -47,7 +47,7 @@ dependencies = [ # versions certified by this PolicyEngine bundle. Cloud Run installs the # complete frozen environment from uv.lock; the generic image installs # this requirement directly. - "policyengine[models]==6.7.5", + "policyengine[models]==6.10.0", "pydantic", "pymysql", "python-dotenv", diff --git a/uv.lock b/uv.lock index 52ec2d144..a5f6f1540 100644 --- a/uv.lock +++ b/uv.lock @@ -2743,7 +2743,7 @@ wheels = [ [[package]] name = "policyengine" -version = "6.7.5" +version = "6.10.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "h5py" }, @@ -2755,9 +2755,9 @@ dependencies = [ { name = "pydantic" }, { name = "requests" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/dc/db/3c18644cc814dcfc1abcea6f58df67d012712d7c6b2d9c8e01b746c18dc0/policyengine-6.7.5.tar.gz", hash = "sha256:872457de79f51dc57f33efb85274cb461d14aa42017e39aabf9d6611a673b30a", size = 1465755, upload-time = "2026-10-18T22:31:35.177Z" } +sdist = { url = "https://files.pythonhosted.org/packages/af/97/b78da2200ea502a1ee624f1dd8d17edda467d96eaaf3d004509e544b9c08/policyengine-6.10.0.tar.gz", hash = "sha256:b9e621f23162c5ce99d004fcba0d34259442244bd59951e4bd19a1dfb1cc4e35", size = 1526638, upload-time = "2026-10-21T16:41:04.984Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/4f/21/8faef0956e0c9a0f4ac73d16e7aae7330088e25ed3b6f81323572c4b8440/policyengine-6.7.5-py3-none-any.whl", hash = "sha256:acb921a6cd78c5bd5692827d3830fad9225a25f7e93e5742b92b2909624f76d3", size = 508363, upload-time = "2026-10-18T22:31:33.068Z" }, + { url = "https://files.pythonhosted.org/packages/a9/77/c65f5f5bbe90d5443c608a8d35a4865a565c5d7b7de68ae3120270acf38d/policyengine-6.10.0-py3-none-any.whl", hash = "sha256:104270016f9967705d30111edaa0015c997d68a28cdd595ea949431a654b200b", size = 536520, upload-time = "2026-10-21T16:41:03.271Z" }, ] [package.optional-dependencies] @@ -2846,7 +2846,7 @@ requires-dist = [ { name = "mypy", marker = "extra == 'dev'", specifier = ">=1.15,<2" }, { name = "openai" }, { name = "packaging", specifier = ">=24,<27" }, - { name = "policyengine", extras = ["models"], specifier = "==6.7.5" }, + { name = "policyengine", extras = ["models"], specifier = "==6.10.0" }, { name = "policyengine-canada", specifier = "==0.96.3" }, { name = "policyengine-il", specifier = "==0.1.0" }, { name = "policyengine-ng", specifier = "==0.5.1" }, @@ -2902,7 +2902,7 @@ wheels = [ [[package]] name = "policyengine-core" -version = "3.33.0" +version = "3.33.3" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "dpath" }, @@ -2922,9 +2922,9 @@ dependencies = [ { name = "standard-imghdr" }, { name = "wheel" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/90/d7/70936abfab724399ea3d8db04bbd5808339ee1534819bf25ab46d00c8d32/policyengine_core-3.33.0.tar.gz", hash = "sha256:5c923828dc84a7a1ff8f6f407f7a704fe067246dd277ede04c15f409804b9c19", size = 674785, upload-time = "2026-10-10T09:43:12.12Z" } +sdist = { url = "https://files.pythonhosted.org/packages/17/af/e8449e8a488468e5ce28b6b9066a2372e05bc7971ad9a441eb4e6efbb740/policyengine_core-3.33.3.tar.gz", hash = "sha256:7da78ae9cc7b1d6833dc30651a0a4775edd2d5e205a73bbafcc0b15c988c880e", size = 693203, upload-time = "2026-10-12T20:53:17.299Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/6d/02/190e06717785ff974120a9136c6c5408a00beaa8cddd792f557a6bf974c4/policyengine_core-3.33.0-py3-none-any.whl", hash = "sha256:2e32d02a8910ab683a2b7600987904b351d058fc2bd5910a8ea6641438bcd789", size = 299495, upload-time = "2026-10-10T09:43:10.318Z" }, + { url = "https://files.pythonhosted.org/packages/c8/d6/253cc99d853e36996f264138f4f8a2c662047da3eda8e37ddf39959397fc/policyengine_core-3.33.3-py3-none-any.whl", hash = "sha256:5847d9938412095fe9848c2ce535838a7dc8fac9238d8fd9e6c9d0a0648e0888", size = 301905, upload-time = "2026-10-12T20:53:15.706Z" }, ] [[package]] @@ -2981,7 +2981,7 @@ otlp-grpc = [ [[package]] name = "policyengine-uk" -version = "2.130.1" +version = "2.132.6" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "microdf-python" }, @@ -2989,9 +2989,9 @@ dependencies = [ { name = "pydantic" }, { name = "tables" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/17/27/82a97f5ec344d4be34db17bd26fe35077f1fe22f15c99818514d704262e9/policyengine_uk-2.130.1.tar.gz", hash = "sha256:1ce0a924a847b4e3e8ea8ba9702a84b267a0490c84a55ef94da6ed2efd965b34", size = 2496922, upload-time = "2026-10-14T01:58:47.232Z" } +sdist = { url = "https://files.pythonhosted.org/packages/3f/88/bc06328a739b335d4ef5f2d07fd36cc6a3a5e0f6ec1b2c774a14e77e43a3/policyengine_uk-2.132.6.tar.gz", hash = "sha256:f9ce261d9881eee32fab96341c7080cd0c86717b40785b5cdaff44b07262a771", size = 2560464, upload-time = "2026-10-15T10:08:14.263Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/1a/21/88e8addba0eabd9a7d3a29fa5293e141feb405baf0432209387f9a1f467a/policyengine_uk-2.130.1-py3-none-any.whl", hash = "sha256:f8b1b8b917c9297ff66937811dbebd9a62d3b1796ce894c326e98051cac4c873", size = 3804651, upload-time = "2026-10-14T01:58:44.937Z" }, + { url = "https://files.pythonhosted.org/packages/11/f2/c003cd7a95cb499737a5b8af0d619ba4e759c8bc7e55a2bb5c7efa46abd1/policyengine_uk-2.132.6-py3-none-any.whl", hash = "sha256:cc54025f2a3bc54be4f8d024d1e6f11ed29fed75ca513b214f2d2963c5bf4354", size = 3882707, upload-time = "2026-10-15T10:08:11.572Z" }, ] [[package]] From 2c1f4c5f221c79941b16861df36326ec98de9486 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:58:30 +0400 Subject: [PATCH 05/18] Review microcosm_uk_2024_25 as the UK default dataset for .py 6.10.0 .py 6.10.0's UK release manifest names `microcosm_uk_2024_25` as its default dataset, so the v2 catalog extractor refused the installed bundle ("uk default dataset does not match the reviewed selection"), which is how `tests/integration/test_v2_catalog_installed.py` failed on #3991. `REVIEWED_DEFAULT_DATASETS["uk"]` is now `microcosm_uk_2024_25`. The catalog fixtures and the publication test name the same dataset. The installed-catalog test's exact counts are updated after review for policyengine-uk 2.132.6: 7 more variables, 1 more parameter node and parameter, 3 more parameter values, and 1,011 more regions, because the 6.10.0 UK registry lists its 650 constituencies and 361 local authorities now that the bundle routes them to a local-area release. Not test-first: this follows the version bump in the previous commit, and is checked by the installed-catalog test and the catalog unit tests, which pass. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- policyengine_api/data/v2/catalog/extraction.py | 2 +- tests/fixtures/v2_catalog.py | 4 ++-- tests/integration/test_v2_catalog_installed.py | 17 ++++++++++------- .../integration/test_v2_catalog_publication.py | 4 ++-- tests/unit/v2/test_catalog_extraction.py | 2 +- 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/policyengine_api/data/v2/catalog/extraction.py b/policyengine_api/data/v2/catalog/extraction.py index 0872eea09..6f1a14b60 100644 --- a/policyengine_api/data/v2/catalog/extraction.py +++ b/policyengine_api/data/v2/catalog/extraction.py @@ -39,7 +39,7 @@ ) REVIEWED_DEFAULT_DATASETS = { "us": "populace_us_2024", - "uk": "enhanced_frs_2024_25", + "uk": "microcosm_uk_2024_25", } CURRENT_LAW_IDS = {"us": 2, "uk": 1} METADATA_TIME_PERIODS = { diff --git a/tests/fixtures/v2_catalog.py b/tests/fixtures/v2_catalog.py index 45dc22016..77f5cbd20 100644 --- a/tests/fixtures/v2_catalog.py +++ b/tests/fixtures/v2_catalog.py @@ -93,7 +93,7 @@ def _model_source( model_name = f"policyengine-{country_id}" default_dataset = { "us": "populace_us_2024", - "uk": "enhanced_frs_2024_25", + "uk": "microcosm_uk_2024_25", }[country_id] default_uri = f"hf://policyengine/{country_id}/{default_dataset}.h5@fixture" version_holder = SimpleNamespace() @@ -163,7 +163,7 @@ def source_models( ) -> dict[str, SimpleNamespace]: """Return US and UK public-model fixtures.""" us_default_uri = "hf://policyengine/us/populace_us_2024.h5@fixture" - uk_default_uri = "hf://policyengine/uk/enhanced_frs_2024_25.h5@fixture" + uk_default_uri = "hf://policyengine/uk/microcosm_uk_2024_25.h5@fixture" return { "us": _model_source( country_id="us", diff --git a/tests/integration/test_v2_catalog_installed.py b/tests/integration/test_v2_catalog_installed.py index 3cb7f1d3a..787fba171 100644 --- a/tests/integration/test_v2_catalog_installed.py +++ b/tests/integration/test_v2_catalog_installed.py @@ -36,17 +36,20 @@ def test_installed_policyengine_catalog_is_complete_and_bounded() -> None: importlib_metadata.version(name) == expected for name, expected in expected_dependencies ) - # Counts for the 6.7.5 bundle's models: US 2.2.1 and UK 2.130.1. Keep + # Counts for the 6.10.0 bundle's models: US 2.2.1 and UK 2.132.6. Keep # this exact assertion so later package changes require explicit review. + # Since 6.10.0 the UK registry also lists its 650 constituencies and 361 + # local authorities, because the bundle routes them to a local-area + # release, so regions grow by 1,011 from 6.7.5's 826. assert catalog.entity_counts() == { "models": 2, "model_versions": 2, - "variables": 7_330, - "parameter_nodes": 29_306, - "parameters": 104_171, - "parameter_values": 1_194_357, + "variables": 7_337, + "parameter_nodes": 29_307, + "parameters": 104_172, + "parameter_values": 1_194_360, "datasets": 3, - "regions": 826, + "regions": 1_837, } for country_id in ("us", "uk"): @@ -74,7 +77,7 @@ def test_installed_policyengine_catalog_is_complete_and_bounded() -> None: "populace_us_2024_acs_local", } assert {dataset.name for dataset in catalog.country("uk").datasets} == { - "enhanced_frs_2024_25" + "microcosm_uk_2024_25" } england = next( region diff --git a/tests/integration/test_v2_catalog_publication.py b/tests/integration/test_v2_catalog_publication.py index 06e48f64c..25bc9178f 100644 --- a/tests/integration/test_v2_catalog_publication.py +++ b/tests/integration/test_v2_catalog_publication.py @@ -163,10 +163,10 @@ def test_publish_and_republish_preserve_complete_catalog( storage_path is None and not output for _, storage_path, output in datasets ) assert dict(region_defaults) == { - "country/england": "enhanced_frs_2024_25", + "country/england": "microcosm_uk_2024_25", "place/CA-44000": "populace_us_2024", "state/ca": "populace_us_ca_2024", - "uk": "enhanced_frs_2024_25", + "uk": "microcosm_uk_2024_25", "us": "populace_us_2024", } diff --git a/tests/unit/v2/test_catalog_extraction.py b/tests/unit/v2/test_catalog_extraction.py index e060c1d43..dda612df5 100644 --- a/tests/unit/v2/test_catalog_extraction.py +++ b/tests/unit/v2/test_catalog_extraction.py @@ -41,7 +41,7 @@ def test_extracts_canonical_policyengine_catalog_and_dataset_defaults() -> None: "populace_us_2024", "populace_us_ca_2024", } - assert {dataset.name for dataset in uk.datasets} == {"enhanced_frs_2024_25"} + assert {dataset.name for dataset in uk.datasets} == {"microcosm_uk_2024_25"} assert all( not dataset.is_output_dataset for dataset in (*us.datasets, *uk.datasets) ) From bb6f253918ff782897958e0bbde7d96ff942008d Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:01:56 +0400 Subject: [PATCH 06/18] Label the UK Microcosm dataset as Microcosm in the metadata With .py 6.10.0 the UK default dataset is `microcosm_uk_2024_25`, which matched no dataset family prefix, so the UK metadata's dataset option fell back to the generic label and title "Certified dataset". The `microcosm_` prefix now maps to "Microcosm" (title "Certified Microcosm dataset"), the label the US Microcosm datasets (`populace_*`) already use. `enhanced_frs_*` keeps "Enhanced FRS". Tests written first. With the 6.10.0 bundle installed, 3 failed before the change: `microcosm_uk_2024_25` and `microcosm_uk_2024_25_dense` were labelled "Certified dataset", and the UK metadata's default dataset label was "Certified dataset" instead of "Microcosm". The v2 metadata route integration test is updated to expect the same label. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.changed.md | 1 + policyengine_api/dataset_display.py | 2 ++ tests/integration/test_v2_metadata_routes.py | 2 +- tests/unit/services/test_metadata_service.py | 2 +- tests/unit/test_constants.py | 2 ++ 5 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 changelog.d/uk-local-area-breakdown.changed.md diff --git a/changelog.d/uk-local-area-breakdown.changed.md b/changelog.d/uk-local-area-breakdown.changed.md new file mode 100644 index 000000000..745430de9 --- /dev/null +++ b/changelog.d/uk-local-area-breakdown.changed.md @@ -0,0 +1 @@ +With .py 6.10.0, the UK default dataset is `microcosm_uk_2024_25`: the v2 catalog extractor accepts it as the reviewed UK default, and the metadata labels it "Microcosm", as it does the US Microcosm dataset, instead of the generic "Certified dataset". diff --git a/policyengine_api/dataset_display.py b/policyengine_api/dataset_display.py index d092fa7e8..a37e57bc6 100644 --- a/policyengine_api/dataset_display.py +++ b/policyengine_api/dataset_display.py @@ -3,7 +3,9 @@ from __future__ import annotations DATASET_FAMILY_DISPLAY_LABELS = ( + # US Microcosm datasets are named populace_*; UK ones microcosm_*. ("populace_", "Microcosm"), + ("microcosm_", "Microcosm"), ("enhanced_frs_", "Enhanced FRS"), ) DEFAULT_DATASET_DISPLAY_LABEL = "Certified dataset" diff --git a/tests/integration/test_v2_metadata_routes.py b/tests/integration/test_v2_metadata_routes.py index a3d29d93b..326eb78d9 100644 --- a/tests/integration/test_v2_metadata_routes.py +++ b/tests/integration/test_v2_metadata_routes.py @@ -119,7 +119,7 @@ def _catalog_counts(engine: Engine) -> tuple[int, ...]: @pytest.mark.parametrize( ("country_id", "current_law_id", "dataset_label"), - [("us", 2, "Microcosm"), ("uk", 1, "Enhanced FRS")], + [("us", 2, "Microcosm"), ("uk", 1, "Microcosm")], ) def test_postgres_resource_collections_are_separate_and_read_only( published_engine: Engine, diff --git a/tests/unit/services/test_metadata_service.py b/tests/unit/services/test_metadata_service.py index 8d72d8eb2..c5418cff9 100644 --- a/tests/unit/services/test_metadata_service.py +++ b/tests/unit/services/test_metadata_service.py @@ -144,7 +144,7 @@ def test_default_dataset_matches_policyengine_bundle(self, country_id): ("country_id", "expected_label"), [ ("us", "Microcosm"), - ("uk", "Enhanced FRS"), + ("uk", "Microcosm"), ], ) def test_default_dataset_uses_user_facing_label(self, country_id, expected_label): diff --git a/tests/unit/test_constants.py b/tests/unit/test_constants.py index 3861b0c30..1eb2b38ff 100644 --- a/tests/unit/test_constants.py +++ b/tests/unit/test_constants.py @@ -24,6 +24,8 @@ class TestBundleDatasetDisplayLabels: ("dataset_name", "expected_label"), [ ("populace_us_2024", "Microcosm"), + ("microcosm_uk_2024_25", "Microcosm"), + ("microcosm_uk_2024_25_dense", "Microcosm"), ("enhanced_frs_2024_25", "Enhanced FRS"), ("future_dataset", "Certified dataset"), (None, "Certified dataset"), From 91d0daec0a323b6f297e289ce47628b63f1029dd Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:35:54 +0400 Subject: [PATCH 07/18] Accept only uk and the published UK country codes for a breakdown `_validate_breakdown` accepted any text after a lower-case `country/`, so `country/%20` (decoded to `country/ `), `country/NI` and `country/france` passed. The simulation API then refused them (a 502 here, where 400 is documented) or the job failed and that failure was cached for 30 days. The region must now be exactly one of `LOCAL_AREA_BREAKDOWN_REGIONS`: `uk`, the four UK country regions the metadata publishes (`country/england`, `country/scotland`, `country/wales`, `country/ni`), or `country/northern_ireland`, the code .py's UK region registry uses for Northern Ireland. Every other value, including other casings and surrounding spaces, is a 400, and the message lists the accepted countries. The UK country list moves from an inline literal in `country.py` to `constants.UK_COUNTRY_REGIONS`, which the metadata and this check both read, and the Northern Ireland mapping moves to `constants.UK_REGION_REGISTRY_CODES`. The UK metadata's region list is unchanged. The query description, `docs/canonical-spm.md` and the changelog name the accepted regions instead of `country/*`. Tests written first. Before the change, 9 failed for the expected reason: `country/ `, `country/%20`, `country/NI`, `country/france` and `country/scotland ` raised no ValueError (7 cases across the setup and service tests), and the route returned 200 instead of 400 for `region=country/france` and for a raw `region=country/%20`. The new service test that an invalid breakdown is refused before any reform-impact cache call, worker lookup or submission failed for `country/france` and `country/ ` and passed for a constituency. `Country/scotland`, `UK`, ` uk` and the accepted regions already behaved correctly and pin that. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.added.md | 2 +- docs/canonical-spm.md | 8 ++-- policyengine_api/constants.py | 11 ++++++ policyengine_api/country.py | 9 +++-- policyengine_api/query_parameters.py | 3 +- policyengine_api/services/economy_service.py | 35 +++++++++++------- tests/unit/routes/test_economy_spm_query.py | 24 +++++++++++- tests/unit/services/test_economy_service.py | 39 ++++++++++++++++++++ 8 files changed, 107 insertions(+), 24 deletions(-) diff --git a/changelog.d/uk-local-area-breakdown.added.md b/changelog.d/uk-local-area-breakdown.added.md index fcced178f..9f2d9fadb 100644 --- a/changelog.d/uk-local-area-breakdown.added.md +++ b/changelog.d/uk-local-area-breakdown.added.md @@ -1 +1 @@ -The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or `country/*`, and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. +The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or one UK country (`country/england`, `country/scotland`, `country/wales` or `country/ni`), and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. diff --git a/docs/canonical-spm.md b/docs/canonical-spm.md index 8874966e9..8f2bedc49 100644 --- a/docs/canonical-spm.md +++ b/docs/canonical-spm.md @@ -329,9 +329,11 @@ parser. local-authority impacts only. It is a calculation option, so the request has its own options hash and its own cached result. Any other value returns HTTP 400, and requests without `breakdown` keep their existing options hash. It is -accepted only for the UK, with `region` `uk` or a UK country (`country/*`), -and not with `target=cliff`; other requests return HTTP 400 before anything is -submitted. The API sends `"breakdown": "local_areas"` to the simulation API, +accepted only for the UK, with `region` exactly `uk` or one UK country +(`country/england`, `country/scotland`, `country/wales` or `country/ni`, or +.py's `country/northern_ireland`), and not with `target=cliff`; other +requests, including other spellings of those regions, return HTTP 400 before +anything is read from the cache or submitted. The API sends `"breakdown": "local_areas"` to the simulation API, which computes the impacts in a run separate from the national result, and returns its result (`kind: "local_areas"`) like any other economy result. If the simulation API refuses the request with HTTP 400 or 422, because the diff --git a/policyengine_api/constants.py b/policyengine_api/constants.py index d78a7fb2e..bc67887bb 100644 --- a/policyengine_api/constants.py +++ b/policyengine_api/constants.py @@ -284,6 +284,17 @@ def get_report_output_cache_version(country_id: str) -> str: ], } +# The UK country regions the metadata publishes, as (region name, label). +UK_COUNTRY_REGIONS = ( + ("country/england", "England"), + ("country/scotland", "Scotland"), + ("country/wales", "Wales"), + ("country/ni", "Northern Ireland"), +) +# UK regions whose metadata name differs from the code .py's UK region +# registry uses, mapped to that code. +UK_REGION_REGISTRY_CODES = {"country/ni": "country/northern_ireland"} + # Modal simulation API status values MODAL_EXECUTION_STATUS_SUBMITTED = "submitted" MODAL_EXECUTION_STATUS_RUNNING = "running" diff --git a/policyengine_api/country.py b/policyengine_api/country.py index 8f6ab79de..8c04362b4 100644 --- a/policyengine_api/country.py +++ b/policyengine_api/country.py @@ -28,6 +28,7 @@ from policyengine_core.types import ArrayLike from policyengine_api.constants import ( + UK_COUNTRY_REGIONS, get_bundle_default_dataset_option, ) from policyengine_api.data.congressional_districts import ( @@ -203,10 +204,10 @@ def build_microsimulation_options(self) -> dict: constituency_names = constituency_names.sort_values("name") region = [ {"name": "uk", "label": "the UK", "type": "national"}, - {"name": "country/england", "label": "England", "type": "country"}, - {"name": "country/scotland", "label": "Scotland", "type": "country"}, - {"name": "country/wales", "label": "Wales", "type": "country"}, - {"name": "country/ni", "label": "Northern Ireland", "type": "country"}, + *( + {"name": name, "label": label, "type": "country"} + for name, label in UK_COUNTRY_REGIONS + ), ] region.extend( { diff --git a/policyengine_api/query_parameters.py b/policyengine_api/query_parameters.py index dc99eeb11..9b362ad33 100644 --- a/policyengine_api/query_parameters.py +++ b/policyengine_api/query_parameters.py @@ -173,7 +173,8 @@ class AnnualEconomyQuery(EconomyQuery): description=( "local_areas computes only the UK constituency and local-authority " "impacts, in a run separate from the national result. UK only, " - "with region uk or country/*, and not with target=cliff" + "with region uk or one UK country (country/england, country/scotland, " + "country/wales or country/ni), and not with target=cliff" ), ) diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 3905ee205..bb8faf3e6 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -20,6 +20,8 @@ EXECUTION_STATUSES_PENDING, EXECUTION_STATUSES_SUCCESS, POLICYENGINE_VERSION, + UK_COUNTRY_REGIONS, + UK_REGION_REGISTRY_CODES, get_economy_impact_cache_version, ) from policyengine_api.data.congressional_districts import ( @@ -110,9 +112,15 @@ class ImpactStatus(Enum): # 400 from a gateway whose routed .py release does not declare breakdowns, and # 422 from an entry deployment that does not know the field. LOCAL_AREA_BREAKDOWN_REFUSAL_STATUS_CODES = frozenset({400, 422}) -UK_COUNTRY_REGION_PREFIX = "country/" -# UK regions whose public metadata name differs from .py's region registry code. -UK_REGISTRY_REGION_CODES = {"country/ni": "country/northern_ireland"} +# The regions a breakdown can cover: the UK, each UK country the metadata +# publishes, and .py's own code for a country the metadata names differently. +LOCAL_AREA_BREAKDOWN_REGIONS = frozenset( + { + "uk", + *(name for name, _label in UK_COUNTRY_REGIONS), + *UK_REGION_REGISTRY_CODES.values(), + } +) BUDGET_WINDOW_CLAIM_ATTEMPTS = 3 REFORM_IMPACT_START_CLAIM_ATTEMPTS = 3 # A budget-window progress message names this many computing years and counts @@ -1056,10 +1064,12 @@ def _build_economic_impact_setup_options( def _validate_breakdown(impact_request: EconomicImpactRequest) -> None: """Reject a local-area breakdown the simulation API has no run for. - The breakdown covers the whole UK or one UK country, so it needs region - ``uk`` or ``country/*``: a constituency or local authority is already a - single area. It computes no cliff impacts, so ``target=cliff`` is - refused too. Raise ValueError, which the route returns as 400. + The breakdown covers the whole UK or one UK country, so the region must + be exactly one of ``LOCAL_AREA_BREAKDOWN_REGIONS``: a constituency or + local authority is already a single area, and any other spelling would + fail in the simulation API instead. It computes no cliff impacts, so + ``target=cliff`` is refused too. Raise ValueError, which the route + returns as 400. """ breakdown = impact_request.options.get("breakdown") if breakdown is None: @@ -1071,14 +1081,11 @@ def _validate_breakdown(impact_request: EconomicImpactRequest) -> None: msg = "breakdown=local_areas is only available for UK economy calculations" raise ValueError(msg) region = impact_request.region - country_name = region.removeprefix(UK_COUNTRY_REGION_PREFIX) - is_uk_country = ( - country_name != region and bool(country_name) and "/" not in country_name - ) - if region != "uk" and not is_uk_country: + if region not in LOCAL_AREA_BREAKDOWN_REGIONS: + countries = ", ".join(name for name, _label in UK_COUNTRY_REGIONS) msg = ( "breakdown=local_areas needs region uk or a UK country " - f"(country/...), not {region!r}" + f"({countries}), not {region!r}" ) raise ValueError(msg) if impact_request.target == "cliff": @@ -1970,7 +1977,7 @@ def _setup_region(self, country_id: str, region: str) -> str: self._validate_us_region(region) if country_id == "uk": - return UK_REGISTRY_REGION_CODES.get(region, region) + return UK_REGION_REGISTRY_CODES.get(region, region) return region def _validate_us_region(self, region: str) -> None: diff --git a/tests/unit/routes/test_economy_spm_query.py b/tests/unit/routes/test_economy_spm_query.py index a20ac7e30..1f7d2ea39 100644 --- a/tests/unit/routes/test_economy_spm_query.py +++ b/tests/unit/routes/test_economy_spm_query.py @@ -463,13 +463,18 @@ def economy_service_http(monkeypatch): [("region", "local_authority/E07000092"), ("time_period", "2026")], "region uk or a UK country", ), + ( + UK_ANNUAL_PATH, + [("region", "country/france"), ("time_period", "2026")], + "region uk or a UK country", + ), ( UK_ANNUAL_PATH, [*UK_ANNUAL_QUERY, ("target", "cliff")], "target=cliff", ), ], - ids=["us", "constituency", "local-authority", "cliff"], + ids=["us", "constituency", "local-authority", "france", "cliff"], ) def test_annual_route_rejects_an_unsupported_breakdown_with_400( economy_service_http, path, query, message @@ -483,6 +488,23 @@ def test_annual_route_rejects_an_unsupported_breakdown_with_400( gateway.run.assert_not_called() +def test_annual_route_rejects_a_blank_country_breakdown_with_400( + economy_service_http, +): + client, gateway = economy_service_http + + # A raw query string, so %20 reaches Flask encoded and is decoded to a space. + response = client.get( + UK_ANNUAL_PATH, + query_string="region=country/%20&time_period=2026&breakdown=local_areas", + ) + + assert response.status_code == 400 + assert "region uk or a UK country" in response.get_json()["message"] + assert "'country/ '" in response.get_json()["message"] + gateway.run.assert_not_called() + + def test_annual_route_returns_502_when_the_simulation_api_refuses_a_breakdown( economy_service_http, ): diff --git a/tests/unit/services/test_economy_service.py b/tests/unit/services/test_economy_service.py index f1c42ce45..6648b3605 100644 --- a/tests/unit/services/test_economy_service.py +++ b/tests/unit/services/test_economy_service.py @@ -1328,6 +1328,35 @@ def test__given_local_area_breakdown__submits_breakdown_to_simulation_api( assert sim_params["region"] == "uk" assert sim_params["include_cliffs"] is False + @pytest.mark.usefixtures( + "mock_policyengine_version", + "mock_policy_service", + "mock_logger", + "mock_datetime", + "mock_submission_claim_id", + ) + @pytest.mark.parametrize( + "region", ["constituency/Aldershot", "country/france", "country/ "] + ) + def test__given_invalid_breakdown__refuses_it_before_any_cache_read( + self, + economy_service, + mock_country_package_versions, + mock_reform_impacts_service, + mock_simulation_entrypoint, + region, + ): + mock_country_package_versions["uk"] = "2.7.8" + + with pytest.raises(ValueError, match="region uk or a UK country"): + economy_service.get_economic_impact( + uk_breakdown_request(region=region), time_period=MOCK_TIME_PERIOD + ) + + assert mock_reform_impacts_service.mock_calls == [] + mock_simulation_entrypoint.resolve_app_name.assert_not_called() + mock_simulation_entrypoint.run.assert_not_called() + @pytest.mark.usefixtures( "mock_policyengine_version", "mock_policy_service", @@ -3118,6 +3147,7 @@ class TestLocalAreaBreakdownSelection: "country/scotland", "country/wales", "country/ni", + "country/northern_ireland", ], ) def test__given_uk_or_uk_country__accepts_breakdown(self, region): @@ -3154,6 +3184,15 @@ def test__given_uk_or_uk_country__accepts_breakdown(self, region): ), ("uk", "scotland", "general", "region uk or a UK country"), ("uk", "country/", "general", "region uk or a UK country"), + # A query value of country/%20 reaches the service decoded. + ("uk", "country/ ", "general", "region uk or a UK country"), + ("uk", "country/%20", "general", "region uk or a UK country"), + ("uk", "country/NI", "general", "region uk or a UK country"), + ("uk", "country/france", "general", "region uk or a UK country"), + ("uk", "Country/scotland", "general", "region uk or a UK country"), + ("uk", "country/scotland ", "general", "region uk or a UK country"), + ("uk", "UK", "general", "region uk or a UK country"), + ("uk", " uk", "general", "region uk or a UK country"), ("uk", "uk", "cliff", "target=cliff"), ("uk", "country/wales", "cliff", "target=cliff"), ], From 1073f1ebeb9aa5f423d4b03b45f3b4835525dcb3 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:37:56 +0400 Subject: [PATCH 08/18] Test that a completed breakdown is stored and returned unchanged A route-level test with the real economy service, in-memory caches and a mocked simulation API submits a breakdown, then polls it to completion (`_handle_execution_state`), then reads it back from storage (`_handle_completed_impact`) without polling again. Both answers carry `MOCK_LOCAL_AREA_BREAKDOWN_RESULT`'s fields unchanged, plus only the `policyengine_bundle` and `resolved_app_name` the service adds to every economy result. One record is stored, with status ok, the breakdown options and a hash that starts `[breakdown=local_areas&`. A national request for the same report then submits its own job instead of reading the breakdown. The fixture's mocked `resolve_app_name` now keeps the requested model version, as the real client does. It returned a fixed version before, so the stored hash never matched a later lookup and every request submitted again. These tests describe existing behaviour and passed when written; no service code changes. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- tests/unit/routes/test_economy_spm_query.py | 72 ++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/tests/unit/routes/test_economy_spm_query.py b/tests/unit/routes/test_economy_spm_query.py index 1f7d2ea39..c871237d7 100644 --- a/tests/unit/routes/test_economy_spm_query.py +++ b/tests/unit/routes/test_economy_spm_query.py @@ -9,6 +9,7 @@ from flask import Flask from policyengine_api import spm +from policyengine_api.libs.simulation_entrypoint import ModalSimulationExecution from policyengine_api.routes import economy_routes from policyengine_api.runtime_cache.core import CacheNamespace from policyengine_api.runtime_cache.fake import InMemoryCacheBackend @@ -16,6 +17,7 @@ from policyengine_api.services.budget_window_cache import BudgetWindowCache from policyengine_api.services.economy_service import EconomyService from policyengine_api.services.reform_impacts_service import ReformImpactsService +from tests.fixtures.libs.simulation_entrypoint import MOCK_LOCAL_AREA_BREAKDOWN_RESULT @pytest.fixture(params=[False, True], ids=["annual", "budget-window"]) @@ -426,7 +428,13 @@ def economy_service_http(monkeypatch): """ namespace = CacheNamespace("test", "api") gateway = Mock() - gateway.resolve_app_name.return_value = ("policyengine-simulation-test", "2.0.0") + # Like the real client, keep the requested model version. + gateway.resolve_app_name.side_effect = ( + lambda _country, version=None, policyengine_version=None: ( + "policyengine-simulation-test", + version or policyengine_version, + ) + ) gateway.get_execution_id.return_value = "submitted-job" policies = Mock() policies.get_policy_json.return_value = {} @@ -532,3 +540,65 @@ def test_annual_route_returns_502_when_the_simulation_api_refuses_a_breakdown( "result": None, } assert gateway.run.call_args.args[0]["breakdown"] == "local_areas" + + +def test_annual_route_stores_and_returns_a_completed_breakdown_unchanged( + economy_service_http, +): + """A finished breakdown job is polled once, stored, and then read back. + + The second request polls the job (``_handle_execution_state``); the third + reads the stored record (``_handle_completed_impact``) without polling. + """ + client, gateway = economy_service_http + breakdown_query = [*UK_ANNUAL_QUERY, ("breakdown", "local_areas")] + gateway.get_execution_by_id.return_value = ModalSimulationExecution( + job_id="submitted-job", + status="complete", + result=MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + resolved_app_name="policyengine-simulation-test", + ) + gateway.get_execution_status.side_effect = lambda execution: execution.status + gateway.get_execution_result.side_effect = lambda execution: execution.result + + submitted = client.get(UK_ANNUAL_PATH, query_string=breakdown_query) + polled = client.get(UK_ANNUAL_PATH, query_string=breakdown_query) + stored = client.get(UK_ANNUAL_PATH, query_string=breakdown_query) + + assert submitted.status_code == 200 + assert submitted.get_json()["status"] == "computing" + for response in (polled, stored): + assert response.status_code == 200, response.get_json() + body = response.get_json() + assert body["status"] == "ok" + result = body["result"] + # The breakdown's own fields come back unchanged; the service adds + # only the bundle identity it adds to every economy result. + assert { + key: result[key] for key in MOCK_LOCAL_AREA_BREAKDOWN_RESULT + } == MOCK_LOCAL_AREA_BREAKDOWN_RESULT + assert set(result) - set(MOCK_LOCAL_AREA_BREAKDOWN_RESULT) == { + "policyengine_bundle", + "resolved_app_name", + } + assert polled.get_json()["result"] == stored.get_json()["result"] + gateway.get_execution_by_id.assert_called_once_with("submitted-job") + + records = economy_routes.economy_service._reform_impacts.get_recent_reform_impacts( + 5 + ) + assert len(records) == 1 + assert records[0].status == "ok" + assert records[0].options_json == {"breakdown": "local_areas"} + assert records[0].options_hash.startswith("[breakdown=local_areas&") + assert records[0].reform_impact_json == stored.get_json()["result"] + + # The national request for the same report finds no stored result: it + # submits its own job instead of reading the breakdown. + national = client.get(UK_ANNUAL_PATH, query_string=UK_ANNUAL_QUERY) + + assert national.get_json()["status"] == "computing" + assert [call.args[0].get("breakdown") for call in gateway.run.call_args_list] == [ + "local_areas", + None, + ] From 2716435032b43da343ab749d76a9e22bef797a33 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:38:52 +0400 Subject: [PATCH 09/18] Share one type for the local_areas breakdown value `services/economic_impact_request.py`, the request type the economy routes pass to the service, now defines `LocalAreaBreakdown = Literal["local_areas"]` and `LOCAL_AREA_BREAKDOWN`. `AnnualEconomyQuery`, `SimulationOptions` and `_breakdown_refusal`'s parameter use the alias, and the service imports the constant instead of defining its own. The module imports nothing else from the package, so `query_parameters.py` can import it without loading the service. No behaviour change: the query, payload and OpenAPI tests pass unchanged. mypy on `economy_service.py` reports the same 11 errors it did before this branch. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- policyengine_api/query_parameters.py | 3 ++- policyengine_api/services/economic_impact_request.py | 6 ++++++ policyengine_api/services/economy_service.py | 11 +++++++---- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/policyengine_api/query_parameters.py b/policyengine_api/query_parameters.py index 9b362ad33..25ef95898 100644 --- a/policyengine_api/query_parameters.py +++ b/policyengine_api/query_parameters.py @@ -28,6 +28,7 @@ from policyengine_api.data.v2.catalog.catalog_selection import ( validate_policyengine_version, ) +from policyengine_api.services.economic_impact_request import LocalAreaBreakdown from policyengine_api.spm import SPMSelection from policyengine_api.utils.budget_window import ( BUDGET_WINDOW_MAX_END_YEAR, @@ -168,7 +169,7 @@ class AnnualEconomyQuery(EconomyQuery): time_period: EconomyYear target: Literal["general", "cliff"] = "general" - breakdown: Literal["local_areas"] | None = Field( + breakdown: LocalAreaBreakdown | None = Field( default=None, description=( "local_areas computes only the UK constituency and local-authority " diff --git a/policyengine_api/services/economic_impact_request.py b/policyengine_api/services/economic_impact_request.py index 506311302..dc5cb6f6e 100644 --- a/policyengine_api/services/economic_impact_request.py +++ b/policyengine_api/services/economic_impact_request.py @@ -3,6 +3,12 @@ from dataclasses import dataclass from typing import Literal +# The one breakdown the annual economy route can request: the UK +# constituency and local-authority impacts, computed apart from the national +# result. The query model, the simulation payload and the service share it. +LocalAreaBreakdown = Literal["local_areas"] +LOCAL_AREA_BREAKDOWN: LocalAreaBreakdown = "local_areas" + @dataclass(frozen=True) class EconomicImpactRequest: diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index bb8faf3e6..34c9368af 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -68,7 +68,11 @@ BudgetWindowCache, BudgetWindowCacheState, ) -from policyengine_api.services.economic_impact_request import EconomicImpactRequest +from policyengine_api.services.economic_impact_request import ( + LOCAL_AREA_BREAKDOWN, + EconomicImpactRequest, + LocalAreaBreakdown, +) from policyengine_api.services.policy_service import PolicyService from policyengine_api.services.reform_impacts_service import ( ReformImpactExecution, @@ -107,7 +111,6 @@ class ImpactStatus(Enum): BUDGET_WINDOW_MAX_YEARS = budget_window_utils.BUDGET_WINDOW_MAX_YEARS BUDGET_WINDOW_MAX_END_YEAR = budget_window_utils.BUDGET_WINDOW_MAX_END_YEAR BUDGET_WINDOW_SUBMISSION_VALIDATION_ERROR_STATUS_CODES = {400, 422} -LOCAL_AREA_BREAKDOWN = "local_areas" # Statuses with which the simulation API refuses a breakdown it cannot run: # 400 from a gateway whose routed .py release does not declare breakdowns, and # 422 from an entry deployment that does not know the field. @@ -165,7 +168,7 @@ class SimulationOptions(BaseModel): model_version: str | None = None policyengine_version: str | None = None # Left out of the payload unless set, so national submissions are unchanged. - breakdown: Literal["local_areas"] | None = Field( + breakdown: LocalAreaBreakdown | None = Field( default=None, exclude_if=lambda value: value is None ) @@ -1096,7 +1099,7 @@ def _breakdown_refusal( self, error: httpx.HTTPStatusError, *, - breakdown: str | None, + breakdown: LocalAreaBreakdown | None, ) -> EconomicImpactResult | None: """Return an error result when the simulation API refuses a breakdown. From 9fc9da5e125151d3823fb0555db78064ca1a5a5a Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:39:57 +0400 Subject: [PATCH 10/18] Name each rejected field in every simulation API 422 message The per-field formatting of a request-validation body (`detail` as a list of `{loc, msg}`) moves from the breakdown path into the shared `_build_submission_error_message`, so budget-window refusals read the same way: `body.window_size: Input should be less than or equal to 75` instead of the Python representation of the list, which also quoted the rejected input. Other bodies keep their existing message (the first of `detail`, `message` or `error`, then the response text, then the exception text). Tests written first. Before the change, 2 failed for the expected reason: the builder and a budget-window 422 refusal both returned the list's Python representation. The breakdown refusal test passes unchanged. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- .../uk-local-area-breakdown.changed.md | 2 +- policyengine_api/services/economy_service.py | 51 +++++++++------- tests/unit/services/test_economy_service.py | 61 +++++++++++++++++++ 3 files changed, 90 insertions(+), 24 deletions(-) diff --git a/changelog.d/uk-local-area-breakdown.changed.md b/changelog.d/uk-local-area-breakdown.changed.md index 745430de9..2e38ab821 100644 --- a/changelog.d/uk-local-area-breakdown.changed.md +++ b/changelog.d/uk-local-area-breakdown.changed.md @@ -1 +1 @@ -With .py 6.10.0, the UK default dataset is `microcosm_uk_2024_25`: the v2 catalog extractor accepts it as the reviewed UK default, and the metadata labels it "Microcosm", as it does the US Microcosm dataset, instead of the generic "Certified dataset". +With .py 6.10.0, the UK default dataset is `microcosm_uk_2024_25`: the v2 catalog extractor accepts it as the reviewed UK default, and the metadata labels it "Microcosm", as it does the US Microcosm dataset, instead of the generic "Certified dataset". When the simulation API refuses a budget-window submission with a request-validation (422) body, the error message now names each field and its problem (for example `body.window_size: Input should be less than or equal to 75`) instead of quoting the raw error list, as breakdown refusals do. diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 34c9368af..87f69a613 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -868,12 +868,22 @@ def _start_budget_window_batch( return self._simulation_gateway.run_budget_window_batch(sim_params) def _build_submission_error_message(self, error: httpx.HTTPStatusError) -> str: + """Return the reason in a simulation API refusal. + + A request-validation body (``detail`` as a list) names each field and + its problem, without echoing the rejected input. Otherwise the first + of ``detail``, ``message`` or ``error``, then the response text, then + the exception text. + """ try: response_json = error.response.json() except ValueError: response_json = None if isinstance(response_json, dict): + field_reasons = self._validation_field_reasons(response_json.get("detail")) + if field_reasons: + return field_reasons for key in ("detail", "message", "error"): value = response_json.get(key) if value: @@ -1121,33 +1131,28 @@ def _breakdown_refusal( return EconomicImpactResult.error( message=( "The simulation API refused the local-area breakdown request " - f"(HTTP {status_code}): {self._refusal_reason(error)}" + f"(HTTP {status_code}): {self._build_submission_error_message(error)}" ) ) - def _refusal_reason(self, error: httpx.HTTPStatusError) -> str: - """Return the reason in a refusal, naming each field of a 422 body.""" - try: - body = error.response.json() - except ValueError: - body = None - details = body.get("detail") if isinstance(body, dict) else None - if isinstance(details, list): - reasons = [ - ": ".join( - part - for part in ( - ".".join(str(item) for item in detail.get("loc", [])), - str(detail.get("msg", "")), - ) - if part + @staticmethod + def _validation_field_reasons(details: object) -> str | None: + """Join each ``loc: msg`` of a request-validation ``detail`` list.""" + if not isinstance(details, list): + return None + reasons = [ + ": ".join( + part + for part in ( + ".".join(str(item) for item in detail.get("loc", [])), + str(detail.get("msg", "")), ) - for detail in details - if isinstance(detail, dict) - ] - if any(reasons): - return "; ".join(reason for reason in reasons if reason) - return self._build_submission_error_message(error) + if part + ) + for detail in details + if isinstance(detail, dict) + ] + return "; ".join(reason for reason in reasons if reason) or None @observability_runtime.span( ECONOMY_ANNUAL_STAGES.name(Stage.ECONOMY_RESOLVE_CACHED_OR_NEW) diff --git a/tests/unit/services/test_economy_service.py b/tests/unit/services/test_economy_service.py index 6648b3605..acab3c6d6 100644 --- a/tests/unit/services/test_economy_service.py +++ b/tests/unit/services/test_economy_service.py @@ -2079,6 +2079,67 @@ def test__given_modal_validation_json_error__extracts_message( assert message == expected_message + def test__given_modal_validation_error_list__names_each_field( + self, economy_service + ): + error = make_http_status_error( + 422, + { + "detail": [ + { + "type": "missing", + "loc": ["body", "start_year"], + "msg": "Field required", + "input": {}, + }, + { + "type": "extra_forbidden", + "loc": ["body", "breakdown"], + "msg": "Extra inputs are not permitted", + "input": "local_areas", + }, + ] + }, + ) + + message = economy_service._build_submission_error_message(error) + + assert message == ( + "body.start_year: Field required; " + "body.breakdown: Extra inputs are not permitted" + ) + + def test__given_modal_rejects_batch_with_error_list__names_each_field( + self, + economy_service, + base_params, + mock_simulation_entrypoint, + mock_budget_window_cache, + ): + mock_simulation_entrypoint.run_budget_window_batch.side_effect = ( + make_http_status_error( + 422, + { + "detail": [ + { + "type": "less_than_equal", + "loc": ["body", "window_size"], + "msg": "Input should be less than or equal to 75", + "input": 80, + } + ] + }, + ) + ) + + result = economy_service.get_budget_window_economic_impact(**base_params) + + assert result.status == ImpactStatus.ERROR + assert result.error == ( + "body.window_size: Input should be less than or equal to 75" + ) + mock_budget_window_cache.set_execution_failure.assert_called_once() + def test__given_modal_validation_plain_text_error__extracts_response_text( self, economy_service ): From 4ba2e8b5a1746620ee0e858cd991acd11d32f1df Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:41:28 +0400 Subject: [PATCH 11/18] Mark local-area breakdown runs in the simulation telemetry A breakdown submission's `_telemetry.simulation_kind` is now `local_area_breakdown`, so observability can tell it apart from the national or country run for the same region. `geography_type` and `geography_code` still name the region (`national`/`uk` or `country`/`scotland`, for example). The mark uses the existing field because the simulation API's `TelemetryEnvelope` (policyengine-simulation-observability) has `extra="forbid"`: a new `breakdown` key would make the entry reject the submission. The test checks the telemetry keeps exactly its existing seven fields. Tests written first. Before the change, 2 failed for the expected reason: breakdown submissions for `uk` and `country/scotland` reported `simulation_kind` `national` and `country`. The national cases passed before and after. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- policyengine_api/services/economy_service.py | 6 +++ .../test_economy_service_request_plumbing.py | 48 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 87f69a613..80739746e 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -115,6 +115,10 @@ class ImpactStatus(Enum): # 400 from a gateway whose routed .py release does not declare breakdowns, and # 422 from an entry deployment that does not know the field. LOCAL_AREA_BREAKDOWN_REFUSAL_STATUS_CODES = frozenset({400, 422}) +# The telemetry simulation_kind of a breakdown run. The simulation API's +# telemetry envelope forbids extra fields, so the run is marked here rather +# than by a new field; geography_type and geography_code still name its region. +LOCAL_AREA_BREAKDOWN_SIMULATION_KIND = "local_area_breakdown" # The regions a breakdown can cover: the UK, each UK country the metadata # publishes, and .py's own code for a country the metadata names differently. LOCAL_AREA_BREAKDOWN_REGIONS = frozenset( @@ -2021,6 +2025,8 @@ def _build_simulation_telemetry( region=setup_options.region, ) ) + if setup_options.options.get("breakdown") is not None: + simulation_kind = LOCAL_AREA_BREAKDOWN_SIMULATION_KIND return { "submission_claim_id": setup_options.submission_claim_id, diff --git a/tests/unit/services/test_economy_service_request_plumbing.py b/tests/unit/services/test_economy_service_request_plumbing.py index 77f9ea6c5..b315e05d0 100644 --- a/tests/unit/services/test_economy_service_request_plumbing.py +++ b/tests/unit/services/test_economy_service_request_plumbing.py @@ -456,6 +456,54 @@ def test_uk_breakdown_submission_has_its_own_hash_and_sends_breakdown( assert stored[0].options_json == {"breakdown": "local_areas"} +# The simulation API's TelemetryEnvelope forbids extra fields, so a breakdown +# is marked through the existing simulation_kind field, not a new one. +TELEMETRY_FIELDS = { + "submission_claim_id", + "requested_at", + "simulation_kind", + "geography_code", + "geography_type", + "config_hash", + "capture_mode", +} + + +@pytest.mark.usefixtures("uk_package_versions") +@pytest.mark.parametrize( + ("options", "region", "expected"), + [ + ( + {"breakdown": "local_areas"}, + "uk", + ("local_area_breakdown", "national", "uk"), + ), + ( + {"breakdown": "local_areas"}, + "country/scotland", + ("local_area_breakdown", "country", "scotland"), + ), + ({}, "uk", ("national", "national", "uk")), + ({}, "country/scotland", ("country", "country", "scotland")), + ], + ids=["breakdown-uk", "breakdown-scotland", "national-uk", "national-scotland"], +) +def test_uk_submission_telemetry_marks_breakdown_runs( + economy_service, mock_simulation_entrypoint, options, region, expected +): + economy_service.get_economic_impact( + _uk_request(options, region=region), time_period="2026" + ) + + telemetry = mock_simulation_entrypoint.run.call_args.args[0]["_telemetry"] + assert set(telemetry) == TELEMETRY_FIELDS + assert ( + telemetry["simulation_kind"], + telemetry["geography_type"], + telemetry["geography_code"], + ) == expected + + @pytest.mark.usefixtures("uk_package_versions") def test_uk_national_submission_sends_no_breakdown_and_keeps_its_hash( economy_service, mock_simulation_entrypoint From c9ef783d4fdce3931b79a0afc8a1e6b54504b726 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:15:09 +0400 Subject: [PATCH 12/18] Give UK area regions the local-area dataset in the v2 catalog `_select_region_datasets` followed a region's `dataset_path` only for US regions. With .py 6.10.0 the UK registry lists 650 constituencies and 361 local authorities whose path names `microcosm_uk_2024_25_dense`, but the catalog published them against `microcosm_uk_2024_25`, the national file, which has no area codes, while filtering them on `constituency_code_oa` or `la_code_oa`. Every subnational region with a dataset path now uses that dataset, so UK constituencies and local authorities point at `microcosm_uk_2024_25_dense`, which joins the UK dataset records the same way US regional datasets do (name and year from the path). UK countries have no path and still filter the national file. Only US regions without a path are counted as fallbacks, as before. The v2 metadata's national dataset is read from the national region, so it is unchanged. The installed-catalog test now checks each UK region type's dataset, and counts 4 datasets instead of 3. docs/migration/stage-9-v2-metadata.md describes the UK selection, and the changelog fragment records it. Tests written first. Before the change, 2 failed for the expected reason: the unit test's UK datasets were only `{'microcosm_uk_2024_25'}`, and the installed 6.10.0 catalog had 3 datasets instead of 4. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.fixed.md | 2 +- docs/migration/stage-9-v2-metadata.md | 7 ++- .../data/v2/catalog/extraction.py | 10 +++- .../integration/test_v2_catalog_installed.py | 25 +++++++- tests/unit/v2/test_catalog_extraction.py | 60 +++++++++++++++++++ 5 files changed, 95 insertions(+), 9 deletions(-) diff --git a/changelog.d/uk-local-area-breakdown.fixed.md b/changelog.d/uk-local-area-breakdown.fixed.md index d7c7b1534..2f1d4275c 100644 --- a/changelog.d/uk-local-area-breakdown.fixed.md +++ b/changelog.d/uk-local-area-breakdown.fixed.md @@ -1 +1 @@ -UK economy requests for Northern Ireland (`country/ni` in the Public API metadata) now send `country/northern_ireland`, the name .py's UK region registry uses, to the simulation API. The cached result keeps the `country/ni` region. +UK economy requests for Northern Ireland (`country/ni` in the Public API metadata) now send `country/northern_ireland`, the name .py's UK region registry uses, to the simulation API. The cached result keeps the `country/ni` region. The v2 catalog gives UK constituency and local-authority regions the local-area dataset .py 6.10.0 routes them to (`microcosm_uk_2024_25_dense`) and lists that dataset, instead of the national file, which has no area codes. diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index 4ce7d2c8e..8ee9dc4b4 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -106,8 +106,11 @@ congressional-district, or place region uses its PolicyEngine.py regional alternative when one is available. Until a later reviewed change removes the fallback, a subnational region without such an alternative uses `populace_us_2024`. The command emits one summary warning containing only the -affected region types and counts. UK regions use the -`enhanced_frs_2024_25` default certified by PolicyEngine.py 5.0.4. +affected region types and counts. The UK national and country regions use +`microcosm_uk_2024_25`, the default certified by PolicyEngine.py 6.10.0; a +country filters that national file. UK constituency and local-authority +regions use the dataset PolicyEngine.py routes them to, +`microcosm_uk_2024_25_dense`, because the national file has no area codes. Review the fallback summary on every new PolicyEngine.py release. A changed count may identify an upstream catalog change that needs a reviewed dataset diff --git a/policyengine_api/data/v2/catalog/extraction.py b/policyengine_api/data/v2/catalog/extraction.py index 6f1a14b60..75b0b213f 100644 --- a/policyengine_api/data/v2/catalog/extraction.py +++ b/policyengine_api/data/v2/catalog/extraction.py @@ -645,8 +645,12 @@ def _select_region_datasets( ) -> _RegionDatasetSelection: """Choose each region's logical input dataset. - A US subnational region with a dataset path uses that dataset; every other - region uses the reviewed default, and the national region may only name it. + A subnational region with a dataset path uses that dataset: US states and + districts, and the UK constituencies and local authorities that .py routes + to its local-area release. Every other region, such as a UK country that + filters the national file, uses the reviewed default, and the national + region may only name it. A US subnational region without a path is + counted as a fallback. """ selected_dataset_names: set[str] = {reviewed_default} region_dataset_names: dict[str, str] = {} @@ -666,7 +670,7 @@ def _select_region_datasets( msg = f"region {code!r} has unsupported type {region_type!r}" raise CatalogExtractionError(msg) dataset_path = getattr(region, "dataset_path", None) - if country_id == "us" and region_type != "national" and dataset_path: + if region_type != "national" and dataset_path: dataset_name = _dataset_name_from_path(dataset_path) selected_dataset_names.add(dataset_name) else: diff --git a/tests/integration/test_v2_catalog_installed.py b/tests/integration/test_v2_catalog_installed.py index 787fba171..0f9106e06 100644 --- a/tests/integration/test_v2_catalog_installed.py +++ b/tests/integration/test_v2_catalog_installed.py @@ -40,7 +40,8 @@ def test_installed_policyengine_catalog_is_complete_and_bounded() -> None: # this exact assertion so later package changes require explicit review. # Since 6.10.0 the UK registry also lists its 650 constituencies and 361 # local authorities, because the bundle routes them to a local-area - # release, so regions grow by 1,011 from 6.7.5's 826. + # release, so regions grow by 1,011 from 6.7.5's 826 and the catalog + # adds that release's dataset. assert catalog.entity_counts() == { "models": 2, "model_versions": 2, @@ -48,7 +49,7 @@ def test_installed_policyengine_catalog_is_complete_and_bounded() -> None: "parameter_nodes": 29_307, "parameters": 104_172, "parameter_values": 1_194_360, - "datasets": 3, + "datasets": 4, "regions": 1_837, } @@ -77,8 +78,26 @@ def test_installed_policyengine_catalog_is_complete_and_bounded() -> None: "populace_us_2024_acs_local", } assert {dataset.name for dataset in catalog.country("uk").datasets} == { - "microcosm_uk_2024_25" + "microcosm_uk_2024_25", + "microcosm_uk_2024_25_dense", } + # The national file has no area codes, so UK constituencies and local + # authorities use the local-area dataset .py routes them to, while the + # UK countries filter the national file. + uk_catalog = catalog.country("uk") + uk_dataset_names = {dataset.id: dataset.name for dataset in uk_catalog.datasets} + uk_region_type_datasets: dict[str, set[str]] = {} + for region in uk_catalog.regions: + uk_region_type_datasets.setdefault(region.region_type, set()).add( + uk_dataset_names[region.default_dataset_id] + ) + assert uk_region_type_datasets == { + "national": {"microcosm_uk_2024_25"}, + "country": {"microcosm_uk_2024_25"}, + "constituency": {"microcosm_uk_2024_25_dense"}, + "local_authority": {"microcosm_uk_2024_25_dense"}, + } + assert uk_catalog.fallback_summaries == () england = next( region for region in catalog.country("uk").regions diff --git a/tests/unit/v2/test_catalog_extraction.py b/tests/unit/v2/test_catalog_extraction.py index dda612df5..f88319cd8 100644 --- a/tests/unit/v2/test_catalog_extraction.py +++ b/tests/unit/v2/test_catalog_extraction.py @@ -62,6 +62,66 @@ def test_extracts_canonical_policyengine_catalog_and_dataset_defaults() -> None: assert all(region.default_dataset_id == uk.datasets[0].id for region in uk.regions) +UK_DENSE_URI = "hf://policyengine/uk/microcosm_uk_2024_25_dense.h5@fixture" + + +def _uk_area_region(code: str, region_type: str, filter_field: str) -> SimpleNamespace: + """Return a UK area region routed to the local-area dataset, as .py 6.10 does.""" + return SimpleNamespace( + code=code, + label=code.split("/", maxsplit=1)[1], + region_type=region_type, + parent_code="uk", + dataset_path=UK_DENSE_URI, + scoping_strategy=SimpleNamespace( + strategy_type="row_filter", + variable_name=filter_field, + variable_value=code.split("/", maxsplit=1)[1], + additional_filters={}, + ), + requires_filter=True, + state_code=None, + state_name=None, + ) + + +def test_uk_area_regions_use_the_dataset_their_path_names() -> None: + models = source_models() + models["uk"].region_registry.extend( + [ + _uk_area_region( + "constituency/E14001063", "constituency", "constituency_code_oa" + ), + _uk_area_region( + "local_authority/E07000092", "local_authority", "la_code_oa" + ), + ] + ) + + uk = extract_catalog( + bundle=bundle(), + policyengine_version=POLICYENGINE_VERSION, + models=models, + installed_version=installed_version, + ).country("uk") + + assert {dataset.name for dataset in uk.datasets} == { + "microcosm_uk_2024_25", + "microcosm_uk_2024_25_dense", + } + uk_datasets = {dataset.id: dataset.name for dataset in uk.datasets} + assert { + region.code: uk_datasets[region.default_dataset_id] for region in uk.regions + } == { + "uk": "microcosm_uk_2024_25", + "country/england": "microcosm_uk_2024_25", + "constituency/E14001063": "microcosm_uk_2024_25_dense", + "local_authority/E07000092": "microcosm_uk_2024_25_dense", + } + # A UK country filters the national dataset by design; it is no fallback. + assert uk.fallback_summaries == () + + def test_durable_ids_are_deterministic_and_stable_at_the_intended_scope() -> None: first = normalized_catalog() repeated = normalized_catalog() From eb2f34a51c4679aeec79dd7e0c915c1a84a3711c Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:18:51 +0400 Subject: [PATCH 13/18] Don't keep a failed or wrong-kind breakdown job for replay A breakdown job that failed (for example a timeout or out-of-memory on the local-area dataset) was stored by `_set_reform_impact_error`, and `_handle_failed_impact` returned that error for the 30-day cache lifetime, so the app's Retry got the same error back without a new run. For breakdown requests only, `_handle_execution_state` now deletes the job's computing record with the existing `delete_reform_impact` (the cleanup used for in-flight records) and returns the error, which the route sends as 502. The next request finds no record and submits a new job. No submission claim is held at poll time; it was released when the job was handed off. Failed national jobs are still stored and replayed. A completed job for a breakdown request must also have `kind == "local_areas"`. Any other result, including a national result from a release that ignored the field, or no result, is handled the same way: an error naming the kind it got, and no record kept, so a national result is never cached under the breakdown hash. docs/canonical-spm.md, the served specification's 502 description and the changelog fragment describe both. Tests written first. Before the change, 4 failed for the expected reason: after a failed breakdown job the retry returned the stored error instead of submitting again, and a completed breakdown job whose result had no kind, kind `national`, or no result was stored and returned as ok. The test that a failed national job is still replayed passed before and after. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.added.md | 2 +- docs/canonical-spm.md | 6 +- policyengine_api/openapi_spec.yaml | 2 +- policyengine_api/services/economy_service.py | 57 ++++++++- .../test_economy_service_request_plumbing.py | 113 ++++++++++++++++++ 5 files changed, 176 insertions(+), 4 deletions(-) diff --git a/changelog.d/uk-local-area-breakdown.added.md b/changelog.d/uk-local-area-breakdown.added.md index 9f2d9fadb..d901737ec 100644 --- a/changelog.d/uk-local-area-breakdown.added.md +++ b/changelog.d/uk-local-area-breakdown.added.md @@ -1 +1 @@ -The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or one UK country (`country/england`, `country/scotland`, `country/wales` or `country/ni`), and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. +The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or one UK country (`country/england`, `country/scotland`, `country/wales` or `country/ni`), and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. A breakdown job that fails, or that completes without `kind: "local_areas"`, returns 502 and is not stored, so the next request submits a new job instead of receiving the same error for 30 days. diff --git a/docs/canonical-spm.md b/docs/canonical-spm.md index 8f2bedc49..bd9151f4b 100644 --- a/docs/canonical-spm.md +++ b/docs/canonical-spm.md @@ -339,7 +339,11 @@ returns its result (`kind: "local_areas"`) like any other economy result. If the simulation API refuses the request with HTTP 400 or 422, because the deployed simulation API or the .py release it routes to cannot run breakdowns, the route returns HTTP 502 with the simulation API's reason and stores -nothing, so a later request tries again. +nothing, so a later request tries again. A breakdown job that fails (for +example by running out of time or memory on the local-area dataset), or that +completes without `kind: "local_areas"`, also returns HTTP 502 and is not kept: +its record is deleted, so the next request submits a new job. A failed +national job is still stored and returned again for the cache lifetime. Before cache access or submission, the API validates the selected worker application's `canonical-spm-v1` capability diff --git a/policyengine_api/openapi_spec.yaml b/policyengine_api/openapi_spec.yaml index 1749afb3d..1961404ca 100644 --- a/policyengine_api/openapi_spec.yaml +++ b/policyengine_api/openapi_spec.yaml @@ -946,7 +946,7 @@ paths: message: type: string 502: - description: The calculation failed, or the simulation API refused a breakdown=local_areas request it cannot run. The message gives the reason. + description: The calculation failed, or the simulation API refused a breakdown=local_areas request it cannot run. The message gives the reason. A failed breakdown=local_areas job, or one whose result is not kind local_areas, is not stored, so the next request submits a new job. content: application/json: schema: diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 80739746e..878003a0d 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -1567,8 +1567,14 @@ def _handle_execution_state( Modal statuses (complete, failed, running, submitted). """ if execution_state in EXECUTION_STATUSES_SUCCESS: + raw_result = self._simulation_gateway.get_execution_result(execution) + kind_error = self._breakdown_result_kind_error(setup_options, raw_result) + if kind_error is not None: + return self._discard_failed_breakdown( + setup_options, reform_impact, kind_error + ) result = self._with_policyengine_bundle( - result=self._simulation_gateway.get_execution_result(execution), + result=raw_result, setup_options=setup_options, execution=execution, ) @@ -1599,6 +1605,10 @@ def _handle_execution_state( error_message = ( f"Simulation entrypoint execution failed: {execution.error}" ) + if setup_options.options.get("breakdown") is not None: + return self._discard_failed_breakdown( + setup_options, reform_impact, error_message + ) self._set_reform_impact_error( setup_options=setup_options, @@ -1623,6 +1633,51 @@ def _handle_execution_state( msg = f"Unexpected sim API execution state: {execution_state}" raise ValueError(msg) + @staticmethod + def _breakdown_result_kind_error( + setup_options: EconomicImpactSetupOptions, result: object + ) -> str | None: + """Return why a breakdown job's result cannot be stored, if it cannot. + + A breakdown request needs the simulation API's local-area result + (``kind: local_areas``). Any other result, such as a national one from + a release that ignored the field, must never be stored under the + breakdown's options hash. Return None for a valid result and for every + request without a breakdown. + """ + if setup_options.options.get("breakdown") is None: + return None + kind = result.get("kind") if isinstance(result, dict) else None + if kind == LOCAL_AREA_BREAKDOWN: + return None + return ( + f"The simulation API returned a result of kind {kind!r} for a " + "local-area breakdown request, which needs kind " + f"{LOCAL_AREA_BREAKDOWN!r}; the result was not stored" + ) + + def _discard_failed_breakdown( + self, + setup_options: EconomicImpactSetupOptions, + reform_impact: ReformImpact, + message: str, + ) -> EconomicImpactResult: + """Return a failed breakdown job's error without keeping its record. + + A breakdown runs on the large local-area dataset, where a timeout or + memory failure may not repeat. Storing the failure would replay it for + the cache lifetime, so the job's computing record is deleted instead + and the next request submits a new job. No submission claim is held at + this point: it was released once the job was handed off. Failures of + requests without a breakdown are still stored and replayed. + """ + options_hash = reform_impact.options_hash or setup_options.options_hash + if options_hash is not None: + self._reform_impacts.delete_reform_impact( + self._reform_impact_scope(setup_options), options_hash + ) + return EconomicImpactResult.error(message=message) + @observability_runtime.span( ECONOMY_ANNUAL_STAGES.name(Stage.ECONOMY_READ_COMPLETED) ) diff --git a/tests/unit/services/test_economy_service_request_plumbing.py b/tests/unit/services/test_economy_service_request_plumbing.py index b315e05d0..11ac02deb 100644 --- a/tests/unit/services/test_economy_service_request_plumbing.py +++ b/tests/unit/services/test_economy_service_request_plumbing.py @@ -10,6 +10,7 @@ import pytest +from policyengine_api.libs.simulation_entrypoint import ModalSimulationExecution from policyengine_api.runtime_cache.core import CacheNamespace from policyengine_api.runtime_cache.fake import InMemoryCacheBackend from policyengine_api.runtime_cache.reform_impacts import ( @@ -28,6 +29,7 @@ RuntimeBundleIdentity, ) from policyengine_api.services.reform_impacts_service import ReformImpactsService +from tests.fixtures.libs.simulation_entrypoint import MOCK_LOCAL_AREA_BREAKDOWN_RESULT from tests.fixtures.services.economy_service import ( MOCK_API_VERSION, MOCK_BASELINE_POLICY_ID, @@ -567,6 +569,117 @@ def test_a_breakdown_request_never_reuses_the_national_job( ) +def _finish_jobs_with( + simulation_entrypoint, *, status: str, result=None, error=None +) -> None: + """Make every polled job report this status, result and error.""" + simulation_entrypoint.get_execution_by_id.side_effect = lambda job_id: ( + ModalSimulationExecution( + job_id=job_id, + status=status, + result=result, + error=error, + resolved_app_name=MOCK_RESOLVED_APP_NAME, + ) + ) + simulation_entrypoint.get_execution_status.side_effect = lambda execution: ( + execution.status + ) + simulation_entrypoint.get_execution_result.side_effect = lambda execution: ( + execution.result + ) + + +@pytest.mark.usefixtures("uk_package_versions") +def test_a_failed_breakdown_job_is_not_replayed_and_the_next_request_resubmits( + economy_service, mock_simulation_entrypoint +): + mock_simulation_entrypoint.get_execution_id.side_effect = [ + "first-breakdown-job", + "second-breakdown-job", + ] + _finish_jobs_with( + mock_simulation_entrypoint, status="failed", error="out of memory" + ) + request = _uk_request({"breakdown": "local_areas"}) + + economy_service.get_economic_impact(request, time_period="2026") + failed = economy_service.get_economic_impact(request, time_period="2026") + retried = economy_service.get_economic_impact(request, time_period="2026") + + assert failed == EconomicImpactResult.error( + "Simulation entrypoint execution failed: out of memory" + ) + # The failure is not kept for replay: the retry submits a new job. + assert retried == EconomicImpactResult.computing() + assert mock_simulation_entrypoint.run.call_count == 2 + stored = economy_service._reform_impacts.get_recent_reform_impacts(5) + assert [(impact.execution_id, impact.status) for impact in stored] == [ + ("second-breakdown-job", "computing") + ] + + +@pytest.mark.usefixtures("uk_package_versions") +def test_a_failed_national_job_is_still_replayed( + economy_service, mock_simulation_entrypoint +): + _finish_jobs_with( + mock_simulation_entrypoint, status="failed", error="out of memory" + ) + request = _uk_request({}) + + economy_service.get_economic_impact(request, time_period="2026") + failed = economy_service.get_economic_impact(request, time_period="2026") + replayed = economy_service.get_economic_impact(request, time_period="2026") + + expected = EconomicImpactResult.error( + "Simulation entrypoint execution failed: out of memory" + ) + assert failed == expected + assert replayed == expected + mock_simulation_entrypoint.run.assert_called_once() + mock_simulation_entrypoint.get_execution_by_id.assert_called_once() + stored = economy_service._reform_impacts.get_recent_reform_impacts(5) + assert [impact.status for impact in stored] == ["error"] + + +@pytest.mark.usefixtures("uk_package_versions") +@pytest.mark.parametrize( + ("result", "reported_kind"), + [ + ({"budget": {"budgetary_impact": 1.0}}, "None"), + ({**MOCK_LOCAL_AREA_BREAKDOWN_RESULT, "kind": "national"}, "'national'"), + (None, "None"), + ], + ids=["national-result", "other-kind", "no-result"], +) +def test_a_breakdown_job_without_a_local_areas_result_is_not_stored( + economy_service, mock_simulation_entrypoint, result, reported_kind +): + mock_simulation_entrypoint.get_execution_id.side_effect = [ + "first-breakdown-job", + "second-breakdown-job", + ] + _finish_jobs_with(mock_simulation_entrypoint, status="complete", result=result) + request = _uk_request({"breakdown": "local_areas"}) + + economy_service.get_economic_impact(request, time_period="2026") + completed = economy_service.get_economic_impact(request, time_period="2026") + retried = economy_service.get_economic_impact(request, time_period="2026") + + assert completed == EconomicImpactResult.error( + "The simulation API returned a result of kind " + f"{reported_kind} for a local-area breakdown request, which needs kind " + "'local_areas'; the result was not stored" + ) + assert retried == EconomicImpactResult.computing() + assert mock_simulation_entrypoint.run.call_count == 2 + stored = economy_service._reform_impacts.get_recent_reform_impacts(5) + assert [(impact.execution_id, impact.status) for impact in stored] == [ + ("second-breakdown-job", "computing") + ] + + @pytest.mark.parametrize( ("bundle_fields", "expected"), [ From 99bdc5e8dc46712ef4f9e0e062cfb8f4124494f3 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:21:30 +0400 Subject: [PATCH 14/18] List every accepted breakdown region in the 400 and the query description `constants.LOCAL_AREA_BREAKDOWN_REGIONS` is now one ordered tuple: `uk`, the four UK countries the metadata publishes, then `country/northern_ireland`, .py's code for Northern Ireland. The breakdown check, its 400 message and the published `breakdown` query description are all built from it, so `country/northern_ireland`, which the check already accepted, is now listed in both places. The message reads `breakdown=local_areas needs region uk or a UK country, one of uk, country/england, country/scotland, country/wales, country/ni, country/northern_ireland; not 'country/france'`. The changelog fragment lists the same regions. `query_parameters.py` imports the tuple from `constants.py`, which reads only package metadata at import and imports nothing from `query_parameters.py`. Tests written first. Before the change, 2 failed for the expected reason: the message listed only the four metadata countries, and the published description did not name `uk` in the list or `country/northern_ireland` at all. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.added.md | 2 +- policyengine_api/constants.py | 8 ++++++++ policyengine_api/query_parameters.py | 7 ++++--- policyengine_api/services/economy_service.py | 16 +++------------- tests/unit/services/test_economy_service.py | 15 +++++++++++++++ tests/unit/test_openapi_spm.py | 4 ++++ 6 files changed, 35 insertions(+), 17 deletions(-) diff --git a/changelog.d/uk-local-area-breakdown.added.md b/changelog.d/uk-local-area-breakdown.added.md index d901737ec..17b0e7098 100644 --- a/changelog.d/uk-local-area-breakdown.added.md +++ b/changelog.d/uk-local-area-breakdown.added.md @@ -1 +1 @@ -The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or one UK country (`country/england`, `country/scotland`, `country/wales` or `country/ni`), and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. A breakdown job that fails, or that completes without `kind: "local_areas"`, returns 502 and is not stored, so the next request submits a new job instead of receiving the same error for 30 days. +The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or one UK country (`country/england`, `country/scotland`, `country/wales`, `country/ni`, or `country/northern_ireland`, the code .py uses), and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. A breakdown job that fails, or that completes without `kind: "local_areas"`, returns 502 and is not stored, so the next request submits a new job instead of receiving the same error for 30 days. diff --git a/policyengine_api/constants.py b/policyengine_api/constants.py index bc67887bb..b53fbb409 100644 --- a/policyengine_api/constants.py +++ b/policyengine_api/constants.py @@ -294,6 +294,14 @@ def get_report_output_cache_version(country_id: str) -> str: # UK regions whose metadata name differs from the code .py's UK region # registry uses, mapped to that code. UK_REGION_REGISTRY_CODES = {"country/ni": "country/northern_ireland"} +# The regions a UK local-area breakdown can cover, in the order messages and +# the query description list them: the UK, each UK country the metadata +# publishes, and .py's own code for a country the metadata names differently. +LOCAL_AREA_BREAKDOWN_REGIONS = ( + "uk", + *(name for name, _label in UK_COUNTRY_REGIONS), + *UK_REGION_REGISTRY_CODES.values(), +) # Modal simulation API status values MODAL_EXECUTION_STATUS_SUBMITTED = "submitted" diff --git a/policyengine_api/query_parameters.py b/policyengine_api/query_parameters.py index 25ef95898..e06e9cdcb 100644 --- a/policyengine_api/query_parameters.py +++ b/policyengine_api/query_parameters.py @@ -25,6 +25,7 @@ model_validator, ) +from policyengine_api.constants import LOCAL_AREA_BREAKDOWN_REGIONS from policyengine_api.data.v2.catalog.catalog_selection import ( validate_policyengine_version, ) @@ -173,9 +174,9 @@ class AnnualEconomyQuery(EconomyQuery): default=None, description=( "local_areas computes only the UK constituency and local-authority " - "impacts, in a run separate from the national result. UK only, " - "with region uk or one UK country (country/england, country/scotland, " - "country/wales or country/ni), and not with target=cliff" + "impacts, in a run separate from the national result. UK only, with " + f"region one of: {', '.join(LOCAL_AREA_BREAKDOWN_REGIONS)}; " + "not with target=cliff" ), ) diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 878003a0d..424e662e3 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -19,8 +19,8 @@ EXECUTION_STATUSES_FAILURE, EXECUTION_STATUSES_PENDING, EXECUTION_STATUSES_SUCCESS, + LOCAL_AREA_BREAKDOWN_REGIONS, POLICYENGINE_VERSION, - UK_COUNTRY_REGIONS, UK_REGION_REGISTRY_CODES, get_economy_impact_cache_version, ) @@ -119,15 +119,6 @@ class ImpactStatus(Enum): # telemetry envelope forbids extra fields, so the run is marked here rather # than by a new field; geography_type and geography_code still name its region. LOCAL_AREA_BREAKDOWN_SIMULATION_KIND = "local_area_breakdown" -# The regions a breakdown can cover: the UK, each UK country the metadata -# publishes, and .py's own code for a country the metadata names differently. -LOCAL_AREA_BREAKDOWN_REGIONS = frozenset( - { - "uk", - *(name for name, _label in UK_COUNTRY_REGIONS), - *UK_REGION_REGISTRY_CODES.values(), - } -) BUDGET_WINDOW_CLAIM_ATTEMPTS = 3 REFORM_IMPACT_START_CLAIM_ATTEMPTS = 3 # A budget-window progress message names this many computing years and counts @@ -1099,10 +1090,9 @@ def _validate_breakdown(impact_request: EconomicImpactRequest) -> None: raise ValueError(msg) region = impact_request.region if region not in LOCAL_AREA_BREAKDOWN_REGIONS: - countries = ", ".join(name for name, _label in UK_COUNTRY_REGIONS) msg = ( - "breakdown=local_areas needs region uk or a UK country " - f"({countries}), not {region!r}" + "breakdown=local_areas needs region uk or a UK country, one of " + f"{', '.join(LOCAL_AREA_BREAKDOWN_REGIONS)}; not {region!r}" ) raise ValueError(msg) if impact_request.target == "cliff": diff --git a/tests/unit/services/test_economy_service.py b/tests/unit/services/test_economy_service.py index acab3c6d6..12ef53704 100644 --- a/tests/unit/services/test_economy_service.py +++ b/tests/unit/services/test_economy_service.py @@ -3276,6 +3276,21 @@ def test__given_unsupported_request__rejects_breakdown( # Rejected before the worker's SPM capability is fetched. gateway.get_spm_capability.assert_not_called() + def test__given_unsupported_region__names_every_accepted_region(self): + with pytest.raises(ValueError, match="breakdown=local_areas") as raised: + EconomyService( + simulation_entrypoint_=MagicMock() + )._build_economic_impact_setup_options( + uk_breakdown_request(region="country/france"), + time_period=MOCK_TIME_PERIOD, + ) + + assert str(raised.value) == ( + "breakdown=local_areas needs region uk or a UK country, one of " + "uk, country/england, country/scotland, country/wales, " + "country/ni, country/northern_ireland; not 'country/france'" + ) + def test__given_unknown_breakdown_option__rejects_it(self): request = replace( uk_breakdown_request(), options={"breakdown": "constituencies"} diff --git a/tests/unit/test_openapi_spm.py b/tests/unit/test_openapi_spm.py index 55deec651..6f5b8942a 100644 --- a/tests/unit/test_openapi_spm.py +++ b/tests/unit/test_openapi_spm.py @@ -57,6 +57,10 @@ def test_economy_query_and_receipt_schemas_match_public_http_contract(): assert breakdown["schema"]["enum"] == ["local_areas"] assert breakdown["schema"]["nullable"] is True assert "constituency" in breakdown["description"] + assert ( + "region one of: uk, country/england, country/scotland, " + "country/wales, country/ni, country/northern_ireland;" + ) in breakdown["description"] if suffix: assert parameters["window_size"]["schema"]["minimum"] == 1 assert parameters["window_size"]["schema"]["maximum"] == 75 From afa35e61523256082ed164951bc89c07c8beb1ae Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:22:17 +0400 Subject: [PATCH 15/18] Test the Northern Ireland breakdown end to end and the early rejection A request-plumbing test submits a Northern Ireland breakdown (`region=country/ni`) and polls it once. The simulation payload sends `country/northern_ireland` with `breakdown`, and the stored record keeps region `country/ni`, the breakdown options and the breakdown hash, which is why the second request polls the same job instead of submitting. The unsupported-breakdown test now checks, for every case, that the request is rejected before the cache version is read and that the simulation client is never called. The SPM capability assertion applies only to the US cases, and the test says why: a UK request has no SPM selection, so it never fetches the capability either way. These tests describe existing behaviour and passed when written; no service code changes. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- tests/unit/services/test_economy_service.py | 18 ++++++++++--- .../test_economy_service_request_plumbing.py | 25 +++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/tests/unit/services/test_economy_service.py b/tests/unit/services/test_economy_service.py index 12ef53704..e4b3507a3 100644 --- a/tests/unit/services/test_economy_service.py +++ b/tests/unit/services/test_economy_service.py @@ -3263,7 +3263,13 @@ def test__given_unsupported_request__rejects_breakdown( ): gateway = MagicMock() - with pytest.raises(ValueError, match=message): + with ( + patch( + "policyengine_api.services.economy_service." + "get_economy_impact_cache_version" + ) as cache_version, + pytest.raises(ValueError, match=message), + ): EconomyService( simulation_entrypoint_=gateway )._build_economic_impact_setup_options( @@ -3273,8 +3279,14 @@ def test__given_unsupported_request__rejects_breakdown( time_period=MOCK_TIME_PERIOD, ) - # Rejected before the worker's SPM capability is fetched. - gateway.get_spm_capability.assert_not_called() + # Every case is rejected before the cache identity is built. + cache_version.assert_not_called() + assert gateway.mock_calls == [] + if country_id == "us": + # Only a US request reaches the worker's SPM capability check, + # which runs right after this one: a UK request has no SPM + # selection, so it would never fetch the capability anyway. + gateway.get_spm_capability.assert_not_called() def test__given_unsupported_region__names_every_accepted_region(self): with pytest.raises(ValueError, match="breakdown=local_areas") as raised: diff --git a/tests/unit/services/test_economy_service_request_plumbing.py b/tests/unit/services/test_economy_service_request_plumbing.py index 11ac02deb..14c5a9000 100644 --- a/tests/unit/services/test_economy_service_request_plumbing.py +++ b/tests/unit/services/test_economy_service_request_plumbing.py @@ -569,6 +569,31 @@ def test_a_breakdown_request_never_reuses_the_national_job( ) +@pytest.mark.usefixtures("uk_package_versions") +def test_a_northern_ireland_breakdown_sends_the_registry_code_and_keeps_its_scope( + economy_service, mock_simulation_entrypoint +): + request = _uk_request({"breakdown": "local_areas"}, region="country/ni") + + economy_service.get_economic_impact(request, time_period="2026") + # The second request finds the stored job under country/ni and polls it. + economy_service.get_economic_impact(request, time_period="2026") + + payload = mock_simulation_entrypoint.run.call_args.args[0] + assert payload["region"] == "country/northern_ireland" + assert payload["breakdown"] == "local_areas" + assert payload["_telemetry"]["geography_code"] == "ni" + mock_simulation_entrypoint.run.assert_called_once() + mock_simulation_entrypoint.get_execution_by_id.assert_called_once_with( + MOCK_MODAL_JOB_ID + ) + stored = economy_service._reform_impacts.get_recent_reform_impacts(5) + assert [(impact.region, impact.options_json) for impact in stored] == [ + ("country/ni", {"breakdown": "local_areas"}) + ] + assert stored[0].options_hash == _stored_options_hash(UK_BREAKDOWN_LOOKUP_HASH) + + def _finish_jobs_with( simulation_entrypoint, *, status: str, result=None, error=None ) -> None: From 9889750082083a2486b2a37a27394837a33b087f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:56:47 +0400 Subject: [PATCH 16/18] Keep a failed breakdown's error for two minutes on its own record The previous commit removed a failed breakdown job's record with `delete_reform_impact`, which matches on scope, options hash and `status == "computing"` and searches only the newest 1,000 records across every country. That caused two faults: - after 1,000 newer economy jobs, the failed job's record was not found, so every request kept polling the failed job and got 502 for 30 days; - a late poll of a failed job J1 that finished after a Retry had stored J2, for the same scope and hash, deleted J2's record, which led to a duplicate submission. It also let a client that retries on every 502 start a new run on the large local-area dataset after each failure. Now `_record_breakdown_failure` marks only the failed job's own record, found by its execution ID, as an error that expires after `LOCAL_AREA_BREAKDOWN_FAILURE_SECONDS` (120). Until then every request gets that error, which ends "A new local-area breakdown can be requested 2 minutes after this failure."; after it, the next request finds no record and submits a new job. No other record is read or changed. To support this, `ReformImpactCache.set` and `update` take an optional `record_ttl_seconds` for the record key only (the scope and recency indexes keep their 30-day lifetime, and an entry whose record has expired is skipped when read), and `ReformImpactsService.set_error_reform_impact` takes `keep_seconds`. National failures are stored for 30 days as before. `_handle_execution_state` and the new helper annotate the stored record as `CachedReformImpact`, the type they receive, instead of the v1 ORM `ReformImpact`. docs/canonical-spm.md, the specification's 502 description and the changelog fragment describe the two-minute window. Tests written first. Before the change, 6 failed for the expected reason: - the failure message had no retry note, and the request two minutes later still got the error instead of a new job; - after 1,000 newer records, the request 121 seconds after the failure polled the failed job again and returned the error, not a new job; - a late poll of failed J1 deleted J2's computing record; - the three wrong-kind cases had no retry note. The test that a failed national job is still replayed, now also after 121 seconds, passed before and after. The cache test that a briefly kept error expires and leaves the next job's record was added with the code; on the previous code it fails with a TypeError, because `keep_seconds` did not exist. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.added.md | 2 +- docs/canonical-spm.md | 9 +- policyengine_api/openapi_spec.yaml | 2 +- .../runtime_cache/reform_impacts.py | 23 ++- policyengine_api/services/economy_service.py | 62 +++++--- .../services/reform_impacts_service.py | 8 +- .../test_economy_service_request_plumbing.py | 143 ++++++++++++++++-- .../services/test_reform_impacts_service.py | 29 ++++ 8 files changed, 237 insertions(+), 41 deletions(-) diff --git a/changelog.d/uk-local-area-breakdown.added.md b/changelog.d/uk-local-area-breakdown.added.md index 17b0e7098..b6b065eca 100644 --- a/changelog.d/uk-local-area-breakdown.added.md +++ b/changelog.d/uk-local-area-breakdown.added.md @@ -1 +1 @@ -The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or one UK country (`country/england`, `country/scotland`, `country/wales`, `country/ni`, or `country/northern_ireland`, the code .py uses), and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. A breakdown job that fails, or that completes without `kind: "local_areas"`, returns 502 and is not stored, so the next request submits a new job instead of receiving the same error for 30 days. +The annual economy route accepts `breakdown=local_areas`, which requests the UK constituency and local-authority impacts in a run separate from the national result. It is accepted only for the UK, with region `uk` or one UK country (`country/england`, `country/scotland`, `country/wales`, `country/ni`, or `country/northern_ireland`, the code .py uses), and not with `target=cliff`; other requests return 400. The option gets its own options hash and cached result, and is sent to the simulation API only when set, so requests without it are unchanged. If the simulation API refuses a breakdown with 400 or 422, the route returns 502 with the reason instead of a generic 500. A breakdown job that fails, or that completes without `kind: "local_areas"`, returns 502; its error is kept for two minutes instead of 30 days, and after that the next request submits a new job. diff --git a/docs/canonical-spm.md b/docs/canonical-spm.md index bd9151f4b..30608e013 100644 --- a/docs/canonical-spm.md +++ b/docs/canonical-spm.md @@ -341,9 +341,12 @@ deployed simulation API or the .py release it routes to cannot run breakdowns, the route returns HTTP 502 with the simulation API's reason and stores nothing, so a later request tries again. A breakdown job that fails (for example by running out of time or memory on the local-area dataset), or that -completes without `kind: "local_areas"`, also returns HTTP 502 and is not kept: -its record is deleted, so the next request submits a new job. A failed -national job is still stored and returned again for the cache lifetime. +completes without `kind: "local_areas"`, also returns HTTP 502. Its result is +not stored. Its error is kept for two minutes on that job's own record, so +every request in that time gets the same error, which says when a new +breakdown can be requested; then the record expires and the next request +submits a new job. Other jobs' records are not changed. A failed national job +is still stored and returned again for the cache lifetime of 30 days. Before cache access or submission, the API validates the selected worker application's `canonical-spm-v1` capability diff --git a/policyengine_api/openapi_spec.yaml b/policyengine_api/openapi_spec.yaml index 1961404ca..6c2f5672d 100644 --- a/policyengine_api/openapi_spec.yaml +++ b/policyengine_api/openapi_spec.yaml @@ -946,7 +946,7 @@ paths: message: type: string 502: - description: The calculation failed, or the simulation API refused a breakdown=local_areas request it cannot run. The message gives the reason. A failed breakdown=local_areas job, or one whose result is not kind local_areas, is not stored, so the next request submits a new job. + description: The calculation failed, or the simulation API refused a breakdown=local_areas request it cannot run. The message gives the reason. A failed breakdown=local_areas job, or one whose result is not kind local_areas, keeps its error for two minutes only; after that the next request submits a new job. content: application/json: schema: diff --git a/policyengine_api/runtime_cache/reform_impacts.py b/policyengine_api/runtime_cache/reform_impacts.py index 874804a92..84ca55865 100644 --- a/policyengine_api/runtime_cache/reform_impacts.py +++ b/policyengine_api/runtime_cache/reform_impacts.py @@ -364,11 +364,15 @@ def set( impact: CachedReformImpact, *, record_execution_id: str | None = None, + record_ttl_seconds: int | None = None, ) -> bool: """Store an impact and index it by scope and recency in one transaction. - Each index keeps only the newest 1,000 entries. Return False if the - write failed; raise ValueError when there is no record identifier. + Each index keeps only the newest 1,000 entries. ``record_ttl_seconds`` + gives this record a shorter lifetime than the usual 30 days; the + indexes keep theirs, and an index entry whose record has expired is + skipped when read. Return False if the write failed; raise ValueError + when there is no record identifier. """ # Terminal errors clear the worker handle in the payload while keeping # the original record key and lookup indexes stable. @@ -388,7 +392,7 @@ def set( REFORM_IMPACT_SCHEMA_VERSION, _impact_to_wire(impact), ), - ex=ttl_seconds, + ex=record_ttl_seconds or ttl_seconds, ) for index in indexes: pipeline.zadd(index, {record_key: self._score(impact)}) @@ -506,12 +510,14 @@ def update( execution_id: str, *, clear_execution_id: bool = False, + record_ttl_seconds: int | None = None, **changes: Any, # noqa: ANN401 # forwarded unchanged to dataclasses.replace ) -> CachedReformImpact | None: """Apply field changes to a cached impact and rewrite it under the same key. - ``clear_execution_id`` drops the worker handle without moving the record. - Return the updated impact, or None if it is missing or the write failed. + ``clear_execution_id`` drops the worker handle without moving the record, + and ``record_ttl_seconds`` shortens its lifetime (see ``set``). Return the + updated impact, or None if it is missing or the write failed. """ impact = self.get_by_execution_id(execution_id) if impact is None: @@ -519,7 +525,12 @@ def update( if clear_execution_id: changes["execution_id"] = None updated = replace(impact, **changes) - return updated if self.set(updated, record_execution_id=execution_id) else None + stored = self.set( + updated, + record_execution_id=execution_id, + record_ttl_seconds=record_ttl_seconds, + ) + return updated if stored else None def delete_matching_computing( self, diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 424e662e3..57f287f6e 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -115,6 +115,15 @@ class ImpactStatus(Enum): # 400 from a gateway whose routed .py release does not declare breakdowns, and # 422 from an entry deployment that does not know the field. LOCAL_AREA_BREAKDOWN_REFUSAL_STATUS_CODES = frozenset({400, 422}) +# How long a failed breakdown's error is returned before the next request may +# submit a new job. Short, so the app's Retry works soon, but long enough that +# a client retrying on every 502 does not start a run on the large local-area +# dataset each time. +LOCAL_AREA_BREAKDOWN_FAILURE_SECONDS = 120 +LOCAL_AREA_BREAKDOWN_RETRY_NOTE = ( + "A new local-area breakdown can be requested " + f"{LOCAL_AREA_BREAKDOWN_FAILURE_SECONDS // 60} minutes after this failure." +) # The telemetry simulation_kind of a breakdown run. The simulation API's # telemetry envelope forbids extra fields, so the run is marked here rather # than by a new field; geography_type and geography_code still name its region. @@ -1548,7 +1557,7 @@ def _handle_execution_state( self, setup_options: EconomicImpactSetupOptions, execution_state: str, - reform_impact: ReformImpact, + reform_impact: CachedReformImpact, execution: ModalSimulationExecution | None = None, ) -> EconomicImpactResult: """Map an execution's state to the impact status and result to return. @@ -1560,7 +1569,7 @@ def _handle_execution_state( raw_result = self._simulation_gateway.get_execution_result(execution) kind_error = self._breakdown_result_kind_error(setup_options, raw_result) if kind_error is not None: - return self._discard_failed_breakdown( + return self._record_breakdown_failure( setup_options, reform_impact, kind_error ) result = self._with_policyengine_bundle( @@ -1596,7 +1605,7 @@ def _handle_execution_state( f"Simulation entrypoint execution failed: {execution.error}" ) if setup_options.options.get("breakdown") is not None: - return self._discard_failed_breakdown( + return self._record_breakdown_failure( setup_options, reform_impact, error_message ) @@ -1646,25 +1655,31 @@ def _breakdown_result_kind_error( f"{LOCAL_AREA_BREAKDOWN!r}; the result was not stored" ) - def _discard_failed_breakdown( + def _record_breakdown_failure( self, setup_options: EconomicImpactSetupOptions, - reform_impact: ReformImpact, - message: str, + reform_impact: CachedReformImpact, + reason: str, ) -> EconomicImpactResult: - """Return a failed breakdown job's error without keeping its record. + """Keep a failed breakdown job's error for two minutes, then let it go. A breakdown runs on the large local-area dataset, where a timeout or - memory failure may not repeat. Storing the failure would replay it for - the cache lifetime, so the job's computing record is deleted instead - and the next request submits a new job. No submission claim is held at - this point: it was released once the job was handed off. Failures of - requests without a breakdown are still stored and replayed. + memory failure may not repeat, so its failure is not kept for the usual + 30 days. This job's own record, found by its execution ID, becomes an + error that expires after ``LOCAL_AREA_BREAKDOWN_FAILURE_SECONDS``. + Until then every request gets the same error, which says when to try + again; after it, the next request finds no record and submits a new + job. No other job's record is read or changed, and no submission claim + is held at this point: it was released once the job was handed off. + Failures of requests without a breakdown are stored as before. """ - options_hash = reform_impact.options_hash or setup_options.options_hash - if options_hash is not None: - self._reform_impacts.delete_reform_impact( - self._reform_impact_scope(setup_options), options_hash + message = f"{reason}. {LOCAL_AREA_BREAKDOWN_RETRY_NOTE}" + if reform_impact.execution_id: + self._set_reform_impact_error( + setup_options=setup_options, + message=message, + execution_id=reform_impact.execution_id, + keep_seconds=LOCAL_AREA_BREAKDOWN_FAILURE_SECONDS, ) return EconomicImpactResult.error(message=message) @@ -2176,13 +2191,24 @@ def _set_reform_impact_error( message: str, execution_id: str, error_code: str | None = None, + keep_seconds: int | None = None, ) -> None: - """Record the impact as "error" with its message; log and re-raise errors.""" + """Record the impact as "error" with its message; log and re-raise errors. + + ``keep_seconds`` keeps the error for that long instead of the usual + 30 days. + """ + # Optional fields are passed only when set, as before keep_seconds existed. + optional_fields: dict[str, Any] = {} + if error_code is not None: + optional_fields["error_code"] = error_code + if keep_seconds is not None: + optional_fields["keep_seconds"] = keep_seconds try: self._reform_impacts.set_error_reform_impact( execution_id=execution_id, message=message, - **({"error_code": error_code} if error_code is not None else {}), + **optional_fields, ) except Exception as e: logger.log_struct( diff --git a/policyengine_api/services/reform_impacts_service.py b/policyengine_api/services/reform_impacts_service.py index 448904b5a..6d10a6765 100644 --- a/policyengine_api/services/reform_impacts_service.py +++ b/policyengine_api/services/reform_impacts_service.py @@ -169,12 +169,15 @@ def set_error_reform_impact( *, message: str, error_code: str | None = None, + keep_seconds: int | None = None, ) -> CachedReformImpact | None: """Mark this execution's cached impact as failed. An ``error_code`` records a typed, terminal failure and clears the worker - handle. Return the updated impact, or None if it is missing or the write - failed. + handle. ``keep_seconds`` keeps the failed record for that long instead of + the usual 30 days; after it expires, a lookup no longer finds it. Only + this execution's record changes. Return the updated impact, or None if it + is missing or the write failed. """ return self._cache.update( execution_id, @@ -182,6 +185,7 @@ def set_error_reform_impact( message=message, error_code=error_code, clear_execution_id=error_code is not None, + record_ttl_seconds=keep_seconds, end_time=self._now(), ) diff --git a/tests/unit/services/test_economy_service_request_plumbing.py b/tests/unit/services/test_economy_service_request_plumbing.py index 14c5a9000..7dcfbc80f 100644 --- a/tests/unit/services/test_economy_service_request_plumbing.py +++ b/tests/unit/services/test_economy_service_request_plumbing.py @@ -6,6 +6,8 @@ """ import json +from dataclasses import replace +from datetime import timedelta from unittest.mock import patch import pytest @@ -14,6 +16,7 @@ from policyengine_api.runtime_cache.core import CacheNamespace from policyengine_api.runtime_cache.fake import InMemoryCacheBackend from policyengine_api.runtime_cache.reform_impacts import ( + REFORM_IMPACT_INDEX_LIMIT, CachedReformImpact, ReformImpactCache, ReformImpactIdentity, @@ -26,6 +29,7 @@ from policyengine_api.services.economy_service import ( EconomicImpactResult, EconomyService, + ImpactStatus, RuntimeBundleIdentity, ) from policyengine_api.services.reform_impacts_service import ReformImpactsService @@ -92,14 +96,21 @@ def service_environment( """ +@pytest.fixture +def reform_impact_backend(): + """Return the in-memory reform-impact cache, so a test can move its clock.""" + return InMemoryCacheBackend() + + @pytest.fixture def economy_service( service_environment, mock_simulation_entrypoint, + reform_impact_backend, ): return EconomyService( reform_impacts_service_=ReformImpactsService( - ReformImpactCache(InMemoryCacheBackend(), NAMESPACE) + ReformImpactCache(reform_impact_backend, NAMESPACE) ), budget_window_cache_=BudgetWindowCache(InMemoryCacheBackend(), NAMESPACE), simulation_entrypoint_=mock_simulation_entrypoint, @@ -615,9 +626,14 @@ def _finish_jobs_with( ) +BREAKDOWN_RETRY_NOTE = ( + "A new local-area breakdown can be requested 2 minutes after this failure." +) + + @pytest.mark.usefixtures("uk_package_versions") -def test_a_failed_breakdown_job_is_not_replayed_and_the_next_request_resubmits( - economy_service, mock_simulation_entrypoint +def test_a_failed_breakdown_is_returned_for_two_minutes_then_resubmitted( + economy_service, mock_simulation_entrypoint, reform_impact_backend ): mock_simulation_entrypoint.get_execution_id.side_effect = [ "first-breakdown-job", @@ -630,12 +646,23 @@ def test_a_failed_breakdown_job_is_not_replayed_and_the_next_request_resubmits( economy_service.get_economic_impact(request, time_period="2026") failed = economy_service.get_economic_impact(request, time_period="2026") + reform_impact_backend.advance(60) + within_two_minutes = economy_service.get_economic_impact( + request, time_period="2026" + ) + reform_impact_backend.advance(61) retried = economy_service.get_economic_impact(request, time_period="2026") - assert failed == EconomicImpactResult.error( - "Simulation entrypoint execution failed: out of memory" + expected = EconomicImpactResult.error( + f"Simulation entrypoint execution failed: out of memory. {BREAKDOWN_RETRY_NOTE}" ) - # The failure is not kept for replay: the retry submits a new job. + assert failed == expected + # Within two minutes the stored failure is returned without a new job. + assert within_two_minutes == expected + mock_simulation_entrypoint.get_execution_by_id.assert_called_once_with( + "first-breakdown-job" + ) + # After that the record has expired and the next request submits again. assert retried == EconomicImpactResult.computing() assert mock_simulation_entrypoint.run.call_count == 2 stored = economy_service._reform_impacts.get_recent_reform_impacts(5) @@ -645,8 +672,93 @@ def test_a_failed_breakdown_job_is_not_replayed_and_the_next_request_resubmits( @pytest.mark.usefixtures("uk_package_versions") -def test_a_failed_national_job_is_still_replayed( +def test_a_failed_breakdown_is_recorded_after_a_thousand_newer_jobs( + economy_service, mock_simulation_entrypoint, reform_impact_backend +): + mock_simulation_entrypoint.get_execution_id.side_effect = [ + "old-breakdown-job", + "new-breakdown-job", + ] + request = _uk_request({"breakdown": "local_areas"}) + economy_service.get_economic_impact(request, time_period="2026") + # 1,000 newer economy jobs push the breakdown out of the recency index, + # which keeps only the newest 1,000 records across every scope. + cache = economy_service._reform_impacts._cache + breakdown_record = cache.get_by_execution_id("old-breakdown-job") + assert breakdown_record is not None + for index in range(REFORM_IMPACT_INDEX_LIMIT): + cache.set( + replace( + breakdown_record, + reform_impact_id=index, + region="country/wales", + execution_id=f"newer-job-{index}", + start_time=breakdown_record.start_time + timedelta(seconds=index + 1), + ) + ) + assert "old-breakdown-job" not in { + impact.execution_id for impact in cache.recent(REFORM_IMPACT_INDEX_LIMIT) + } + _finish_jobs_with( + mock_simulation_entrypoint, status="failed", error="out of memory" + ) + + failed = economy_service.get_economic_impact(request, time_period="2026") + reform_impact_backend.advance(121) + retried = economy_service.get_economic_impact(request, time_period="2026") + + assert failed.status == ImpactStatus.ERROR + assert retried == EconomicImpactResult.computing() + assert mock_simulation_entrypoint.run.call_count == 2 + mock_simulation_entrypoint.get_execution_by_id.assert_called_once_with( + "old-breakdown-job" + ) + + +@pytest.mark.usefixtures("uk_package_versions") +def test_a_late_poll_of_a_failed_breakdown_leaves_the_next_job_alone( economy_service, mock_simulation_entrypoint +): + mock_simulation_entrypoint.get_execution_id.side_effect = ["first-breakdown-job"] + request = _uk_request({"breakdown": "local_areas"}) + economy_service.get_economic_impact(request, time_period="2026") + cache = economy_service._reform_impacts._cache + first_record = cache.get_by_execution_id("first-breakdown-job") + assert first_record is not None + # A Retry elsewhere has already stored the next job for the same scope and + # options hash, also computing. + cache.set( + replace( + first_record, + reform_impact_id=first_record.reform_impact_id + 1, + execution_id="second-breakdown-job", + start_time=first_record.start_time + timedelta(seconds=1), + ) + ) + setup_options = economy_service._build_economic_impact_setup_options( + request, time_period="2026" + ) + economy_service._resolve_runtime_bundle_for_setup_options(setup_options) + + # A slow poller that read the first job before the Retry now sees it fail. + result = economy_service._handle_execution_state( + setup_options=setup_options, + execution_state="failed", + reform_impact=first_record, + execution=ModalSimulationExecution( + job_id="first-breakdown-job", status="failed", error="out of memory" + ), + ) + + assert result.status == ImpactStatus.ERROR + second_record = cache.get_by_execution_id("second-breakdown-job") + assert second_record is not None + assert second_record.status == "computing" + + +@pytest.mark.usefixtures("uk_package_versions") +def test_a_failed_national_job_is_still_replayed( + economy_service, mock_simulation_entrypoint, reform_impact_backend ): _finish_jobs_with( mock_simulation_entrypoint, status="failed", error="out of memory" @@ -655,6 +767,7 @@ def test_a_failed_national_job_is_still_replayed( economy_service.get_economic_impact(request, time_period="2026") failed = economy_service.get_economic_impact(request, time_period="2026") + reform_impact_backend.advance(121) replayed = economy_service.get_economic_impact(request, time_period="2026") expected = EconomicImpactResult.error( @@ -679,7 +792,11 @@ def test_a_failed_national_job_is_still_replayed( ids=["national-result", "other-kind", "no-result"], ) def test_a_breakdown_job_without_a_local_areas_result_is_not_stored( - economy_service, mock_simulation_entrypoint, result, reported_kind + economy_service, + mock_simulation_entrypoint, + reform_impact_backend, + result, + reported_kind, ): mock_simulation_entrypoint.get_execution_id.side_effect = [ "first-breakdown-job", @@ -690,13 +807,19 @@ def test_a_breakdown_job_without_a_local_areas_result_is_not_stored( economy_service.get_economic_impact(request, time_period="2026") completed = economy_service.get_economic_impact(request, time_period="2026") + within_two_minutes = economy_service.get_economic_impact( + request, time_period="2026" + ) + reform_impact_backend.advance(121) retried = economy_service.get_economic_impact(request, time_period="2026") - assert completed == EconomicImpactResult.error( + expected = EconomicImpactResult.error( "The simulation API returned a result of kind " f"{reported_kind} for a local-area breakdown request, which needs kind " - "'local_areas'; the result was not stored" + f"'local_areas'; the result was not stored. {BREAKDOWN_RETRY_NOTE}" ) + assert completed == expected + assert within_two_minutes == expected assert retried == EconomicImpactResult.computing() assert mock_simulation_entrypoint.run.call_count == 2 stored = economy_service._reform_impacts.get_recent_reform_impacts(5) diff --git a/tests/unit/services/test_reform_impacts_service.py b/tests/unit/services/test_reform_impacts_service.py index 00f7af708..80858811f 100644 --- a/tests/unit/services/test_reform_impacts_service.py +++ b/tests/unit/services/test_reform_impacts_service.py @@ -197,3 +197,32 @@ def test_reform_impact_service_deletes_only_matching_computing_rows( def test_reform_impact_transitions_return_none_for_missing_execution(service): assert service.set_error_reform_impact("missing-job", message="missing") is None + + +def test_a_briefly_kept_error_expires_and_leaves_other_records() -> None: + backend = InMemoryCacheBackend() + service = ReformImpactsService( + ReformImpactCache(backend, CacheNamespace("test", "api")) + ) + _create_impact(service, execution_id="failed-job", options_hash="hash", day=1) + _create_impact(service, execution_id="next-job", options_hash="hash", day=2) + identity = ReformImpactIdentity(scope=SCOPE, api_version="1", options_hash="hash") + + failed = service.set_error_reform_impact( + "failed-job", message="out of memory", keep_seconds=120 + ) + backend.advance(119) + kept = service.get_all_reform_impacts(identity) + backend.advance(2) + after_expiry = service.get_all_reform_impacts(identity) + + assert failed is not None + assert failed.status == "error" + assert [(impact.execution_id, impact.status) for impact in kept] == [ + ("next-job", "computing"), + ("failed-job", "error"), + ] + # Only the failed job's record expires; the next job's record is untouched. + assert [(impact.execution_id, impact.status) for impact in after_expiry] == [ + ("next-job", "computing") + ] From c2cd5c1344b0a911aaaef739ecf0fcfbf49a18cf Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:56:58 +0400 Subject: [PATCH 17/18] Note that UK national results no longer carry area impacts A changelog fragment for callers: after the .py 6.10.0 update, UK national and country economy results no longer include `constituency_impact` or `local_authority_impact`, because .py runs them on a national dataset with no area codes. Clients get those impacts with a separate request that adds `breakdown=local_areas`. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown-national-results.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/uk-local-area-breakdown-national-results.changed.md diff --git a/changelog.d/uk-local-area-breakdown-national-results.changed.md b/changelog.d/uk-local-area-breakdown-national-results.changed.md new file mode 100644 index 000000000..aaf2ef310 --- /dev/null +++ b/changelog.d/uk-local-area-breakdown-national-results.changed.md @@ -0,0 +1 @@ +After the .py 6.10.0 update, UK national and country economy results (`region=uk` or `country/*`) no longer include `constituency_impact` or `local_authority_impact`, because .py runs them on a national dataset with no area codes. Clients get those impacts with a separate request that adds `breakdown=local_areas`. From 7cdf9f5f69507d568e46bb4cc03c1d57a80d94de Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 23 Oct 2026 01:41:29 +0400 Subject: [PATCH 18/18] Add Ards and North Down and the 2023 unitaries' positions to the UK list `policyengine_api/data/local_authorities_2021.csv`, which the UK metadata's `economy_options.region` list reads, had 360 local authorities. It lacked Ards and North Down (`N09000011`), the 11th Northern Ireland district, so `local_authority/Ards and North Down` could not be selected, and it gave the four unitary authorities created on 1 April 2023 (Cumberland `E06000063`, Westmorland and Furness `E06000064`, North Yorkshire `E06000065`, Somerset `E06000066`) the shared (0, 0) placeholder position. The file now has the missing row (`N09000011,-3.0,16.0,Ards and North Down`, after `N09000010`, in the file's code order and number format), and the four unitaries take their x and y from .py 6.10.0's packaged `local_authorities_2023.csv`. Its codes, names and positions now equal that 2023 list for all 361 authorities. The file name is unchanged, to avoid renaming it everywhere it is read. The new name resolves in .py 6.10.0's installed UK region registry, whose label "Ards and North Down" belongs to `local_authority/N09000011`; the simulation API matches a `local_authority/{name}` request to the registry by code or label, ignoring case. A test checks that every offered local-authority name resolves this way. Tests written first. Before the change, 6 failed for the expected reason: the metadata and the file had 360 local authorities, not 361; `local_authority/Ards and North Down` was not offered; Northern Ireland had 10 districts, not 11; the file differed from .py's list in the missing code and the four unitaries' (0, 0) positions; and those four were still at (0, 0). The registry-resolution test passed before and after for the 360 existing names. Refs #4021 Co-Authored-By: Claude Opus 5.5 --- changelog.d/uk-local-area-breakdown.fixed.md | 2 +- .../data/local_authorities_2021.csv | 9 +- tests/unit/test_country.py | 90 +++++++++++++++++-- 3 files changed, 91 insertions(+), 10 deletions(-) diff --git a/changelog.d/uk-local-area-breakdown.fixed.md b/changelog.d/uk-local-area-breakdown.fixed.md index 2f1d4275c..7c2dbf73a 100644 --- a/changelog.d/uk-local-area-breakdown.fixed.md +++ b/changelog.d/uk-local-area-breakdown.fixed.md @@ -1 +1 @@ -UK economy requests for Northern Ireland (`country/ni` in the Public API metadata) now send `country/northern_ireland`, the name .py's UK region registry uses, to the simulation API. The cached result keeps the `country/ni` region. The v2 catalog gives UK constituency and local-authority regions the local-area dataset .py 6.10.0 routes them to (`microcosm_uk_2024_25_dense`) and lists that dataset, instead of the national file, which has no area codes. +UK economy requests for Northern Ireland (`country/ni` in the Public API metadata) now send `country/northern_ireland`, the name .py's UK region registry uses, to the simulation API. The cached result keeps the `country/ni` region. The v2 catalog gives UK constituency and local-authority regions the local-area dataset .py 6.10.0 routes them to (`microcosm_uk_2024_25_dense`) and lists that dataset, instead of the national file, which has no area codes. The UK metadata now offers Ards and North Down (`N09000011`), the 11th Northern Ireland district, which the local-authority list lacked, so it lists 361 local authorities. The four unitary authorities created in April 2023 (Cumberland, Westmorland and Furness, North Yorkshire and Somerset) now have their own hex positions instead of the shared (0, 0) placeholder. The list's codes, names and positions now match .py 6.10.0's packaged 2023 list. diff --git a/policyengine_api/data/local_authorities_2021.csv b/policyengine_api/data/local_authorities_2021.csv index 9fcf922ed..2cd23eac8 100644 --- a/policyengine_api/data/local_authorities_2021.csv +++ b/policyengine_api/data/local_authorities_2021.csv @@ -58,10 +58,10 @@ E06000059,-1.0,0.0,Dorset E06000060,5.0,5.0,Buckinghamshire E06000061,9.0,9.0,North Northamptonshire E06000062,7.0,6.0,West Northamptonshire -E06000063,0.0,0.0,Cumberland -E06000064,0.0,0.0,Westmorland and Furness -E06000065,0.0,0.0,North Yorkshire -E06000066,0.0,0.0,Somerset +E06000063,4.0,18.0,Cumberland +E06000064,4.0,17.0,Westmorland and Furness +E06000065,8.0,17.0,North Yorkshire +E06000066,0.0,1.0,Somerset E07000008,12.0,8.0,Cambridge E07000009,12.0,9.0,East Cambridgeshire E07000010,13.0,10.0,Fenland @@ -305,6 +305,7 @@ N09000007,-5.0,15.0,Lisburn and Castlereagh N09000008,-4.0,18.0,Mid and East Antrim N09000009,-5.0,17.0,Mid Ulster N09000010,-4.0,15.0,"Newry, Mourne and Down" +N09000011,-3.0,16.0,Ards and North Down S12000005,2.0,24.0,Clackmannanshire S12000006,4.0,20.0,Dumfries and Galloway S12000008,3.0,20.0,East Ayrshire diff --git a/tests/unit/test_country.py b/tests/unit/test_country.py index 75a6b3321..48af0f3eb 100644 --- a/tests/unit/test_country.py +++ b/tests/unit/test_country.py @@ -5,6 +5,8 @@ import numpy as np import pandas as pd import pytest +from policyengine.countries.uk.regions import build_uk_region_registry +from policyengine.data.uk_geography_assets import packaged_uk_area_list_path from policyengine_core.enums import EnumArray from policyengine_core.model_api import Enum @@ -213,12 +215,45 @@ def test__uk_metadata_contains_local_authorities(self, uk_regions): ] assert len(local_authority_regions) > 0 - def test__uk_has_360_local_authorities(self, uk_regions): + def test__uk_has_361_local_authorities(self, uk_regions): """Verify the correct number of local authorities are loaded.""" local_authority_regions = [ r for r in uk_regions if r.get("type") == "local_authority" ] - assert len(local_authority_regions) == 360 + assert len(local_authority_regions) == 361 + + def test__ards_and_north_down_is_offered(self, uk_regions): + """The 11th Northern Ireland district (N09000011) is a selectable region.""" + assert { + "name": "local_authority/Ards and North Down", + "label": "Ards and North Down", + "type": "local_authority", + } in uk_regions + + def test__every_local_authority_resolves_in_the_policyengine_registry( + self, uk_regions + ): + """Each offered name matches a .py registry label, ignoring case. + + The simulation API resolves ``local_authority/{name}`` to the registry + code whose label matches the name, ignoring case. + """ + registry = build_uk_region_registry( + include_constituencies=False, include_local_authorities=True + ) + registry_labels = { + region.label.casefold(): region.code + for region in registry.get_by_type("local_authority") + } + offered = [ + region["label"] + for region in uk_regions + if region.get("type") == "local_authority" + ] + assert [ + label for label in offered if label.casefold() not in registry_labels + ] == [] + assert registry_labels["ards and north down"] == "local_authority/N09000011" def test__local_authority_regions_have_correct_name_format(self, uk_regions): """Verify local authority region names have the correct prefix.""" @@ -295,9 +330,44 @@ def test__file_has_correct_columns(self, local_authorities_df): expected_columns = {"code", "name", "x", "y"} assert expected_columns == set(local_authorities_df.columns) - def test__file_has_360_local_authorities(self, local_authorities_df): + def test__file_has_361_local_authorities(self, local_authorities_df): """Verify the correct number of local authorities in file.""" - assert len(local_authorities_df) == 360 + assert len(local_authorities_df) == 361 + + def test__file_matches_the_installed_policyengine_list(self, local_authorities_df): + """Codes, names and hex positions equal .py's packaged 2023 list. + + .py's list is read from the installed package, so a .py update that + changes the area frame makes this test fail until the file follows. + """ + policyengine_list = pd.read_csv(packaged_uk_area_list_path("local_authority")) + + def by_code(frame: pd.DataFrame) -> dict[str, tuple[str, float, float]]: + return { + row.code: (row.name, float(row.x), float(row.y)) + for row in frame.itertuples(index=False) + } + + assert by_code(local_authorities_df) == by_code(policyengine_list) + + def test__april_2023_unitaries_leave_the_origin_placeholder( + self, local_authorities_df + ): + """The four unitaries created in April 2023 have their own hex positions. + + They used to share the (0, 0) placeholder. Another authority may still + sit at (0, 0) when .py's list puts it there; the comparison with .py's + list above covers every position. + """ + april_2023_unitaries = local_authorities_df[ + local_authorities_df["code"].isin( + ["E06000063", "E06000064", "E06000065", "E06000066"] + ) + ] + assert len(april_2023_unitaries) == 4 + assert not ( + (april_2023_unitaries["x"] == 0) & (april_2023_unitaries["y"] == 0) + ).any() def test__all_codes_are_valid_ons_codes(self, local_authorities_df): """Verify all codes follow ONS local authority code patterns.""" @@ -321,7 +391,7 @@ def test__english_local_authorities_have_e_prefix(self, local_authorities_df): english_las = local_authorities_df[ local_authorities_df["code"].str.startswith("E") ] - # England has 296 local authorities (majority of the 360 total) + # England has 296 local authorities (majority of the 361 total) assert len(english_las) == 296 def test__scottish_local_authorities_have_s_prefix(self, local_authorities_df): @@ -340,6 +410,16 @@ def test__welsh_local_authorities_have_w_prefix(self, local_authorities_df): # Wales has 22 principal areas assert len(welsh_las) == 22 + def test__northern_irish_local_authorities_have_n_prefix( + self, local_authorities_df + ): + """Verify all 11 Northern Ireland districts have N prefix codes.""" + northern_irish_las = local_authorities_df[ + local_authorities_df["code"].str.startswith("N") + ] + assert len(northern_irish_las) == 11 + assert "N09000011" in set(northern_irish_las["code"]) + class TestSimulationCompatibility: def test__create_simulation_uses_legacy_tax_benefit_system_signature(self):