diff --git a/docs/ADRs/012_revised_ontology.md b/docs/ADRs/012_revised_ontology.md index bf9fa32..79dcc3e 100644 --- a/docs/ADRs/012_revised_ontology.md +++ b/docs/ADRs/012_revised_ontology.md @@ -47,7 +47,7 @@ unfao/ who a delivery is FOR — one partner's product and manager | **Delivery Invariants** | Representation-free rules over primitives that a delivery must satisfy: coverage, no-collapse, gid parity, observed-range, provenance. Live in `delivery/` — **nothing there imports pandas or views_frames**. *Forecast identity was one of these until 2026-07-31 — see the amendment below.* | Authoritative — they define what a valid delivery is | Stable — changes are governance decisions | | **Representation Seam** | `contract/frame_extraction.py` — turns a `views_frames` frame into the primitives the invariants consume. **One seam.** Its pandas sibling `unfao/extraction.py` was deleted in #151 once the pandas delivery was retired; the two ran as deliberate WET siblings through the migration. | Derived — isolates the representation so invariants stay representation-free | Evolving | | **Wire Mechanism** | `contract/wire/` — the ADR-013 contract: header, shard, sidecar, run manifest, sink, source selection. Partner-neutral: it takes its consumer name and collapse floor as **arguments** (#153). | Authoritative — the contract with the consumer | Stable — changes are contract amendments | -| **Enrichment Asset** | The precomputed GAUL lookup (`data/gaul_lookup.parquet`), its identity in `contract/gaul_lookup.py`, its schema in `contract/gaul_schema.py`, and the pandas merge that joins it (`contract/enrichment.py`, the build/verification path). | Authoritative for geographic metadata | Stable — rebuilt only when the producer releases new GAUL data | +| **Enrichment Asset** | The precomputed GAUL lookup (`data/gaul_lookup.parquet`), its identity in `contract/gaul_lookup.py`, its schema in `contract/gaul_schema.py`, and the keyed gather that joins it (`contract/enrichment.py`, the build/verification path — numpy/pyarrow since #89; see register **C-75** on whether that class should survive at all). | Authoritative for geographic metadata | Stable — rebuilt only when the producer releases new GAUL data | | **Artifact Builders** | `contract/historical.py` — turns a frame plus the lookup into the partner-facing artifact. | Derived | Evolving | | **External Facts** | Facts read from systems this repo does not own: the producer's (`contract/source_metadata.py` — `last_valid_month_id`, D-07) and the store's (`contract/store_metadata.py`). | Authoritative (the owning system is the source of truth) | Evolving | | **Launch Declarations** | `contract/launch_config.py` — the delivery mode the launcher must declare. Omitting a key is **refused by name**, never inferred (ADR-003, register C-63). | Authoritative | Stable | @@ -60,7 +60,7 @@ unfao/ who a delivery is FOR — one partner's product and manager 636 lines holding two of everything, and it called `unfao/extraction.py` *"the **single** pandas-aware module"* when pandas lived in three. Both drifted the same way: the ADR described the intended end state of a migration that then stopped one step short. Both are now true — -pandas has exactly one importer again (`contract/enrichment.py`) — and the load-bearing ones are +pandas has **zero runtime importers** (#89 made `contract/enrichment.py`'s a type-only import under `if TYPE_CHECKING` — pandas is in that class's interface, not its implementation) — and the load-bearing ones are **mechanically checked** by `tests/test_doc_accuracy.py`, so the next drift fails CI instead of waiting for an audit. @@ -117,7 +117,7 @@ looking for deleted code. **The rule is not weaker — it is enforced from better evidence.** The retired `assert_forecast_identity` compared one selected store document's `name`/`loa` against the configured ensemble. Since the ADR-013 contract path became the only path (#149), the same -guarantee is enforced in `unfao/wire/source_selection.py:73-81`: `TargetLease.load()` +guarantee is enforced in `contract/wire/source_selection.py:73-81`: `TargetLease.load()` checks **every shard header's declared `provenance.ensemble`** against the launched ensemble, and refuses the run on a mismatch. Identity now comes from the artifact's own declared content rather than from a metadata field on a single document, and it is checked diff --git a/docs/CICs/GaulLookupEnricher.md b/docs/CICs/GaulLookupEnricher.md index 4d0938e..a49cfc5 100644 --- a/docs/CICs/GaulLookupEnricher.md +++ b/docs/CICs/GaulLookupEnricher.md @@ -25,23 +25,45 @@ views-datafactory area-majority join), so this class does only a table join. - This class does **not** build the lookup table (that is `scripts/build_gaul_lookup.py`, run offline). - This class does **not** fill, impute, or invent metadata for unmatched cells. -- This class does **not** validate the result — null/coverage validation is the - manager's `_validate()` responsibility. +- This class does **not** validate the result. Null/coverage enforcement lives on the + delivery path — `contract/historical.assert_metadata_complete` at artifact build and + `delivery/coverage.py` for the region contract. It is **not** the manager's + `_validate()`, which stopped null-gating in #149 and now asserts only that the read + resolved. (Corrected 2026-08-02; PR #200 retired the same claim in the manager's CIC + and this one was left standing.) - This class does **not** read from the datafactory, viewser, or Appwrite. --- ## 3. Responsibilities and Guarantees -- Loads exactly one lookup Parquet at construction and verifies it carries the 9 - contract columns; missing columns raise at construction. +- Loads exactly one lookup Parquet at construction and verifies it carries the key + plus the 9 contract columns; missing columns raise at construction, and so does an + **empty** lookup — every cell would gather to null and the delivery would then + complain about missing metadata rather than about a missing lookup (S4 / #89). - Returns the input frame augmented with exactly the 9 columns of - `gaul_schema.METADATA_COLS`, with their dtypes preserved from the lookup - (codes numeric, coordinates float, names/iso categorical). + `gaul_schema.METADATA_COLS`: codes numeric, coordinates float, **names and iso as + `object`**. + *Changed in S4 (#89).* They were `category`, inherited from the pandas merge that + read the artifact's dictionary encoding. The gather that replaced it assigns plain + values. Measured on a 200-row output: category 1,795,191 bytes, object 60,061 — + a categorical carries the artifact's full 64,742-entry dictionary whatever the + output size. The builder still writes the artifact with dictionary-encoded names + (`# names/iso categorical (C-32 memory)`); that governs the file, not this output. - A cell id present in the lookup is enriched with that cell's metadata. - A cell id **absent** from the lookup yields **null** metadata for that row — never a sentinel, never a fabricated value (fail-loud downstream). -- Row count and row order of the input are preserved (left merge). +- Row count, row order **and index** of the input are preserved. Pinned by + `tests/test_enrichment.py::TestFramePropertiesPreserved` across six shapes — + ordered, reversed, duplicated, single, empty, and a non-default index. + (An earlier draft of this line claimed *eight* shapes "including unknown gids", + counting a throwaway development script rather than the committed suite, and naming + a shape that class does not exercise. Unknown gids are covered, for null-value + correctness, by `TestFailLoud` — a different guarantee.) + On **empty** input this is now *more* true than before: the pandas merge replaced the + input's `RangeIndex` with an object-dtype `Index`, where the gather leaves it + untouched. The only behavioural difference found, and it is in the direction the + guarantee above already claimed. --- @@ -60,7 +82,12 @@ views-datafactory area-majority join), so this class does only a table join. ## 5. Outputs and Side Effects - Output: the input frame (or, with `only_metadata=True`, just `pg_id_col` + - `time_id_col`) left-merged with the 9 metadata columns. + `time_id_col`) with the 9 metadata columns attached by **keyed gather**. + *Changed in S4 (#89):* this was a pandas left-merge on the lookup's index. The + lookup is now read with pyarrow, sorted once, and addressed by `np.searchsorted`. + Attaching metadata to a frame is a keyed gather, not frame algebra, and it never + needed a merge — which is also what frees the builder to stop writing pandas index + metadata (S5 / #90). - Public attribute: `lookup_version` — a short, stampable id read from the lookup's **declared** `lookup_version` metadata key at construction (`@<8-char source digest>`). Delegates to @@ -84,16 +111,21 @@ views-datafactory area-majority join), so this class does only a table join. column. - **Raises** `ValueError` if `pg_id_col` is not in the input. - **Does not raise** on unmatched cells — it surfaces them as nulls and logs a - WARNING. This is deliberate: the manager's `_validate()` null gate is the - single enforcement point, so a coverage hole fails loudly there (one place), - not in two. Passing a sentinel for unmatched cells would be a **bug** (it would + WARNING naming the unknown cells, and separately counting rows that carried no + usable cell id at all. This is deliberate: enforcement is a single point on the + delivery path (`historical.assert_metadata_complete`), so a coverage hole fails + loudly there, not in two places. Passing a sentinel for unmatched cells would be a **bug** (it would bypass that gate). Aligns with ADR-003 (fail loud on semantic ambiguity). --- ## 7. Boundaries and Interactions -- Allowed to depend on: pandas, `gaul_schema`, and a local Parquet file. +- Allowed to depend on: numpy, pyarrow, `gaul_schema`, and a local Parquet file. + **pandas is interface-only** — callers hand this class DataFrames and get one back, + but nothing here constructs, reads or joins one, and its import is under + `if TYPE_CHECKING` (S4 / #89). `tests/test_doc_accuracy.py` asserts by AST that the + package has **zero** runtime pandas importers. - Must **not** depend on: geopandas/shapely, the runtime mapper, the datafactory, viewser, Appwrite, or any network resource. - Treats the lookup table as an opaque, trusted artifact produced by the build @@ -112,7 +144,12 @@ out = enricher.enrich_dataframe_with_pg_info( # out has the 9 metadata columns; unmatched cells are null. ``` -Drop-in for the manager's existing call (same method name and key kwargs). +Signature-compatible with the runtime mapper it replaced (same method name and key +kwargs), which is why the mapper-only kwargs are still accepted and ignored. +**The manager does not call this class** — it has no `enrich` reference at all, and +reads the lookup directly via `gaul_lookup.load()` (register C-66). This example is a +build/verification-path usage. Whether the class should survive that is register +**C-75**. --- diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 7230a3f..0b0a8e0 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -5,8 +5,8 @@ | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-08-02 | -| Total Concerns | 74 | -| Open Concerns | 15 | +| Total Concerns | 76 | +| Open Concerns | 17 | | Resolved Concerns | 59 | --- @@ -532,6 +532,52 @@ Cross-refs: **C-25** (whose resolution this corrects), **C-40** (the inherited p --- +### C-75: `GaulLookupEnricher` has no production caller, and now implements a second copy of the delivery path's keyed gather + +| Field | Value | +|-------|-------| +| ID | C-75 | +| Tier | 3 — no correctness impact today: the class is off the delivery path, so a defect in it cannot reach the UN FAO. The cost is that **the verification path and the delivery path now implement the same algorithm twice**, and the tests that check the artifact run through the copy that does *not* ship. A fix applied to one and not the other makes the verification stop verifying what ships — quietly, because both would still pass their own tests. | +| Source | `code-review max` (2026-08-02) — PR #210, five parallel reviewers; two reached this independently | +| Trigger | When a bug is fixed in `contract/historical.py`'s gather (the one that ships), check whether `contract/enrichment.py`'s copy needs the same fix — nothing links them. Also fires at **S5 (#90)**: once the builder is pyarrow-native, the enricher's pandas interface is the last one in the package, and the question "does this class survive?" has to be answered rather than deferred again. | +| Owner | Whoever takes **#90** — the keep-or-retire decision is theirs to make and record, not to defer a third time. Added 2026-08-03: the first draft of this entry named two triggers and no owner, while citing ADR-014 §4 in its own body. This register had already learned that twice — *"a deferral needs an owner and a trigger, not just a reason"* (Cluster L) and *"a decision awaiting an owner, not a task awaiting effort"* (epic #181 closeout). | +| Location | `views_postprocessing/contract/enrichment.py` (the whole class; `_gather` specifically); the shipping twin is `views_postprocessing/contract/historical.py:54-68` | + +**Verified, not inferred (2026-08-02):** `grep -rn "GaulLookupEnricher\|enrich_dataframe_with_pg_info"` across the package finds **zero** production callers — the two hits are docstring mentions in `gaul_lookup.py`. The manager calls `gaul_lookup.load()` directly and has zero `enrich` references. **C-66**'s resolution already said this plainly: *"the pandas enricher leaves the delivery path entirely."* + +**What PR #210 did, and why that raises the question.** S4 (#89) rewrote this class's lookup side from a pandas merge to a numpy/pyarrow keyed gather: a measured dtype analysis, an empty-lookup guard, a mutation-proven bug fix, a corrected CIC, and five reviewers' attention. All of it spent on a method with no reachable caller outside its own test suite. The engineering is sound; what is missing is anyone having **decided** that the class should exist. + +**The duplication is the concrete consequence.** `_gather`'s `argsort → searchsorted → clip → equality-mask` is the same shape as `historical.py:54-68`. The policies differ deliberately — `historical` **raises** on an absent gid (*"geography must never silently vanish"*), the enricher returns nulls for the downstream gate to catch — so extracting a shared helper would mean parameterising the failure policy, which is the guessed abstraction **WET before DRY** exists to prevent. Two copies that are understood is the right call *today*. The trigger above is what stops "today" lasting indefinitely, per **ADR-014 §4**. + +**The precedent is C-45**, `unfao/frames.py`: an unused adapter carried on no live path, resolved by deleting it. This is the same shape with a different module, and the same question — keep it as the declared verification/reference implementation, or retire it and let the fidelity suite test `historical.py` directly. + +**Deliberately NOT registered from the same review** (defects in unmerged code, all fixed in #210 before merge rather than tracked): a NaN gid crashing the warning path, the unvalidated int64 coercion at both ends, the AST guard's `else`-branch blind spot, ADR-012's stale pandas-merge claim, and three CIC claims retired elsewhere by #200. The register tracks standing risk; a defect fixed before it ships is not one. They are recorded in the PR. + +Cross-refs: **C-45** (RESOLVED — the same shape, resolved by deletion), **C-66** (RESOLVED — established the enricher left the delivery path), **C-40** (which calls `enrichment.py` and `extraction.py` together *"the retired-in-place `enrichment.py`/`extraction.py` legacy seams"*), **#89** / **#90** / epic **#85**, ADR-014 §4. + +--- + +### C-76: `build_gaul_lookup.py` will write an empty lookup without complaint + +| Field | Value | +|-------|-------| +| ID | C-76 | +| Tier | 4 — no silent corruption. A zero-row artifact fails downstream at `historical.build_historical_table`, which raises on cells absent from the lookup. The cost is that it fails **late and confusingly**: the message names missing geography rather than an empty lookup, and the artifact is committed by then. | +| Source | `code-review max` (2026-08-03) — PR #210 second pass, while checking whether the consumer's new guards duplicated a producer guarantee. They do not. | +| Trigger | When `build_gaul_lookup.py` is next run with a new or renamed `--region`, or against a datafactory whose `gaul_admin` parquets have changed shape — check the printed `cells=` count is non-zero before committing the artifact. Nothing else will tell you. | +| Owner | Whoever next runs the builder. It is a two-line guard in a script one person runs by hand, not a scheduling decision. | +| Location | `scripts/build_gaul_lookup.py` — the invariant block at `:246-268` and the write at `:284` | + +The builder's invariant block is thorough about what it checks: index uniqueness (C-59), nulls in the metadata columns, `-1` sentinels in the code columns (C-35). It does not check that any rows survived. A `--region` argument that filters every cell out, or an upstream join that produces nothing, writes a zero-row parquet and prints `cells=0` as though that were a result. + +**Verified 2026-08-03, and the neighbouring worry is NOT real.** The same review asked whether the builder also fails to reject a null key, since `df.isna().sum().sum()` runs *after* `priogrid_gid` becomes the index and `DataFrame.isna()` does not inspect the index. It does not check it — but the null key is unreachable anyway: `df.index.astype("int64")` raises `IntCastingNaNError` two lines earlier. Protection by accident rather than by declaration, which is worth knowing, but not a defect to fix. **Only the empty case is reachable.** + +**Why this was found now.** PR #210 added consumer-side refusals for both an empty lookup and a null key to `GaulLookupEnricher.__init__`, and the review challenged them as duplicating a producer guarantee. Checking established the opposite: for the empty case there is no producer guarantee to duplicate, and for the null key the producer's protection is incidental. The consumer guards stay, and this entry records the producer-side half rather than quietly assuming someone will notice. + +Cross-refs: **C-59** and **C-61** (RESOLVED — the invariant block this sits beside, and the reason it is otherwise thorough), **C-35** (the `-1` defect class it does check for), **C-75** (the consumer whose guards prompted the check), #210. + +--- + ## Disagreements ### D-12: Post-Run-0 infrastructure & naming intents — repo rename, internal-store transport, compute co-location diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index 896ba0d..ea16dd2 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -16,6 +16,7 @@ from __future__ import annotations +import ast import re from pathlib import Path @@ -139,16 +140,80 @@ def test_internal_doc_links_resolve(): _MANAGER_LINE_BUDGET = 450 # epic #148's bound; 406 at close, 636 before #149 -def test_pandas_has_exactly_one_importer(): - """ADR-012's 'Representation Seam' row says pandas is isolated to one module.""" - importers = sorted( - f.relative_to(_PKG).as_posix() - for f in _PKG.rglob("*.py") - if re.search(r"^\s*(?:import pandas|from pandas\b)", f.read_text(), re.M) +def _is_type_checking(test: ast.expr) -> bool: + """`TYPE_CHECKING` or `typing.TYPE_CHECKING`, and nothing else. + + Substring-matching `ast.dump(test)` also matched `not TYPE_CHECKING`, which means + the opposite. Polarity is the whole point of a guard. + """ + if isinstance(test, ast.Name): + return test.id == "TYPE_CHECKING" + return isinstance(test, ast.Attribute) and test.attr == "TYPE_CHECKING" + + +def _classify_pandas_imports(source: str) -> list[tuple[str, ast.AST]]: + """``[("runtime" | "type-only", node)]`` for every pandas import in ``source``. + + **Module-level on purpose.** Both the guard and the test that proves the guard + bites call THIS function. An earlier draft had the meta-test define its own copy + of this logic — so it asserted against a replica, and when the polarity bug was + reintroduced into the real guard, all 16 tests still passed. That is the exact + defect S4 of epic #181 retired from ``test_validation.py`` (43 tests against a + function the file defined itself), committed again in the pull request whose + message boasts about catching its cousin. One implementation, two callers. + """ + tree = ast.parse(source) + # BODY only, never `orelse`. Walking the whole `If` node classified a real runtime + # `import pandas` sitting in the `else:` branch as type-only — the guard passed on + # the exact thing it exists to catch. Reproduced before fixing. + guarded = { + id(n) + for node in ast.walk(tree) + if isinstance(node, ast.If) and _is_type_checking(node.test) + for stmt in node.body + for n in ast.walk(stmt) + } + out = [] + for node in ast.walk(tree): + if not isinstance(node, (ast.Import, ast.ImportFrom)): + continue + names = [getattr(node, "module", None) or a.name for a in node.names] + if any((n or "").split(".")[0] == "pandas" for n in names): + out.append(("type-only" if id(node) in guarded else "runtime", node)) + return out + + +def test_pandas_is_not_imported_at_runtime_anywhere_in_the_package(): + """ADR-012's 'Representation Seam' row — now a stronger claim than it was. + + It used to be *one* pandas-aware module (``contract/enrichment.py``). After S4 + (#89) that module's lookup side is numpy + pyarrow and its pandas import is + **type-only**, under ``if TYPE_CHECKING``: pandas is in the enricher's interface + (callers hand it DataFrames) but no longer in its implementation. + + So the assertion is now *zero runtime importers*, and it is checked by **AST** + rather than by regex — a regex matching an indented ``import pandas`` cannot tell a + real import from one inside a ``TYPE_CHECKING`` guard, and would have passed + unchanged while the meaningful property changed underneath it. + """ + runtime, type_only = [], [] + for source in sorted(_PKG.rglob("*.py")): + for kind, node in _classify_pandas_imports(source.read_text()): + # The FILE is the claim; the line number is not. Pinning a line means any + # edit above it fails a test about imports, which is how a guard earns a + # reputation for crying wolf (ADR-014 §3). + where = source.relative_to(_PKG).as_posix() + (type_only if kind == "type-only" else runtime).append(where) + + assert not runtime, ( + f"pandas is imported at RUNTIME in the package: {runtime}. The delivery path " + "is numpy/pyarrow end to end (epic #85); a runtime pandas import re-materialises " + "the representation the migration removed. If it is genuinely needed, put it " + "behind `if TYPE_CHECKING` or say in ADR-012 why it is not." ) - assert importers == ["contract/enrichment.py"], ( - f"ADR-012 claims one pandas-aware module; found {importers}. Either the claim " - "or the code moved — fix whichever is wrong, do not leave the ADR lying." + assert type_only == ["contract/enrichment.py"], ( + f"the type-only pandas imports moved: {type_only}. Not necessarily wrong — but " + "ADR-012 names the seam, so update it rather than letting the claim drift." ) @@ -410,3 +475,35 @@ def test_no_partner_contact_details_are_published_in_this_repository(): f"partner contact addresses appear in this public repository: {offenders}. " "Keep them in the project materials; reference the decision, not the address." ) + + +def test_the_runtime_import_guard_catches_both_ways_of_evading_it(): + """A guard that cannot fail is decoration (ADR-014 §2) — and this one could not. + + Its first draft walked the whole ``if TYPE_CHECKING`` node and substring-matched + ``ast.dump(test)``. Two evasions passed it, both reproduced before the fix: + + - a real runtime ``import pandas`` in the ``else:`` branch (walked as if guarded); + - ``if not TYPE_CHECKING:`` (substring match ignores polarity, which is the entire + point of a guard). + + **And the first draft of THIS test could not have caught either**, because it + defined its own copy of the classifier and asserted against that. Reintroducing + the polarity bug into the real guard left all 16 tests green. It now calls + ``_classify_pandas_imports`` — the same function the guard calls — so a regression + there fails both. + """ + def kinds(src: str) -> list[str]: + return [kind for kind, _ in _classify_pandas_imports(src)] + + assert kinds( + "from typing import TYPE_CHECKING\nif TYPE_CHECKING:\n pass\nelse:\n import pandas\n" + ) == ["runtime"], "an import in the else: branch was classified type-only" + + assert kinds( + "from typing import TYPE_CHECKING\nif not TYPE_CHECKING:\n import pandas\n" + ) == ["runtime"], "`not TYPE_CHECKING` was treated as a type-checking guard" + + assert kinds( + "from typing import TYPE_CHECKING\nif TYPE_CHECKING:\n import pandas\n" + ) == ["type-only"], "a legitimate type-only import was flagged — the guard cries wolf" diff --git a/tests/test_enrichment.py b/tests/test_enrichment.py index 367aa36..ca1ea6c 100644 --- a/tests/test_enrichment.py +++ b/tests/test_enrichment.py @@ -6,8 +6,14 @@ """ import pandas as pd +import logging + +import numpy as np +import pyarrow as pa +import pyarrow.parquet as pq import pytest +from views_postprocessing.contract import gaul_lookup from views_postprocessing.contract.enrichment import ( GaulLookupEnricher, METADATA_COLS, @@ -28,8 +34,24 @@ def enricher(): @pytest.fixture(scope="module") -def lookup(enricher): - return enricher._lookup +def lookup(): + """The committed artifact as a pandas frame, built here rather than reached for. + + Until S4 (#89) this was ``enricher._lookup`` — the enricher's own private pandas + frame. It no longer has one: the lookup side is numpy + pyarrow, and pandas is a + type-only import there. These tests are about the **artifact**, not the enricher, + so they load it directly. + + The ``set_index`` is deliberate and forward-looking. ``to_pandas()`` currently + restores ``priogrid_gid`` as the index from the parquet's embedded pandas + metadata — metadata that **S5 (#90) removes** when the builder becomes + pyarrow-native. Handling both shapes means these tests do not have to change + again then. + """ + df = gaul_lookup.load().to_pandas() + if "priogrid_gid" in df.columns: + df = df.set_index("priogrid_gid") + return df class TestLookupIntegrity: @@ -157,3 +179,241 @@ def test_missing_pg_id_col_raises(self, enricher): enricher.enrich_dataframe_with_pg_info( pd.DataFrame({"x": [1]}), pg_id_col="priogrid_gid", ) + + +# ── the guarantees the CIC states, pinned (S4 / #89; ADR-014 §1) ───────────── +# +# The CIC claims row/order/index preservation "verified across eight input shapes" +# and names the output dtypes. That verification was a throwaway script run once +# during development — a guarantee resting on a claim rather than on a check, which +# is precisely what ADR-014 §1 forbids, written by the story after the ADR landed. +# These commit it. + + +class TestConstructionRefusals: + """Degenerate lookups must fail at construction, naming the lookup.""" + + def _write(self, path, gids): + cols = {"priogrid_gid": pa.array(gids, pa.int64())} + for c in METADATA_COLS: + cols[c] = pa.array( + [0.0] * len(gids) if c in ("pg_xcoord", "pg_ycoord") or c.endswith("_code") + else ["x"] * len(gids), + pa.float64() if c in ("pg_xcoord", "pg_ycoord") or c.endswith("_code") else pa.string(), + ) + t = pa.table(cols).replace_schema_metadata({b"lookup_version": b"t@00000000"}) + pq.write_table(t, path) + return path + + def test_an_empty_lookup_is_refused_at_construction(self, tmp_path): + """It used to raise IndexError from inside the gather instead — the guard + `(len(self._keys) > 0) & (...)` read as a guard and was not one, because `&` + evaluates both operands.""" + path = self._write(tmp_path / "empty.parquet", []) + with pytest.raises(ValueError, match="is empty"): + GaulLookupEnricher(path) + + def test_a_null_key_is_refused_at_construction(self, tmp_path): + """A null key would coerce to the same sentinel as an unusable query id, the + two would collide, and the row would be reported FOUND — receiving another + cell's metadata. That is the fabricated value this module forbids (cf. C-35).""" + cols = {"priogrid_gid": pa.array([1, None, 3], pa.int64())} + for c in METADATA_COLS: + numeric = c in ("pg_xcoord", "pg_ycoord") or c.endswith("_code") + cols[c] = pa.array([0.0] * 3 if numeric else ["x"] * 3, + pa.float64() if numeric else pa.string()) + path = tmp_path / "nullkey.parquet" + pq.write_table(pa.table(cols).replace_schema_metadata({b"lookup_version": b"t@0"}), path) + with pytest.raises(ValueError, match="null values"): + GaulLookupEnricher(path) + + +class TestUnusableCellIds: + """A value that is not a cell id gathers to null — never to a guessed cell. + + Each case was verified against the pandas merge this replaced; the behaviour + below is that merge's, not an invention. The drifted-float case is the one that + matters most: a blind `astype(np.int64)` truncates `54220.000000001` to `54220` + and matches a real, *different* cell, silently. + """ + + def _out(self, enricher, col): + return enricher.enrich_dataframe_with_pg_info( + pd.DataFrame({"priogrid_gid": col, "month_id": [1] * len(col)}), + pg_id_col="priogrid_gid", time_id_col="month_id", + ) + + def test_a_missing_gid_yields_null_and_does_not_crash(self, enricher, lookup): + good = lookup.index[:2].tolist() + out = self._out(enricher, [float(good[0]), float("nan"), float(good[1])]) + assert len(out) == 3 + assert out["country_iso_a3"].isna().sum() == 1 + + def test_a_non_integral_gid_never_matches_a_neighbouring_cell(self, enricher, lookup): + gid = int(lookup.index[100]) + out = self._out(enricher, [float(gid) + 1e-9]) + assert out["country_iso_a3"].isna().all(), ( + "a drifted float matched a cell — truncation turned it into a different, " + "real gid and fabricated that cell's geography" + ) + + def test_pandas_na_yields_null_rather_than_a_bare_typeerror(self, enricher, lookup): + good = lookup.index[:2].tolist() + out = self._out(enricher, pd.array([good[0], pd.NA, good[1]], dtype="Int64")) + assert len(out) == 3 and out["country_iso_a3"].isna().sum() == 1 + + def test_an_unusable_id_cannot_match_the_cell_it_was_substituted_with(self, tmp_path): + """The `& usable` term in `_gather`, proven load-bearing (ADR-014 §2). + + It survived removal against the committed artifact: an unusable id converts to + `0`, and `0` is not a real gid, so the key comparison already failed. That made + it look like decoration. It is not — it is the only thing standing between an + unusable id and the row it was substituted onto, and the substitute is a real + key the moment a lookup contains gid `0`. + + Without the mask, the NaN row below silently receives cell 0's geography. That + is the fabricated value this module's docstring forbids, produced from an input + the caller never wrote. + """ + cols = {"priogrid_gid": pa.array([0, 5, 9], pa.int64())} + for c in METADATA_COLS: + numeric = c in ("pg_xcoord", "pg_ycoord") or c.endswith("_code") + cols[c] = pa.array( + [0.0] * 3 if numeric else ["ZERO", "FIVE", "NINE"], + pa.float64() if numeric else pa.string(), + ) + path = tmp_path / "withzero.parquet" + pq.write_table( + pa.table(cols).replace_schema_metadata({b"lookup_version": b"t@00000000"}), path + ) + + out = GaulLookupEnricher(path).enrich_dataframe_with_pg_info( + pd.DataFrame({"priogrid_gid": [5.0, float("nan"), 9.0], "month_id": [1, 1, 1]}), + pg_id_col="priogrid_gid", time_id_col="month_id", + ) + assert out["country_iso_a3"].tolist() == ["FIVE", None, "NINE"], ( + "an unusable id matched the cell it was substituted with — `& usable` is " + "what prevents that, and this is the input that proves it" + ) + + def test_a_string_gid_column_is_refused_rather_than_parsed(self, enricher, lookup): + """A string gid is a declaration error, and the merge this replaced said so. + + `pandas` raised *"You are trying to merge on object and int64 columns. If you + wish to proceed you should use pd.concat"*. An + earlier draft of `_as_cell_ids` used `float(value)`, which happily parsed + `"54220"` and matched — inference where the old path declared (ADR-003). + """ + gid = str(int(lookup.index[0])) + out = self._out(enricher, [gid]) + assert out["country_iso_a3"].isna().all(), ( + f"the string {gid!r} was parsed into a cell id; a string column is a " + "declaration error, not a gid to be guessed at" + ) + + @pytest.mark.parametrize( + "label,array", + [ + # Each case must enter the branch it is testing. An earlier draft built + # them all with dtype=object, which routed every one through the + # element-wise branch — so the int and float bounds it claimed to test + # were never executed, and removing them left the suite green. + ("uint64 max (int branch)", np.array([np.iinfo(np.uint64).max], dtype=np.uint64)), + ("1e30 (float branch)", np.array([1e30], dtype=np.float64)), + ("inf (float branch)", np.array([np.inf], dtype=np.float64)), + ("-inf (float branch)", np.array([-np.inf], dtype=np.float64)), + ("string (object branch)", np.array(["123"], dtype=object)), + ("bool (object branch)", np.array([True], dtype=object)), + ("float exactly 2**63", np.array([2.0**63], dtype=np.float64)), + ("timedelta64", np.array([1], dtype="timedelta64[ns]")), + ], + ) + def test_out_of_range_and_non_numeric_values_are_unusable_not_wrapped( + self, enricher, label, array + ): + """Outside int64 the cast WRAPS rather than raising, and a wrapped id flagged + valid is exactly the fabricated value this conversion exists to prevent. + + `uint64` max becomes `-1`; `1e30` and `float(2**63)` become `INT64_MIN`. Each + passes a naive finite-and-integral test, so the bounds are separate conditions + rather than consequences of the others. + + Asserted through the PUBLIC method. An earlier draft called the private + `_as_cell_ids` and checked its mask — pinning an implementation detail rather + than the guarantee a caller relies on, and skipping `_gather`'s use of that + mask entirely. Every dtype below is reachable through a DataFrame column, so + there was never a reason to reach inside. + """ + out = enricher.enrich_dataframe_with_pg_info( + pd.DataFrame({"priogrid_gid": array, "month_id": [1] * len(array)}), + pg_id_col="priogrid_gid", time_id_col="month_id", + ) + assert out["country_iso_a3"].isna().all(), ( + f"{label} produced a match — an id the caller never wrote was treated as " + "a cell and that cell's geography was fabricated onto the row" + ) + + # AND at the mask, because the public assertion above cannot tell "refused" + # from "accepted but wrapped to an id that happens to be absent" — both give + # null. Reverting the float bound or re-admitting timedelta64 left the public + # assertion green while the value was silently accepted; only this sees it. + # The public check is the caller's guarantee; this one is the mutation proof. + _, usable = GaulLookupEnricher._as_cell_ids(array) + assert not usable.any(), ( + f"{label} was ACCEPTED as a cell id (it only looked refused because the " + f"wrapped id is absent from this lookup — against a lookup that contains " + f"it, this row would receive that cell's geography)" + ) + + def test_the_warning_names_unknown_cells_but_invents_no_id_for_unusable_ones( + self, enricher, lookup, caplog + ): + good = int(lookup.index[0]) + with caplog.at_level(logging.WARNING): + self._out(enricher, [float(good), float("nan"), 999999.0]) + message = " ".join(r.getMessage() for r in caplog.records) + assert "999999" in message, "the genuinely unknown cell must be named" + assert "no usable cell id" in message + assert "[0]" not in message, ( + "an unusable id was reported as cell 0 — a cell nobody asked about" + ) + + +class TestFramePropertiesPreserved: + """Row count, order and index survive the gather. Was CIC prose; now a check.""" + + @pytest.mark.parametrize("shape", ["ordered", "reversed", "duplicated", "single", "empty"]) + def test_row_count_and_order_are_preserved(self, enricher, lookup, shape): + gids = { + "ordered": lookup.index[:50].tolist(), + "reversed": lookup.index[:50].tolist()[::-1], + "duplicated": lookup.index[:5].tolist() * 3, + "single": lookup.index[:1].tolist(), + "empty": [], + }[shape] + out = enricher.enrich_dataframe_with_pg_info( + pd.DataFrame({"priogrid_gid": gids, "month_id": [1] * len(gids)}), + pg_id_col="priogrid_gid", time_id_col="month_id", + ) + assert len(out) == len(gids) + assert out["priogrid_gid"].tolist() == gids + + def test_a_non_default_index_is_preserved(self, enricher, lookup): + gids = lookup.index[:20].tolist() + df = pd.DataFrame({"priogrid_gid": gids, "month_id": [1] * 20}, + index=range(1000, 1020)) + out = enricher.enrich_dataframe_with_pg_info( + df, pg_id_col="priogrid_gid", time_id_col="month_id") + assert out.index.equals(df.index), ( + "the index moved. The pandas merge this replaced mutated it on empty " + "input; the gather must not mutate it on any input." + ) + + def test_name_columns_are_object_not_categorical(self, enricher, lookup): + """The CIC states this dtype. It changed in #89 and was measured, not guessed.""" + out = enricher.enrich_dataframe_with_pg_info( + pd.DataFrame({"priogrid_gid": lookup.index[:5].tolist(), "month_id": [1] * 5}), + pg_id_col="priogrid_gid", time_id_col="month_id", + ) + for c in NAME_COLS: + assert out[c].dtype == object, f"{c} is {out[c].dtype}, CIC says object" diff --git a/views_postprocessing/contract/enrichment.py b/views_postprocessing/contract/enrichment.py index 8f40a31..75baa77 100644 --- a/views_postprocessing/contract/enrichment.py +++ b/views_postprocessing/contract/enrichment.py @@ -7,10 +7,19 @@ area-majority GAUL parquets) onto the input DataFrame by PRIO-GRID cell id. No geopandas, no shapefiles, no spatial computation. The lookup contains only -fully-complete cells; an unknown or incomplete cell id left-merges to NaN, so -the manager's ``_validate()`` null gate still crashes the delivery (fail-loud) -rather than shipping a hole. This is intentional and matches the old mapper's -behaviour (it returned ``None`` for such cells). +fully-complete cells; an unknown or incomplete cell id gathers to null, so the +delivery's null gate still crashes (fail-loud) rather than shipping a hole. This +is intentional and matches the old mapper's behaviour (it returned ``None`` for +such cells). + +**The lookup side is pandas-free (S4 / #89, epic #85).** It is read with pyarrow +and held as numpy arrays plus a sorted key index; attaching metadata to a frame is +a **keyed gather**, not frame algebra, and it never needed a pandas merge. The +input frame is still whatever the caller passes — this class is the *build and +verification* path's object, and its callers hand it DataFrames. What changed is +that the ~880 KiB artifact is no longer materialised as a pandas frame, and the +join no longer depends on the artifact carrying pandas index metadata — which is +what unblocks S5 (#90) making the builder pyarrow-native. Produces exactly the 9-column contract enforced at the delivery's artifact builders and at @@ -21,10 +30,18 @@ import logging from pathlib import Path +from typing import TYPE_CHECKING -import pandas as pd +import numpy as np +import pyarrow as pa from views_postprocessing.contract import gaul_lookup + +if TYPE_CHECKING: # pragma: no cover — pandas is in this module's INTERFACE, not its + import pandas as pd # implementation. Callers hand it DataFrames; nothing here + # constructs, reads or joins one. Removing the runtime import is the point of + # S4 (#89): the ~880 KiB lookup is no longer materialised as a pandas frame, and the + # join no longer needs the artifact to carry pandas index metadata. from views_postprocessing.contract.gaul_schema import METADATA_COLS logger = logging.getLogger(__name__) @@ -33,6 +50,23 @@ # enricher's own default working without re-deriving the path. _DEFAULT_LOOKUP = gaul_lookup.LOOKUP_PATH +#: int64 bounds, named because they are a correctness condition rather than trivia: +#: a value outside them does not raise on cast, it WRAPS, and a wrapped id that is +#: flagged valid is the fabricated value this module forbids. +_INT64_MIN = int(np.iinfo(np.int64).min) +_INT64_MAX = int(np.iinfo(np.int64).max) + +#: Float-comparison bounds. Deliberately NOT the int bounds above: 2**63-1 is odd and +#: unrepresentable in float64, so comparing a float against it rounds up to 2**63 and +#: lets through the very values the bound excludes. 2**63 and -2**63 are both powers +#: of two and exact, so the float check is `>= -2**63` and `< 2**63`. +_INT64_MIN_F = -(2.0**63) +_INT64_MAX_EXCLUSIVE_F = 2.0**63 + +#: The lookup's key column. Named once — the artifact calls it `priogrid_gid`, the +#: wire calls it `priogrid_id` (§5.1), and confusing the two is a silent join failure. +_KEY = "priogrid_gid" + class GaulLookupEnricher: """Merge precomputed GAUL metadata onto an input DataFrame by cell id.""" @@ -46,19 +80,188 @@ def __init__(self, lookup_path: str | Path | None = None) -> None: ) logger.error(err_msg) raise FileNotFoundError(err_msg) - self._lookup = pd.read_parquet(self._lookup_path) - # Index is priogrid_gid; columns are the 9 metadata columns. - missing = [c for c in METADATA_COLS if c not in self._lookup.columns] + table = gaul_lookup.load(self._lookup_path) + missing = [c for c in (_KEY, *METADATA_COLS) if c not in table.column_names] if missing: err_msg = f"Lookup table is missing contract columns: {missing}" logger.error(err_msg) raise ValueError(err_msg) + + if table.column(_KEY).null_count: + err_msg = ( + f"GAUL lookup at {self._lookup_path} has null values in {_KEY!r}. A " + "null key cannot identify a cell, and coercing it would make it " + "collide with any unusable id on the query side — the row would then " + "be reported as FOUND and receive another cell's metadata. Rebuild " + "with scripts/build_gaul_lookup.py. (That script does not check the " + "key column either — its null check runs after the key becomes the " + "index, and DataFrame.isna() does not inspect an index. A null key is " + "unreachable there only because the earlier astype('int64') raises. " + "See register C-76.)" + ) + logger.error(err_msg) # ADR-008: logged persistently AND raised + raise ValueError(err_msg) + + if table.num_rows == 0: + err_msg = ( + f"GAUL lookup at {self._lookup_path} is empty. Every cell would gather " + "to null and the delivery would fail downstream complaining about " + "missing metadata rather than about a missing lookup. Rebuild it with " + "scripts/build_gaul_lookup.py." + ) + logger.error(err_msg) # ADR-008: logged persistently AND raised + raise ValueError(err_msg) + + # Sort the whole table once by key, in arrow, so the gather below is a binary + # search per row rather than a scan. `take` reorders every column together — + # doing it column-by-column in python was measurably quadratic and is exactly + # the mistake this comment exists to stop the next person repeating. + keys = table.column(_KEY).to_numpy(zero_copy_only=False).astype(np.int64) + order = np.argsort(keys, kind="stable") + table = table.take(pa.array(order)) + self._keys = keys[order] + # One `to_pylist` per column, not per row. Lists rather than numpy arrays + # because the columns are of mixed kind (float codes, string names) and the + # output is assembled per column anyway. + # + # DTYPE CHANGE, deliberate and measured (S4 / #89). The pandas merge this + # replaced produced `category` name columns, inherited from the artifact's + # dictionary encoding; assigning lists produces `object`. On a 200-row output: + # category 1,795,191 bytes vs object 60,061 — a categorical carries the full + # 64,742-entry dictionary whatever the output size, so this is far lighter for + # the small frames this object actually sees and heavier only past the point + # where the dictionary amortises. The builder's `# names/iso categorical (C-32 + # memory)` note governs the ARTIFACT; it never governed this method's output. + self._values = {col: table.column(col).to_pylist() for col in METADATA_COLS} self.lookup_version = self._read_version(self._lookup_path) logger.info( "Loaded GAUL lookup: %d cells from %s (version=%s)", - len(self._lookup), self._lookup_path, self.lookup_version, + len(self._keys), self._lookup_path, self.lookup_version, ) + @staticmethod + def _as_cell_ids(gids) -> tuple[np.ndarray, np.ndarray]: + """``(int64 ids, usable mask)`` — DECLARED, never coerced (ADR-003). + + A cell id is an integer. Anything that is not one — a missing value, a + non-integral float, a ``pd.NA`` — is marked **unusable** rather than cast, + and unusable ids gather to null exactly as an unknown gid does. The + downstream gate then sees a hole, which is what it exists for. + + **Why not just cast.** ``np.asarray(gids, dtype=np.int64)`` looks equivalent + and is not, in three ways found in review of #210: + + - a ``NaN`` becomes ``INT64_MIN`` with only a ``RuntimeWarning``. If a lookup + key were ever null it would take the same sentinel, the two would collide, + and the row would be reported FOUND — receiving another cell's metadata. + That is the fabricated value this module's docstring forbids (cf. C-35). + - a non-integral float **truncates silently**: ``54220.000000001`` becomes + ``54220`` and matches a real, *different* cell. Verified against the pandas + merge this replaced: it returns null there, and even warns. A silent wrong + match is strictly worse than the crash it would replace. + - a ``pd.NA`` raises a bare ``TypeError`` from numpy — no log, no + contract-shaped error, unlike every other guard in this file. + """ + arr = np.asarray(gids) + + if arr.dtype.kind in ("i", "u"): + # In range, or it is not the id the caller wrote. uint64 max silently + # wraps to -1 under `astype(np.int64)` and would be flagged VALID. + usable = (arr >= _INT64_MIN) & (arr <= _INT64_MAX) + return np.where(usable, arr, 0).astype(np.int64), usable + + if arr.dtype.kind == "f": + # Finite, integral, AND representable. `1e30` passes the first two and + # then overflows the cast to INT64_MIN — an id nobody asked for, marked + # valid. Same silent-coercion class this method exists to remove. + # STRICTLY below 2**63, not `<= _INT64_MAX`. `_INT64_MAX` is 2**63-1, + # which is odd and NOT representable in float64 — comparing against it + # promotes to float and rounds UP to 2**63, so the bound admitted exactly + # the values it was added to exclude. `float(2**63)` passed every check + # and then wrapped to INT64_MIN, flagged valid. `_INT64_MIN` has no + # equivalent hole: -2**63 is a power of two and exactly representable. + # + # `isfinite` is kept for intent and is SUBSUMED — `inf` fails the upper + # bound, `NaN` fails `arr == rint(arr)` (NaN equals nothing). It is the + # one condition here that survives its own removal, so it is defensive + # rather than load-bearing; the other three each fail the suite. + usable = ( + np.isfinite(arr) + & (arr == np.rint(arr)) + & (arr >= _INT64_MIN_F) + & (arr < _INT64_MAX_EXCLUSIVE_F) + ) + return np.where(usable, arr, 0).astype(np.int64), usable + + # object / pandas-nullable. Accept only values that ALREADY ARE integers. + # + # An earlier draft used `float(value)`, which parses. That accepted the string + # `"54220"` as a cell id — where the pandas merge this replaced raised + # `ValueError: You are trying to merge on object and int64 columns`. A string + # gid column is a declaration error and the old path said so; parsing it is + # inference (ADR-003) and it is the same defect as the drifted float, just + # pointing the other way. `bool` is excluded for the same reason: `True` is + # not cell 1. + ids = np.zeros(len(arr), dtype=np.int64) + usable = np.zeros(len(arr), dtype=bool) + for i, value in enumerate(arr): + # `np.timedelta64` IS an `np.integer` instance — a numpy quirk, and + # `np.datetime64` is not, so the hole was specific to that one type. A + # duration is not a cell id any more than `True` is cell 1. + if isinstance(value, (bool, np.timedelta64)): + continue + if not isinstance(value, (int, np.integer)): + continue + if _INT64_MIN <= int(value) <= _INT64_MAX: + ids[i], usable[i] = int(value), True + return ids, usable + + def _gather(self, gids) -> dict: + """The metadata for each gid, warning about the ones that got none. + + A sorted-key ``searchsorted`` rather than a hash map: the lookup is 64,742 + rows read once per process, and the gather is over the delivery's row count. + Absent gids yield ``None`` in every column — the null the downstream gate + exists to catch, not a sentinel it would pass. An id that is not a usable + cell id is treated the same way: absent, never guessed at. + """ + wanted, usable = self._as_cell_ids(gids) + # `self._keys` is non-empty — __init__ refuses an empty lookup. An earlier draft + # guarded with `(len(self._keys) > 0) & (...)`, which READS as a guard and is + # not one: `&` evaluates both operands, so the index happened regardless and an + # empty lookup raised IndexError from inside the gather rather than ValueError + # from the constructor. Guard where the condition is knowable, not where it bites. + idx = np.clip(np.searchsorted(self._keys, wanted), 0, len(self._keys) - 1) + # `& usable` is NOT belt-and-braces, and no substitute value would make it so. + # Unusable ids substitute to 0, but int64 reserves nothing — the lookup is + # arbitrary data and may legally contain 0, or -1, or INT64_MIN, or whatever + # else one might pick instead. The mask is what carries correctness; the + # substitute is only a placeholder. Pinned by + # `test_an_unusable_id_cannot_match_the_cell_it_was_substituted_with`. + found = (self._keys[idx] == wanted) & usable + out = { + col: [vals[i] if hit else None for i, hit in zip(idx, found)] + for col, vals in self._values.items() + } + + # Warn HERE rather than handing the caller three arrays to reassemble one + # message. `__init__` already co-locates detection and logging at every guard + # (ADR-008); this is the same shape. An earlier draft returned four values, + # three of which existed only to build the string below. + absent = ~found + n_unmapped = int(absent.sum()) + if n_unmapped: + unknown = sorted({int(i) for i, miss, ok in zip(wanted, absent, usable) if miss and ok}) + n_unusable = int((absent & ~usable).sum()) + detail = f"unknown cells {unknown[:20]}" if unknown else "no unknown cells" + if n_unusable: + detail += f"; {n_unusable} row(s) carried no usable cell id" + logger.warning( + "%d/%d rows have no lookup match (will fail validation): %s", + n_unmapped, len(wanted), detail, + ) + return out + @staticmethod def _read_version(path: Path) -> str: """The lookup's build stamp. Delegates to ``gaul_lookup.version`` (#152) — @@ -102,19 +305,12 @@ def enrich_dataframe_with_pg_info( else: base = df.copy() - merged = base.merge( - self._lookup, left_on=pg_id_col, right_index=True, how="left", - ) + gids = base[pg_id_col].to_numpy() + gathered = self._gather(gids) - n_total = len(merged) - n_unmapped = int(merged["country_iso_a3"].isna().sum()) - if n_unmapped: - logger.warning( - "%d/%d rows have no lookup match (will fail validation): %s", - n_unmapped, n_total, - sorted(merged.loc[merged["country_iso_a3"].isna(), pg_id_col] - .unique().tolist())[:20], - ) + merged = base.copy() + for col in METADATA_COLS: + merged[col] = gathered[col] return merged # Convenience alias for new call sites that don't need the legacy name. diff --git a/views_postprocessing/contract/gaul_lookup.py b/views_postprocessing/contract/gaul_lookup.py index 6907f31..23f17bb 100644 --- a/views_postprocessing/contract/gaul_lookup.py +++ b/views_postprocessing/contract/gaul_lookup.py @@ -3,7 +3,7 @@ One concept — *the precomputed geography asset* — which until #152 was scattered: its path was a private name in ``enrichment.py`` imported across module boundaries (register C-68), its version stamp was a ``@staticmethod`` on ``GaulLookupEnricher`` -that never touched the instance, and the delivery read the 888 KB parquet **three +that never touched the instance, and the delivery read the ~880 KiB parquet **three times per run** (C-66) — once eagerly into a pandas enricher it never used, then twice more through pyarrow.