From ac0fd53ebec589fcb6a99552b6b75f111b3f8885 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Sun, 2 Aug 2026 17:13:28 +0200 Subject: [PATCH 1/5] =?UTF-8?q?feat(enrichment):=20S4=20(#89)=20=E2=80=94?= =?UTF-8?q?=20numpy/pyarrow=20keyed=20gather,=20drop=20the=20pandas=20merg?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite GaulLookupEnricher's lookup side to be pandas-free (epic #85): - Read the 888 KB GAUL lookup with pyarrow; hold it as numpy arrays + a sorted key index. Attaching metadata is a keyed `searchsorted` gather, not a pandas merge — absent gids gather to None so the downstream null gate still fails loud. The artifact is no longer materialised as a pandas frame, and the join no longer needs it to carry pandas index metadata (which unblocks S5/#90's pyarrow-native builder). - pandas becomes a TYPE_CHECKING-only import (it's in the enricher's interface — callers hand it DataFrames — not its implementation). - test_enrichment: the `lookup` fixture loads the artifact directly (the enricher no longer exposes a private pandas frame), tolerant of both the current pandas-index shape and S5's index-free one. - test_doc_accuracy: the pandas-importer guard now checks by AST, asserting ZERO runtime importers + type-only in enrichment.py (a regex couldn't tell a TYPE_CHECKING guard from a real import). Recovered from an uncommitted working tree on this (correctly-named) branch — complete, its 31 tests pass. (The repo's 5 wire shard/sidecar byte-parity + toolchain-pin failures are pre-existing and unrelated to this change.) Co-Authored-By: Claude Opus 4.8 --- tests/test_doc_accuracy.py | 51 +++++++++--- tests/test_enrichment.py | 21 ++++- views_postprocessing/contract/enrichment.py | 88 +++++++++++++++++---- 3 files changed, 132 insertions(+), 28 deletions(-) diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index 896ba0d..fa0d661 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,48 @@ 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 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")): + tree = ast.parse(source.read_text()) + guards = [ + n for n in ast.walk(tree) + if isinstance(n, ast.If) and "TYPE_CHECKING" in ast.dump(n.test) + ] + guarded = {id(n) for g in guards for n in ast.walk(g)} + 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 not any((n or "").split(".")[0] == "pandas" for n in names): + continue + # 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 id(node) in guarded 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." ) diff --git a/tests/test_enrichment.py b/tests/test_enrichment.py index 367aa36..f087760 100644 --- a/tests/test_enrichment.py +++ b/tests/test_enrichment.py @@ -8,6 +8,7 @@ import pandas as pd import pytest +from views_postprocessing.contract import gaul_lookup from views_postprocessing.contract.enrichment import ( GaulLookupEnricher, METADATA_COLS, @@ -28,8 +29,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: diff --git a/views_postprocessing/contract/enrichment.py b/views_postprocessing/contract/enrichment.py index 8f40a31..2cd08a3 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 888 KB 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 888 KB 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,10 @@ # enricher's own default working without re-deriving the path. _DEFAULT_LOOKUP = gaul_lookup.LOOKUP_PATH +#: 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 +67,51 @@ 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) + + # 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. + self._values = {col: table.column(col).to_pylist() for col in METADATA_COLS} + self._n_cells = table.num_rows 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, + self._n_cells, self._lookup_path, self.lookup_version, ) + def _gather(self, gids) -> tuple[dict, np.ndarray]: + """Metadata for each gid, plus a mask of the ones absent from the lookup. + + 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. + """ + wanted = np.asarray(gids, dtype=np.int64) + pos = np.searchsorted(self._keys, wanted) + clipped = np.clip(pos, 0, max(len(self._keys) - 1, 0)) + found = (len(self._keys) > 0) & (self._keys[clipped] == wanted) + idx = clipped + out = { + col: [vals[i] if hit else None for i, hit in zip(idx, found)] + for col, vals in self._values.items() + } + return out, ~found + @staticmethod def _read_version(path: Path) -> str: """The lookup's build stamp. Delegates to ``gaul_lookup.version`` (#152) — @@ -102,18 +155,19 @@ 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, absent = self._gather(gids) + + merged = base.copy() + for col in METADATA_COLS: + merged[col] = gathered[col] - n_total = len(merged) - n_unmapped = int(merged["country_iso_a3"].isna().sum()) + n_unmapped = int(absent.sum()) if n_unmapped: + unmatched = sorted({int(g) for g, miss in zip(gids, absent) if miss}) 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], + n_unmapped, len(merged), unmatched[:20], ) return merged From 425d537909b86089a54601e9122d128d2f087062 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Sun, 2 Aug 2026 18:32:29 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(enrichment):=20S4=20review=20findings?= =?UTF-8?q?=20=E2=80=94=20refuse=20an=20empty=20lookup,=20and=20say=20what?= =?UTF-8?q?=20the=20dtype=20change=20costs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the diff review, applied to the commit that preceded them. An empty lookup raised IndexError from inside the gather. The guard read `(len(self._keys) > 0) & (self._keys[clipped] == wanted)`, which LOOKS like a guard and is not one: `&` evaluates both operands, so the index happened anyway. Guard where the condition is knowable — __init__ now refuses an empty lookup with a message about the lookup, rather than letting the delivery later complain about missing metadata. The parity proof passed check_dtype=False, check_categorical=False and compared after reset_index — three flags that hide exactly what a merge-to-assign rewrite is most likely to change. Re-checked without them. Two results worth having: - name columns went category -> object. Measured, not guessed: on a 200-row output, category is 1,795,191 bytes and object is 60,061, because a categorical carries the artifact's full 64,742-entry dictionary whatever the output size. Lighter for the frames this object actually sees. Recorded in the code beside the C-32 note it might otherwise be read as contradicting. - on EMPTY input the old merge replaced the input's RangeIndex with an object-dtype Index; the gather leaves it alone. The only behavioural difference found, and it is in the direction the CIC already claimed. The CIC is corrected — fourth story running where it lagged the change, which is the pattern ADR-014 §5 was written for, recurring in the first story after the ADR landed. It now states the gather, the dtype change, the empty-lookup refusal, and that pandas is interface-only. Also dropped a dead alias and a duplicated row count. Co-Authored-By: Claude Opus 5 (1M context) --- docs/CICs/GaulLookupEnricher.md | 37 +++++++++++++++++---- views_postprocessing/contract/enrichment.py | 33 ++++++++++++++---- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/docs/CICs/GaulLookupEnricher.md b/docs/CICs/GaulLookupEnricher.md index 4d0938e..02d83ac 100644 --- a/docs/CICs/GaulLookupEnricher.md +++ b/docs/CICs/GaulLookupEnricher.md @@ -33,15 +33,29 @@ views-datafactory area-majority join), so this class does only a table join. ## 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. Verified against the + previous implementation across eight input shapes, including a non-default index, + duplicated gids, unknown gids and empty input. + 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 +74,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 @@ -93,7 +112,11 @@ views-datafactory area-majority join), so this class does only a table join. ## 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 diff --git a/views_postprocessing/contract/enrichment.py b/views_postprocessing/contract/enrichment.py index 2cd08a3..7625eb9 100644 --- a/views_postprocessing/contract/enrichment.py +++ b/views_postprocessing/contract/enrichment.py @@ -74,6 +74,16 @@ def __init__(self, lookup_path: str | Path | None = None) -> None: logger.error(err_msg) 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 @@ -85,12 +95,20 @@ def __init__(self, lookup_path: str | Path | None = None) -> None: # 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._n_cells = table.num_rows self.lookup_version = self._read_version(self._lookup_path) logger.info( "Loaded GAUL lookup: %d cells from %s (version=%s)", - self._n_cells, self._lookup_path, self.lookup_version, + len(self._keys), self._lookup_path, self.lookup_version, ) def _gather(self, gids) -> tuple[dict, np.ndarray]: @@ -102,10 +120,13 @@ def _gather(self, gids) -> tuple[dict, np.ndarray]: exists to catch, not a sentinel it would pass. """ wanted = np.asarray(gids, dtype=np.int64) - pos = np.searchsorted(self._keys, wanted) - clipped = np.clip(pos, 0, max(len(self._keys) - 1, 0)) - found = (len(self._keys) > 0) & (self._keys[clipped] == wanted) - idx = clipped + # `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) + found = self._keys[idx] == wanted out = { col: [vals[i] if hit else None for i, hit in zip(idx, found)] for col, vals in self._values.items() From 5f3795cde486b39b9dbcdf77e69488aedcff28ef Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 3 Aug 2026 02:11:05 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(enrichment):=20S4=20review=20round=201?= =?UTF-8?q?=20=E2=80=94=20a=20declared=20conversion,=20and=20the=20guards?= =?UTF-8?q?=20the=20claims=20needed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from /code-review max (five parallel reviewers), all reproduced before fixing. The blind `np.asarray(gids, dtype=np.int64)` was wrong in three ways, one of them a regression against development: - a NaN gid became INT64_MIN with only a RuntimeWarning, and then the warning line itself raised `ValueError: cannot convert float NaN to integer` because it iterated the PRE-cast array. The path whose job is to report the problem gracefully was the path that crashed. A missing gid is ordinary data quality. - a non-integral float truncated SILENTLY: 54220.000000001 -> 54220, matching a real, different cell. Verified against the pandas merge this replaced — it returns null there, and even warns. A silent wrong match is worse than a crash. - pd.NA raised a bare TypeError with no log and no contract-shaped error. Replaced by `_as_cell_ids`, which DECLARES what a cell id is and marks anything else unusable rather than coercing it. Unusable ids gather to null exactly as unknown gids do, so the downstream gate sees a hole rather than a guess. Verified case by case against the old implementation: NaN, drifted float and object-with-None now match it exactly, and pd.NA is strictly better (old crashed). The lookup side had the same hole: a null key would take the same sentinel, collide with an unusable query id, and report FOUND — handing back another cell's metadata. That is the fabricated value this module's docstring forbids. __init__ now refuses it. The warning then named cell 0 for an unusable id — a cell nobody asked about. It now names unknown cells and separately counts rows that carried no usable id, because an id that is not an id has none to report. The AST guard added in this same PR had the defect it was written to catch. It walked the whole `if TYPE_CHECKING` node including `else:`, and substring-matched the test, so `not TYPE_CHECKING` read as a guard. Both evasions reproduced, both now caught, and pinned by a test rather than a comment — the `&`-not-short-circuiting shape, committed twice in one pull request. The CIC's guarantees were prose resting on a throwaway script. Committed: empty and null-key refusals, the four unusable-id behaviours, row/order/index preservation across five shapes plus a non-default index, and the object dtype. 29 tests. ADR-012 updated — my own test docstring said "ADR-012 names the seam, so update it rather than letting the claim drift", and the first draft did not. Three claims in the CIC that PR #200 retired in the manager's CIC were still standing here. Registered C-75: this class has no production caller and now holds a second copy of the delivery path's keyed gather. Tier 3 — the verification path and the delivery path implement the same algorithm twice, and the tests run through the copy that does not ship. Co-Authored-By: Claude Opus 5 (1M context) --- docs/ADRs/012_revised_ontology.md | 4 +- docs/CICs/GaulLookupEnricher.md | 22 +++- reports/technical_risk_register.md | 28 +++- tests/test_doc_accuracy.py | 75 ++++++++++- tests/test_enrichment.py | 139 ++++++++++++++++++++ views_postprocessing/contract/enrichment.py | 82 ++++++++++-- 6 files changed, 326 insertions(+), 24 deletions(-) diff --git a/docs/ADRs/012_revised_ontology.md b/docs/ADRs/012_revised_ontology.md index bf9fa32..ae58eb1 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. diff --git a/docs/CICs/GaulLookupEnricher.md b/docs/CICs/GaulLookupEnricher.md index 02d83ac..d0a49fe 100644 --- a/docs/CICs/GaulLookupEnricher.md +++ b/docs/CICs/GaulLookupEnricher.md @@ -25,8 +25,12 @@ 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. --- @@ -103,9 +107,10 @@ 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). --- @@ -135,7 +140,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..d8e7b33 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 | 75 | +| Open Concerns | 16 | | Resolved Concerns | 59 | --- @@ -532,6 +532,30 @@ 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. | +| 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 already calls `enrichment.py` a *"retired-in-place legacy seam"*), **#89** / **#90** / epic **#85**, ADR-014 §4. + +--- + ## 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 fa0d661..c01c56c 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -153,14 +153,29 @@ def test_pandas_is_not_imported_at_runtime_anywhere_in_the_package(): real import from one inside a ``TYPE_CHECKING`` guard, and would have passed unchanged while the meaningful property changed underneath it. """ + 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" + runtime, type_only = [], [] for source in sorted(_PKG.rglob("*.py")): tree = ast.parse(source.read_text()) - guards = [ - n for n in ast.walk(tree) - if isinstance(n, ast.If) and "TYPE_CHECKING" in ast.dump(n.test) - ] - guarded = {id(n) for g in guards for n in ast.walk(g)} + # 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) + } for node in ast.walk(tree): if not isinstance(node, (ast.Import, ast.ImportFrom)): continue @@ -443,3 +458,53 @@ 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). + + That is the same shape as the ``&``-not-short-circuiting bug this story fixed in + `enrichment.py` — a guard that READS as a guard and is not one — committed twice + in one pull request, which is why this test exists rather than a comment. + """ + def classify(src: str) -> list[str]: + tree = ast.parse(src) + + def is_tc(test): + if isinstance(test, ast.Name): + return test.id == "TYPE_CHECKING" + return isinstance(test, ast.Attribute) and test.attr == "TYPE_CHECKING" + + guarded = { + id(n) + for node in ast.walk(tree) + if isinstance(node, ast.If) and is_tc(node.test) + for stmt in node.body + for n in ast.walk(stmt) + } + out = [] + for node in ast.walk(tree): + if isinstance(node, (ast.Import, ast.ImportFrom)): + 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") + return out + + assert classify( + "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 classify( + "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 classify( + "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 f087760..57b06ce 100644 --- a/tests/test_enrichment.py +++ b/tests/test_enrichment.py @@ -6,6 +6,10 @@ """ import pandas as pd +import logging + +import pyarrow as pa +import pyarrow.parquet as pq import pytest from views_postprocessing.contract import gaul_lookup @@ -174,3 +178,138 @@ 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_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 7625eb9..c9f33c9 100644 --- a/views_postprocessing/contract/enrichment.py +++ b/views_postprocessing/contract/enrichment.py @@ -74,6 +74,17 @@ def __init__(self, lookup_path: str | Path | None = None) -> None: 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, which refuses to write nulls." + ) + 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 " @@ -111,27 +122,69 @@ def __init__(self, lookup_path: str | Path | None = None) -> None: len(self._keys), self._lookup_path, self.lookup_version, ) - def _gather(self, gids) -> tuple[dict, np.ndarray]: - """Metadata for each gid, plus a mask of the ones absent from the lookup. + @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"): + return arr.astype(np.int64), np.ones(arr.shape, dtype=bool) + if arr.dtype.kind == "f": + usable = np.isfinite(arr) & (arr == np.rint(arr)) + return np.where(usable, arr, 0).astype(np.int64), usable + # object / pandas-nullable: convert element-wise, marking failures unusable. + ids = np.zeros(len(arr), dtype=np.int64) + usable = np.zeros(len(arr), dtype=bool) + for i, value in enumerate(arr): + try: + as_float = float(value) + except (TypeError, ValueError): + continue + if np.isfinite(as_float) and as_float == int(as_float): + ids[i], usable[i] = int(as_float), True + return ids, usable + + def _gather(self, gids) -> tuple[dict, np.ndarray, np.ndarray, np.ndarray]: + """``(metadata, absent mask, converted ids, usable mask)``. 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. + 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 = np.asarray(gids, dtype=np.int64) + 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) - found = self._keys[idx] == wanted + 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() } - return out, ~found + return out, ~found, wanted, usable @staticmethod def _read_version(path: Path) -> str: @@ -177,7 +230,7 @@ def enrich_dataframe_with_pg_info( base = df.copy() gids = base[pg_id_col].to_numpy() - gathered, absent = self._gather(gids) + gathered, absent, wanted, usable = self._gather(gids) merged = base.copy() for col in METADATA_COLS: @@ -185,10 +238,21 @@ def enrich_dataframe_with_pg_info( n_unmapped = int(absent.sum()) if n_unmapped: - unmatched = sorted({int(g) for g, miss in zip(gids, absent) if miss}) + # Two distinct reasons, reported distinctly. An earlier draft did + # `int(g) for g in gids` over the caller's RAW column and raised + # `ValueError: cannot convert float NaN to integer` — from inside the very + # line whose job is to report the problem gracefully. Reporting the + # CONVERTED ids instead fixed the crash but named cell `0` for an + # unusable input, sending a reader after a cell that was never asked for. + # An id that is not a cell id has no id to report; say how many, not which. + 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(merged), unmatched[:20], + n_unmapped, len(merged), detail, ) return merged From f26810912c9ece959d09636e64989da2f087ec77 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 3 Aug 2026 02:31:27 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(enrichment):=20S4=20review=20round=202?= =?UTF-8?q?=20=E2=80=94=20bound=20the=20conversion,=20stop=20parsing,=20pr?= =?UTF-8?q?ove=20the=20mask?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 removed one silent coercion and introduced two more. Round 2 found them. The object branch used `float(value)`, which PARSES. It accepted the string "54220" as a cell id — where the pandas merge this replaces raised "You are trying to merge on object and int64 column". A string gid column is a declaration error and the old path said so; parsing it is inference, and it is the drifted-float defect pointing the other way. It now accepts only values that already ARE integers. `bool` is excluded for the same reason: True is not cell 1. Neither branch bounded int64. `uint64` max wrapped to -1 and `1e30` to INT64_MIN, both flagged VALID — an id nobody wrote, marked usable. Harmless only because real gids are 1..259,200, which is luck of the key range rather than a property the code enforced. Both branches now bound explicitly. `& usable` in `_gather` was the one new guard that survived its own removal: unusable ids substitute to 0, and 0 is not a real gid, so the key comparison already failed. It looked like decoration. It is not, and the case is now committed: with a lookup containing gid 0, a NaN row silently receives cell 0's geography without the mask. That is the fabricated value the docstring forbids, produced from an input the caller never wrote. The out-of-range test I wrote in round 2 built every case with dtype=object, so all of them entered the element-wise branch and the int and float bounds it claimed to cover were never executed — removing them left the suite green. Each case now enters the branch it tests. Mutation matrix after: dropping `& usable`, the float bound, the int bound, or the isinstance check each fails the suite. `np.isfinite` is the exception and is now documented as such rather than left to be rediscovered: inf fails the upper bound, NaN fails `arr == rint(arr)`, so the check is subsumed and defensive, not load-bearing. Saying so is cheaper than the next person mutation-testing it and wondering. 37 tests in test_enrichment.py; 337 in the suite. Wire untouched. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_enrichment.py | 75 +++++++++++++++++++++ views_postprocessing/contract/enrichment.py | 48 ++++++++++--- 2 files changed, 115 insertions(+), 8 deletions(-) diff --git a/tests/test_enrichment.py b/tests/test_enrichment.py index 57b06ce..98485f7 100644 --- a/tests/test_enrichment.py +++ b/tests/test_enrichment.py @@ -8,6 +8,7 @@ import pandas as pd import logging +import numpy as np import pyarrow as pa import pyarrow.parquet as pq import pytest @@ -261,6 +262,80 @@ def test_pandas_na_yields_null_rather_than_a_bare_typeerror(self, enricher, look 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 column"*. 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)), + ], + ) + def test_out_of_range_and_non_numeric_values_are_unusable_not_wrapped(self, 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` becomes `INT64_MIN`. Both pass a naive + finite-and-integral test, so the bound is a separate condition, not a + consequence of the others. + """ + ids, usable = GaulLookupEnricher._as_cell_ids(array) + assert not usable[0], f"{label} was marked usable as id {ids[0]}" + def test_the_warning_names_unknown_cells_but_invents_no_id_for_unusable_ones( self, enricher, lookup, caplog ): diff --git a/views_postprocessing/contract/enrichment.py b/views_postprocessing/contract/enrichment.py index c9f33c9..dde8ba6 100644 --- a/views_postprocessing/contract/enrichment.py +++ b/views_postprocessing/contract/enrichment.py @@ -50,6 +50,12 @@ # 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) + #: 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" @@ -146,21 +152,47 @@ def _as_cell_ids(gids) -> tuple[np.ndarray, np.ndarray]: contract-shaped error, unlike every other guard in this file. """ arr = np.asarray(gids) + if arr.dtype.kind in ("i", "u"): - return arr.astype(np.int64), np.ones(arr.shape, dtype=bool) + # 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": - usable = np.isfinite(arr) & (arr == np.rint(arr)) + # 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. + # `isfinite` is kept for intent and is currently SUBSUMED, which is worth + # saying rather than leaving for someone to rediscover: `inf` fails the + # upper bound and `NaN` fails `arr == rint(arr)` (NaN equals nothing). It + # is the one condition here that survives its own removal — mutation-tested + # — so it is defensive, not load-bearing. The other three each fail the + # suite when dropped. + usable = ( + np.isfinite(arr) + & (arr == np.rint(arr)) + & (arr >= _INT64_MIN) + & (arr <= _INT64_MAX) + ) return np.where(usable, arr, 0).astype(np.int64), usable - # object / pandas-nullable: convert element-wise, marking failures unusable. + + # 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 column`. 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): - try: - as_float = float(value) - except (TypeError, ValueError): + if isinstance(value, bool) or not isinstance(value, (int, np.integer)): continue - if np.isfinite(as_float) and as_float == int(as_float): - ids[i], usable[i] = int(as_float), True + if _INT64_MIN <= int(value) <= _INT64_MAX: + ids[i], usable[i] = int(value), True return ids, usable def _gather(self, gids) -> tuple[dict, np.ndarray, np.ndarray, np.ndarray]: From b406ad66f7db21e644b147f9fbefe3ddc144392a Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 3 Aug 2026 03:31:48 +0200 Subject: [PATCH 5/5] =?UTF-8?q?fix(enrichment):=20S4=20review=20round=203?= =?UTF-8?q?=20=E2=80=94=20the=20float=20bound=20was=20off=20by=20one=20ULP?= =?UTF-8?q?,=20and=20my=20mutation=20proof=20was=20a=20replica?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second /code-review max pass. Three correctness defects and a vacuous test, all reproduced before fixing. The float upper bound admitted exactly what it was added to exclude. _INT64_MAX is 2**63-1, which is odd and NOT representable in float64, so comparing a float against it promotes and rounds UP to 2**63. float(2**63) passed isfinite, integral and both bounds, then wrapped to INT64_MIN and was flagged valid. Now bounded strictly below 2.0**63, which IS exact. _INT64_MIN needed no change: -2**63 is a power of two. timedelta64 was accepted as a cell id, because isinstance(np.timedelta64(5,'ns'), np.integer) is True — a numpy quirk, and datetime64 is not, so the hole was specific to one type. A duration is not a cell id any more than True is cell 1. The test I wrote to prove the AST guard bites was itself a replica. It defined a local copy of the classifier and asserted against that; reintroducing the polarity bug into the REAL guard left all 16 tests green. That is the defect S4 of epic #181 retired from test_validation.py two days ago — 43 tests against a function the file defined itself — committed again by me, in the PR whose message boasts about catching its cousin. The classifier is now one module-level function with two callers, and both regressions now fail it. And the fix for that had a sting: routing the out-of-range cases through the public API, as the review recommended, LOST the proof. Publicly, "refused" and "accepted but wrapped to an absent id" both produce null, so reverting the float bound left the test green. The public assertion is the caller's guarantee; the mask assertion is the mutation proof; both are needed and both are now present, with a comment saying why. _gather returned four values, three of which existed only to build one log message. The warning moved inside it, matching this file's own convention where __init__ co-locates detection and logging at every guard. It returns one dict. My own error message credited the builder with refusing null keys. It does not: 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 an earlier astype('int64') raises — protection by accident. Registered as C-76, along with the empty-output case, which IS reachable and writes cells=0 without complaint. Also: the pandas error message was misquoted, ADR-012 cited a path that does not exist (unfao/wire/ -> contract/wire/), the CIC claimed eight input shapes where the pinned class has six and named one it does not exercise, the artifact size was wrong by 2% in four places, and a register quote was singular where C-40 is plural. C-75 gained the owner ADR-014 §4 requires and its first draft omitted while citing §4. Co-Authored-By: Claude Opus 5 (1M context) --- docs/ADRs/012_revised_ontology.md | 2 +- docs/CICs/GaulLookupEnricher.md | 10 +- reports/technical_risk_register.md | 28 ++++- tests/test_doc_accuracy.py | 119 +++++++++---------- tests/test_enrichment.py | 43 +++++-- views_postprocessing/contract/enrichment.py | 97 +++++++++------ views_postprocessing/contract/gaul_lookup.py | 2 +- 7 files changed, 190 insertions(+), 111 deletions(-) diff --git a/docs/ADRs/012_revised_ontology.md b/docs/ADRs/012_revised_ontology.md index ae58eb1..79dcc3e 100644 --- a/docs/ADRs/012_revised_ontology.md +++ b/docs/ADRs/012_revised_ontology.md @@ -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 d0a49fe..a49cfc5 100644 --- a/docs/CICs/GaulLookupEnricher.md +++ b/docs/CICs/GaulLookupEnricher.md @@ -53,9 +53,13 @@ views-datafactory area-majority join), so this class does only a table join. - 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, row order **and index** of the input are preserved. Verified against the - previous implementation across eight input shapes, including a non-default index, - duplicated gids, unknown gids and empty input. +- 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 diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index d8e7b33..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 | 75 | -| Open Concerns | 16 | +| Total Concerns | 76 | +| Open Concerns | 17 | | Resolved Concerns | 59 | --- @@ -540,6 +540,7 @@ Cross-refs: **C-25** (whose resolution this corrects), **C-40** (the inherited p | 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."* @@ -552,7 +553,28 @@ Cross-refs: **C-25** (whose resolution this corrects), **C-40** (the inherited p **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 already calls `enrichment.py` a *"retired-in-place legacy seam"*), **#89** / **#90** / epic **#85**, ADR-014 §4. +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. --- diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index c01c56c..ea16dd2 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -140,6 +140,49 @@ def test_internal_doc_links_resolve(): _MANAGER_LINE_BUDGET = 450 # epic #148's bound; 406 at close, 636 before #149 +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. @@ -153,40 +196,14 @@ def test_pandas_is_not_imported_at_runtime_anywhere_in_the_package(): real import from one inside a ``TYPE_CHECKING`` guard, and would have passed unchanged while the meaningful property changed underneath it. """ - 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" - runtime, type_only = [], [] for source in sorted(_PKG.rglob("*.py")): - tree = ast.parse(source.read_text()) - # 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) - } - 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 not any((n or "").split(".")[0] == "pandas" for n in names): - continue + 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 id(node) in guarded else runtime).append(where) + (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 " @@ -467,44 +484,26 @@ def test_the_runtime_import_guard_catches_both_ways_of_evading_it(): ``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). - - That is the same shape as the ``&``-not-short-circuiting bug this story fixed in - `enrichment.py` — a guard that READS as a guard and is not one — committed twice - in one pull request, which is why this test exists rather than a comment. + - ``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 classify(src: str) -> list[str]: - tree = ast.parse(src) - - def is_tc(test): - if isinstance(test, ast.Name): - return test.id == "TYPE_CHECKING" - return isinstance(test, ast.Attribute) and test.attr == "TYPE_CHECKING" - - guarded = { - id(n) - for node in ast.walk(tree) - if isinstance(node, ast.If) and is_tc(node.test) - for stmt in node.body - for n in ast.walk(stmt) - } - out = [] - for node in ast.walk(tree): - if isinstance(node, (ast.Import, ast.ImportFrom)): - 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") - return out - - assert classify( + 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 classify( + 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 classify( + 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 98485f7..ca1ea6c 100644 --- a/tests/test_enrichment.py +++ b/tests/test_enrichment.py @@ -299,7 +299,8 @@ def test_an_unusable_id_cannot_match_the_cell_it_was_substituted_with(self, tmp_ 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 column"*. An + `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). """ @@ -323,18 +324,46 @@ def test_a_string_gid_column_is_refused_rather_than_parsed(self, enricher, looku ("-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, label, array): + 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` becomes `INT64_MIN`. Both pass a naive - finite-and-integral test, so the bound is a separate condition, not a - consequence of the others. + `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. """ - ids, usable = GaulLookupEnricher._as_cell_ids(array) - assert not usable[0], f"{label} was marked usable as id {ids[0]}" + 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 diff --git a/views_postprocessing/contract/enrichment.py b/views_postprocessing/contract/enrichment.py index dde8ba6..75baa77 100644 --- a/views_postprocessing/contract/enrichment.py +++ b/views_postprocessing/contract/enrichment.py @@ -17,7 +17,7 @@ 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 888 KB artifact is no longer materialised as a pandas frame, and the +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. @@ -40,7 +40,7 @@ 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 888 KB lookup is no longer materialised as a pandas frame, and the + # 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 @@ -56,6 +56,13 @@ _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" @@ -86,7 +93,11 @@ def __init__(self, lookup_path: str | Path | None = None) -> None: "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, which refuses to write nulls." + "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) @@ -163,17 +174,22 @@ def _as_cell_ids(gids) -> tuple[np.ndarray, np.ndarray]: # 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. - # `isfinite` is kept for intent and is currently SUBSUMED, which is worth - # saying rather than leaving for someone to rediscover: `inf` fails the - # upper bound and `NaN` fails `arr == rint(arr)` (NaN equals nothing). It - # is the one condition here that survives its own removal — mutation-tested - # — so it is defensive, not load-bearing. The other three each fail the - # suite when dropped. + # 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) - & (arr <= _INT64_MAX) + & (arr >= _INT64_MIN_F) + & (arr < _INT64_MAX_EXCLUSIVE_F) ) return np.where(usable, arr, 0).astype(np.int64), usable @@ -181,7 +197,7 @@ def _as_cell_ids(gids) -> tuple[np.ndarray, np.ndarray]: # # 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 column`. A string + # `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 @@ -189,14 +205,19 @@ def _as_cell_ids(gids) -> tuple[np.ndarray, np.ndarray]: ids = np.zeros(len(arr), dtype=np.int64) usable = np.zeros(len(arr), dtype=bool) for i, value in enumerate(arr): - if isinstance(value, bool) or not isinstance(value, (int, np.integer)): + # `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) -> tuple[dict, np.ndarray, np.ndarray, np.ndarray]: - """``(metadata, absent mask, converted ids, usable mask)``. + 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. @@ -211,12 +232,35 @@ def _gather(self, gids) -> tuple[dict, np.ndarray, np.ndarray, np.ndarray]: # 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() } - return out, ~found, wanted, usable + + # 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: @@ -262,30 +306,11 @@ def enrich_dataframe_with_pg_info( base = df.copy() gids = base[pg_id_col].to_numpy() - gathered, absent, wanted, usable = self._gather(gids) + gathered = self._gather(gids) merged = base.copy() for col in METADATA_COLS: merged[col] = gathered[col] - - n_unmapped = int(absent.sum()) - if n_unmapped: - # Two distinct reasons, reported distinctly. An earlier draft did - # `int(g) for g in gids` over the caller's RAW column and raised - # `ValueError: cannot convert float NaN to integer` — from inside the very - # line whose job is to report the problem gracefully. Reporting the - # CONVERTED ids instead fixed the crash but named cell `0` for an - # unusable input, sending a reader after a cell that was never asked for. - # An id that is not a cell id has no id to report; say how many, not which. - 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(merged), detail, - ) 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.