Skip to content

Stop sending x and y on UK area results - #919

Merged
anth-volk merged 7 commits into
mainfrom
chore/uk-area-records-without-xy
Nov 2, 2026
Merged

anth-volk merged 7 commits into
mainfrom
chore/uk-area-records-without-xy

Conversation

@anth-volk

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

Copy link
Copy Markdown
Contributor

Fixes #918

Merge order: step 2 of 3 of removing x and y from UK constituency and local-authority records. Both requirements are met: PolicyEngine/policyengine-app-v2#1436 is deployed, so the app reads no x or y, and #985 (step 1, fixes #984) is deployed, so the gateway accepts records without them. #924 (step 3, the contract removes them) follows once this is deployed and no breakdown started before this deploy is still running.

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

New results stop carrying x and y (each area's integer map position) on UK constituency and local-authority records, and the executor stops shipping coordinates_lad22.csv and coordinates_lad23.csv. Requests never reach apps built for older .py versions, so nothing is kept for them. This PR first also made x and y optional in the contract; that change is now #985, on main, and with main merged in (1ecb51d9) this PR's diff is only the executor change.

Three commits, each with one behavioural change, a fourth that rewords one sentence of the migration doc, and, after the merges of main, b54712ce, which drops the wording that kept x and y for older apps:

  1. Accept UK area records without x and y. Now on main as Accept UK area records with or without x and y #985, which carries the same code with reworded comments and test names. x and y on ConstituencyImpactRecord and LocalAuthorityImpactRecord default to None and use pydantic's field-level exclude_if, so a record without them validates and every dump leaves the keys out instead of writing null. Records with integer x and y still validate and keep them. The contract's pydantic floor rises to 2.12, the first release with exclude_if; the contract and the four packages whose locks record its metadata are relocked, and only that specifier line changes in each lock. The gateway's golden OpenAPI no longer lists x and y as required.
  2. Stop sending x and y on UK area records. The executor reads only the code and name columns of .py's constituency lookup, attaches only names to local authorities, and no longer refuses an area without a map position (the "missing lookup coordinates" check). The beta integration check of a local-area breakdown now requires records without x or y.
  3. Ship local-authority code lists instead of coordinate files. codes_lad22.csv and codes_lad23.csv (one code column) replace the coordinate files, with exactly the same codes in the same order. UKLocalAuthorityResources carries codes per boundary version and names_for(...) replaces metadata_for(...). The Stage 12 static runtime file smoke reports lad22_codes and lad23_codes.

How the JSON loses the keys

  • The executor returns output.model_dump(mode="json") from SimulationOutputBuilder.serialize and LocalAreaBreakdownBuilder.serialize; Stage 12 stores the same dump as its aggregate.
  • The gateway validates that dict into JobStatusResponse and /jobs/{job_id} serializes it with response_model_exclude_none=True, which already drops null fields.
  • With exclude_if (from Accept UK area records with or without x and y #985), the dump itself has no x or y, so neither the Modal result, the Stage 12 aggregate nor the gateway JSON carries the keys. The gateway tests check the response JSON for a breakdown and a national result whose records omit x and y, and for records that send them as null.

Boundary-version detection

detect_uk_local_authority_boundary_version reads the names file and the two hard-coded distinguishing code sets, which this PR doesn't change. Each version's full code list, which names_for and the "not part of the detected boundary version" check use, now comes from the code files. The load-time checks stay: the names must exactly cover both lists, and each version's distinguishing codes must match its list. A test pins each list to a SHA-256 of the codes the coordinate files listed (374 for LAD22, 361 for LAD23), and new tests check the loader refuses a repeated code, a list with another version's distinguishing code, and a list with x and y columns.

Deployment code and environment variables

The Modal images change content only: add_static_runtime_files copies the whole static_runtime_files directory, which now holds names.csv and the two code lists. src/modal/v2_static_runtime_files_smoke.py changes only the keys it reports. No environment variable is added, removed or read differently. For reference, the smoke's wiring is unchanged:

  • MODAL_TOKEN_ID and MODAL_TOKEN_SECRET: GitHub Actions secrets, set in the Run image smokes step's env: in pr-image-smoke.yml.
  • PE_UK_PRIVATE_HF_READ_TOKEN: GitHub Actions secret, set in the same step; .github/scripts/modal-image-smoke.sh writes it to the Modal secret pe-uk-private-hf-read-token as HUGGING_FACE_TOKEN, which the smoke function attaches through hf_secret (required_keys=["HUGGING_FACE_TOKEN"]).

Compatibility

Results the executor returns after this deploy carry neither key, which #985's contract accepts. Breakdowns that started before this deploy may still return records with x and y; the contract keeps accepting them until #924 removes the fields, which is why #924 waits until no such breakdown is still running.

Testing

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

  • Commit 1 (now Accept UK area records with or without x and y #985): contract tests failed on Field required for x and y and on a null x and y in the dump; the gateway returned 500 for records without x and y; the generated client did not read such a breakdown as a LocalAreaBreakdownOutput.
  • Commit 2: executor tests failed on "missing lookup coordinates" and on x in the constituency and local-authority records; the breakdown check's new cases failed with DID NOT RAISE.
  • Commit 3: tests failed because the resources had no codes or names_for, the package still held the coordinate files, and the smoke reported the coordinate keys.

Commands run (focused, locally):

  • uv run --frozen pytest on test_macro_output_uk_geography.py, test_local_area_breakdown.py and test_gateway_models.py (contract); test_endpoints.py and test_openapi_golden.py (gateway); test_openapi.py (entry); test_simulation_output_geographic.py, test_simulation_output_builder.py, test_uk_local_authority_metadata.py, test_stage12_modal_app.py, test_smoke_app.py, test_stage12_runtime.py, test_stage12_output_planning.py, test_stage12_worker_validation.py and test_modal_bundle_image.py (executor).
  • Golden OpenAPI regenerated with python -m policyengine_simulation_gateway.generate_openapi; the diff removes x and y from both records' required lists and updates the constituency description.
  • ./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 (ruff and Pyright in every package), python -m scripts.rules pyright --sync projects/policyengine-simulation-executor, 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.
  • After merging main with Accept UK area records with or without x and y #985: test_generated_client_reads_area_records_with_x_and_y (apis-integ) passes against the regenerated client, ruff passes on the changed test file, and .github/scripts/check-changelog-fragment.sh passes.
  • Not run locally: the full test suites, the Modal image smoke and the beta integration tests, which CI and the deployment run.

🤖 Generated with Claude Code

anth-volk and others added 4 commits October 27, 2026 15:59
Make x and y optional on ConstituencyImpactRecord and
LocalAuthorityImpactRecord. Both default to None and are left out of
every dump while None (pydantic's field-level exclude_if), so a record
without them validates and its JSON has no x or y keys rather than
"x": null. Apps deployed for older policyengine.py versions are never
rebuilt and keep sending both; their records still validate and keep
the values.

The gateway's /jobs/{job_id} route already drops null fields
(response_model_exclude_none), and the new endpoint tests check the
JSON it returns for a breakdown result and a national result whose
records carry no x or y, and for records that send them as null.

Raise the contract's pydantic floor to 2.12, the first release with
exclude_if, and relock the contract and the four packages whose locks
record its metadata; only that specifier line changes in each lock.
Regenerate the gateway's golden OpenAPI, which no longer lists x and y
as required.

Written red-green: the contract, gateway and generated-client tests
failed first because x and y were required ("Field required", and a
500 from the gateway), and because a null x or y was written to the
dump.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The executor no longer puts x or y on constituency or local-authority
records. It reads only each constituency's code and name from
policyengine.py's lookup, whose x and y columns it now ignores, and no
longer attaches the packaged local-authority positions. The check that
refused a published area without a map position ("missing lookup
coordinates") is removed; the code and lookup-name checks stay. With the
contract leaving absent x and y out of every dump, the result an app
deployed from now on returns has neither key.

Apps deployed for older policyengine.py versions are not rebuilt and
keep sending both, which the contract still accepts.

The beta integration check of a local-area breakdown now requires its
records to carry no x or y, since it runs against the app the same
deployment builds.

Written red-green: the executor tests failed first on the "missing
lookup coordinates" error and on x in the constituency and
local-authority records, and the breakdown check's new cases failed
with "DID NOT RAISE".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The executor no longer ships coordinates_lad22.csv or
coordinates_lad23.csv. Nothing reads their x and y any more, but they
were also the only list of each boundary version's authority codes, so
codes_lad22.csv and codes_lad23.csv replace them with a single code
column holding exactly the same codes in the same order.

UKLocalAuthorityResources now carries those codes per boundary version
(codes) and returns one version's names with names_for, which replaces
metadata_for. The load-time checks are unchanged apart from the
dropped coordinate comparison: names must exactly cover both lists, and
each version's distinguishing codes must match its list. LAD22 and LAD23
detection reads the names and the distinguishing code sets, so it is
unchanged. The Stage 12 static runtime file smoke reports lad22_codes
and lad23_codes in place of lad22_coordinates and lad23_coordinates,
with the same expected counts (374 and 361).

A test pins each version's codes to a SHA-256 of the codes the
coordinate files listed, and new tests check the loader refuses a
repeated code, a list with other distinguishing codes, and a list with
x and y columns.

Written red-green: the tests failed first because the resources had no
codes or names_for, the package still held the coordinate files and no
code lists, and the smoke reported the coordinate keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It said map positions are "now" drawn by the front end, which depends
on an app change that has not shipped. Say that results carry no map
positions and that the app draws them from its own layout files.

Docs only; no behaviour changes, so there is no test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves three conflicts with changes that landed on main:

- changelog_entry.yaml: main replaced it with changelog fragments. This
  branch's three entries become changelog.d/918.changed.md.
- The executor's test_simulation_output_builder.py: main split it, and
  the two UK record helpers this branch changed moved to
  test_simulation_output_fakes.py, so they lose x and y there.
- The gateway's test_endpoints.py: main split it. This branch's two
  tests of records without x and y, and their helper, move to
  test_simulation_job_status.py, after the breakdown polling test they
  followed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk added a commit that referenced this pull request Nov 2, 2026
…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>
anth-volk and others added 2 commits November 2, 2026 21:36
Brings in #985, which made x and y optional in the contract as this
branch's first commit (5b5fbde) did, with reworded comments, docstrings
and test names. The five files where the two versions differ
(macro_output.py, its two contract test modules, the gateway's golden
OpenAPI and test_simulation_job_status.py) take main's version, so this
branch's diff against main is now only the executor change: it stops
sending x and y and ships local-authority code lists instead of the
coordinate files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Requests never reach apps built for older policyengine.py versions, and
#985 (fixes #984) already made x and y optional in the contract, so:

- The changelog fragment keeps only this branch's executor change and
  code lists; the contract paragraph is #984's fragment.
- The breakdown check's docstring says the executor no longer sends x
  or y.
- The generated-client test that read records with and without x and y
  now covers only records with them, from breakdowns that started
  before this deploy; the case without them is #985's test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@anth-volk
anth-volk marked this pull request as ready for review November 2, 2026 17:50
@anth-volk
anth-volk merged commit f621140 into main Nov 2, 2026
25 checks passed
@anth-volk
anth-volk deleted the chore/uk-area-records-without-xy branch November 2, 2026 17:50
anth-volk added a commit that referenced this pull request Nov 2, 2026
Brings in #985 (x and y optional in the contract, with reworded
comments and test names) and #919 (the executor stops sending them).
In each of the six conflicts this branch's side wins, because this
branch removes the text main reworded:

- macro_output.py: the record docstrings lose the x and y paragraph;
  the fields, _is_none and the pydantic 2.12 floor stay removed.
- The contract tests read records without a map position and refuse
  records that carry x or y, even as null, instead of keeping them.
- apis-integ's generated-client test checks the client's record models
  have no x or y, replacing the tests that read records with and
  without them.
- The gateway's job-status tests use the shared breakdown fixture,
  which has no x or y, and the golden OpenAPI equals the document the
  merged code generates.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

Accept UK area records with or without x and y before the executor stops sending them Stop sending x and y on UK area results

1 participant