Skip to content

Remove x and y from the contract's UK area records - #924

Draft
anth-volk wants to merge 2 commits into
chore/uk-area-records-without-xyfrom
chore/remove-uk-area-xy-fields
Draft

anth-volk wants to merge 2 commits into
chore/uk-area-records-without-xyfrom
chore/remove-uk-area-xy-fields

Conversation

@anth-volk

@anth-volk anth-volk commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #923

Merge only after #919 is deployed. Both records forbid extra fields, so once this deploys, a result from an app built before #919, which still sends x and y, fails validation and the gateway reports the job as failed. Those apps stay routable (see below), but no client sends requests pinned to them, so this was accepted on 27 Oct 2026.

Same-Repository Draft PR Check

  • This PR is a draft.
  • This PR is from a branch in PolicyEngine/policyengine-sim-api, not a personal fork.

Summary

Stacked on #919, which stops sending x and y and makes them optional in the contract. This PR removes them from the contract's area records altogether:

  • ConstituencyImpactRecord and LocalAuthorityImpactRecord have no x or y. A record that carries either, even as null, is refused (extra="forbid").
  • Nothing uses pydantic's exclude_if any more, so the contract's pydantic floor returns to 2.0, and the five lock files that record it match main again.
  • The gateway's golden OpenAPI drops both properties and the description's paragraph about them; the generated clients' record models no longer take x or y.

Which routed .py versions still send x and y

What the repository shows:

  • Routes are only ever added. build_next_routing_state (in src/modal/utils/update_version_registry.py) adds the deployed app's routes and bundle manifest and never removes another version's; the restore commands put back earlier snapshots. Nothing in the repository retires a route. The gateway's resolve_route serves pinned version and policyengine_version requests from that routing state, or from the legacy version dicts.
  • Only the current pin is redeployed. The Deploy versioned Modal executor step in .github/workflows/simulation-deploy.reusable.yml deploys policyengine-simulation-py<version> for the .py version pinned in the executor's pyproject.toml. Apps for earlier versions keep the code they were built with.
  • Every app built before Stop sending x and y on UK area results #919 sends both. From 3e99e35 (24 July 2026) the contract required x and y, and from bae99c5 (28 September 2026) the executor refused an area without them. Since the executor moved to projects/policyengine-simulation-executor (3 July 2026), its .py pins on main have been 4.18.9, 4.20.3, 4.21.1 (reverted the same day), 4.22.0, 5.0.4, 5.2.0, 6.1.2, 6.2.1, 6.2.3, 6.2.7, 6.3.4, 6.4.1, 6.5.0, 6.6.0, 6.7.0, 6.7.5 and 6.10.0; earlier pins were not traced.
  • 6.10.0 stops sending them only if Stop sending x and y on UK area results #919 deploys while 6.10.0 is still the pin, because that deployment rebuilds policyengine-simulation-py6-10-0. If the pin moves first (an automated update to 6.11.1 exists as a branch), the 6.10.0 app keeps sending x and y too.

Unverified: the routing state lives in Modal Dicts (simulation-api-routing-state and the legacy simulation-api-*-versions dicts) in each Modal environment, not in the repository, so I can't tell which of those versions are actually routed in beta or prod, or whether any of their apps were stopped by hand. I also haven't checked that apps built before 24 July produced UK area records at all. Those builds passed .py's area rows through, and those rows carry x and y.

So meeting the merge condition needs a step this repository doesn't have: retire the routes to every app built before #919 deployed (or decide that pinned requests to those versions may fail). Jobs those apps computed before the change, and that are still pollable, would also fail validation once this deploys.

Testing

Written red-green: the tests were written first and seen failing for the expected reason, then the code was written and both were committed together.

  • Contract: test_records_with_x_or_y_are_refused (both, either one, or both null, on each record) and test_a_job_status_refuses_area_records_with_x_and_y (breakdown and national results) failed with DID NOT RAISE; test_records_validate_and_dump_without_x_or_y failed because the records still declared both fields.
  • Gateway: test__given_records_with_x_and_y_from_an_older_app__then_the_job_fails got 200 instead of 500.
  • Generated client: test_generated_client_area_records_have_no_x_or_y failed because the record models still took x and y.

Commands run (focused, locally):

  • uv run --frozen pytest on the contract's test_macro_output_uk_geography.py, test_local_area_breakdown.py and test_gateway_models.py; the gateway's test_endpoints.py and test_openapi_golden.py; the entry's test_openapi.py; and the executor's test_simulation_output_geographic.py, test_simulation_output_builder.py, test_uk_local_authority_metadata.py and test_stage12_runtime.py.
  • Golden OpenAPI regenerated with python -m policyengine_simulation_gateway.generate_openapi; ./scripts/generate-clients.sh, then uv run --frozen pytest tests/ -m "not requires_gcp and not beta_only" in policyengine-apis-integ: everything passed except test_ping, which needs a gateway running on localhost.
  • make check, python scripts/run_quality_guards.py with GITHUB_BASE_REF=main, python -m scripts.rules render (no changes) and .github/scripts/check-uv-locks.sh.
  • As a stacked PR, this one runs only the CI complete and PR description jobs until Stop sending x and y on UK area results #919 merges.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@anth-volk
anth-volk added this pull request to stack #925 October 27, 2026 12:22
…ields

Brings in #919's merge of main and resolves two conflicts with it:

- changelog_entry.yaml: main replaced it with changelog fragments. This
  branch's entry becomes changelog.d/923.changed.md.
- The gateway's test_endpoints.py: main split it. This branch drops x
  and y from the shared breakdown result, now in
  fixtures/gateway_responses.py, and its changes to the x-and-y tests,
  with its new test of an older app's records, go to
  test_simulation_job_status.py, where #919's tests now are.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove x and y from the contract's UK area records

1 participant