From 41bdcc09e2cb83e07ec51979fca814c1a06ad41a Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:20:15 +0400 Subject: [PATCH 1/2] Remove x and y from the contract's UK area records ConstituencyImpactRecord and LocalAuthorityImpactRecord no longer have x or y fields. Both records forbid extra fields, so a record carrying either, even as null, is refused, and the gateway reports a job whose result has one as failed. Apps deployed for older policyengine.py versions still send both, so this must not merge until no such app is routed. Nothing uses pydantic's exclude_if any more, so the contract's pydantic floor returns to 2.0, and the five locks that record it return to their state on main. Regenerate the gateway's golden OpenAPI, which drops both properties and the description's paragraph about them. Written red-green: the contract tests failed first with "DID NOT RAISE" for records and job results carrying x or y, and because the records still declared both fields; the gateway returned 200 instead of 500 for a result with x and y; and the generated client's record models still took x and y. Co-Authored-By: Claude Opus 5.5 --- changelog_entry.yaml | 1 + .../pyproject.toml | 2 +- .../macro_output.py | 14 +--- .../tests/test_local_area_breakdown.py | 67 +++++++++---------- .../tests/test_macro_output_uk_geography.py | 65 +++++++----------- libs/policyengine-simulation-contract/uv.lock | 2 +- libs/policyengine-stage12-persistence/uv.lock | 2 +- .../tests/simulation/test_calculate.py | 25 ++----- .../policyengine-simulation-entry/uv.lock | 2 +- .../policyengine-simulation-executor/uv.lock | 2 +- .../tests/golden/openapi.json | 46 +------------ .../tests/test_endpoints.py | 49 +++++++------- .../policyengine-simulation-gateway/uv.lock | 2 +- 13 files changed, 95 insertions(+), 184 deletions(-) diff --git a/changelog_entry.yaml b/changelog_entry.yaml index d9042e4e0..4a8f0516c 100644 --- a/changelog_entry.yaml +++ b/changelog_entry.yaml @@ -18,6 +18,7 @@ - Made x and y, each UK constituency and local-authority record's integer map position, optional in the contract; a record now validates without them and its JSON leaves them out instead of writing null, while records from apps deployed for older policyengine.py versions, which still send both, validate and keep them; the gateway's OpenAPI no longer lists them as required, and the contract now requires pydantic 2.12 or later, the first release with field-level exclude_if - Stopped sending x and y on UK constituency and local-authority records; the executor no longer reads the x and y columns of policyengine.py's constituency lookup or attaches its packaged local-authority positions, and no longer refuses a published area without a map position, so results from apps deployed from now on carry neither key, while apps deployed for older policyengine.py versions keep sending both; the beta integration check of a local-area breakdown now requires records without them - Stopped shipping coordinates_lad22.csv and coordinates_lad23.csv in the executor; each boundary version's authority codes now come from codes_lad22.csv and codes_lad23.csv, which list exactly the codes the coordinate files did, so the LAD22 and LAD23 detection and its checks (names cover both lists, and each version's distinguishing codes match its list) are unchanged, and the Stage 12 static runtime file smoke reports lad22_codes and lad23_codes instead of lad22_coordinates and lad23_coordinates + - Removed x and y from UK constituency and local-authority records in the contract and the gateway's OpenAPI; a record that carries either, even as null, is now refused, so a result from an app deployed for an older policyengine.py version that still sends them fails validation, and the contract's pydantic floor returns to 2.0 since nothing uses exclude_if any more - Removed the UK constituency and local-authority weight-matrix region fallback (_build_uk_weight_replacement_region, _require_certified_uk_weight_matrix and its Stage 12 call); each deployment rebuilds only the app for the current policyengine.py version, now 6.10.0, which routes those regions to the local-area dataset, and apps deployed for older versions are not rebuilt and keep their own code; on a bundle that does not route them, an area region is now an unsupported region - Made UK-wide and nation-level runs on a policyengine.py bundle that routes UK area regions to a local-area release dataset (6.10.0) load only the national dataset and return null for constituency_impact and local_authority_impact, whose results come from a breakdown=local_areas run instead; on a bundle that does not route them, those runs still compute both from the national dataset, and Stage 12 leaves the same breakdowns out of the same reports - Enabled every ruff rule (select ALL), ignoring only COM812 and ISC001, which conflict with ruff format, and the D203 and D213 halves of the mutually exclusive docstring pairs; complex functions, long parameter lists and magic numbers were split into smaller helpers and grouped parameters, each behind new characterization tests, across the contract and persistence validation, entry settings, app factory and Stage 12 adapter, gateway route resolution, Stage 12 coordination, workers, parity and qualification, the precompute and national reduce, the routing publisher and its command line, the release sentinel, the package update helper and the shared rules tool; Modal function, FastAPI route, Pydantic validator and command-line signatures and every OpenAPI document are unchanged; the contract library's batch-state builder and SPM selection resolver take their route values as one grouped argument, the simulation observability initializer drops a service_version parameter no caller passed, and no PLR0913 suppression is needed diff --git a/libs/policyengine-simulation-contract/pyproject.toml b/libs/policyengine-simulation-contract/pyproject.toml index 1de678f91..7214eee3b 100644 --- a/libs/policyengine-simulation-contract/pyproject.toml +++ b/libs/policyengine-simulation-contract/pyproject.toml @@ -8,7 +8,7 @@ authors = [ license = {file = "../../LICENSE"} requires-python = ">=3.13" dependencies = [ - "pydantic>=2.12", + "pydantic>=2.0", "packaging>=24", # gateway_models embeds the TelemetryEnvelope from the observability lib. "policyengine-simulation-observability", diff --git a/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/macro_output.py b/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/macro_output.py index 1fbece187..29cbb52b0 100644 --- a/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/macro_output.py +++ b/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/macro_output.py @@ -4,7 +4,7 @@ from typing import Generic, Literal, TypeVar -from pydantic import BaseModel, ConfigDict, Field, RootModel +from pydantic import BaseModel, ConfigDict, RootModel from policyengine_simulation_contract.spm import SPMComparisonProvenance @@ -237,10 +237,6 @@ class CliffImpactOutput(MacroOutputModel): UKGeographyDisclosureTier = Literal["reported", "approximate"] -def _is_none(value: object) -> bool: - return value is None - - class UKAreaDisclosureFields(MacroOutputModel): """Disclosure tier and 90% intervals of a published UK area's income changes. @@ -269,16 +265,10 @@ class ConstituencyImpactRecord(UKAreaDisclosureFields): ``relative_household_income_change`` the relative change in the area's total household net income, both rounded to the order of their standard error. ``population`` is the weighted number of households. - - ``x`` and ``y`` are the area's integer map position. Results from apps - deployed for older policyengine.py versions still carry them; current - results leave both out rather than sending them as null. """ constituency_code: str constituency_name: str - x: int | None = Field(default=None, exclude_if=_is_none) - y: int | None = Field(default=None, exclude_if=_is_none) average_household_income_change: float relative_household_income_change: float population: float @@ -292,8 +282,6 @@ class LocalAuthorityImpactRecord(UKAreaDisclosureFields): local_authority_code: str local_authority_name: str - x: int | None = Field(default=None, exclude_if=_is_none) - y: int | None = Field(default=None, exclude_if=_is_none) average_household_income_change: float relative_household_income_change: float population: float diff --git a/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py b/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py index 11cda0173..c031cdc82 100644 --- a/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py +++ b/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py @@ -23,8 +23,6 @@ CONSTITUENCY = { "constituency_code": "E14001063", "constituency_name": "Aldershot", - "x": 56, - "y": -40, "average_household_income_change": -150.0, "relative_household_income_change": -0.0026, "population": 40_530.0, @@ -37,8 +35,6 @@ LOCAL_AUTHORITY = { "local_authority_code": "E06000063", "local_authority_name": "Cumberland", - "x": 4, - "y": 18, "average_household_income_change": -120.0, "relative_household_income_change": -0.002, "population": 120_000.0, @@ -203,43 +199,42 @@ def test_each_area_level_reads_its_records_as_its_own_type(): ] -def _without_map_position(record: dict[str, object]) -> dict[str, object]: - return {key: value for key, value in record.items() if key not in {"x", "y"}} - - -# Newer executors leave out each area's map position (x and y). -BREAKDOWN_WITHOUT_MAP_POSITIONS = { - **BREAKDOWN, - "constituency_impact": [_without_map_position(CONSTITUENCY)], - "local_authority_impact": [_without_map_position(LOCAL_AUTHORITY)], -} - - -def test_a_breakdown_without_map_positions_round_trips_without_them(): - output = LocalAreaBreakdownOutput.model_validate(BREAKDOWN_WITHOUT_MAP_POSITIONS) - - assert output.model_dump(mode="json") == BREAKDOWN_WITHOUT_MAP_POSITIONS - - -def test_a_job_status_reads_a_breakdown_without_map_positions(): - response = JobStatusResponse( - status="complete", result=BREAKDOWN_WITHOUT_MAP_POSITIONS - ) - - assert isinstance(response.result, LocalAreaBreakdownOutput) - assert response.model_dump(mode="json")["result"] == ( - BREAKDOWN_WITHOUT_MAP_POSITIONS - ) - - -def test_a_national_result_reads_area_lists_without_map_positions(): +def test_a_national_result_reads_its_area_lists(): national = { **NATIONAL, - "constituency_impact": [_without_map_position(CONSTITUENCY)], - "local_authority_impact": [_without_map_position(LOCAL_AUTHORITY)], + "constituency_impact": [CONSTITUENCY], + "local_authority_impact": [LOCAL_AUTHORITY], } response = JobStatusResponse(status="complete", result=national) assert isinstance(response.result, SingleYearMacroOutput) assert response.model_dump(mode="json")["result"] == national + + +# Apps deployed for older policyengine.py versions still send each area's map +# position as x and y. The contract has no such fields, so their results are +# refused: this contract is deployed only once no such app is routed. +@pytest.mark.parametrize( + "result", + [ + pytest.param( + {**BREAKDOWN, "constituency_impact": [{**CONSTITUENCY, "x": 56, "y": -40}]}, + id="breakdown-constituency", + ), + pytest.param( + { + **BREAKDOWN, + "local_authority_impact": [{**LOCAL_AUTHORITY, "x": 4, "y": 18}], + }, + id="breakdown-local-authority", + ), + pytest.param( + {**NATIONAL, "constituency_impact": [{**CONSTITUENCY, "x": 56, "y": -40}]}, + id="national-constituency", + ), + ], +) +def test_a_job_status_refuses_area_records_with_x_and_y(result): + with pytest.raises(ValidationError, match="Extra inputs are not permitted"): + JobStatusResponse(status="complete", result=result) diff --git a/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py b/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py index 9adf5984f..085d0e2d4 100644 --- a/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py +++ b/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py @@ -12,11 +12,9 @@ ) from pydantic import ValidationError -STORED_BEFORE_DISCLOSURE = { +RECORD_WITHOUT_DISCLOSURE = { "constituency_code": "E14001063", "constituency_name": "Aldershot", - "x": 56, - "y": -40, "average_household_income_change": -150.95, "relative_household_income_change": -0.0026, "population": 40_530.65, @@ -30,8 +28,8 @@ } -def test_records_stored_before_disclosure_control_still_validate() -> None: - record = ConstituencyImpactRecord.model_validate(STORED_BEFORE_DISCLOSURE) +def test_records_without_disclosure_fields_still_validate() -> None: + record = ConstituencyImpactRecord.model_validate(RECORD_WITHOUT_DISCLOSURE) assert record.disclosure_tier is None assert record.average_household_income_change_lower is None @@ -43,8 +41,6 @@ def test_records_carry_the_disclosure_tier_and_intervals() -> None: { "local_authority_code": "E06000063", "local_authority_name": "Cumberland", - "x": 4, - "y": 18, "average_household_income_change": -150.0, "relative_household_income_change": -0.0026, "population": 120_000.0, @@ -60,12 +56,12 @@ def test_a_withheld_tier_is_not_a_record() -> None: # Withheld areas are left out of the list, so no record holds that tier. with pytest.raises(ValidationError): ConstituencyImpactRecord.model_validate( - {**STORED_BEFORE_DISCLOSURE, "disclosure_tier": "withheld"} + {**RECORD_WITHOUT_DISCLOSURE, "disclosure_tier": "withheld"} ) def test_published_values_stay_required() -> None: - record = {**STORED_BEFORE_DISCLOSURE, **DISCLOSURE_FIELDS} + record = {**RECORD_WITHOUT_DISCLOSURE, **DISCLOSURE_FIELDS} record["average_household_income_change"] = None with pytest.raises(ValidationError): @@ -76,17 +72,10 @@ def test_an_empty_area_list_is_valid() -> None: assert GeographicImpactOutput.model_validate([]).root == [] -# Executors built for older policyengine.py versions are never rebuilt, and -# they keep sending each area's integer map position as x and y. Newer -# executors send neither. -CONSTITUENCY_WITHOUT_MAP_POSITION = { - **{ - key: value - for key, value in STORED_BEFORE_DISCLOSURE.items() - if key not in {"x", "y"} - }, - **DISCLOSURE_FIELDS, -} +# The records have no x or y (an area's integer map position). Apps deployed +# for older policyengine.py versions still send both, and their records are +# refused, so this contract is deployed only once no such app is routed. +CONSTITUENCY_WITHOUT_MAP_POSITION = {**RECORD_WITHOUT_DISCLOSURE, **DISCLOSURE_FIELDS} LOCAL_AUTHORITY_WITHOUT_MAP_POSITION = { "local_authority_code": "E06000063", "local_authority_name": "Cumberland", @@ -108,41 +97,35 @@ def test_an_empty_area_list_is_valid() -> None: @pytest.mark.parametrize(("record_type", "payload"), AREA_RECORDS_WITHOUT_MAP_POSITION) -def test_records_without_a_map_position_validate_and_dump_without_one( +def test_records_validate_and_dump_without_x_or_y( record_type: type[ConstituencyImpactRecord | LocalAuthorityImpactRecord], payload: dict[str, object], ) -> None: record = record_type.model_validate(payload) - assert record.x is None - assert record.y is None + assert {"x", "y"}.isdisjoint(record_type.model_fields) assert record.model_dump(mode="python") == payload assert record.model_dump(mode="json") == payload assert json.loads(record.model_dump_json()) == payload @pytest.mark.parametrize(("record_type", "payload"), AREA_RECORDS_WITHOUT_MAP_POSITION) -def test_records_from_older_executors_keep_their_map_position( - record_type: type[ConstituencyImpactRecord | LocalAuthorityImpactRecord], - payload: dict[str, object], -) -> None: - sent_by_older_executor = {**payload, "x": 56, "y": -40} - - record = record_type.model_validate(sent_by_older_executor) - - assert (record.x, record.y) == (56, -40) - assert record.model_dump(mode="json") == sent_by_older_executor - - -@pytest.mark.parametrize(("record_type", "payload"), AREA_RECORDS_WITHOUT_MAP_POSITION) -def test_a_null_map_position_is_left_out_of_the_dump( +@pytest.mark.parametrize( + "map_position", + [ + pytest.param({"x": 56, "y": -40}, id="both"), + pytest.param({"x": 56}, id="x-only"), + pytest.param({"y": -40}, id="y-only"), + pytest.param({"x": None, "y": None}, id="null"), + ], +) +def test_records_with_x_or_y_are_refused( record_type: type[ConstituencyImpactRecord | LocalAuthorityImpactRecord], payload: dict[str, object], + map_position: dict[str, int | None], ) -> None: - record = record_type.model_validate({**payload, "x": None, "y": None}) - - assert record.model_dump(mode="json") == payload - assert json.loads(record.model_dump_json()) == payload + with pytest.raises(ValidationError, match="Extra inputs are not permitted"): + record_type.model_validate({**payload, **map_position}) def test_an_area_list_without_map_positions_dumps_without_them() -> None: diff --git a/libs/policyengine-simulation-contract/uv.lock b/libs/policyengine-simulation-contract/uv.lock index 0d52148a2..a1b23fb94 100644 --- a/libs/policyengine-simulation-contract/uv.lock +++ b/libs/policyengine-simulation-contract/uv.lock @@ -1122,7 +1122,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.12" }, + { name = "pydantic", specifier = ">=2.0" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, diff --git a/libs/policyengine-stage12-persistence/uv.lock b/libs/policyengine-stage12-persistence/uv.lock index 0fc6b2969..e8d8bbe36 100644 --- a/libs/policyengine-stage12-persistence/uv.lock +++ b/libs/policyengine-stage12-persistence/uv.lock @@ -604,7 +604,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.12" }, + { name = "pydantic", specifier = ">=2.0" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, diff --git a/projects/policyengine-apis-integ/tests/simulation/test_calculate.py b/projects/policyengine-apis-integ/tests/simulation/test_calculate.py index cd02f1aff..75f97e300 100644 --- a/projects/policyengine-apis-integ/tests/simulation/test_calculate.py +++ b/projects/policyengine-apis-integ/tests/simulation/test_calculate.py @@ -4,6 +4,7 @@ that economy-wide simulations complete successfully. """ +import inspect import json import time from collections.abc import Mapping @@ -14,10 +15,12 @@ import pytest from policyengine_api_simulation_client import AuthenticatedClient, Client from policyengine_api_simulation_client.models import ( + ConstituencyImpactRecord, JobStage, JobStatusResponse, JobSubmitResponse, LocalAreaBreakdownOutput, + LocalAuthorityImpactRecord, SimulationRequest, ) @@ -373,26 +376,10 @@ def test_breakdown_assertions_accept_generated_client_objects() -> None: @pytest.mark.parametrize( - "map_position", - [ - pytest.param({}, id="current-app"), - pytest.param({"x": 56, "y": -40}, id="older-app"), - ], + "record_model", [ConstituencyImpactRecord, LocalAuthorityImpactRecord] ) -def test_generated_client_reads_area_records_with_and_without_x_and_y( - map_position: dict[str, int], -) -> None: - # Apps deployed for older policyengine.py versions still send x and y. - breakdown = { - **_BREAKDOWN_RESULT, - "constituency_impact": [{**_BREAKDOWN_CONSTITUENCY, **map_position}], - "local_authority_impact": [{**_BREAKDOWN_LOCAL_AUTHORITY, **map_position}], - } - - status = JobStatusResponse.from_dict({"status": "complete", "result": breakdown}) - - assert isinstance(status.result, LocalAreaBreakdownOutput) - assert status.result.to_dict() == breakdown +def test_generated_client_area_records_have_no_x_or_y(record_model: type) -> None: + assert {"x", "y"}.isdisjoint(inspect.signature(record_model).parameters) @pytest.mark.parametrize( diff --git a/projects/policyengine-simulation-entry/uv.lock b/projects/policyengine-simulation-entry/uv.lock index 654495aa8..943b788d5 100644 --- a/projects/policyengine-simulation-entry/uv.lock +++ b/projects/policyengine-simulation-entry/uv.lock @@ -881,7 +881,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../../libs/policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.12" }, + { name = "pydantic", specifier = ">=2.0" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, diff --git a/projects/policyengine-simulation-executor/uv.lock b/projects/policyengine-simulation-executor/uv.lock index 8469d617e..7453cde31 100644 --- a/projects/policyengine-simulation-executor/uv.lock +++ b/projects/policyengine-simulation-executor/uv.lock @@ -1883,7 +1883,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../../libs/policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.12" }, + { name = "pydantic", specifier = ">=2.0" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, diff --git a/projects/policyengine-simulation-gateway/tests/golden/openapi.json b/projects/policyengine-simulation-gateway/tests/golden/openapi.json index f7a431ce2..81bfccca2 100644 --- a/projects/policyengine-simulation-gateway/tests/golden/openapi.json +++ b/projects/policyengine-simulation-gateway/tests/golden/openapi.json @@ -1100,28 +1100,6 @@ "type": "string", "title": "Constituency Name" }, - "x": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "title": "X" - }, - "y": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "title": "Y" - }, "average_household_income_change": { "type": "number", "title": "Average Household Income Change" @@ -1145,7 +1123,7 @@ "population" ], "title": "ConstituencyImpactRecord", - "description": "Reform impact on household net income in one UK parliamentary constituency.\n\n``average_household_income_change`` is the average change per household and\n``relative_household_income_change`` the relative change in the area's total\nhousehold net income, both rounded to the order of their standard error.\n``population`` is the weighted number of households.\n\n``x`` and ``y`` are the area's integer map position. Results from apps\ndeployed for older policyengine.py versions still carry them; current\nresults leave both out rather than sending them as null." + "description": "Reform impact on household net income in one UK parliamentary constituency.\n\n``average_household_income_change`` is the average change per household and\n``relative_household_income_change`` the relative change in the area's total\nhousehold net income, both rounded to the order of their standard error.\n``population`` is the weighted number of households." }, "DecileOutput": { "properties": { @@ -1862,28 +1840,6 @@ "type": "string", "title": "Local Authority Name" }, - "x": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "title": "X" - }, - "y": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "title": "Y" - }, "average_household_income_change": { "type": "number", "title": "Average Household Income Change" diff --git a/projects/policyengine-simulation-gateway/tests/test_endpoints.py b/projects/policyengine-simulation-gateway/tests/test_endpoints.py index 1c6664f52..be4bfb932 100644 --- a/projects/policyengine-simulation-gateway/tests/test_endpoints.py +++ b/projects/policyengine-simulation-gateway/tests/test_endpoints.py @@ -28,8 +28,6 @@ { "constituency_code": "E14001063", "constituency_name": "Aldershot", - "x": 56, - "y": -40, "average_household_income_change": -150.0, "relative_household_income_change": -0.0026, "population": 40530.0, @@ -44,10 +42,6 @@ } -def _without_x_and_y(record: dict[str, object]) -> dict[str, object]: - return {key: value for key, value in record.items() if key not in {"x", "y"}} - - def expected_bundle( country: str, model_version: str, @@ -1015,21 +1009,10 @@ def test__given_complete_local_area_breakdown__then_polling_returns_it( assert response.json()["status"] == "complete" assert response.json()["result"] == LOCAL_AREA_BREAKDOWN_RESULT - @pytest.mark.parametrize( - "map_position", - [ - pytest.param({}, id="left-out"), - pytest.param({"x": None, "y": None}, id="null"), - ], - ) def test__given_breakdown_records_without_x_and_y__then_json_has_no_x_or_y( - self, mock_modal, client: TestClient, map_position: dict[str, None] + self, mock_modal, client: TestClient ): - # Executors built for the current policyengine.py version send no - # x or y on UK area records; the JSON must not invent null keys. - constituency = _without_x_and_y( - LOCAL_AREA_BREAKDOWN_RESULT["constituency_impact"][0] - ) + constituency = LOCAL_AREA_BREAKDOWN_RESULT["constituency_impact"][0] local_authority = { "local_authority_code": "E06000063", "local_authority_name": "Cumberland", @@ -1048,8 +1031,7 @@ def test__given_breakdown_records_without_x_and_y__then_json_has_no_x_or_y( ) mock_modal["func"].last_call.result = { **LOCAL_AREA_BREAKDOWN_RESULT, - "constituency_impact": [{**constituency, **map_position}], - "local_authority_impact": [{**local_authority, **map_position}], + "local_authority_impact": [local_authority], } response = client.get(f"/jobs/{submit_response.json()['job_id']}") @@ -1062,9 +1044,7 @@ def test__given_breakdown_records_without_x_and_y__then_json_has_no_x_or_y( def test__given_national_records_without_x_and_y__then_json_has_no_x_or_y( self, mock_modal, client: TestClient ): - constituency = _without_x_and_y( - LOCAL_AREA_BREAKDOWN_RESULT["constituency_impact"][0] - ) + constituency = LOCAL_AREA_BREAKDOWN_RESULT["constituency_impact"][0] submit_response = client.post( "/simulate/economy/comparison", json={"country": "uk", "scope": "macro", "region": "uk", "reform": {}}, @@ -1077,6 +1057,27 @@ def test__given_national_records_without_x_and_y__then_json_has_no_x_or_y( assert response.status_code == 200 assert response.json()["result"]["constituency_impact"] == [constituency] + def test__given_records_with_x_and_y_from_an_older_app__then_the_job_fails( + self, mock_modal, client: TestClient + ): + # Apps deployed for older policyengine.py versions still send x and y, + # which the contract no longer has: their results fail validation. + # This is why the contract change waits until no such app is routed. + constituency = LOCAL_AREA_BREAKDOWN_RESULT["constituency_impact"][0] + submit_response = client.post( + "/simulate/economy/comparison", + json={"country": "uk", "scope": "macro", "region": "uk", "reform": {}}, + ) + mock_modal["func"].last_call.result = { + **LOCAL_AREA_BREAKDOWN_RESULT, + "constituency_impact": [{**constituency, "x": 56, "y": -40}], + } + + response = client.get(f"/jobs/{submit_response.json()['job_id']}") + + assert response.status_code == 500 + assert response.json()["status"] == "failed" + def test__given_running_job__then_polling_returns_202( self, mock_modal, client: TestClient ): diff --git a/projects/policyengine-simulation-gateway/uv.lock b/projects/policyengine-simulation-gateway/uv.lock index be8dfc5bd..a17de18fd 100644 --- a/projects/policyengine-simulation-gateway/uv.lock +++ b/projects/policyengine-simulation-gateway/uv.lock @@ -1304,7 +1304,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../../libs/policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.12" }, + { name = "pydantic", specifier = ">=2.0" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, From d3a195d9a2bbf0c251252b48769c9a482dd36ada Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 2 Nov 2026 21:53:41 +0400 Subject: [PATCH 2/2] Drop the older-app wording from the x and y removal Requests never reach apps built for older policyengine.py versions, so the tests and changelog no longer explain the refusal of x and y by them: - The contract tests' comments say records carrying x or y are refused. - The gateway test is test__given_records_with_x_and_y__then_the_job_fails, and its comment says the change waits until no breakdown started before the executor stopped sending x and y is still running. - The changelog fragment drops its sentence about older apps. - apis-integ's _without_x_and_y, which nothing calls after the merge, is removed. Co-Authored-By: Claude Opus 5.5 --- changelog.d/923.changed.md | 2 +- .../tests/test_local_area_breakdown.py | 5 ++--- .../tests/test_macro_output_uk_geography.py | 5 ++--- .../tests/simulation/test_calculate.py | 4 ---- .../tests/test_simulation_job_status.py | 8 ++++---- 5 files changed, 9 insertions(+), 15 deletions(-) diff --git a/changelog.d/923.changed.md b/changelog.d/923.changed.md index 02567211e..8d6aff6c7 100644 --- a/changelog.d/923.changed.md +++ b/changelog.d/923.changed.md @@ -1 +1 @@ -UK constituency and local-authority records no longer have `x` or `y` fields in the contract or in the gateway's OpenAPI document. A record that carries either, even as `null`, is now refused, so a result from an app deployed for an older policyengine.py version that still sends them fails validation. The contract's pydantic requirement returns to 2.0 or later, since nothing uses field-level `exclude_if` any more. +UK constituency and local-authority records no longer have `x` or `y` fields in the contract or in the gateway's OpenAPI document. A record that carries either, even as `null`, is now refused. The contract's pydantic requirement returns to 2.0 or later, since nothing uses field-level `exclude_if` any more. diff --git a/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py b/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py index c031cdc82..a3a1d3bcf 100644 --- a/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py +++ b/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py @@ -212,9 +212,8 @@ def test_a_national_result_reads_its_area_lists(): assert response.model_dump(mode="json")["result"] == national -# Apps deployed for older policyengine.py versions still send each area's map -# position as x and y. The contract has no such fields, so their results are -# refused: this contract is deployed only once no such app is routed. +# The contract has no x or y (each area's map position), so a result whose +# records carry either is refused. @pytest.mark.parametrize( "result", [ diff --git a/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py b/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py index 085d0e2d4..2ac68a493 100644 --- a/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py +++ b/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py @@ -72,9 +72,8 @@ def test_an_empty_area_list_is_valid() -> None: assert GeographicImpactOutput.model_validate([]).root == [] -# The records have no x or y (an area's integer map position). Apps deployed -# for older policyengine.py versions still send both, and their records are -# refused, so this contract is deployed only once no such app is routed. +# The records have no x or y (an area's integer map position), and a record +# that carries either is refused. CONSTITUENCY_WITHOUT_MAP_POSITION = {**RECORD_WITHOUT_DISCLOSURE, **DISCLOSURE_FIELDS} LOCAL_AUTHORITY_WITHOUT_MAP_POSITION = { "local_authority_code": "E06000063", diff --git a/projects/policyengine-apis-integ/tests/simulation/test_calculate.py b/projects/policyengine-apis-integ/tests/simulation/test_calculate.py index 75586817a..c17f1d599 100644 --- a/projects/policyengine-apis-integ/tests/simulation/test_calculate.py +++ b/projects/policyengine-apis-integ/tests/simulation/test_calculate.py @@ -366,10 +366,6 @@ def test_result_assertions_accept_generated_client_objects() -> None: } -def _without_x_and_y(record: Mapping) -> dict: - return {key: value for key, value in record.items() if key not in {"x", "y"}} - - def test_breakdown_assertions_accept_generated_client_objects() -> None: status = JobStatusResponse.from_dict( {"status": "complete", "result": _BREAKDOWN_RESULT} diff --git a/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py b/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py index 40d4b8ac0..f42ef2dbf 100644 --- a/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py +++ b/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py @@ -210,12 +210,12 @@ def test__given_national_records_without_x_and_y__then_json_has_no_x_or_y( assert response.status_code == 200 assert response.json()["result"]["constituency_impact"] == [constituency] - def test__given_records_with_x_and_y_from_an_older_app__then_the_job_fails( + def test__given_records_with_x_and_y__then_the_job_fails( self, mock_modal, client: TestClient ): - # Apps deployed for older policyengine.py versions still send x and y, - # which the contract no longer has: their results fail validation. - # This is why the contract change waits until no such app is routed. + # The contract no longer has x and y, so a result whose records carry + # them fails validation. This is why the contract change waits until no + # breakdown started before the executor stopped sending them is running. constituency = LOCAL_AREA_BREAKDOWN_RESULT["constituency_impact"][0] submit_response = client.post( "/simulate/economy/comparison",