From 72fda59aea3dc4a9c8e1fb6a81271c46c3942a9e Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 26 Jun 2026 09:29:34 +0200 Subject: [PATCH 1/8] =?UTF-8?q?feat(delivery):=20input-integrity=20invaria?= =?UTF-8?q?nts=20S0-S2=20=E2=80=94=20coverage=20+=20observed-range=20guard?= =?UTF-8?q?s=20(#51)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces the representation-free delivery-integrity layer and wires the first two guards into the FAO manager (epic #51, S0-S2). - views_postprocessing/delivery/: partner-agnostic, pandas-free invariants — coverage (S1/C-34) and observed_range (S2/C-26). Called by managers, never inherited. - unfao/extraction.py: the single pandas-aware seam feeding primitives to the invariants (swappable when the delivery representation leaves pandas, C-40). - unfao/source_metadata.py: producer data-facts read straight from views-datafactory (last_valid_month_id), never via pipeline-core (D-07). - unfao manager: _clip_observed_history (drop fabricated tail months > last_valid_month_id, historical frame only) and _check_coverage (enforce pinned-region cell counts, else log-skip). - enrichment.py: "prediction frame" -> "input DataFrame" naming (frame conflation). - unit + design-contract tests for each. S3-S6 (#54-#57) remain. Reconciliation/ still present here (predates PR #63; deferred sync). Co-Authored-By: Claude Opus 4.8 --- tests/test_delivery_coverage.py | 44 ++++++++++++ tests/test_delivery_observed_range.py | 26 +++++++ tests/test_extraction.py | 35 +++++++++ tests/test_input_integrity_design_contract.py | 72 +++++++++++++++++++ views_postprocessing/delivery/__init__.py | 12 ++++ views_postprocessing/delivery/coverage.py | 59 +++++++++++++++ .../delivery/observed_range.py | 32 +++++++++ views_postprocessing/unfao/enrichment.py | 4 +- views_postprocessing/unfao/extraction.py | 61 ++++++++++++++++ views_postprocessing/unfao/managers/unfao.py | 70 ++++++++++++++++++ views_postprocessing/unfao/source_metadata.py | 39 ++++++++++ 11 files changed, 452 insertions(+), 2 deletions(-) create mode 100644 tests/test_delivery_coverage.py create mode 100644 tests/test_delivery_observed_range.py create mode 100644 tests/test_extraction.py create mode 100644 tests/test_input_integrity_design_contract.py create mode 100644 views_postprocessing/delivery/__init__.py create mode 100644 views_postprocessing/delivery/coverage.py create mode 100644 views_postprocessing/delivery/observed_range.py create mode 100644 views_postprocessing/unfao/extraction.py create mode 100644 views_postprocessing/unfao/source_metadata.py diff --git a/tests/test_delivery_coverage.py b/tests/test_delivery_coverage.py new file mode 100644 index 0000000..313d261 --- /dev/null +++ b/tests/test_delivery_coverage.py @@ -0,0 +1,44 @@ +"""Unit tests for the representation-free coverage invariant (S1 / C-34). + +Pure primitives in, raise-or-pass out — no framework, no pandas. +""" + +import pytest + +from views_postprocessing.delivery.coverage import ( + EXPECTED_CELLS, + CoverageError, + assert_complete_coverage, + expected_for, +) + + +def test_correct_count_passes(): + assert assert_complete_coverage({1, 2, 3}, 3) is None + + +def test_under_coverage_raises(): + with pytest.raises(CoverageError, match="under-coverage"): + assert_complete_coverage({1, 2}, 3) + + +def test_over_coverage_raises(): + with pytest.raises(CoverageError, match="over-coverage"): + assert_complete_coverage({1, 2, 3, 4}, 3) + + +def test_label_appears_in_message(): + with pytest.raises(CoverageError, match="forecast"): + assert_complete_coverage(set(), 1, label="forecast") + + +def test_land_gaul_is_pinned(): + assert expected_for("land_gaul") == 64_736 + assert EXPECTED_CELLS["land_gaul"] == 64_736 + + +def test_unpinned_or_missing_region_is_none(): + # africa_me_legacy is deliberately unpinned (ambiguous count); None must not raise. + assert expected_for("africa_me_legacy") is None + assert expected_for(None) is None + assert expected_for("does_not_exist") is None diff --git a/tests/test_delivery_observed_range.py b/tests/test_delivery_observed_range.py new file mode 100644 index 0000000..bd05b47 --- /dev/null +++ b/tests/test_delivery_observed_range.py @@ -0,0 +1,26 @@ +"""Unit tests for the representation-free observed-range invariant (S2 / C-26).""" + +import numpy as np + +from views_postprocessing.delivery.observed_range import fabricated_months, is_observed + + +def test_fabricated_months_returns_months_beyond_boundary(): + out = fabricated_months(np.array([100, 101, 102, 103]), last_valid_month_id=101) + np.testing.assert_array_equal(out, np.array([102, 103])) + + +def test_fabricated_months_empty_when_all_observed(): + out = fabricated_months(np.array([98, 99, 100]), last_valid_month_id=100) + assert out.size == 0 + + +def test_fabricated_months_distinct_and_sorted(): + out = fabricated_months(np.array([105, 102, 105, 103, 102]), last_valid_month_id=101) + np.testing.assert_array_equal(out, np.array([102, 103, 105])) + + +def test_is_observed_boundary_is_inclusive(): + assert is_observed(100, 100) is True + assert is_observed(101, 100) is False + assert is_observed(50, 100) is True diff --git a/tests/test_extraction.py b/tests/test_extraction.py new file mode 100644 index 0000000..f17e4d9 --- /dev/null +++ b/tests/test_extraction.py @@ -0,0 +1,35 @@ +"""Unit tests for the pandas->primitives extraction seam (unfao/extraction.py).""" + +import numpy as np +import pandas as pd + +from views_postprocessing.unfao import extraction + + +def _frame(rows): + """rows: list of (month_id, priogrid_gid).""" + idx = pd.MultiIndex.from_tuples(rows, names=["month_id", "priogrid_gid"]) + return pd.DataFrame({"lr_ged_sb": np.zeros(len(rows))}, index=idx) + + +def test_cells_of_returns_distinct_gids(): + df = _frame([(100, 1), (100, 2), (101, 1), (101, 2), (101, 3)]) + assert extraction.cells_of(df) == {1, 2, 3} + + +def test_months_of_returns_distinct_sorted_months(): + df = _frame([(101, 1), (100, 1), (100, 2), (102, 1)]) + np.testing.assert_array_equal(extraction.months_of(df), np.array([100, 101, 102])) + + +def test_drop_months_above_removes_padding_rows(): + df = _frame([(100, 1), (101, 1), (102, 1), (103, 1)]) + clipped = extraction.drop_months_above(df, last_valid_month_id=101) + np.testing.assert_array_equal(extraction.months_of(clipped), np.array([100, 101])) + assert len(clipped) == 2 + + +def test_works_on_flat_columns_too(): + df = pd.DataFrame({"month_id": [100, 100, 101], "priogrid_gid": [1, 2, 1]}) + assert extraction.cells_of(df) == {1, 2} + np.testing.assert_array_equal(extraction.months_of(df), np.array([100, 101])) diff --git a/tests/test_input_integrity_design_contract.py b/tests/test_input_integrity_design_contract.py new file mode 100644 index 0000000..5648702 --- /dev/null +++ b/tests/test_input_integrity_design_contract.py @@ -0,0 +1,72 @@ +"""Design-contract falsification stubs for the FAO input-integrity sprint (#51). + +Surfaced by `/falsify` (2026-06-26), adjusted after review. The recommended +direction ("representation-agnostic invariants over primitives, fed by a thin +extraction adapter") only ALIGNS with the maintainer's SOLID + component + +screaming rubric if the implementation honours three tightenings: + + ① TWO homes, not one: + views_postprocessing/delivery/ -> representation-free invariants + + constants (partner-agnostic, reusable) + views_postprocessing/unfao/extraction.py -> the pandas->primitives seam + (FAO-local, representation-specific) + ② Primitives are the abstraction (DIP); extraction isolated in one module (OCP). + No premature Extractor Protocol (YAGNI/ISP) — a migration, not a coexistence. + ③ Guards are CALLED by the manager, never METHODS of it — so C-40's eventual + de-inheritance does not touch them. Manager LSP/SDP/SAP is DEFERRED to C-40. + +Plus housekeeping: no "frame" name overload (DataFrame vs views_frames), and the +dead unfao/mapping/ husk removed. + +All xfail(strict=True): GREEN while the gap exists; each PASSES the moment the +design is implemented correctly, which flips the strict-xfail RED and forces the +marker to be removed (the contract becomes a real assertion). +""" + +import importlib.util +import re +from pathlib import Path + +import pytest + +_IMPORTS_PANDAS = re.compile(r"^\s*(?:import\s+pandas|from\s+pandas\b)", re.MULTILINE) + +_PKG = Path(__file__).resolve().parent.parent / "views_postprocessing" + + +def _spec(name: str): + try: + return importlib.util.find_spec(name) + except ModuleNotFoundError: + return None + + +# ① — two homes ------------------------------------------------------------- +def test_delivery_contract_package_exists(): + assert _spec("views_postprocessing.delivery") is not None + + +def test_delivery_invariants_are_pandas_free(): + d = _PKG / "delivery" + assert d.exists() and not any(_IMPORTS_PANDAS.search(f.read_text()) for f in d.glob("*.py")) + + +def test_extraction_seam_is_isolated_in_one_module(): + assert (_PKG / "unfao" / "extraction.py").exists() + + +# ③ — called, never inherited (deferred to C-40) ---------------------------- +@pytest.mark.xfail(strict=True, reason="③/P2/LSP/SDP/SAP: manager is-a ForecastingModelManager — DEFERRED to C-40, not fixed by this sprint") +def test_manager_does_not_inherit_forecasting_model_manager(): + src = (_PKG / "unfao" / "managers" / "unfao.py").read_text() + assert "ForecastingModelManager" not in src + + +# housekeeping -------------------------------------------------------------- +def test_enrichment_does_not_call_a_dataframe_a_prediction_frame(): + src = (_PKG / "unfao" / "enrichment.py").read_text().lower() + assert "prediction frame" not in src + + +def test_no_lingering_mapping_directory(): + assert not (_PKG / "unfao" / "mapping").exists() diff --git a/views_postprocessing/delivery/__init__.py b/views_postprocessing/delivery/__init__.py new file mode 100644 index 0000000..69ef4d4 --- /dev/null +++ b/views_postprocessing/delivery/__init__.py @@ -0,0 +1,12 @@ +"""``views_postprocessing.delivery`` — representation-free delivery-integrity invariants. + +Partner-agnostic: FAO and the coming UN-agency deliveries reuse these. Every module +in this package operates on **primitives only** (sets of ints, numpy arrays, scalars, +dicts) and **must not import pandas or views_frames** — the representation seam lives +in ``views_postprocessing/unfao/extraction.py``. + +Invariants here are **called by** delivery managers, **never inherited into** them +(see the epic design contract, views-postprocessing#51). The manager's pattern is +``extract → call invariant → raise``; this package owns the *rule*, the extraction +module owns the *representation*, and the two never share a file. +""" diff --git a/views_postprocessing/delivery/coverage.py b/views_postprocessing/delivery/coverage.py new file mode 100644 index 0000000..c9cadd7 --- /dev/null +++ b/views_postprocessing/delivery/coverage.py @@ -0,0 +1,59 @@ +"""Delivery coverage invariant: a delivery must cover exactly its region's cells. + +Representation-free — primitives only (a set of cell ids + an int). No pandas, no +views_frames. The manager feeds it primitives via +``views_postprocessing/unfao/extraction.py``; the rule lives here (S1 / register C-34). +""" + +from __future__ import annotations + + +class CoverageError(ValueError): + """A delivery did not cover exactly the expected set of cells.""" + + +def assert_complete_coverage( + received_gids: set[int], expected_count: int, *, label: str = "delivery" +) -> None: + """Raise unless exactly ``expected_count`` distinct cells were delivered. + + Args: + received_gids: the distinct PRIO-GRID cell ids actually delivered. + expected_count: the number of cells the configured region must cover. + label: short tag for the message (e.g. ``"historical"`` / ``"forecast"``). + + Raises: + CoverageError: on under- or over-coverage. + """ + actual = len(received_gids) + if actual != expected_count: + kind = "under-coverage" if actual < expected_count else "over-coverage" + raise CoverageError( + f"{label}: {kind} — delivered {actual} cells, expected {expected_count} " + f"(diff {actual - expected_count:+d}). A wrong/stale region or dropped " + f"cells would ship partial coverage to the partner." + ) + + +# Region -> expected complete cell count. +# +# SSOT CAVEAT: these are *views-datafactory* facts (it defines the region cell-sets: +# regions.py / *_pgids.json). They are declared here as the delivery's statement of +# intent; source them from datafactory region metadata once it publishes a count +# (the same companion shape as S2's `last_valid_month_id`). Until then only a region +# whose count is *verified* is pinned — an unpinned region logs a skipped gate, never +# a guess. +# +# land_gaul : 64,736 = land ∩ gaul0_code != -1 (register C-30/D-10). +# The 82 excluded sub-Antarctic gids are pinned in S4 (#55). +# africa_me_legacy : intentionally NOT pinned — the register cites 13,110 region +# cells with 5 pure-ocean cells unassigned, so "complete" is +# ambiguous (13,110 vs 13,105). Verify against a real run first. +EXPECTED_CELLS: dict[str, int] = { + "land_gaul": 64_736, +} + + +def expected_for(region: str | None) -> int | None: + """The pinned expected cell count for ``region``, or None if unpinned/unknown.""" + return EXPECTED_CELLS.get(region) if region else None diff --git a/views_postprocessing/delivery/observed_range.py b/views_postprocessing/delivery/observed_range.py new file mode 100644 index 0000000..cbe19e0 --- /dev/null +++ b/views_postprocessing/delivery/observed_range.py @@ -0,0 +1,32 @@ +"""Observed-range invariant: a delivery of *observed* history must not ship months +the producer has not actually observed (register C-26, S2). + +Representation-free — primitives only (a month array + a scalar boundary). The +boundary (``last_valid_month_id``) is a **producer** fact, read from views-datafactory +(see ``views_postprocessing/unfao/source_metadata.py``); this module only decides which +months are fabricated, never how to fetch or filter them. + +Why this matters: the historical request runs to the current calendar month, but UCDP +data ends earlier (reporting lag). The tail months come back as zero-padding and would +ship to the partner as "zero conflict" — fabricated. Months at or below the boundary +are observed; months above it are padding and must be dropped before delivery. +""" + +from __future__ import annotations + +import numpy as np +from numpy.typing import NDArray + + +def fabricated_months(months: NDArray, last_valid_month_id: int) -> NDArray[np.int64]: + """The distinct months strictly beyond the producer's last observed month. + + These carry no observed data (zero-padding) and must not ship as observed history. + """ + m = np.asarray(months, dtype=np.int64) + return np.unique(m[m > int(last_valid_month_id)]) + + +def is_observed(month: int, last_valid_month_id: int) -> bool: + """Whether ``month`` is at or below the producer's last observed month.""" + return int(month) <= int(last_valid_month_id) diff --git a/views_postprocessing/unfao/enrichment.py b/views_postprocessing/unfao/enrichment.py index c96f26e..9e133e9 100644 --- a/views_postprocessing/unfao/enrichment.py +++ b/views_postprocessing/unfao/enrichment.py @@ -4,7 +4,7 @@ ``enrich_dataframe_with_pg_info``. Instead of loading 774 MB of shapefiles and computing spatial intersections at run time, it merges a precomputed lookup table (built by ``scripts/build_gaul_lookup.py`` from the views-datafactory's -area-majority GAUL parquets) onto the prediction frame by PRIO-GRID cell id. +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 @@ -34,7 +34,7 @@ class GaulLookupEnricher: - """Merge precomputed GAUL metadata onto a prediction frame by cell id.""" + """Merge precomputed GAUL metadata onto an input DataFrame by cell id.""" def __init__(self, lookup_path: str | Path | None = None) -> None: self._lookup_path = Path(lookup_path) if lookup_path else _DEFAULT_LOOKUP diff --git a/views_postprocessing/unfao/extraction.py b/views_postprocessing/unfao/extraction.py new file mode 100644 index 0000000..4d7f913 --- /dev/null +++ b/views_postprocessing/unfao/extraction.py @@ -0,0 +1,61 @@ +"""FAO-local representation seam: extract primitives from the pandas delivery frame. + +This is the **only** pandas-aware module the delivery invariants +(``views_postprocessing.delivery``) are fed from. It turns the current pandas +``DataFrame`` representation into the plain primitives the invariants consume +(sets of ints, numpy arrays, dicts). + +When the delivery representation migrates from pandas to views-frames (gated on +pipeline-core's DataFrame retirement, register C-40), **only this module changes** — +a sibling ``extraction`` for the new representation is added and the manager calls it; +the invariants in ``views_postprocessing/delivery/`` are untouched (OCP). + +Per the epic design contract (views-postprocessing#51) there is deliberately **no +``Extractor`` Protocol** — pandas and views-frames do not coexist at runtime (it is a +migration), so a polymorphic interface would be speculative (YAGNI/ISP). +""" + +from __future__ import annotations + +import numpy as np +import pandas as pd +from numpy.typing import NDArray + +# VIEWS PRIO-GRID-month identifier conventions (index levels or flat columns). +_PG_ID = "priogrid_gid" +_TIME_ID = "month_id" + + +def _level_or_column(df: pd.DataFrame, name: str) -> NDArray: + """Return ``name`` as a flat array whether it is an index level or a column.""" + if name in (df.index.names or []): + return np.asarray(df.index.get_level_values(name)) + if name in df.columns: + return np.asarray(df[name]) + raise KeyError( + f"'{name}' is neither an index level nor a column " + f"(have index={list(df.index.names or [])}, columns={list(df.columns)[:8]}...)" + ) + + +def cells_of(df: pd.DataFrame, pg_id: str = _PG_ID) -> set[int]: + """The set of PRIO-GRID cell ids present in the delivery frame.""" + return {int(x) for x in np.unique(_level_or_column(df, pg_id))} + + +def months_of(df: pd.DataFrame, time_id: str = _TIME_ID) -> NDArray[np.int64]: + """The distinct month ids present in the delivery frame, ascending.""" + return np.unique(_level_or_column(df, time_id).astype(np.int64)) + + +def drop_months_above( + df: pd.DataFrame, last_valid_month_id: int, time_id: str = _TIME_ID +) -> pd.DataFrame: + """Return ``df`` with rows whose month exceeds ``last_valid_month_id`` removed. + + The representation-specific half of the S2 observed-range clip: the *decision* + (which months are fabricated) is made by ``delivery.observed_range``; this applies + it to the pandas frame. + """ + months = _level_or_column(df, time_id).astype(np.int64) + return df[months <= int(last_valid_month_id)] diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 0e9e80b..3f2cf17 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -18,6 +18,8 @@ from dotenv import load_dotenv from views_postprocessing.unfao.enrichment import GaulLookupEnricher from views_postprocessing.unfao.gaul_schema import METADATA_COLS +from views_postprocessing.unfao import extraction, source_metadata +from views_postprocessing.delivery import coverage, observed_range from pathlib import Path logger = logging.getLogger(__name__) @@ -55,6 +57,7 @@ def _read_historical_data(self): self._historical_dataframe = read_dataframe( self._data_loader.cached_data_path ) + self._clip_observed_history() self._historical_dataset = PGMDataset( source=self._historical_dataframe, targets=self.configs.get("targets") ) @@ -171,6 +174,73 @@ def _validate(self) -> pd.DataFrame: raise ValueError(err_msg) logger.info("Forecast dataframe metadata validation passed.") + self._check_coverage() + + def _clip_observed_history(self) -> None: + """Drop fabricated (unobserved) months from the historical delivery (S2/C-26). + + The historical request runs to the current calendar month, but UCDP data ends + earlier (reporting lag) at datafactory's ``last_valid_month_id``; the tail months + are zero-padding, not observed zeros, and would ship to FAO as "zero conflict". + + The boundary is read straight from the **producer** (views-datafactory) via + ``source_metadata`` — never pipeline-core. Only the *historical* (observed) frame + is clipped; the forecast frame is future-dated by design and untouched. + """ + lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + if lv is None: + logger.warning( + "last_valid_month_id unavailable from datafactory; the historical " + "delivery was NOT clipped to observed range (C-26 guard skipped)." + ) + return + fabricated = observed_range.fabricated_months( + extraction.months_of(self._historical_dataframe), lv + ) + if len(fabricated): + logger.warning( + "Clipping %d fabricated (unobserved) month(s) > last_valid_month_id=%d " + "from the historical delivery: %s", + len(fabricated), + lv, + fabricated.tolist(), + ) + self._historical_dataframe = extraction.drop_months_above( + self._historical_dataframe, lv + ) + + def _check_coverage(self) -> None: + """Log delivered cell counts and enforce the region coverage contract (S1/C-34). + + Orchestration only: extract primitives via ``extraction`` (the pandas seam), + then call the representation-free ``delivery.coverage`` invariant — the rule is + *called*, not embedded. The count-gate fires only for regions pinned in + ``coverage.EXPECTED_CELLS``; an unpinned/unresolved region logs a skipped gate + rather than guessing. + """ + region = self.configs.get("region") + expected = coverage.expected_for(region) + for label, df in ( + ("historical", self._historical_dataframe), + ("forecast", self._forecast_dataframe), + ): + cells = extraction.cells_of(df) + logger.info( + "%s delivery coverage: %d distinct cells, %d rows.", + label, + len(cells), + len(df), + ) + if expected is not None: + coverage.assert_complete_coverage(cells, expected, label=label) + else: + logger.warning( + "Coverage count-gate skipped for %s: region %r is not pinned in " + "delivery.coverage.EXPECTED_CELLS — verify and pin before relying on it.", + label, + region, + ) + def _save(self) -> list: if self._historical_dataset is None or self._forecast_dataset is None: raise ValueError("Datasets could not be initialized properly.") diff --git a/views_postprocessing/unfao/source_metadata.py b/views_postprocessing/unfao/source_metadata.py new file mode 100644 index 0000000..49b163a --- /dev/null +++ b/views_postprocessing/unfao/source_metadata.py @@ -0,0 +1,39 @@ +"""Producer data-facts for the delivery — read straight from views-datafactory. + +**Architectural principle (maintainer, 2026-06-26):** data-related facts — data, +metadata, validity dates, country/admin codes, ... — come from the **producer** +(views-datafactory, or viewser until it is phased out), **not** routed through +pipeline-core. pipeline-core is the orchestration framework, not a data pass-through; +depending on it for data facts couples the delivery to an unstable, mid-migration hub +(violates SDP, risks ADP cycles). This module is the **single place** the FAO delivery +asks datafactory for source metadata, so that coupling is isolated and named. + +Today it serves ``last_valid_month_id`` (the time-validity boundary for S2/C-26). The +region cell-count (S1/C-34 SSOT) and other producer facts belong here too as datafactory +publishes them. +""" + +from __future__ import annotations + +import logging + +logger = logging.getLogger(__name__) + + +def last_valid_month_id(zarr_url: str | None = None) -> int | None: + """The producer's last *observed* month for the served zarr (a datafactory fact). + + Reads datafactory's published ``.zattrs["last_valid_month_id"]`` directly via + ``datafactory_query`` (a timeout-protected HTTP read of the producer's own metadata). + Returns ``None`` if the store predates the attribute. + + Args: + zarr_url: the served zarr; ``None`` uses datafactory's default store (which the + FAO queryset itself uses — ``ZARR_URL = DEFAULT_REMOTE.zarr_url``). + + The import is lazy so this module loads without the heavy datafactory dependency + present (e.g. in unit-test environments). + """ + from datafactory_query.defaults import get_last_valid_month_id + + return get_last_valid_month_id(zarr_url) From ff909813a188852e854a7c01f0c55722d79fb8a5 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 26 Jun 2026 09:29:34 +0200 Subject: [PATCH 2/8] =?UTF-8?q?docs(risk-register):=20session=20updates=20?= =?UTF-8?q?=E2=80=94=20reconciliation=20cutover,=20D-07,=20curation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accumulated risk-register edits that rode this branch: - Reconciliation migration end-state (verified on origin/development): C-42 corrected (the false "vpp unused" claim; then release v1.7.0 -> repoint views-models#191/PR#202 -> delete #62/PR#63; pipeline-core chose Decision K, not C; mis-stated-state hazard resolved; residual ForecastReconciler coupling tracked by pipeline-core#198, no separate entry). C-37/C-38 relocate to views-frames with the code; C-38 dead-link to the deleted CIC repaired. - D-07 resolved: data-facts come from the producer (datafactory/viewser), not pipeline-core. - /review-rr curation + input-integrity entries (C-26, C-34). Co-Authored-By: Claude Opus 4.8 --- reports/technical_risk_register.md | 195 ++++++++++++++++------------- 1 file changed, 105 insertions(+), 90 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 5fcdc48..ea5d1e2 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -4,10 +4,10 @@ |-------------------|--------------------------------------| | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | -| Last Updated | 2026-06-24 | -| Total Concerns | 41 | -| Open Concerns | 24 | -| Resolved Concerns | 17 | +| Last Updated | 2026-06-26 | +| Total Concerns | 42 | +| Open Concerns | 22 | +| Resolved Concerns | 20 | --- @@ -38,6 +38,7 @@ **Highest tier:** 2 (C-12, C-21) **Fix strategy (5/9 done):** ✅ Replace global warning suppression with targeted filter. ◻ Promote geometry errors from DEBUG to WARNING. ✅ Add `make_valid()` preprocessing. ◻ Narrow exception scope. ✅ Zero-area guard clause (all 7 sites). ◻ `logger.error` before all raises (3 of ~23 done). ✅ Surface batch failures to caller (both methods). ◻ Enrichment provenance in upload (timestamp added, no shapefile version). ◻ Post-delivery correction procedure (C-22). **Resolution scope:** Full (code mechanisms) + Partial (operational impact — C-22 requires process documentation). **Note:** If D-05 resolves toward mapper elimination, remaining code fixes become moot. +**✅ MOSTLY RESOLVED 2026-06-24:** the mapper deletion (C-39) removed the `mapping.py` error-hiding sites — **C-12, C-20, C-21 are resolved**, and the remaining ◻ fix-strategy items (geometry-error log level, exception-scope narrowing, `logger.error` before `mapping.py` raises) describe deleted code and are **moot**. Only the manager-side residue remains: **C-19** (3 `unfao.py` raises) and **C-22** (post-delivery correction process). ### Cluster C: Module-level import side effect **Root cause:** `set_default_mapper()` couples class definition with instantiation and shapefile loading at import time. @@ -105,6 +106,8 @@ Tier recalibrated from 2 to 3 during review-rr (2026-06-02): the gap is maintain `mapping.py` directly imports `geopandas`, `shapely`, `numpy`, `pandas`, `joblib`, and `multiprocessing`. `unfao.py` directly imports `pandas`, `polars`, and `python-dotenv`. Only `views-pipeline-core` and `cachetools` are declared in `pyproject.toml`. The undeclared dependencies presumably arrive transitively via `views-pipeline-core`, but this coupling is implicit and fragile. If the upstream package refactors its dependency tree, this package will break with `ImportError` at install time. +**Update 2026-06-24 (narrowed):** the `mapping.py` dimension is gone (C-39 — the `geopandas`/`shapely`/`joblib`/`multiprocessing` imports were deleted; `cachetools` dropped from `pyproject.toml`). Residual: `unfao.py` imports `pandas`/`polars`/`python-dotenv` undeclared, arriving transitively via `views-pipeline-core` (which *is* declared). Much smaller surface (Tier 4-ish); consider resolving outright if the transitive-via-pipeline-core guarantee is deemed sufficient. + --- ### C-08: Planar area calculation on geographic (degree-based) coordinates @@ -121,6 +124,8 @@ All overlap ratio calculations use `.area` on EPSG:4326 geometries, which produc **Update 2026-06-12 (expert-code-review):** The "equatorial/mid-latitude, limiting the impact" rationale dies with the planned global coverage — Russia, Scandinavia, and Canada (55°N+) enter scope when the region switches to `"land"`. Mitigating consideration: within a single 0.5° cell, all candidate polygon intersections sit at the same latitude band, so the cos(lat) distortion multiplies all candidates roughly equally and largely cancels in the *ranking* — this applies to both this repo's mapper and the datafactory's area-majority script. Required action before global delivery: one falsification probe on ~20 border cells above 55°N comparing degree-based assignment against an equal-area-projected computation. See C-31 (mapper unverified at global scale). +**Update 2026-06-24 (narrowed to the datafactory dimension):** this repo's mapper area-math (`mapping.py:649-650,920,1192,1428`) is deleted (C-39); no degree-based area math runs in this repo anymore. The remaining concern is the **views-datafactory** area-majority script's degree-based area math at high latitudes — a cross-repo views-datafactory concern (this repo now consumes the lookup built from those parquets, so any distortion is upstream). Tracked there, not here. + --- ### C-09: Publish workflow validates version against wrong PyPI package @@ -203,24 +208,6 @@ Part of Cluster B (operational impact dimension). See also C-14, C-15. --- -### C-23: Algorithmic divergence — area-based vs centroid-based GAUL mapping across VIEWS platform - -| Field | Value | -|-------|-------| -| ID | C-23 | -| Tier | 2 | -| Source | `manual` (2026-06-02) — external assessment | -| Trigger | When datafactory's assembled grid is used alongside postprocessor enrichment for the same GIDs, verify that gaul0/gaul1/gaul2 assignments agree — currently they use different algorithms (centroid vs area-based) that disagree on border cells | -| Location | `views_postprocessing/unfao/mapping/mapping.py` (area-based), external `views-datafactory/datafactory/gaul_admin.py` (centroid-based) | - -The VIEWS platform has two independent PRIO-GRID-to-GAUL mapping implementations using different algorithms. For the ~95% of cells entirely within one region, both agree. For border cells, they disagree — and nothing reconciles them. It is unclear whether area-based is a deliberate FAO requirement or historical accident. - -**Update 2026-06-12 — divergence resolved upstream.** views-datafactory shipped area-majority GAUL assignment (issue #115 → PR #127, v1.2.28/29); all 7 GAUL parquets regenerated June 11 as area-majority. Both implementations now use the same algorithm family. Residual difference: this repo's mapper sources `country_iso_a3` from Natural Earth while the factory uses GAUL boundaries — disputed-border cells can still differ on ISO code. This residual disappears when ADR-011's lookup (built from factory parquets) replaces the mapper. See `docs/cross_repo_integration_report.md`. - -Part of Cluster E. See also D-05 (the gating decision), C-11 (god class — moot if mapper eliminated). - ---- - ### C-24: Postprocessor output schema diverges from FAO-confirmed API contract | Field | Value | @@ -337,22 +324,6 @@ See also D-10 (handling decision), C-34 (coverage contract). --- -### C-31: Runtime mapper unverified and unverifiable at global scale - -| Field | Value | -|-------|-------| -| ID | C-31 | -| Tier | 2 — choosing this path for global delivery converts unknown runtime, unknown memory, and unknown Natural-Earth coverage into delivery-day discoveries | -| Source | `expert-code-review` (2026-06-12) | -| Trigger | Before any global (`land` region) run that enriches via `mapping.py`, verify runtime, peak memory, and Natural-Earth assignment coverage for all 64,818 cells — none has ever been measured, and none can be measured in the development environment (shapefiles are LFS stubs) | -| Location | `views_postprocessing/unfao/mapping/mapping.py:2727` (per-gid boolean mask over all 259,200 grid rows — O(N) per lookup), `mapping.py:649-650,920,1192,1428` (degree-based area math, C-08, newly in scope above 55°N) | - -The mapper is production-proven at 13,110 cells and never executed at 64,818. Per-cell linear scans put plausible global runtime in the hours; any cell Natural Earth fails to assign produces a null that crashes `_validate()` **at the end of those hours**. The equivalent completeness number for the lookup path is known exactly (C-30: 64,736/82); for the mapper path it is unknowable before a production-machine run. This asymmetry — enumerable versus discoverable failures — is the core argument in D-08. - -See also C-08 (high-latitude math, escalated), C-11 (god class), D-08. - ---- - ### C-32: Unbudgeted memory at global enrichment volume | Field | Value | @@ -399,24 +370,6 @@ See also C-30, C-26 (both are coverage-integrity failures with no signal). --- -### C-35: Invalid `-99` country code shipped to FAO for Somaliland cells - -| Field | Value | -|-------|-------| -| ID | C-35 | -| Tier | 1 — silent invalid data delivered to the partner: a non-ISO sentinel string passes the null-only validation gate and reaches FAO as a country code | -| Source | `enrichment-diff` (2026-06-18) — empirically measured, old mapper vs new lookup on africa_me | -| Trigger | Whenever the current runtime mapper enriches cells in the Somaliland region (and any other Natural Earth `ISO_A3 = "-99"` territory), it emits `country_iso_a3 = "-99"`; `_validate()` checks only for nulls, so the invalid code ships | -| Location | `views_postprocessing/unfao/mapping/mapping.py` (country from Natural Earth `ISO_A3`); `unfao.py:188-221` (`_validate` — null-only, no code-validity check); Natural Earth `ne_10m_admin_0_countries` (`ADMIN="Somaliland", ISO_A3="-99"`) | - -The current mapper sources `country_iso_a3` from Natural Earth's `ISO_A3` field. Natural Earth represents Somaliland as a separate de-facto entity but assigns it the sentinel `ISO_A3 = "-99"` (no recognized ISO code). The diff measured **64 africa_me cells** delivered with `country_iso_a3 = "-99"`. Because `"-99"` is a non-null string, the `_validate()` gate (which only rejects nulls) passes it, and it reaches the FAO Appwrite bucket as the country code for those cells. FAO consumers filtering or aggregating by country code receive an invalid value. - -**Resolved by ADR-011's lookup.** The new GAUL-sourced lookup has zero `-99` codes anywhere (verified across all 64,742 global cells); Somaliland cells become `SOM` (Somalia), matching GAUL — FAO's own boundary product. So the engine swap (Stage 3) eliminates this defect as a side effect. Until the swap ships, the current production output carries it. Note: other Natural Earth `-99` territories (e.g. N. Cyprus, Kosovo) could surface the same way outside africa_me — the global swap covers them too. - -See also C-01 (null-validation re-enabled — but it does not check code *validity*), and the disputed-territories section of `reports/enrichment_diff/report.md`. - ---- - ### C-37: Reconciliation uses a pragmatic per-draw approximation, not principled probabilistic reconciliation | Field | Value | @@ -435,6 +388,10 @@ See also the migration plan (reconciliation slices 2-4) and views-reporting issu **Update 2026-06-24 (reframe — now near-term, not deferred):** this is no longer a someday concern. FAO (`rusty_bucket`) sidesteps reconciliation entirely (pure-grid ensemble aggregated *up* — sums by construction), but the **next UN-agency deliverable** is an FAO-like grid ensemble that **does** reconcile against a CM model, with **full pooled draws (~1024)** → **probabilistic** reconciliation. The reconciler is already probabilistic-ready (fully vectorized over samples), so the open question is purely C-37's: does that deliverable reconcile grid draws to a country **point total** (well-defined, no alignment needed) or to country **draws** (needs a defined draw-alignment — and today CM models are point-only / independently trained, so no aligned draws exist)? **This decision gates the probabilistic-reconciliation-on-`PredictionFrameEnsembleManager` work** (pipeline-core#200, under epic #193); resolve it once the UN models / CM target are defined. +**Calibration note (review-rr 2026-06-24):** stays **Tier 3 while unwired**, but **escalate to Tier 2 the moment reconciliation is wired into a delivery** — at that point a wrong sample-alignment assumption silently delivers *meaningless uncertainty* (a correctness risk, not maintainability). The wiring (pipeline-core#200) is the escalation trigger. + +**Update 2026-06-26 (home change):** the reconciler (`proportional.py`, `grouping.py`, `module.py`) is relocating from this repo to the **`views_frames_reconcile` sibling** in the views-frames distribution (Epic 11, views-platform/views-frames#131) — its correct foundation home (CRP/SDP: a frame operation belongs in the frames family, not bolted onto FAO delivery). **C-37 and C-38 move with it** — track them in views-frames going forward. vpp's copy is deleted in #62 once views-frames v1.7.0 ships (release → repoint views-models#191 → delete). + --- ### C-38: Reconciliation grouping is O(groups × N) and materializes the whole grid frame — won't scale to global volume @@ -451,7 +408,7 @@ See also the migration plan (reconciliation slices 2-4) and views-reporting issu Tier 2: structural fragility under the realistic change of wiring to global, with a clear trigger; not Tier 1 (no silent corruption — parity is exact; this is a runtime/memory failure). See also C-31 (mapper scale), C-32 (enricher memory), C-37 (the algorithm), epic #31 / views-reporting#72. -**Update 2026-06-24 — compute RESOLVED.** The per-group `np.nonzero(inverse == gi)` was replaced with **group-by-sort** (`argsort(inverse)` + contiguous slices from `np.unique` counts, O(N log N), one index array). Parity stays **bit-exact** (`tests/test_reconciliation_grouping.py`, `test_reconciliation_e2e_parity.py` → 0.0) and a scale guard (`tests/test_reconciliation_scale.py`) protects against regression. **Residual (still open):** the module holds the whole pgm frame in memory at once; at global volume the **caller must chunk by time** (reconciliation is independent across months) — the chunk-by-time contract is documented in the CIC (`docs/CICs/ReconciliationModule.md` §5), to be **verified on a global-volume dry-run at S7 (#39)**. This entry stays open until that verification. +**Update 2026-06-24 — compute RESOLVED.** The per-group `np.nonzero(inverse == gi)` was replaced with **group-by-sort** (`argsort(inverse)` + contiguous slices from `np.unique` counts, O(N log N), one index array). Parity stays **bit-exact** (`tests/test_reconciliation_grouping.py`, `test_reconciliation_e2e_parity.py` → 0.0) and a scale guard (`tests/test_reconciliation_scale.py`) protects against regression. **Residual (relocated with the code):** the module holds the whole pgm frame in memory at once; at global volume the **caller must chunk by time** (reconciliation is independent across months). The reconciler — and this chunk-by-time obligation — **left vpp**: the algorithm now lives in `views_frames_reconcile` and the vpp `ReconciliationModule` CIC was retired (#62 / PR #63, merged to `development` 2026-06-26). The global-volume verification is now a **consumer-side obligation at reconciliation-wiring time** (pipeline-core#200/#221), not a vpp concern. Tracked cross-repo via C-42; no further vpp action. **Update 2026-06-24 (reframe — residual now near-term):** the memory residual is no longer "verify someday." The upcoming UN-agency deliverable reconciles **frames with ~1024 pooled draws** — squarely in the >100 GB-at-global regime. When pipeline-core's `PredictionFrameEnsembleManager` wires probabilistic reconciliation (pipeline-core#200, under epic #193), the **caller must chunk by time** (reconciliation is independent across months) and **measure peak memory on a global-volume dry-run** as part of that work. The reconciler code itself is unchanged (compute already O(N log N)); this is a consumer-side obligation. @@ -473,44 +430,32 @@ See also C-07/C-27/C-29 (pipeline-core coupling symptoms), C-39 (the dead-mapper --- -## Disagreements - -### D-05: Strategic direction — eliminate runtime mapper vs keep area-based algorithm +### C-42: Reconciliation migration is stranded across three repos; production runs the old path and the migration-state was mis-stated | Field | Value | |-------|-------| -| ID | D-05 | -| Source | `manual` (2026-06-02) — external assessment | -| Perspectives | Path A: area-based is a FAO requirement → precompute lookup table. Path B: centroid-based acceptable → eliminate mapping.py entirely. Both eliminate 774 MB shapefiles, geopandas, and 3,100-line mapper. | -| Resolution | **Resolved (2026-06-02): Path A confirmed.** FAO-FSFC provided written confirmation (Release Note 02, `summary.tex`) agreeing to area-majority allocation as the locked aggregation rule: "Each PRIO-GRID cell is assigned to a single country using an area-majority rule." The area-based algorithm is a contractual requirement, not a historical accident. Path B (centroid-based) is off the table. Next step: build a one-time precomputed area-based lookup table (~65K rows, Parquet) and replace the 3,100-line runtime mapper with a dictionary lookup. This still eliminates geopandas, the shapefile bundle, and the runtime spatial operations — but preserves the area-majority assignment rule. | -| Update 2026-06-12 | **Upstream resolution deployed.** views-datafactory shipped area-majority GAUL assignment (issue #115 → PR #127, ADR-039 there, v1.2.28/29). All 7 GAUL parquets (codes + names + iso3) regenerated June 11 as area-majority, 259,200 rows each, mutually consistent (13,105/13,110 africa_me cells fully attributed; 5 pure-ocean cells unassigned). The precomputed lookup table can now be built by joining the factory parquets — no LFS, no shapefiles, no this-repo mapper run needed. The platform-level mapping divergence (centroid in factory vs area-majority here) no longer exists. See `docs/cross_repo_integration_report.md` and ADR-011 assessment §10. | +| ID | C-42 | +| Tier | 3 | +| Source | `manual` (2026-06-26) — cross-repo verification on `origin/development` | +| Trigger | When a cross-repo agent next acts on reconciliation (merges pipeline-core PR #217, repoints any consumer, **deletes vpp's copy**, or builds a new reconciliation feature), confirm which copy it targets against `origin/development` first — do **not** assume #217 is merged, the cycle is broken, or vpp's copy is unused (views-models imports it live; deletion is hard-gated on C1) | +| Location | pipeline-core `origin/development`: `managers/ensemble/ensemble.py:747`, `managers/ensemble/dataframe_ensemble.py:921`, `modules/reconciliation/__init__.py:4` (live `views_reporting.reconciliation` imports); `views-models/reconciliation/reconciler_factory.py:54` (**unguarded live import of `views_postprocessing.reconciliation`** — ADR-014 composition root); `views_postprocessing/reconciliation/` (parity-proven, **consumed by views-models**); the plan file + issue #39 (carried the false "merged" premise) | ---- +Verified 2026-06-26 on `origin/development`: pipeline-core **still imports `views_reporting.reconciliation`** (3 sites above) — the pipeline-core↔views-reporting reconciliation **cycle is live**, and production reconciliation still runs through views-reporting (torch). **PR #217** (the DIP port + adapter that would decouple it, #195) is **OPEN/unmerged** — the port `domain/reconciliation.py` exists on dev but the adapter does not. So **three reconciler copies are in flight**: views-reporting (live via pipeline-core), vpp (parity-proven, PR #30), views-frames (now SHIPPED — v1.7.0 on PyPI 2026-06-26). -### D-07: Historical data route — keep pipeline-core dispatcher vs call datafactory directly +**CORRECTION 2026-06-26 (exploration-verified): vpp's `reconciliation/` is NOT "unused/stranded."** `views-models/reconciliation/reconciler_factory.py:54` does an **unguarded** live import `from views_postprocessing.reconciliation import ReconciliationModule` (the ADR-014 composition root, constructed at runtime by reconciling ensembles). The earlier "unused by any production consumer" wording here was wrong — and it was itself an instance of this entry's own hazard (state-drift that could prompt an unsafe action). **Deletion-safety consequence:** deleting vpp's `reconciliation/` **hard-breaks views-models** unless C1 (repoint views-models → `views_frames_reconcile`) lands and goes green **first**. All other importers are guarded (`pytest.importorskip`): views-frames `test_reconcile_head_to_head.py`, views-models `test_reconciliation_factory.py`. pipeline-core / views-reporting do not import vpp. The hazard remains **acting on a false state** (e.g. merging the throwaway #217 port, or deleting a copy that is in fact a live dependency). No silent data corruption (the production path works; it is just the old one) → **Tier 3** (coordination / state-drift / cost-of-change). -| Field | Value | -|-------|-------| -| ID | D-07 | -| Source | `expert-code-review` (2026-06-12) | -| Perspectives | Ousterhout/Hickey: the postprocessor uses ~20% of `ViewsDataLoader`'s services (it passes `use_saved=False, validate=False, self_test=False`) while paying 100% of the seven-hop indirection — call `datafactory_query.load_dataset()` directly and own the three renames. Martin/GoF/Feathers: the dispatcher seam absorbed the viewser→datafactory migration with zero consumer changes and will absorb the next one; bypassing it re-couples the postprocessor to the current backend. | -| Location | `views_postprocessing/unfao/managers/unfao.py:44-59`; views-pipeline-core `modules/dataloaders/dataloaders.py:1088-1224` | -| Status | Open. Review recommendation: keep the pipeline-core route, but pass explicit `month_first/month_last` instead of partition semantics, consume `get_data()`'s return value (C-29), and re-enable validation once it supports datafactory sources. Decide alongside ADR-011 since both touch the same manager. | +**Mitigation:** do **not** merge #217 as a throwaway bridge. The "release" leg is complete (v1.7.0 on PyPI). Cutover order: **repoint** (views-models#191 = C1; pipeline-core#221 = collapse the port, parallel) → **delete** (vpp #62 = C2, only after C1 green) → retire views-reporting reconciliation (vpp #40 / views-reporting#72, after #221). ---- +**Update 2026-06-26 (cutover landed — the mis-stated-state hazard has resolved):** all three legs are done and verified on `origin/development`. **release** — views-frames v1.7.0 on PyPI. **repoint** — views-models **PR #202** merged (`reconciler_factory.py` imports `views_frames_reconcile`); **pipeline-core chose Decision K, not C** — **PR #217 merged** (`6427b9d`): it reconciles via its `Reconciler` DIP port with `views_frames_reconcile.ReconciliationModule` injected (the C-cutover issue #221 was closed unused). **delete** — vpp's copy removed in **#62 / PR #63** (merged to `development`, `c9e38402`). A full cross-repo sweep confirms no unguarded importer of `views_postprocessing.reconciliation` anywhere. The original hazard (acting on a *mis-stated* migration state) is **resolved** — every actor was verified against `origin/development` before acting. **The one residual is split out as C-43:** pipeline-core still imports `views_reporting.statistics.ForecastReconciler`, so the pipeline-core↔views-reporting edge is not fully severed and views-reporting retirement (#40 / views-reporting#72) is still blocked. This entry can move to **Resolved** once C-43 is filed (it is); kept open here only pending a /review-rr relocation. -### D-08: Global delivery path — scale up the runtime mapper vs swap to the precomputed lookup first +See also C-37 / C-38 (the reconciler concerns — relocating to views-frames with the code), views-platform/views-frames#131 (the final home), #62 (retire vpp's copy), views-platform/views-models#191 (repoint), pipeline-core PR #217 (the merged DIP port, Decision K). -| Field | Value | -|-------|-------| -| ID | D-08 | -| Source | `expert-code-review` (2026-06-12) | -| Perspectives | Feathers' instinct: the mapper is production-proven and the region change is one config line — don't swap components days before a deadline. Kleppmann/Nygard/Ousterhout: the mapper is *unverifiable at global scale before running it* (C-31: unknown null count, runtime, memory; C-08 newly in scope), while the lookup's complete global failure set is exactly 82 named cells, verified locally (C-30). | -| Location | `views_postprocessing/unfao/managers/unfao.py:154-160`; `mapping.py`; views-datafactory `data/raw/gaul_admin/*.parquet` | -| Status | Open — user decision pending. Review adjudication: **swap to the lookup first, then go global.** The "don't swap before a deadline" rule assumes the old part is known-good for the new job; here it is known-good only for a job 5× smaller and cannot be tested for the new job until the moment it matters. Enumerable risk beats discoverable risk on a deadline. Prerequisite: shadow diff old-vs-new on africa_me (13,110 cells) on the production machine. | +**Residual tail (verified 2026-06-26 — already tracked cross-repo, no new vpp entry):** pipeline-core still imports `views_reporting.statistics.ForecastReconciler` (`modules/statistics/__init__.py:5`), so the pipeline-core↔views-reporting edge is not yet fully severed and views-reporting's `reconciliation/` + `torch` retirement (#40 / views-reporting#72) is still blocked. But this is **not an untracked hazard**: the re-export is explicitly marked *"remove after downstream consumers update"*; `ForecastReconciler`'s only pipeline-core consumers are the transitional **golden-output equivalence tests** (#119 / #196, "new frames-native == old torch"); and **pipeline-core #198** already owns the removal — its scope note names *"the `reconciliation/` package **and** the `ForecastReconciler` class"* and it triggers vpp **#40** / views-reporting#72. Blocked on pipeline-core #197 (views-postprocessing/views-frames must be "default and stable") first. So C-42 stays open as a thin tracker until #198 lands; no separate vpp entry warranted (would duplicate #198). *(A speculative C-43 was drafted then withdrawn here after verification showed it was a duplicate.)* --- +## Disagreements + ### D-09: Multi-store support — parameterize the manager now vs after the FAO global delivery | Field | Value | @@ -523,21 +468,37 @@ See also C-07/C-27/C-29 (pipeline-core coupling symptoms), C-39 (the dead-mapper --- -### D-10: The 82 GAUL-uncovered cells — exclude, crash, or negotiate with FAO +## Resolved Concerns + +### C-35: Invalid `-99` country code shipped to FAO for Somaliland cells — RESOLVED | Field | Value | |-------|-------| -| ID | D-10 | -| Source | `expert-code-review` (2026-06-12) | -| Perspectives | Fail-loud purism: let validation crash and force the conversation — never silently drop cells. Pragmatic exclusion: drop with a named, count-asserted, logged exclusion list. Diplomatic: ask FAO before shipping anything. | -| Location | `views_postprocessing/unfao/managers/unfao.py:188-221`; the 82 gids enumerated in C-30 | -| Status | **Resolved direction (2026-06-12): fix upstream in views-datafactory.** User decision, superseding the review's exclusion-list adjudication. A new bundled curated region (`land ∩ gaul0_code != -1`, 64,736 cells — e.g. `land_gaul`) is added to `datafactory_query` alongside `land` and `africa_me_legacy`, with generation script, provenance, and a count-pinning test. The postprocessor keeps zero spatial knowledge; its invariant simplifies to "every arriving cell must enrich completely — any null crashes" (the existing `_validate()` gate, unchanged). The `land` region itself is NOT redefined (other consumers depend on its physical-land semantics). Forecast-path residual: unmatched gids null→crash via left merge + validation; pin with one test. FAO disclosure of the 82 excluded sub-Antarctic cells still required in the release note. Closes the postprocessor side of C-30; C-34's coverage test becomes "100% completeness for the configured region." | +| ID | C-35 | +| Resolved | 2026-06-24 | +| Resolution | The mapper that sourced `country_iso_a3` from Natural Earth (emitting the `-99` sentinel for Somaliland, N. Cyprus, Kosovo, …) was deleted (C-39, PR #42). Enrichment now uses the GAUL lookup, which has **zero `-99` codes** across all 64,742 global cells (Somaliland → `SOM`, matching FAO's GAUL). The defect is eliminated as a side effect of the engine swap. | --- +### C-31: Runtime mapper unverified and unverifiable at global scale — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-31 | +| Resolved | 2026-06-24 | +| Resolution | The runtime mapper (`mapping.py`) was deleted (C-39, PR #42), so a global run *via the mapper* can no longer happen — this entry's hazard is moot. The enricher-path global-scale concerns are tracked separately (C-30 coverage, C-32 memory, C-34 coverage contract). D-08 (the swap-to-lookup-first decision this entry argued for) was executed. | + --- -## Resolved Concerns +### C-23: Algorithmic divergence — area-based vs centroid-based GAUL mapping across VIEWS platform — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-23 | +| Resolved | 2026-06-24 | +| Resolution | The platform mapping divergence was resolved upstream (views-datafactory area-majority, 2026-06-12), and the residual ISO-code difference disappeared when ADR-011's GAUL lookup replaced the runtime mapper (C-39, PR #42). The lookup (built from the factory's GAUL parquets) is now the single enrichment source — no second algorithm remains to diverge from. | + +--- ### C-41: Vestigial Git LFS config breaks routine git operations (no git-lfs installed) — RESOLVED @@ -711,6 +672,60 @@ See also C-07/C-27/C-29 (pipeline-core coupling symptoms), C-39 (the dead-mapper ## Resolved Disagreements +### D-07: Historical data route — keep pipeline-core dispatcher vs call datafactory directly — RESOLVED + +| Field | Value | +|-------|-------| +| ID | D-07 | +| Source | `expert-code-review` (2026-06-12) | +| Perspectives | Ousterhout/Hickey: the postprocessor uses ~20% of `ViewsDataLoader`'s services (it passes `use_saved=False, validate=False, self_test=False`) while paying 100% of the seven-hop indirection — call `datafactory_query.load_dataset()` directly and own the three renames. Martin/GoF/Feathers: the dispatcher seam absorbed the viewser→datafactory migration with zero consumer changes and will absorb the next one; bypassing it re-couples the postprocessor to the current backend. | +| Location | `views_postprocessing/unfao/managers/unfao.py:44-59`; views-pipeline-core `modules/dataloaders/dataloaders.py:1088-1224` | +| Status | Open. Review recommendation: keep the pipeline-core route, but pass explicit `month_first/month_last` instead of partition semantics, consume `get_data()`'s return value (C-29), and re-enable validation once it supports datafactory sources. Decide alongside ADR-011 since both touch the same manager. | + +**Resolved (2026-06-26) — maintainer's data-sourcing principle.** *Data-related facts — data, metadata, validity dates, country/admin codes — come from the **producer** (views-datafactory, or viewser until phased out), **not** routed through pipeline-core. pipeline-core is the orchestration framework, not a data pass-through; depending on it for data facts couples the delivery to an unstable, mid-migration hub (SDP) and risks cycles (ADP).* + +Concretely: **(1)** producer-published facts (e.g. `last_valid_month_id`, region cell-counts) are read **directly from the producer** — pipeline-core must not be a lossy intermediary that drops them. First instantiated in S2 (#52): `views_postprocessing/unfao/source_metadata.py` reads `last_valid_month_id` straight from datafactory's `.zattrs`, never via the loader that discards it. **(2)** This decides the disagreement toward the Ousterhout/Hickey side **for facts**, but does **not** mandate ripping out the dispatcher wholesale: its source-abstraction (viewser↔datafactory routing) retains value for the **bulk data fetch** during the viewser phase-out. The principle is *'don't route through pipeline-core just for the sake of it / don't depend on it for what it merely passes through or drops'* — not *'never use the dispatcher.'* The earlier review recommendations for the bulk fetch (explicit month range; consume `get_data()`'s return value — C-29) still stand. Net: **the producer is the source of truth for data + facts; pipeline-core orchestrates.** + +--- + +### D-10: The 82 GAUL-uncovered cells — exclude, crash, or negotiate with FAO — RESOLVED + +| Field | Value | +|-------|-------| +| ID | D-10 | +| Source | `expert-code-review` (2026-06-12) | +| Perspectives | Fail-loud purism: let validation crash and force the conversation — never silently drop cells. Pragmatic exclusion: drop with a named, count-asserted, logged exclusion list. Diplomatic: ask FAO before shipping anything. | +| Location | `views_postprocessing/unfao/managers/unfao.py:188-221`; the 82 gids enumerated in C-30 | +| Status | **Resolved direction (2026-06-12): fix upstream in views-datafactory.** User decision, superseding the review's exclusion-list adjudication. A new bundled curated region (`land ∩ gaul0_code != -1`, 64,736 cells — e.g. `land_gaul`) is added to `datafactory_query` alongside `land` and `africa_me_legacy`, with generation script, provenance, and a count-pinning test. The postprocessor keeps zero spatial knowledge; its invariant simplifies to "every arriving cell must enrich completely — any null crashes" (the existing `_validate()` gate, unchanged). The `land` region itself is NOT redefined (other consumers depend on its physical-land semantics). Forecast-path residual: unmatched gids null→crash via left merge + validation; pin with one test. FAO disclosure of the 82 excluded sub-Antarctic cells still required in the release note. Closes the postprocessor side of C-30; C-34's coverage test becomes "100% completeness for the configured region." | + +--- + +### D-08: Global delivery path — scale up the runtime mapper vs swap to the precomputed lookup first — RESOLVED + +| Field | Value | +|-------|-------| +| ID | D-08 | +| Source | `expert-code-review` (2026-06-12) | +| Perspectives | Feathers' instinct: the mapper is production-proven and the region change is one config line — don't swap components days before a deadline. Kleppmann/Nygard/Ousterhout: the mapper is *unverifiable at global scale before running it* (C-31: unknown null count, runtime, memory; C-08 newly in scope), while the lookup's complete global failure set is exactly 82 named cells, verified locally (C-30). | +| Location | `views_postprocessing/unfao/managers/unfao.py:154-160`; `mapping.py`; views-datafactory `data/raw/gaul_admin/*.parquet` | +| Status | Open — user decision pending. Review adjudication: **swap to the lookup first, then go global.** The "don't swap before a deadline" rule assumes the old part is known-good for the new job; here it is known-good only for a job 5× smaller and cannot be tested for the new job until the moment it matters. Enumerable risk beats discoverable risk on a deadline. Prerequisite: shadow diff old-vs-new on africa_me (13,110 cells) on the production machine. | + +**Resolved by events (2026-06-24):** the swap was executed — the runtime mapper was deleted (C-39, PR #42) and `GaulLookupEnricher` is the enrichment path. The review adjudication ('swap to the lookup first, then go global') is now the shipped state. + +--- + +### D-05: Strategic direction — eliminate runtime mapper vs keep area-based algorithm — RESOLVED + +| Field | Value | +|-------|-------| +| ID | D-05 | +| Source | `manual` (2026-06-02) — external assessment | +| Perspectives | Path A: area-based is a FAO requirement → precompute lookup table. Path B: centroid-based acceptable → eliminate mapping.py entirely. Both eliminate 774 MB shapefiles, geopandas, and 3,100-line mapper. | +| Resolution | **Resolved (2026-06-02): Path A confirmed.** FAO-FSFC provided written confirmation (Release Note 02, `summary.tex`) agreeing to area-majority allocation as the locked aggregation rule: "Each PRIO-GRID cell is assigned to a single country using an area-majority rule." The area-based algorithm is a contractual requirement, not a historical accident. Path B (centroid-based) is off the table. Next step: build a one-time precomputed area-based lookup table (~65K rows, Parquet) and replace the 3,100-line runtime mapper with a dictionary lookup. This still eliminates geopandas, the shapefile bundle, and the runtime spatial operations — but preserves the area-majority assignment rule. | +| Update 2026-06-12 | **Upstream resolution deployed.** views-datafactory shipped area-majority GAUL assignment (issue #115 → PR #127, ADR-039 there, v1.2.28/29). All 7 GAUL parquets (codes + names + iso3) regenerated June 11 as area-majority, 259,200 rows each, mutually consistent (13,105/13,110 africa_me cells fully attributed; 5 pure-ocean cells unassigned). The precomputed lookup table can now be built by joining the factory parquets — no LFS, no shapefiles, no this-repo mapper run needed. The platform-level mapping divergence (centroid in factory vs area-majority here) no longer exists. See `docs/cross_repo_integration_report.md` and ADR-011 assessment §10. | + +--- + ### D-01: Cache strategy refactoring — extract now vs. characterize first — RESOLVED | Field | Value | From 235bc75c4ba657f79ca40d45612691aba348b715 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 26 Jun 2026 19:32:01 +0200 Subject: [PATCH 3/8] =?UTF-8?q?docs(risk-register):=20add=20C-43=20?= =?UTF-8?q?=E2=80=94=20ADR-011=20swap=20shipped=20without=20its=20equivale?= =?UTF-8?q?nce=20proof=20(Tier=202)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ADR-011 enrichment swap (runtime mapper -> GAUL lookup enricher) went live without the output-equivalence proof its own plan required (umbrella #20: Stage 0 baseline + Stage 2 diff, "zero unexplained differences"). That proof was never produced, and the old mapper + both diff scripts were since deleted (eba1df8, PR #42), so it is now unrecoverable. Accepting option A (a smoke-test delivery) verifies the path runs, not that it produces the same/correct values; _validate only checks the 9 columns are non-null, not correct, so a lookup/merge bug ships wrong-but-non-null geo metadata to FAO silently. Tier 2 (not 1: the lookup sources from datafactory's authoritative GAUL parquets, D-07); trigger is the go-global flip to land_gaul (64k cells). Header 42/22/20 -> 43/23/20. Cross-refs C-03, C-22, C-23, C-30/32/34, C-39, D-08. Co-Authored-By: Claude Opus 4.8 --- reports/technical_risk_register.md | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index ea5d1e2..6229931 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-06-26 | -| Total Concerns | 42 | -| Open Concerns | 22 | +| Total Concerns | 43 | +| Open Concerns | 23 | | Resolved Concerns | 20 | --- @@ -450,7 +450,29 @@ Verified 2026-06-26 on `origin/development`: pipeline-core **still imports `view See also C-37 / C-38 (the reconciler concerns — relocating to views-frames with the code), views-platform/views-frames#131 (the final home), #62 (retire vpp's copy), views-platform/views-models#191 (repoint), pipeline-core PR #217 (the merged DIP port, Decision K). -**Residual tail (verified 2026-06-26 — already tracked cross-repo, no new vpp entry):** pipeline-core still imports `views_reporting.statistics.ForecastReconciler` (`modules/statistics/__init__.py:5`), so the pipeline-core↔views-reporting edge is not yet fully severed and views-reporting's `reconciliation/` + `torch` retirement (#40 / views-reporting#72) is still blocked. But this is **not an untracked hazard**: the re-export is explicitly marked *"remove after downstream consumers update"*; `ForecastReconciler`'s only pipeline-core consumers are the transitional **golden-output equivalence tests** (#119 / #196, "new frames-native == old torch"); and **pipeline-core #198** already owns the removal — its scope note names *"the `reconciliation/` package **and** the `ForecastReconciler` class"* and it triggers vpp **#40** / views-reporting#72. Blocked on pipeline-core #197 (views-postprocessing/views-frames must be "default and stable") first. So C-42 stays open as a thin tracker until #198 lands; no separate vpp entry warranted (would duplicate #198). *(A speculative C-43 was drafted then withdrawn here after verification showed it was a duplicate.)* +**Residual tail (verified 2026-06-26 — already tracked cross-repo, no new vpp entry):** pipeline-core still imports `views_reporting.statistics.ForecastReconciler` (`modules/statistics/__init__.py:5`), so the pipeline-core↔views-reporting edge is not yet fully severed and views-reporting's `reconciliation/` + `torch` retirement (#40 / views-reporting#72) is still blocked. But this is **not an untracked hazard**: the re-export is explicitly marked *"remove after downstream consumers update"*; `ForecastReconciler`'s only pipeline-core consumers are the transitional **golden-output equivalence tests** (#119 / #196, "new frames-native == old torch"); and **pipeline-core #198** already owns the removal — its scope note names *"the `reconciliation/` package **and** the `ForecastReconciler` class"* and it triggers vpp **#40** / views-reporting#72. Blocked on pipeline-core #197 (views-postprocessing/views-frames must be "default and stable") first. So C-42 stays open as a thin tracker until #198 lands; no separate vpp entry warranted (would duplicate #198). *(A speculative residual-coupling entry was drafted then withdrawn here after verification showed it was a duplicate of pipeline-core #198.)* + +--- + +### C-43: ADR-011 enrichment swap shipped without its output-equivalence proof — and the proof is now unrecoverable + +| Field | Value | +|-------|-------| +| ID | C-43 | +| Tier | 2 | +| Source | `manual` (2026-06-26) — user-flagged rigor loss on accepting option A; verified against git history (`eba1df8` / PR #42) | +| Trigger | When the `africa_me_legacy` smoke-test delivery (option A) is accepted as the swap's verification, and — more acutely — when Stage 4 flips the region to `land_gaul` (64,736 cells, views-platform/views-models#127): the go-global run is the first time the lookup enricher's output reaches FAO at scale with **no** equivalence check against the previously-trusted mapper. Also fires if FAO / faoapi reports geographic metadata that looks wrong for specific cells. | +| Location | `views_postprocessing/unfao/enrichment.py` (`GaulLookupEnricher`); `views_postprocessing/unfao/managers/unfao.py:129` (`_append_metadata`), `:147-172` (`_validate` — the 9-column NULL gate, checks presence not correctness); umbrella #20 / issues #21, #23, #24 (the baseline+diff procedure, now unrunnable); deleted in `eba1df8` (PR #42): `mapping.py` + both ADR-011 diff scripts | + +ADR-011 swapped FAO geo-enrichment from the runtime geopandas mapper to the GAUL lookup enricher (commit `65635b6`). The swap's own plan (umbrella #20) required an **output-equivalence proof** before trusting it in production: Stage 0 (#21) run the OLD mapper on real `africa_me_legacy` data to archive a ground-truth baseline; Stage 2 (#23) diff the new enricher against it with *"zero unexplained differences."* That proof was **never produced** — no `baseline_schema.md` or baseline parquet was ever committed — and on 2026-06-24 the old mapper **and both diff scripts** were deleted (`eba1df8`, PR #42, C-39). So the equivalence check is now **unrecoverable** short of `git revert`-ing the mapper back. + +The accepted path forward (**option A**) is a single smoke-test delivery: "the run is green and the output looks sane," which proves the path *runs*, not that it produces the *same / correct* values the trusted mapper did. The manager's `_validate` enforces only that the 9 GAUL columns are **non-null** — it does not check value correctness — so a latent bug in the lookup build or the merge-by-gid (wrong join key, stale `lookup_version`, gid misalignment) would ship **wrong-but-non-null** geographic metadata to FAO with **no error signal**. + +**Why not Tier 1:** the lookup is built from views-datafactory's authoritative area-majority GAUL parquets — the canonical *producer* source (D-07). The new path sources from the gold standard; the old mapper was the *less*-trusted path being retired (C-31, C-23). So the missing diff is a lost cross-check, not "unverified code," and the Stage-1 enricher unit tests + coverage guards (C-30/C-34) cover part of the build. **Why Tier 2:** the residual silent-wrong-value path is real, the null gate cannot catch it, the one guard that would have is gone for good, and the trigger (go-global to 64k cells) is concrete and imminent. + +**Mitigation if assurance is wanted before go-global** (cheaper than reverting the mapper): forward-check a sample of `land_gaul` cell assignments directly against the datafactory GAUL parquet, or add a lightweight value-level assertion into the enricher path (a forward check against the producer source — *not* a resurrection of the deleted old-mapper diff). + +See also C-03 (the sibling enrich→validate test-coverage gap), C-22 (no post-delivery correction/recall process — the consequence if wrong values do ship), C-39 / C-31 / C-23 (the resolved mapper-deletion cluster this emerged from), C-30 / C-32 / C-34 (the go-global scale risks where this bites), D-08 (the swap-to-lookup-first decision whose verification debt this is). --- From f325cf3e3fd41d766780719df679bfd78e1e7b22 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 26 Jun 2026 21:56:39 +0200 Subject: [PATCH 4/8] =?UTF-8?q?feat(delivery):=20input-integrity=20invaria?= =?UTF-8?q?nt=20S3=20=E2=80=94=20forecast-file=20identity=20guard=20(#54,?= =?UTF-8?q?=20C-25)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FAO forecast is selected from the prediction store by category alone (newest-wins) with no filter on ensemble name or loa, so a stray category="forecast" upload (a test run, a second model, a backfill) would be silently enriched and shipped to FAO (C-25). S3 adds a representation-free guard that fails loud when the selected file's identity does not match the configured ensemble. Mirrors the S0-S2 pattern exactly: a primitives-only invariant in delivery/ (assert_forecast_identity), fed by the extraction seam (file_metadata normalizes the datastore metadata record to a plain dict), called by the manager. _read_forecast_data now resolves the file id, asserts identity ({name: model_name, loa: "pgm"}) before download, instead of the single download_latest_file convenience call. Residual tracked in C-25: the producer's uploaded name/loa contract is verified against pipeline-core's code but not a live upload — confirmed at the first real run; the guard fails loud (not silent) if the contract is off. Tests: unit on dicts (match/mismatch/missing-key/label) + extraction normalization. ruff clean; 101 passed, 44 xfailed. Co-Authored-By: Claude Opus 4.8 --- reports/technical_risk_register.md | 4 +- tests/test_extraction.py | 39 ++++++++++++++ tests/test_identity.py | 46 +++++++++++++++++ views_postprocessing/delivery/identity.py | 53 ++++++++++++++++++++ views_postprocessing/unfao/extraction.py | 28 +++++++++-- views_postprocessing/unfao/managers/unfao.py | 23 ++++++++- 6 files changed, 185 insertions(+), 8 deletions(-) create mode 100644 tests/test_identity.py create mode 100644 views_postprocessing/delivery/identity.py diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 6229931..88bb562 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -242,7 +242,9 @@ See also C-17 (implicit column naming between mapper and manager), D-06 (resolve This concern became visible during the cross-repo investigation (`docs/cross_repo_integration_report.md` §2.4, §4.2); it was previously implicit in the C-13 narrative (timeouts) but is a distinct failure mode: C-13 is "the call hangs," C-25 is "the call succeeds with the wrong file." -See also C-13 (no timeout on the same calls), C-15 (upload metadata lacks provenance to detect this downstream). +**Mitigation landed (S3, 2026-06-26, `sprint/fao-input-integrity`):** `_read_forecast_data` now resolves the file id, fetches its metadata, and asserts identity (`delivery/identity.assert_forecast_identity`) against the configured ensemble (`{name: ensemble_path_manager.model_name, loa: "pgm"}`) **before** download — a stray `category="forecast"` upload now fails loud instead of shipping silently. **Residual (verify before relying on it):** the guard assumes the producer's uploaded `name`/`loa` equal `model_name`/`"pgm"`; this is checked against pipeline-core's code but **not a live Appwrite upload**. If the contract differs, the guard fails loud on *every* run — caught at the first smoke-test delivery (option A / S6 #57), **not** silently — so the residual is an availability / false-positive risk, not a corruption one. Confirm the field match at the option-A run; until then C-25 stays open. + +See also C-13 (no timeout on the same calls), C-15 (upload metadata lacks provenance to detect this downstream), C-43 (the same "verify at the first live run" debt pattern on the enrichment swap). --- diff --git a/tests/test_extraction.py b/tests/test_extraction.py index f17e4d9..133e447 100644 --- a/tests/test_extraction.py +++ b/tests/test_extraction.py @@ -33,3 +33,42 @@ def test_works_on_flat_columns_too(): df = pd.DataFrame({"month_id": [100, 100, 101], "priogrid_gid": [1, 2, 1]}) assert extraction.cells_of(df) == {1, 2} np.testing.assert_array_equal(extraction.months_of(df), np.array([100, 101])) + + +class _FakeResult: + """Stands in for a DatastoreModule.get_file_metadata OperationResult.""" + + def __init__(self, document): + self._document = document + + def to_dict(self): + return {"data": self._document, "code": "FOUND"} + + +def test_file_metadata_normalizes_record_to_identity_dict(): + record = _FakeResult( + { + "name": "fatalities_ensemble", + "loa": "pgm", + "category": "forecast", + "targets": ["pred_a", "pred_b"], + "$id": "doc123", # appwrite system fields are dropped + "fileId": "file456", + } + ) + assert extraction.file_metadata(record) == { + "name": "fatalities_ensemble", + "loa": "pgm", + "category": "forecast", + "targets": ["pred_a", "pred_b"], + } + + +def test_file_metadata_missing_fields_become_none(): + record = _FakeResult({"name": "m"}) + assert extraction.file_metadata(record) == { + "name": "m", + "loa": None, + "category": None, + "targets": None, + } diff --git a/tests/test_identity.py b/tests/test_identity.py new file mode 100644 index 0000000..bcef518 --- /dev/null +++ b/tests/test_identity.py @@ -0,0 +1,46 @@ +"""Unit tests for the representation-free forecast-identity invariant (S3 / C-25).""" + +import pytest + +from views_postprocessing.delivery.identity import ( + ForecastIdentityError, + assert_forecast_identity, +) + + +def test_matching_identity_passes(): + selected = {"name": "fatalities_ensemble", "loa": "pgm", "category": "forecast"} + expected = {"name": "fatalities_ensemble", "loa": "pgm"} + assert_forecast_identity(selected, expected) # no raise + + +def test_name_mismatch_fails_loud_naming_expected_and_found(): + selected = {"name": "stray_model", "loa": "pgm"} + expected = {"name": "fatalities_ensemble", "loa": "pgm"} + with pytest.raises(ForecastIdentityError) as exc: + assert_forecast_identity(selected, expected) + msg = str(exc.value) + assert "name" in msg + assert "fatalities_ensemble" in msg # expected + assert "stray_model" in msg # found + + +def test_loa_mismatch_raises(): + with pytest.raises(ForecastIdentityError): + assert_forecast_identity({"name": "m", "loa": "cm"}, {"name": "m", "loa": "pgm"}) + + +def test_missing_key_is_treated_as_mismatch(): + with pytest.raises(ForecastIdentityError): + assert_forecast_identity({"loa": "pgm"}, {"name": "m", "loa": "pgm"}) + + +def test_only_expected_keys_are_checked(): + # extra metadata on `selected` is ignored — the caller decides what identity means. + selected = {"name": "m", "loa": "pgm", "category": "forecast", "targets": ["a"]} + assert_forecast_identity(selected, {"name": "m"}) # no raise + + +def test_label_appears_in_message(): + with pytest.raises(ForecastIdentityError, match="historical"): + assert_forecast_identity({"name": "x"}, {"name": "y"}, label="historical") diff --git a/views_postprocessing/delivery/identity.py b/views_postprocessing/delivery/identity.py new file mode 100644 index 0000000..d05b0d2 --- /dev/null +++ b/views_postprocessing/delivery/identity.py @@ -0,0 +1,53 @@ +"""Forecast-file identity invariant: deliver the forecast we asked for, not whatever +landed last (register C-25, S3). + +Representation-free — primitives only (plain dicts of file metadata). No pandas, no +datastore types. The manager feeds it the selected file's identity via +``views_postprocessing/unfao/extraction.py``; the rule lives here. + +Why this matters: the FAO forecast is selected from the prediction store by ``category`` +alone (newest-wins) — no filter on ensemble name or level. Today only the production +ensemble uploads with ``category="forecast"``, so the newest file is the right one *by +circumstance, not by contract*. This guard fails loud if the selected file's identity +does not match the configured ensemble, so a stray upload (a test run, a second model, a +backfill) can never be silently shipped to the partner. +""" + +from __future__ import annotations + + +class ForecastIdentityError(ValueError): + """The selected forecast file does not match the requested ensemble identity.""" + + +def assert_forecast_identity( + selected: dict, expected: dict, *, label: str = "forecast" +) -> None: + """Raise unless ``selected`` matches ``expected`` on every key ``expected`` names. + + Only the keys present in ``expected`` are checked (e.g. ``{"name", "loa"}``); any other + metadata on ``selected`` (targets, category, timestamps) is ignored. This keeps the + caller in control of what identity means without the invariant guessing. + + Args: + selected: metadata of the file actually selected for delivery. + expected: the identity the configured ensemble requires; each key must match. + label: short tag for the message (e.g. ``"forecast"``). + + Raises: + ForecastIdentityError: naming every mismatched key, expected vs found. + """ + mismatches = { + key: (expected[key], selected.get(key)) + for key in expected + if selected.get(key) != expected[key] + } + if mismatches: + detail = ", ".join( + f"{key}: expected {exp!r}, found {got!r}" + for key, (exp, got) in mismatches.items() + ) + raise ForecastIdentityError( + f"{label}: identity mismatch — {detail}. A stray upload to the prediction " + f"store would otherwise ship the wrong predictions to the partner." + ) diff --git a/views_postprocessing/unfao/extraction.py b/views_postprocessing/unfao/extraction.py index 4d7f913..309c077 100644 --- a/views_postprocessing/unfao/extraction.py +++ b/views_postprocessing/unfao/extraction.py @@ -1,9 +1,11 @@ -"""FAO-local representation seam: extract primitives from the pandas delivery frame. +"""FAO-local representation seam: extract primitives from the delivery's external +representations for the ``views_postprocessing.delivery`` invariants. -This is the **only** pandas-aware module the delivery invariants -(``views_postprocessing.delivery``) are fed from. It turns the current pandas -``DataFrame`` representation into the plain primitives the invariants consume -(sets of ints, numpy arrays, dicts). +This is the **only** pandas-aware module the delivery invariants are fed from: it turns +the pandas ``DataFrame`` representation into plain primitives (sets of ints, numpy +arrays). It also normalizes the prediction-store **metadata record** into a plain dict +for the forecast-identity invariant (``file_metadata`` below) — keeping all knowledge of +external representations here, so the invariants stay representation-free. When the delivery representation migrates from pandas to views-frames (gated on pipeline-core's DataFrame retirement, register C-40), **only this module changes** — @@ -59,3 +61,19 @@ def drop_months_above( """ months = _level_or_column(df, time_id).astype(np.int64) return df[months <= int(last_valid_month_id)] + + +# Prediction-store identity fields (written by pipeline-core's FileMetadata on upload). +_FILE_META_KEYS = ("name", "loa", "category", "targets") + + +def file_metadata(record) -> dict: + """The identity metadata of a selected prediction-store file, as a plain dict. + + ``record`` is the ``get_file_metadata`` result from pipeline-core's ``DatastoreModule``; + its ``.to_dict()["data"]`` is the Appwrite metadata document. This is the only place + that knows that shape — the forecast-identity invariant + (``views_postprocessing.delivery.identity``) consumes the returned primitives. + """ + doc = record.to_dict().get("data", {}) or {} + return {key: doc.get(key) for key in _FILE_META_KEYS} diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 3f2cf17..c9a6af7 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -19,7 +19,7 @@ from views_postprocessing.unfao.enrichment import GaulLookupEnricher from views_postprocessing.unfao.gaul_schema import METADATA_COLS from views_postprocessing.unfao import extraction, source_metadata -from views_postprocessing.delivery import coverage, observed_range +from views_postprocessing.delivery import coverage, identity, observed_range from pathlib import Path logger = logging.getLogger(__name__) @@ -98,7 +98,26 @@ def _read_forecast_data(self): try: prediction_store_manager = DatastoreModule(appwrite_file_manager_config=appwrite_config) - self._forecast_dataframe = pd.read_parquet(io.BytesIO(prediction_store_manager.download_latest_file(filters={"category": "forecast"}).to_dict().get("data", {}).get("file_bytes", None))) + + # The store is filtered by category alone (newest-wins), so resolve the file, + # then verify its identity before delivering it (S3/C-25): a stray upload with + # category="forecast" must not be silently shipped as the configured ensemble. + file_id = prediction_store_manager.get_latest_file_id(filters={"category": "forecast"}) + if file_id is None: + raise FileNotFoundError( + "No forecast file found in the prediction store (category='forecast')." + ) + selected = extraction.file_metadata(prediction_store_manager.get_file_metadata(file_id)) + # Identity contract: the producer's FileMetadata `name`/`loa` written on upload + # must equal the configured ensemble's model_name/loa. Verified against + # pipeline-core's code but NOT a live upload (register C-25) — confirm at the + # first real run. The guard fails loud (not silent) if the contract is off, so + # a mismatch surfaces immediately rather than shipping the wrong file. + expected = {"name": self.ensemble_path_manager.model_name, "loa": loa} + logger.info("Selected forecast file identity: %s (expected %s).", selected, expected) + identity.assert_forecast_identity(selected, expected) + + self._forecast_dataframe = pd.read_parquet(io.BytesIO(prediction_store_manager.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None))) self._forecast_dataset = PGMDataset(self._forecast_dataframe) except Exception as e: From 0bb03b36830cf2162cd4363ff1721ae28a3aefd2 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 26 Jun 2026 22:15:45 +0200 Subject: [PATCH 5/8] =?UTF-8?q?feat(delivery):=20input-integrity=20invaria?= =?UTF-8?q?nt=20S4=20=E2=80=94=20land=5Fgaul=20exclusion=20manifest=20(#55?= =?UTF-8?q?,=20C-30)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the land_gaul switch, the GAUL-uncovered sub-Antarctic cells have no country assignment — shipping them attributes FAO rows to a non-country (-1), or crashes on delivery day (C-30, Tier 1). S4 pins the excluded cells as an explicit frozen manifest and fails loud if any reach the delivery. Mirrors S1-S3: a representation-free manifest + check in delivery/coverage.py (EXCLUDED_GIDS_BY_REGION, excluded_for, assert_no_excluded_cells), called region-gated by the manager's _check_coverage (a no-op for unpinned africa_me_legacy, so its 5 ocean cells are unaffected). The 76 are disclosed in docs/fao_excluded_cells.md. Count correction (the frozen-list tripwire working as designed): deriving from the live producer (datafactory v1.4.0) gives 64,742 complete + 76 excluded, not the 64,736 + 82 the register/issue carried. Cause: datafactory #163 (ADR-043) supplemented 6 Azorean cells into land_gaul (82 - 6 = 76). vpp's own built lookup already ships 64,742, so the stale pin would have false-positived against our own artifact. Authoritative source = land - land_gaul (76), not raw gaul0_code == -1 (82). EXPECTED_CELLS pin corrected; C-30 + #55 updated. Tests (8 new): exclusion count == 76; known sub-Antarctic gids excluded; Azorean cells NOT excluded; leaked cell raises naming the gid; unpinned region no-op; plus a drift tripwire cross-checking the manifest against the datafactory sibling when present. ruff clean; 109 passed, 44 xfailed. Co-Authored-By: Claude Opus 4.8 --- docs/fao_excluded_cells.md | 45 ++++++++++++ reports/technical_risk_register.md | 6 +- tests/test_delivery_coverage.py | 67 +++++++++++++++++- views_postprocessing/delivery/coverage.py | 74 +++++++++++++++++++- views_postprocessing/unfao/managers/unfao.py | 7 ++ 5 files changed, 193 insertions(+), 6 deletions(-) create mode 100644 docs/fao_excluded_cells.md diff --git a/docs/fao_excluded_cells.md b/docs/fao_excluded_cells.md new file mode 100644 index 0000000..3dc6d1d --- /dev/null +++ b/docs/fao_excluded_cells.md @@ -0,0 +1,45 @@ +# Cells excluded from FAO global delivery + +**Status:** disclosure note for the `land_gaul` global rollout (register C-30 / D-10, sprint S4 / #55). +**Source of truth:** views-datafactory v1.4.0 — `land_gaul` curated region (`land − land_gaul`). +**Pinned in code:** `views_postprocessing/delivery/coverage.py` (`EXCLUDED_GIDS_BY_REGION["land_gaul"]`). + +## What FAO should know + +The VIEWS global historical delivery covers the `land_gaul` region: **64,742** PRIO-GRID +cells — every land cell for which FAO's **GAUL 2024** boundaries provide an administrative +assignment. + +**76 land cells are deliberately excluded.** They are remote sub-Antarctic islands that +fall outside GAUL 2024 coverage (e.g. Macquarie Island, the Auckland Islands, Prince +Edward Islands, South Sandwich Islands). They have **no** country/admin assignment in any +source, so they are dropped at the region level rather than shipped with a placeholder — +shipping them would attribute partner rows to a non-country (`-1`). Conflict activity in +these cells is negligible to nil. + +This is the full, frozen exclusion list. If a future delivery's coverage changes, the +frozen list in code diverges loudly (it is asserted in `tests/test_delivery_coverage.py` +against the producer), so any drift is caught before delivery rather than silently +absorbed. + +## History + +The exclusion count was **82** when `land_gaul` was first curated (datafactory #159). +It dropped to **76** when datafactory #163 (ADR-043) supplemented **6 Azorean cells** +(gids 182470, 183190, 183909, 183910, 186058, 186778) into `land_gaul` — those are now +**covered and delivered**, not excluded. + +## The 76 excluded gids + +``` + 51078 51798 53979 54699 56852 56853 58318 62356 + 94776 99027 107733 107742 110367 112944 114769 116931 +118753 121625 123748 124038 124425 124759 126561 126624 +128012 129079 129387 129574 130919 131639 132525 146125 +153966 157829 159987 160708 173423 179900 190724 201850 +202249 203404 206592 208007 210596 212353 212774 221061 +223941 225390 227961 229161 229429 229430 229866 233436 +233743 234640 235586 235942 235943 235951 235954 236054 +237351 238123 238231 238954 239705 240318 240319 240975 +240977 241153 247862 248595 +``` diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 88bb562..55cbe65 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -320,7 +320,11 @@ See also C-13. | Trigger | When the region switches from `africa_me_legacy` to `land` for global historical delivery, verify the 82 unassigned cells are explicitly excluded before `_validate()` — they have no GAUL assignment in any source | | Location | `views_postprocessing/unfao/managers/unfao.py:188-221`; views-datafactory `data/raw/gaul_admin/gaul0_code.parquet` (value = -1) | -Verified 2026-06-12: of the datafactory's 64,818 `land`-region cells, 64,736 have complete area-majority metadata; exactly 82 are unassigned across all 7 GAUL fields — all remote sub-Antarctic islands FAO's GAUL 2024 boundaries do not cover (Macquarie, Auckland Islands, Prince Edward; sample gids 51078, 51798, 53979, 62356, 94776, 99027). The mitigation must be a named exclusion-list constant with the 82 gids, count-asserted (`== 82`) in both the enricher and a test, logged at WARNING, and disclosed to FAO — not a generic `code != -1` filter, which would silently absorb future coverage regressions. Generalizes the previously documented "5 ocean cells" of africa_me_legacy (those 5 are among the 82). +Verified 2026-06-12: of the datafactory's 64,818 `land`-region cells, 64,736 have complete area-majority metadata; exactly 82 are unassigned across all 7 GAUL fields — all remote sub-Antarctic islands FAO's GAUL 2024 boundaries do not cover (Macquarie, Auckland Islands, Prince Edward; sample gids 51078, 51798, 53979, 62356, 94776, 99027). The mitigation must be a named exclusion-list constant with the gids, count-asserted in both the enricher and a test, logged at WARNING, and disclosed to FAO — not a generic `code != -1` filter, which would silently absorb future coverage regressions. Generalizes the previously documented "5 ocean cells" of africa_me_legacy (those 5 are among the excluded set). + +**Count drift corrected 2026-06-26 (the frozen-list tripwire working as designed):** deriving the exclusions from the live producer (datafactory **v1.4.0**) gives **64,742** complete + **76** excluded, *not* the 64,736 / 82 verified on 2026-06-12. Cause: datafactory **#163 (ADR-043)** supplemented **6 Azorean cells** (gids 182470, 183190, 183909, 183910, 186058, 186778) into `land_gaul` — they are now covered, not excluded. vpp's *own* built lookup (`data/gaul_lookup.parquet`) already ships 64,742, so the old 64,736 pin would have false-positived against our own artifact. NB the authoritative exclusion source is the **region complement** `land − land_gaul` (76), not the raw `gaul0_code == -1` (82) — the latter does not reflect the ADR-043 curation. + +**Mitigation landed (S4, 2026-06-26, `sprint/fao-input-integrity`):** the 76 excluded gids are pinned as a frozen manifest in `delivery/coverage.py` (`EXCLUDED_GIDS_BY_REGION`), the count is corrected to 64,742, `assert_no_excluded_cells` is wired into the manager's `_check_coverage` **region-gated** (a no-op for unpinned `africa_me_legacy`, so its 5 ocean cells are unaffected), the 76 are disclosed in `docs/fao_excluded_cells.md`, and a test cross-checks the manifest against the datafactory sibling when present (drift tripwire). **Residual:** still Tier 1 until the live `land_gaul` run (views-platform/views-models#127) exercises it end-to-end — the guard is unit-proven but not yet run against a real global delivery. See also D-10 (handling decision), C-34 (coverage contract). diff --git a/tests/test_delivery_coverage.py b/tests/test_delivery_coverage.py index 313d261..cebab92 100644 --- a/tests/test_delivery_coverage.py +++ b/tests/test_delivery_coverage.py @@ -3,12 +3,17 @@ Pure primitives in, raise-or-pass out — no framework, no pandas. """ +import json +from pathlib import Path + import pytest from views_postprocessing.delivery.coverage import ( EXPECTED_CELLS, CoverageError, assert_complete_coverage, + assert_no_excluded_cells, + excluded_for, expected_for, ) @@ -33,8 +38,9 @@ def test_label_appears_in_message(): def test_land_gaul_is_pinned(): - assert expected_for("land_gaul") == 64_736 - assert EXPECTED_CELLS["land_gaul"] == 64_736 + # 64,742 after datafactory #163 (ADR-043) supplemented 6 Azorean cells; was 64,736. + assert expected_for("land_gaul") == 64_742 + assert EXPECTED_CELLS["land_gaul"] == 64_742 def test_unpinned_or_missing_region_is_none(): @@ -42,3 +48,60 @@ def test_unpinned_or_missing_region_is_none(): assert expected_for("africa_me_legacy") is None assert expected_for(None) is None assert expected_for("does_not_exist") is None + + +# --- S4 / C-30: the GAUL-uncovered exclusion manifest ------------------------------- + + +def test_land_gaul_exclusions_count_is_pinned(): + # land 64,818 − land_gaul 64,742 = 76 sub-Antarctic cells (was 82 pre-ADR-043). + assert len(excluded_for("land_gaul")) == 76 + + +def test_unpinned_region_has_no_exclusions(): + # africa_me_legacy keeps its 5 ocean cells today — they must NOT be force-excluded. + assert excluded_for("africa_me_legacy") == frozenset() + assert excluded_for(None) == frozenset() + assert excluded_for("does_not_exist") == frozenset() + + +def test_known_sub_antarctic_gids_are_excluded(): + # Sample gids from register C-30 (and the africa_me ocean cells, a subset). + for gid in (62356, 94776, 99027, 107733, 107742, 51078): + assert gid in excluded_for("land_gaul") + + +def test_supplemented_azorean_cells_are_NOT_excluded(): + # The 6 cells datafactory #163 added to land_gaul must be delivered, not dropped. + for gid in (182470, 183190, 183909, 183910, 186058, 186778): + assert gid not in excluded_for("land_gaul") + + +def test_no_excluded_cells_passes_on_clean_delivery(): + assert assert_no_excluded_cells({1, 2, 3}, frozenset({999})) is None + + +def test_leaked_excluded_cell_raises_naming_the_gid(): + with pytest.raises(CoverageError, match="62356"): + assert_no_excluded_cells({1, 62356, 3}, excluded_for("land_gaul"), label="forecast") + + +def test_no_excluded_cells_is_noop_when_region_unpinned(): + # Empty exclusion set (e.g. africa_me_legacy) never raises, even on ocean cells. + assert assert_no_excluded_cells({62356, 94776}, excluded_for("africa_me_legacy")) is None + + +# Cross-check the frozen manifest against the live producer when its checkout is present +# (CI has no sibling → skip). This is the drift tripwire C-30 asks for. +_DF = Path(__file__).resolve().parents[2] / "views-datafactory" / "src" / "datafactory_query" + + +@pytest.mark.skipif( + not (_DF / "land_pgids.json").exists(), + reason="views-datafactory sibling checkout not present", +) +def test_manifest_matches_datafactory_land_minus_land_gaul(): + land = {int(x) for x in json.loads((_DF / "land_pgids.json").read_text())} + land_gaul = {int(x) for x in json.loads((_DF / "land_gaul_pgids.json").read_text())} + assert excluded_for("land_gaul") == frozenset(land - land_gaul) + assert EXPECTED_CELLS["land_gaul"] == len(land_gaul) diff --git a/views_postprocessing/delivery/coverage.py b/views_postprocessing/delivery/coverage.py index c9cadd7..06722e6 100644 --- a/views_postprocessing/delivery/coverage.py +++ b/views_postprocessing/delivery/coverage.py @@ -44,16 +44,84 @@ def assert_complete_coverage( # whose count is *verified* is pinned — an unpinned region logs a skipped gate, never # a guess. # -# land_gaul : 64,736 = land ∩ gaul0_code != -1 (register C-30/D-10). -# The 82 excluded sub-Antarctic gids are pinned in S4 (#55). +# land_gaul : 64,742 = the cells in the datafactory `land_gaul` curated region +# (land 64,818 − 76 GAUL-uncovered, register C-30/D-10). The 76 +# excluded gids are pinned below (S4 / #55). NB this was 64,736 / 82 +# excluded until datafactory #163 (ADR-043) supplemented 6 Azorean +# cells into land_gaul — see EXCLUDED_GIDS_BY_REGION. # africa_me_legacy : intentionally NOT pinned — the register cites 13,110 region # cells with 5 pure-ocean cells unassigned, so "complete" is # ambiguous (13,110 vs 13,105). Verify against a real run first. EXPECTED_CELLS: dict[str, int] = { - "land_gaul": 64_736, + "land_gaul": 64_742, } def expected_for(region: str | None) -> int | None: """The pinned expected cell count for ``region``, or None if unpinned/unknown.""" return EXPECTED_CELLS.get(region) if region else None + + +# Region -> the GAUL-uncovered cells the curated region deliberately drops. +# +# A *views-datafactory* fact: EXCLUDED = land − land_gaul (the producer's published +# region complement). Frozen here as an explicit manifest — a tripwire, per C-30/D-10: +# if the producer's coverage drifts, this frozen set diverges *loudly* rather than a +# generic `gaul0_code != -1` filter silently absorbing the change. Re-derive and re-pin +# when the land_gaul region version changes; tests/test_delivery_coverage.py cross-checks +# this against the datafactory sibling checkout when one is present. +# +# land_gaul: 76 remote sub-Antarctic island cells (Macquarie, Auckland Islands, Prince +# Edward, South Sandwich, ...) outside FAO GAUL 2024 coverage. Derived from datafactory +# v1.4.0 (land 64,818 − land_gaul 64,742 = 76). The earlier "82" (register C-30, +# pre-ADR-043) dropped to 76 when datafactory #163 supplemented 6 Azorean cells into +# land_gaul. Disclosed to FAO in docs/fao_excluded_cells.md. +_LAND_GAUL_EXCLUDED: frozenset[int] = frozenset({ + 51078, 51798, 53979, 54699, 56852, 56853, 58318, 62356, + 94776, 99027, 107733, 107742, 110367, 112944, 114769, 116931, + 118753, 121625, 123748, 124038, 124425, 124759, 126561, 126624, + 128012, 129079, 129387, 129574, 130919, 131639, 132525, 146125, + 153966, 157829, 159987, 160708, 173423, 179900, 190724, 201850, + 202249, 203404, 206592, 208007, 210596, 212353, 212774, 221061, + 223941, 225390, 227961, 229161, 229429, 229430, 229866, 233436, + 233743, 234640, 235586, 235942, 235943, 235951, 235954, 236054, + 237351, 238123, 238231, 238954, 239705, 240318, 240319, 240975, + 240977, 241153, 247862, 248595, +}) + +EXCLUDED_GIDS_BY_REGION: dict[str, frozenset[int]] = { + "land_gaul": _LAND_GAUL_EXCLUDED, +} + + +def excluded_for(region: str | None) -> frozenset[int]: + """The GAUL-uncovered gids the region must exclude — empty for unpinned regions.""" + return EXCLUDED_GIDS_BY_REGION.get(region, frozenset()) if region else frozenset() + + +def assert_no_excluded_cells( + received_gids: set[int], excluded_gids: frozenset[int], *, label: str = "delivery" +) -> None: + """Raise if the delivery includes any GAUL-uncovered cell the region must drop. + + More specific than the count gate: it names *which* uncovered cells leaked, so a + coverage regression is diagnosed rather than read as generic over-coverage. + + Args: + received_gids: the distinct cell ids actually delivered. + excluded_gids: the cells the configured region must exclude (see ``excluded_for``). + label: short tag for the message (e.g. ``"historical"`` / ``"forecast"``). + + Raises: + CoverageError: naming the leaked cells (these have no GAUL assignment, so + shipping them would attribute partner rows to a non-country). + """ + leaked = sorted(received_gids & excluded_gids) + if leaked: + shown = leaked[:10] + more = "" if len(leaked) <= 10 else f" (+{len(leaked) - 10} more)" + raise CoverageError( + f"{label}: {len(leaked)} GAUL-uncovered cell(s) the curated region must " + f"exclude were delivered: {shown}{more}. These cells have no GAUL " + f"assignment; shipping them attributes partner rows to a non-country." + ) diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index c9a6af7..0f6a995 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -239,6 +239,7 @@ def _check_coverage(self) -> None: """ region = self.configs.get("region") expected = coverage.expected_for(region) + excluded = coverage.excluded_for(region) for label, df in ( ("historical", self._historical_dataframe), ("forecast", self._forecast_dataframe), @@ -250,6 +251,12 @@ def _check_coverage(self) -> None: len(cells), len(df), ) + # GAUL-uncovered cells the curated region must drop (S4/C-30) — checked + # before the count gate so a leaked island names itself, not "over-coverage + # by 1". Empty for unpinned regions (e.g. africa_me_legacy keeps its ocean + # cells), so this is a no-op there. + if excluded: + coverage.assert_no_excluded_cells(cells, excluded, label=label) if expected is not None: coverage.assert_complete_coverage(cells, expected, label=label) else: From c4317dc74788c88256e65c5fa4a817e717d27a7c Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 26 Jun 2026 22:30:51 +0200 Subject: [PATCH 6/8] =?UTF-8?q?feat(delivery):=20input-integrity=20invaria?= =?UTF-8?q?nt=20S5=20=E2=80=94=20structured=20delivery=20provenance=20(#56?= =?UTF-8?q?,=20C-15)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upload metadata carried only a free-text description, so FAO could not audit what produced their data (C-15). S5 stamps each upload with structured, sourced provenance. Mirrors S1-S4: a representation-free delivery/provenance.py (build_provenance -> dict) assembling lookup_version, region, expected/actual cell counts, and unmapped_count from the enricher + S1 coverage + a new extraction.unmapped_cell_count seam — nothing hardcoded. The manager's _delivery_description builds it per file and both _save uploads carry it. Carrier constraint (surfaced): pipeline-core's upload_data exposes no structured field — only free-text description — so the dict is JSON-encoded into description behind a human prefix for now. A dedicated metadata field is requested upstream (pipeline-core #245); when it lands, only the manager's attach step changes (the provenance shape is already representation-free). fill_count omitted until a fabricated-value count exists (cf. C-26). Tests (9 new): provenance dict fields/optionality/JSON-serializability; unmapped_cell_count on mapped/null/missing-col frames. ruff clean; 117 passed. Co-Authored-By: Claude Opus 4.8 --- reports/technical_risk_register.md | 4 +- tests/test_extraction.py | 22 ++++++ tests/test_provenance.py | 73 ++++++++++++++++++++ views_postprocessing/delivery/provenance.py | 54 +++++++++++++++ views_postprocessing/unfao/extraction.py | 17 +++++ views_postprocessing/unfao/managers/unfao.py | 33 +++++++-- 6 files changed, 198 insertions(+), 5 deletions(-) create mode 100644 tests/test_provenance.py create mode 100644 views_postprocessing/delivery/provenance.py diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 55cbe65..9228e4c 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -170,7 +170,9 @@ Both `dsm.upload_data()` calls in `_save()` carry metadata: `name`, `loa`, `type Tier recalibrated from 4 to 3 during falsification audit (2026-06-02): the missing provenance affects the partner's ability to audit data quality. -See also C-14 (stale cache without version tracking), C-22 (no post-delivery correction process). +**Mitigation landed (S5, 2026-06-26, `sprint/fao-input-integrity`):** a representation-free `delivery/provenance.py` (`build_provenance`) assembles structured provenance — `lookup_version`, `region`, `expected_cell_count`, `actual_cell_count`, `unmapped_count` — sourced from the enricher + S1 coverage + a new `extraction.unmapped_cell_count` seam (nothing hardcoded). Both `_save` uploads now carry it via `_delivery_description`. **Carrier constraint:** pipeline-core's `upload_data` exposes **no structured field** — only free-text `description` — so the dict is JSON-encoded into `description` behind a human prefix for now. A dedicated metadata field is requested upstream (**pipeline-core #245**); when it lands, only the manager's attach step changes (the provenance shape is already representation-free). `fill_count` is omitted until a fabricated-value count is available (cf. C-26). Residual is now just the carrier abuse, tracked by #245. + +See also C-14 (stale cache without version tracking), C-22 (no post-delivery correction process), C-26 (fabricated zeros — the eventual `fill_count` source). --- diff --git a/tests/test_extraction.py b/tests/test_extraction.py index 133e447..53a9356 100644 --- a/tests/test_extraction.py +++ b/tests/test_extraction.py @@ -35,6 +35,28 @@ def test_works_on_flat_columns_too(): np.testing.assert_array_equal(extraction.months_of(df), np.array([100, 101])) +def _meta_frame(rows, meta): + """rows: list of (month_id, priogrid_gid); meta: list of metadata values (None = null).""" + idx = pd.MultiIndex.from_tuples(rows, names=["month_id", "priogrid_gid"]) + return pd.DataFrame({"country_iso_a3": meta}, index=idx) + + +def test_unmapped_cell_count_is_zero_when_all_mapped(): + df = _meta_frame([(100, 1), (100, 2), (101, 1)], ["AUS", "NZL", "AUS"]) + assert extraction.unmapped_cell_count(df, ["country_iso_a3"]) == 0 + + +def test_unmapped_cell_count_counts_distinct_cells_with_nulls(): + # cell 2 is null in one row; cell 3 null too; cell 1 fully mapped. + df = _meta_frame([(100, 1), (100, 2), (101, 2), (101, 3)], ["AUS", None, None, None]) + assert extraction.unmapped_cell_count(df, ["country_iso_a3"]) == 2 + + +def test_unmapped_cell_count_zero_when_no_metadata_cols_present(): + df = _meta_frame([(100, 1)], ["AUS"]) + assert extraction.unmapped_cell_count(df, ["not_a_column"]) == 0 + + class _FakeResult: """Stands in for a DatastoreModule.get_file_metadata OperationResult.""" diff --git a/tests/test_provenance.py b/tests/test_provenance.py new file mode 100644 index 0000000..0ac3355 --- /dev/null +++ b/tests/test_provenance.py @@ -0,0 +1,73 @@ +"""Unit tests for the representation-free delivery-provenance invariant (S5 / C-15). + +Pure primitives in, a JSON-serializable dict out — no framework, no pandas. +""" + +import json + +from views_postprocessing.delivery.provenance import build_provenance + + +def test_carries_all_core_fields_from_primitives(): + prov = build_provenance( + lookup_version="v1.4.0", + region="land_gaul", + expected_cell_count=64_742, + actual_cell_count=64_742, + unmapped_count=0, + ) + assert prov == { + "lookup_version": "v1.4.0", + "region": "land_gaul", + "expected_cell_count": 64_742, + "actual_cell_count": 64_742, + "unmapped_count": 0, + } + + +def test_fill_count_omitted_when_not_supplied(): + prov = build_provenance( + lookup_version="v1", + region="land_gaul", + expected_cell_count=1, + actual_cell_count=1, + unmapped_count=0, + ) + assert "fill_count" not in prov + + +def test_fill_count_included_when_supplied(): + prov = build_provenance( + lookup_version="v1", + region="land_gaul", + expected_cell_count=1, + actual_cell_count=1, + unmapped_count=0, + fill_count=7, + ) + assert prov["fill_count"] == 7 + + +def test_unpinned_region_keeps_none_expected_count(): + prov = build_provenance( + lookup_version="v1", + region="africa_me_legacy", + expected_cell_count=None, + actual_cell_count=13_110, + unmapped_count=0, + ) + assert prov["expected_cell_count"] is None + assert prov["region"] == "africa_me_legacy" + + +def test_result_is_json_serializable(): + prov = build_provenance( + lookup_version="v1.4.0", + region="land_gaul", + expected_cell_count=64_742, + actual_cell_count=64_700, + unmapped_count=0, + fill_count=3, + ) + # round-trips cleanly — it must survive serialization into the upload description. + assert json.loads(json.dumps(prov)) == prov diff --git a/views_postprocessing/delivery/provenance.py b/views_postprocessing/delivery/provenance.py new file mode 100644 index 0000000..1baa785 --- /dev/null +++ b/views_postprocessing/delivery/provenance.py @@ -0,0 +1,54 @@ +"""Delivery provenance invariant: stamp each upload with structured, auditable +provenance so the partner can verify what produced their data (register C-15, S5). + +Representation-free — primitives in, a plain JSON-serializable dict out. No pandas, no +datastore types. The manager sources the primitives (the enricher's lookup version, the +configured region, the S1 coverage counts, the unmapped-cell count) and attaches the +returned dict to the upload; the *shape* of provenance lives here. + +Carrier note: pipeline-core's ``upload_data`` exposes no structured-metadata field — only +a free-text ``description`` — so the manager serializes this dict into ``description`` as +JSON for now. A dedicated field is requested upstream (see C-15); when it lands, only the +manager's attach step changes, not this shape. +""" + +from __future__ import annotations + + +def build_provenance( + *, + lookup_version: str, + region: str | None, + expected_cell_count: int | None, + actual_cell_count: int, + unmapped_count: int, + fill_count: int | None = None, +) -> dict: + """Assemble the structured provenance for one delivered file. + + Every value is sourced by the caller from the actual delivery — the enricher's + lookup version, the configured region, the coverage counts (S1), the post-enrich + unmapped-cell count — nothing is hardcoded. ``fill_count`` is included only when the + caller can supply it (it is omitted rather than reported as a misleading zero). + + Args: + lookup_version: the GAUL lookup table version that produced the metadata. + region: the configured delivery region (e.g. ``"land_gaul"``), or None. + expected_cell_count: the region's pinned cell count, or None if unpinned. + actual_cell_count: distinct cells actually delivered in this file. + unmapped_count: delivered cells with missing metadata (0 once validation passes). + fill_count: optional count of fabricated/filled values, if known. + + Returns: + A JSON-serializable dict of the provenance fields. + """ + provenance: dict = { + "lookup_version": lookup_version, + "region": region, + "expected_cell_count": expected_cell_count, + "actual_cell_count": actual_cell_count, + "unmapped_count": unmapped_count, + } + if fill_count is not None: + provenance["fill_count"] = fill_count + return provenance diff --git a/views_postprocessing/unfao/extraction.py b/views_postprocessing/unfao/extraction.py index 309c077..610ffc4 100644 --- a/views_postprocessing/unfao/extraction.py +++ b/views_postprocessing/unfao/extraction.py @@ -50,6 +50,23 @@ def months_of(df: pd.DataFrame, time_id: str = _TIME_ID) -> NDArray[np.int64]: return np.unique(_level_or_column(df, time_id).astype(np.int64)) +def unmapped_cell_count( + df: pd.DataFrame, metadata_cols, pg_id: str = _PG_ID +) -> int: + """Distinct cells with a null in any metadata column (the post-enrich unmapped count). + + Feeds the provenance invariant (``delivery.provenance``). 0 once ``_validate`` passes, + but recorded as an explicit audit value rather than assumed. + """ + cols = [c for c in metadata_cols if c in df.columns] + if not cols: + return 0 + mask = df[cols].isnull().any(axis=1) + if not bool(mask.any()): + return 0 + return len(cells_of(df[mask], pg_id)) + + def drop_months_above( df: pd.DataFrame, last_valid_month_id: int, time_id: str = _TIME_ID ) -> pd.DataFrame: diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 0f6a995..4c86d46 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -13,13 +13,14 @@ from views_pipeline_core.managers.ensemble import EnsemblePathManager import pandas as pd import io +import json from datetime import datetime import os from dotenv import load_dotenv from views_postprocessing.unfao.enrichment import GaulLookupEnricher from views_postprocessing.unfao.gaul_schema import METADATA_COLS from views_postprocessing.unfao import extraction, source_metadata -from views_postprocessing.delivery import coverage, identity, observed_range +from views_postprocessing.delivery import coverage, identity, observed_range, provenance from pathlib import Path logger = logging.getLogger(__name__) @@ -295,7 +296,6 @@ def _save(self) -> list: dsm = DatastoreModule(appwrite_file_manager_config=unfao_appwrite_config) timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") - enrichment_description = f"Enriched with geographic metadata on {timestamp} using precomputed GAUL lookup (ADR-011, version={self._enricher.lookup_version})." historical_file_path = self._model_path.data_generated / f"historical_dataset_{timestamp}.parquet" forecast_file_path = self._model_path.data_generated / f"forecast_dataset_{timestamp}.parquet" @@ -307,7 +307,8 @@ def _save(self) -> list: name=self._model_path.model_name, loa="pgm", type="model", targets=self.configs.get("targets", []), - description=enrichment_description, category="historical") + description=self._delivery_description(self._historical_dataframe, timestamp), + category="historical") self._forecast_dataframe.to_parquet( forecast_file_path @@ -317,4 +318,28 @@ def _save(self) -> list: name=self.ensemble_path_manager.model_name, loa="pgm", type="model", targets=["pred_ln_sb_best", "pred_ln_ns_best", "pred_ln_os_best", "pred_ln_sb_prob", "pred_ln_ns_prob", "pred_ln_os_prob"], - description=enrichment_description, category="forecast") + description=self._delivery_description(self._forecast_dataframe, timestamp), + category="forecast") + + def _delivery_description(self, df: pd.DataFrame, timestamp: str) -> str: + """Human prefix + structured provenance (S5/C-15) for an upload's metadata. + + Sources every provenance field from the actual delivery — the enricher's lookup + version, the configured region, the S1 coverage counts, the post-enrich unmapped + count — and serializes the representation-free ``delivery.provenance`` dict into + the only structured carrier ``upload_data`` exposes today (the ``description`` + free-text field; a dedicated metadata field is requested upstream, C-15). + """ + region = self.configs.get("region") + prov = provenance.build_provenance( + lookup_version=self._enricher.lookup_version, + region=region, + expected_cell_count=coverage.expected_for(region), + actual_cell_count=len(extraction.cells_of(df)), + unmapped_count=extraction.unmapped_cell_count(df, METADATA_COLS), + ) + return ( + f"Enriched with geographic metadata on {timestamp} using precomputed GAUL " + f"lookup (ADR-011, version={self._enricher.lookup_version}). " + f"provenance={json.dumps(prov, separators=(',', ':'))}" + ) From cac5f0d47c6e150e8f9413e199f6405495df705b Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 26 Jun 2026 22:33:38 +0200 Subject: [PATCH 7/8] =?UTF-8?q?test(delivery):=20input-integrity=20invaria?= =?UTF-8?q?nt=20S6=20=E2=80=94=20end-to-end=20suite=20through=20the=20mana?= =?UTF-8?q?ger=20seam=20(#57)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Proves all five guards fire end-to-end on synthetic delivery frames. The manager can't be instantiated (pipeline-core + Appwrite env, C-40), so — like test_append_metadata.py / test_validation.py — each test replicates the manager's own extraction->invariant chain and asserts the guard: S1 coverage cells_of -> assert_complete_coverage (correct passes; under/over raise) S2 observed range months_of -> fabricated_months -> drop_months_above (month > boundary not shipped as zero) S3 forecast identity file_metadata -> assert_forecast_identity (decoy rejected; match passes) S4 land_gaul cells_of -> assert_no_excluded_cells (injected 62356 crashes loud) S5 provenance build_provenance <- cells_of/unmapped_cell_count (description carries structured fields) S4 uses the corrected 64,742 + 76 manifest (not the issue's stale 64,736 + 82). The design-contract module is now 5 passed / 1 xfail — the only remaining input-integrity xfail is the C-40-deferred manager de-inheritance. ruff clean; 126 passed, 44 xfailed. Co-Authored-By: Claude Opus 4.8 --- tests/test_input_integrity_e2e.py | 146 ++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 tests/test_input_integrity_e2e.py diff --git a/tests/test_input_integrity_e2e.py b/tests/test_input_integrity_e2e.py new file mode 100644 index 0000000..a099e16 --- /dev/null +++ b/tests/test_input_integrity_e2e.py @@ -0,0 +1,146 @@ +"""End-to-end input-integrity suite (S6 / epic #51) — the integration layer above each +story's primitives unit tests. + +``UNFAOPostProcessorManager`` cannot be instantiated here (it needs views-pipeline-core + +Appwrite env — register C-40), so — exactly as ``test_append_metadata.py`` and +``test_validation.py`` do — each test **replicates the manager's own extraction→invariant +chain** on a synthetic delivery frame and proves the guard fires. Keep these in lockstep +with ``views_postprocessing/unfao/managers/unfao.py``: + + S1 coverage _check_coverage cells_of -> assert_complete_coverage + S2 observed range _clip_observed_history months_of -> fabricated_months -> drop_months_above + S3 forecast identity _read_forecast_data file_metadata -> assert_forecast_identity + S4 land_gaul _check_coverage cells_of -> assert_no_excluded_cells + S5 provenance _delivery_description build_provenance(<- cells_of/unmapped_cell_count) +""" + +import json + +import pandas as pd +import pytest + +from views_postprocessing.delivery import coverage, identity, observed_range, provenance +from views_postprocessing.unfao import extraction +from views_postprocessing.unfao.gaul_schema import METADATA_COLS + +_TIME_ID = "month_id" +_ENTITY_ID = "priogrid_gid" + + +def _delivery_frame(gids, months=(100, 101), *, unmapped_gids=()): + """A synthetic delivery frame: MultiIndex (month, gid) + a pred col + the 9 GAUL cols.""" + rows = [(m, g) for m in months for g in gids] + idx = pd.MultiIndex.from_tuples(rows, names=[_TIME_ID, _ENTITY_ID]) + df = pd.DataFrame({"pred_ln_sb_best": 0.0}, index=idx) + for c in METADATA_COLS: + df[c] = "X" + if unmapped_gids: + mask = df.index.get_level_values(_ENTITY_ID).isin(unmapped_gids) + df.loc[mask, list(METADATA_COLS)] = None + return df + + +class _FakeMetaResult: + """Stands in for a DatastoreModule.get_file_metadata OperationResult (S3).""" + + def __init__(self, document): + self._document = document + + def to_dict(self): + return {"data": self._document, "code": "FOUND"} + + +# S1 — coverage (C-34) -------------------------------------------------------------- +def test_s1_coverage_correct_count_passes_through_seam(): + df = _delivery_frame([1, 2, 3]) + cells = extraction.cells_of(df) + assert coverage.assert_complete_coverage(cells, 3, label="historical") is None + + +def test_s1_coverage_under_and_over_raise_through_seam(): + df = _delivery_frame([1, 2, 3]) + cells = extraction.cells_of(df) + with pytest.raises(coverage.CoverageError, match="under-coverage"): + coverage.assert_complete_coverage(cells, 4) + with pytest.raises(coverage.CoverageError, match="over-coverage"): + coverage.assert_complete_coverage(cells, 2) + + +# S2 — observed range (C-26) -------------------------------------------------------- +def test_s2_month_beyond_boundary_is_not_delivered_as_zero(): + # historical request padded to month 103; producer observed only through 101. + df = _delivery_frame([1, 2], months=(100, 101, 102, 103)) + last_valid = 101 + fabricated = observed_range.fabricated_months(extraction.months_of(df), last_valid) + assert set(fabricated.tolist()) == {102, 103} + clipped = extraction.drop_months_above(df, last_valid) + delivered = set(extraction.months_of(clipped).tolist()) + assert delivered == {100, 101} + assert 102 not in delivered and 103 not in delivered # not shipped as observed zero + + +# S3 — forecast identity (C-25) ----------------------------------------------------- +def test_s3_decoy_forecast_file_is_rejected_through_seam(): + decoy = _FakeMetaResult({"name": "stray_model", "loa": "pgm", "category": "forecast"}) + selected = extraction.file_metadata(decoy) + expected = {"name": "fatalities_ensemble", "loa": "pgm"} + with pytest.raises(identity.ForecastIdentityError, match="stray_model"): + identity.assert_forecast_identity(selected, expected) + + +def test_s3_matching_forecast_file_passes_through_seam(): + good = _FakeMetaResult({"name": "fatalities_ensemble", "loa": "pgm", "category": "forecast"}) + selected = extraction.file_metadata(good) + expected = {"name": "fatalities_ensemble", "loa": "pgm"} + assert identity.assert_forecast_identity(selected, expected) is None + + +# S4 — land_gaul exclusions (C-30) -------------------------------------------------- +def test_s4_injected_unassigned_cell_crashes_loud(): + # 62356 is a real sub-Antarctic gid in the land_gaul exclusion manifest. + df = _delivery_frame([1, 2, 62356]) + cells = extraction.cells_of(df) + excluded = coverage.excluded_for("land_gaul") + with pytest.raises(coverage.CoverageError, match="62356"): + coverage.assert_no_excluded_cells(cells, excluded, label="forecast") + + +def test_s4_clean_land_gaul_delivery_passes(): + df = _delivery_frame([1, 2, 3]) + cells = extraction.cells_of(df) + assert coverage.assert_no_excluded_cells(cells, coverage.excluded_for("land_gaul")) is None + + +# S5 — provenance (C-15) ------------------------------------------------------------ +def test_s5_upload_description_carries_structured_provenance(): + df = _delivery_frame([1, 2, 3]) + # replica of _delivery_description's chain + prov = provenance.build_provenance( + lookup_version="v1.4.0", + region="land_gaul", + expected_cell_count=coverage.expected_for("land_gaul"), + actual_cell_count=len(extraction.cells_of(df)), + unmapped_count=extraction.unmapped_cell_count(df, METADATA_COLS), + ) + description = f"Enriched ... provenance={json.dumps(prov, separators=(',', ':'))}" + + # the consumer can recover the structured fields from the description carrier + payload = json.loads(description.split("provenance=", 1)[1]) + assert payload["lookup_version"] == "v1.4.0" + assert payload["region"] == "land_gaul" + assert payload["expected_cell_count"] == 64_742 + assert payload["actual_cell_count"] == 3 + assert payload["unmapped_count"] == 0 + + +def test_s5_provenance_records_unmapped_cells_when_present(): + # a cell with null metadata is counted (audit value), before _validate would reject it. + df = _delivery_frame([1, 2, 3], unmapped_gids=(3,)) + prov = provenance.build_provenance( + lookup_version="v1", + region="land_gaul", + expected_cell_count=coverage.expected_for("land_gaul"), + actual_cell_count=len(extraction.cells_of(df)), + unmapped_count=extraction.unmapped_cell_count(df, METADATA_COLS), + ) + assert prov["unmapped_count"] == 1 From 979a9c4f7eb520271c6c96b0790984298fce5234 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 26 Jun 2026 23:19:49 +0200 Subject: [PATCH 8/8] fix(delivery): address review-diff findings on the input-integrity PR (#64) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _clip_observed_history: degrade open consistently. The boundary fetch returned None when the attribute is absent (skip clip, warn) but RAISED on a transient datafactory network error (URLError/HTTPError/TimeoutError), which propagated through _read_historical_data (no try/except) and crashed the whole delivery. Wrap the fetch so both unresolved cases are treated alike. - CIC: document the four new fail-loud guards (ForecastIdentityError, CoverageError on coverage/exclusion, observed-range clip, structured provenance) in §6, and the new invariant + e2e test modules in §10 — the contract doc had drifted from the manager's now-richer loudness contract. - extraction.unmapped_cell_count: type the metadata_cols param (Sequence[str]). ruff clean; 126 passed, 44 xfailed. Co-Authored-By: Claude Opus 4.8 --- docs/CICs/UNFAOPostProcessorManager.md | 8 ++++++++ views_postprocessing/unfao/extraction.py | 4 +++- views_postprocessing/unfao/managers/unfao.py | 16 +++++++++++++++- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/docs/CICs/UNFAOPostProcessorManager.md b/docs/CICs/UNFAOPostProcessorManager.md index a269e9c..eb9c68c 100644 --- a/docs/CICs/UNFAOPostProcessorManager.md +++ b/docs/CICs/UNFAOPostProcessorManager.md @@ -77,11 +77,17 @@ Assumptions that are not met **must cause failure**, not fallback behavior. - **Null values in required metadata columns:** Raises `ValueError` with null count and affected column name (C-01 resolved — validation active) - **Dataset initialization failure:** Raises `ValueError` in `_save()` if datasets are None - **Appwrite upload failure:** Propagates exception from `DatastoreModule` +- **Wrong forecast file selected:** Raises `ForecastIdentityError` in `_read_forecast_data()` if the newest `category="forecast"` file's identity (name/loa) does not match the configured ensemble (S3/C-25 — a stray upload cannot be silently shipped) +- **Region coverage mismatch:** Raises `CoverageError` in `_check_coverage()` (called from `_validate()`) if a pinned region's delivered cell count is wrong (S1/C-34) or a GAUL-uncovered excluded cell leaks into the delivery (S4/C-30) +- **Fabricated historical tail:** `_clip_observed_history()` drops months beyond the producer's `last_valid_month_id` so unobserved zero-padding is not shipped as observed history (S2/C-26); **degrades open** (skips the clip with a WARNING) if the boundary cannot be resolved +- **Upload provenance:** every upload's `description` carries structured provenance (lookup version, region, expected/actual cell counts, unmapped count) via `_delivery_description()` (S5/C-15) The following **must never** fail silently: - Missing or None environment variables for Appwrite - Network failures during download or upload - Schema validation failures (missing columns or null values) +- A forecast file whose identity does not match the configured ensemble (S3/C-25) +- Wrong region coverage or a leaked GAUL-uncovered cell (S1/C-34, S4/C-30) --- @@ -140,6 +146,8 @@ manager._save() Currently: the manager cannot be instantiated without `views-pipeline-core`, so its stage logic is covered by **replica tests** that mirror the real methods — `tests/test_validation.py` (`_validate`) and `tests/test_append_metadata.py` (`_append_metadata`). A full end-to-end test against the live manager (C-03) still requires a production-like environment. +The input-integrity guards (S0–S6, epic #51) are representation-free invariants in `views_postprocessing/delivery/` that the manager **calls** (never inherits). Each has primitives unit tests — `tests/test_delivery_coverage.py` (S1/S4), `tests/test_delivery_observed_range.py` (S2), `tests/test_identity.py` (S3), `tests/test_provenance.py` (S5), `tests/test_extraction.py` (the seam) — and `tests/test_input_integrity_e2e.py` replicates the manager's extract→invariant chain end-to-end. The design contract (representation-free, called-not-inherited) is pinned by `tests/test_input_integrity_design_contract.py`. + --- ## 11. Evolution Notes diff --git a/views_postprocessing/unfao/extraction.py b/views_postprocessing/unfao/extraction.py index 610ffc4..8c2beb5 100644 --- a/views_postprocessing/unfao/extraction.py +++ b/views_postprocessing/unfao/extraction.py @@ -19,6 +19,8 @@ from __future__ import annotations +from collections.abc import Sequence + import numpy as np import pandas as pd from numpy.typing import NDArray @@ -51,7 +53,7 @@ def months_of(df: pd.DataFrame, time_id: str = _TIME_ID) -> NDArray[np.int64]: def unmapped_cell_count( - df: pd.DataFrame, metadata_cols, pg_id: str = _PG_ID + df: pd.DataFrame, metadata_cols: Sequence[str], pg_id: str = _PG_ID ) -> int: """Distinct cells with a null in any metadata column (the post-enrich unmapped count). diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 4c86d46..8e1e60b 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -206,8 +206,22 @@ def _clip_observed_history(self) -> None: The boundary is read straight from the **producer** (views-datafactory) via ``source_metadata`` — never pipeline-core. Only the *historical* (observed) frame is clipped; the forecast frame is future-dated by design and untouched. + + Degrade-open policy: if the boundary cannot be resolved — the store predates the + attribute (``None``) or the producer read fails (network) — the clip is skipped + with a WARNING rather than blocking delivery. Both unresolved cases are treated + identically so a transient datafactory hiccup does not crash the historical read. """ - lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + try: + lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + except Exception as e: # producer unreachable — degrade open, like lv is None + logger.warning( + "last_valid_month_id could not be read from datafactory (%s); the " + "historical delivery was NOT clipped to observed range (C-26 guard " + "skipped).", + e, + ) + return if lv is None: logger.warning( "last_valid_month_id unavailable from datafactory; the historical "