diff --git a/README.md b/README.md index 6c0bd7b..ff12b06 100644 --- a/README.md +++ b/README.md @@ -72,11 +72,14 @@ In practice the manager is constructed and run by **views-models** | Stage | Method(s) | What happens | |-------|-----------|--------------| -| **Read** | `_read_historical_data`, `_read_forecast_data` | Historical actuals from views-datafactory (via the inherited loader); the forecast file from the Appwrite prediction store. The forecast file's identity is checked before use (C-25). | -| **Transform** | `_transform` → `_append_metadata` | Joins the 9 GAUL metadata columns onto each frame via `GaulLookupEnricher` (a parquet lookup). Prediction values are **not** transformed. | -| **Validate** | `_validate`, `_check_coverage` | Null-gate on the metadata columns; region coverage + GAUL-excluded-cell guards (C-34 / C-30). | -| **Clip** | `_clip_observed_history` | Drops fabricated zero-padded tail months from the historical actuals (C-26); the forecast is untouched. | -| **Save** | `_save` | Writes parquet and uploads to the UN FAO bucket with structured provenance (C-15). | +| **Read** | `_read_historical_frame`, `_read_forecast_data_contract` | Historical actuals from views-datafactory arrive **frame-native** (#126); the forecast run is resolved from the Appwrite store by its **run manifest**, with each shard's header verified on load (ADR-013 §4.3). | +| **Transform** | `_transform` | Resolution only. Prediction values are **not** transformed — no collapse, no reconciliation. | +| **Validate** | `_validate`, `_check_coverage` | Asserts the read resolved, then enforces the region coverage + GAUL-excluded-cell contract (C-34 / C-30). The metadata null-gate fires later, at artifact build (`contract/historical.assert_metadata_complete`). | +| **Save** | `_save` → `_save_contract` | Builds the ADR-013 wire — arrow shards, the §5 GAUL sidecar, the historical artifact — commits the run **manifest last**, and stamps each upload with structured provenance (C-15). | + +The pandas metadata-join and history-clip stages were retired with the legacy delivery path +in #149; their rules survive as called invariants under `delivery/`. See the +[manager README](views_postprocessing/unfao/managers/README.md) for what moved where. ### Output schema (geographic metadata columns) diff --git a/docs/CICs/UNFAOPostProcessorManager.md b/docs/CICs/UNFAOPostProcessorManager.md index e543a40..cf281c9 100644 --- a/docs/CICs/UNFAOPostProcessorManager.md +++ b/docs/CICs/UNFAOPostProcessorManager.md @@ -77,11 +77,11 @@ Assumptions that are not met **must cause failure**, not fallback behavior. **Kn - **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 selected:** structurally impossible since #149. Selection is by **run manifest** — a commit marker whose contents are hash-verified — not by scanning the bucket for the newest `category="forecast"` upload. Declared identity is additionally checked **per shard header** against the launched ensemble inside `TargetLease.load()` (`contract/wire/source_selection.py:73-81`), so identity comes from the artifact's own content. The metadata-field check this bullet used to describe (`delivery/identity.py`) was retired in #150 and the legacy reader it served in #149; register C-25 is closed as *superseded by mechanism* +- **Wrong forecast selected:** structurally impossible since #149. Selection is by **run manifest** — a commit marker whose contents are hash-verified — not by scanning the bucket for the newest `category="forecast"` upload. Declared identity is additionally checked **per shard header** against the launched ensemble inside `TargetLease.load()` (`contract/wire/source_selection.py:73-81`), so identity comes from the artifact's own content. The metadata-field check this bullet used to describe (`delivery/identity.py`) was retired in #150 and the legacy reader it served in #149; register C-25 is closed as *superseded by mechanism* - **Launch config incomplete:** raises `LaunchConfigError` naming the missing key. A launcher that omits `wire_contract` or declares a `data_format` other than `feature_frame` is **refused**, never quietly routed into a fallback (ADR-003, register C-63) - **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:** `_read_historical_frame()` drops months beyond the producer's `last_valid_month_id` at the read (`_clip_observed_history` was the pandas equivalent, retired with that path in #149) 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) +- **Fabricated historical tail:** `_read_historical_frame()` drops months beyond the producer's `last_valid_month_id` at the read (`_clip_observed_history` was the pandas equivalent, retired with that path in #149) 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:** the historical artifact's `description` carries structured provenance (lookup version, region, expected/actual cell counts, unmapped count) built by `delivery/provenance.py` (`build_provenance` → `compact_description`) via the manager's `_historical_frame_description()` (S5/C-15). The **forecast** side carries no such description: its guarantee is the wire's verified chain — per-shard content hashes recorded in the §4.2 run manifest, header asserts on load, and manifest-last commit ordering. That is identity and integrity, not the C-15 provenance field set; the §4.2 manifest's keys are exactly `contract_version`, `run_id`, `targets`, `shards`, `expected_months`, `expected_cell_count`, `sidecar` — and it carries **no** `lookup_version`, `region` or `unmapped_count`. `_delivery_description()` was the pandas-path equivalent and was deleted with it in #149 The following **must never** fail silently: - Missing or None environment variables for Appwrite @@ -145,7 +145,7 @@ manager._save() - **Beige tests:** Missing ensemble name in config; None environment variables; empty forecast bucket; DataFrames with unexpected index structure - **Red tests:** Corrupted parquet downloads; network timeouts during upload; DataFrames where all cells map to None (all-ocean input) -Currently: the manager cannot be instantiated without `views-pipeline-core`, so its stage logic is covered by **source-scan and replica tests** — `tests/test_validation.py`, `tests/test_launch_config.py` (the refusals), `tests/test_gaul_lookup_access.py` (one lookup read, threaded). `tests/test_append_metadata.py` was deleted in #149 with the method it mirrored. A full end-to-end test against the live manager (C-03) still requires a production-like environment. +Currently: the manager cannot be instantiated without `views-pipeline-core`, so its stage logic is covered by **source-scan and replica tests** — `tests/test_validation.py`, `tests/test_launch_config.py` (the refusals), `tests/test_gaul_lookup_access.py` (one lookup read, threaded). `tests/test_append_metadata.py` was deleted in #149 with the method it mirrored. 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_provenance.py` (S5), `tests/test_frame_extraction.py` (the seam), `tests/test_store_metadata.py` (store identity) — and `tests/test_input_integrity_e2e.py` drives the invariants on **primitives**, which is how the manager calls them. S3's forecast-identity rule moved to the wire layer (#150) and is covered by `tests/test_wire_source_selection.py`. The design contract (representation-free, called-not-inherited) is pinned by `tests/test_input_integrity_design_contract.py`. diff --git a/docs/architecture/role_and_seams.md b/docs/architecture/role_and_seams.md index 9587d0a..7813b2c 100644 --- a/docs/architecture/role_and_seams.md +++ b/docs/architecture/role_and_seams.md @@ -55,8 +55,9 @@ pattern: `_read → _transform → _validate → _save` in order. - this repo **fills in the steps** for the FAO path (the `_read*/_transform/_validate/_save` overrides in `unfao/managers/unfao.py`). -- pipeline-core also **provides the tools** the steps use: `ViewsDataLoader`, `PGMDataset`, +- pipeline-core also **provides the tools** the steps use: `ViewsDataLoader`, `DatastoreModule`, `AppwriteConfig`, the path managers. + (`PGMDataset` was on this list until #149 retired the pandas delivery path; this repo no longer references it — see Seam B.) So the runtime control flow is *inverted* ("don't call us, we'll call you"): views-models calls `manager.execute()`, which lives in **pipeline-core's base**, which calls back into @@ -110,12 +111,12 @@ The manager **calls** the invariants; it never makes them methods of itself. The always `extract (seam) → call invariant → raise`. This is why the guards are testable without the framework, and why they survive a representation change untouched. -**And they did survive one.** This seam was `unfao/extraction.py` (pandas) until #151. +**And they did survive one.** This seam was `unfao/extraction.py` (pandas) until #151. When the pandas delivery was retired, the invariants needed **no change at all** — only which module fed them. That is the design working exactly as this section claims, and it is -the evidence for the claim rather than a restatement of it. `delivery/identity.py` was also -retired (#150): the forecast-identity rule now lives in the wire layer, checked per shard -header against declared provenance — see §5 Seam C. +the evidence for the claim rather than a restatement of it. `delivery/identity.py` was also +retired (#150): the forecast-identity rule now lives in the wire layer, checked per shard +header against declared provenance — see §5 Seam C. ### Seam B — the inherited pandas base, and the C-40 gate @@ -123,7 +124,7 @@ Because this repo *is-a* pipeline-core postprocessor (section 3), three **concre pieces are inherited, not chosen: 1. the input loader (`ViewsDataLoader` → parquet → pandas), -2. the dataset container (`PGMDataset`, a pandas `DataFrame` with object-dtype cells), +2. the dataset container (`PGMDataset`, a pandas `DataFrame` with object-dtype cells), 3. the prediction-store parquet I/O. **This section described the state until 2026-07-27; it is no longer true and is kept @@ -147,7 +148,7 @@ samples per cell). This is where representation matters most: - views-frames stores a distribution natively as a contiguous `(N, S)` float32 array (sample axis explicit; a point is just `S=1`). -- pandas `PGMDataset` stores it as **object-dtype list-in-cell** — a separate numpy array +- pandas `PGMDataset` stores it as **object-dtype list-in-cell** — a separate numpy array boxed in each of N cells. Cost scales ~linearly with S (memory, an encode/decode tax at every parquet/API boundary, a silent resize on mismatched sample counts). diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index 2203627..de7500b 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -24,10 +24,36 @@ # --- 1. deleted-symbol scan ------------------------------------------------------------- -# Symbols that name code removed in ADR-011 / C-39. None should appear as *current* in a -# living onboarding doc. +# Symbols naming code this repo has deleted. None should appear as *current* in a living +# onboarding doc. +# +# **This list must grow with every deletion, and that is the lesson of S11 (#197).** It +# carried only the ADR-011 / C-39 entries for six weeks while epic #148 deleted a whole +# delivery path (#149) and moved the machinery out of `unfao/` (#153). The guard kept +# passing over four documents describing methods and files that no longer existed — +# including `unfao/managers/README.md`, which documented the Template Method as +# `_transform → _append_metadata` and pointed readers at a deleted test file, sitting +# inside the package next to the code it misdescribed. +# +# The guard was not wrong. It was not updated. A deletion PR that does not extend this +# regex has not finished. _BANNED = re.compile( - r"PriogridCountryMapper|mapping/README|use_disk_cache|cachetools|geopandas", + "|".join(( + # ADR-011 / C-39 — the runtime mapper and its shapefile toolchain (June 2026) + r"PriogridCountryMapper", r"mapping/README", r"use_disk_cache", r"cachetools", + r"geopandas", + # #149 — methods of the retired pandas delivery path (2026-07-31) + r"_append_metadata", r"_delivery_description", r"_clip_observed_history", + r"LEGACY_FORECAST_FILTERS", r"test_append_metadata", + # #150 / #151 / #153 — modules retired or moved out of `unfao/` by epic #148 + r"delivery/identity", r"unfao/extraction", r"unfao/frames", + r"unfao/historical", r"unfao/gaul_schema", r"unfao/wire", + # #149 — pipeline-core's pandas container, no longer referenced by this repo. + # Negative lookbehind: `FAO_PGMDataset` is views-faoapi's class and is live. + # It inherits IGNORECASE, so `fao_PGMDataset` is spared too — no such spelling + # exists, but the lookbehind is not case-exact and a reader should not assume it. + r"(? list[Path]: return [d for d in docs if d.exists()] -def test_living_docs_have_no_deleted_symbol_references(): +def _label(doc: Path) -> str: + """Repo-relative path where possible — three living docs are named ``README.md``, + so a bare basename cannot say which one failed.""" + try: + return doc.relative_to(_REPO).as_posix() + except ValueError: + return doc.name # a doc injected from tmp_path by the guard's own tests + + +def _deleted_symbol_offenders(docs: list[Path]) -> list[str]: + """Scan ``docs`` for banned symbols, honouring the line-scoped ``legacy-ok`` opt-out.""" offenders = [] - for doc in _living_docs(): + for doc in docs: for i, line in enumerate(doc.read_text().splitlines(), start=1): if "legacy-ok" in line: # explicit opt-out for an intentional historical mention continue if _BANNED.search(line): - offenders.append(f"{doc.relative_to(_REPO)}:{i}: {line.strip()}") + offenders.append(f"{_label(doc)}:{i}: {line.strip()}") + return offenders + + +def test_living_docs_have_no_deleted_symbol_references(): + offenders = _deleted_symbol_offenders(_living_docs()) assert not offenders, "deleted-symbol references in living docs:\n" + "\n".join(offenders) @@ -193,3 +234,65 @@ def test_the_retired_contract_name_is_gone_from_code(): "pinned URL (views-appwrite/docs/ADRs/platform/coordinate_registry.toml). " "Historical narration belongs in docs/ADRs or reports/, not here." ) + + +def test_the_ban_covers_the_post_148_deletions_and_spares_the_live_lookalike(): + """A ban list that was not updated when the code changed is why S11 (#197) existed. + + Two failure modes, both real, so both are pinned: + + 1. **Entries going missing** — someone trims the list and a document quietly starts + describing deleted code again. The loop below fails if any known entry stops + matching. Note what it *cannot* catch: a future deletion whose symbol nobody adds + is invisible to this test and to the guard alike, which is why the comment above + ``_BANNED`` says a deletion PR that does not extend it has not finished. No test + can substitute for that; only the habit can. + 2. **Too broad** — `FAO_PGMDataset` is a *views-faoapi* class, alive and correctly + cited in `contract/gaul_schema.py` and `contract/enrichment.py`. A careless + `PGMDataset` pattern would ban a true statement about another repo's code. + """ + for deleted in ( + "_append_metadata", "_delivery_description", "_clip_observed_history", + "LEGACY_FORECAST_FILTERS", "tests/test_append_metadata.py", + "delivery/identity.py", "unfao/extraction.py", "unfao/frames.py", + "unfao/historical.py", "unfao/gaul_schema.py", "unfao/wire/sink.py", + "PGMDataset", "PriogridCountryMapper", "geopandas", + ): + assert _BANNED.search(deleted), ( + f"{deleted!r} names code this repo deleted, but the ban list does not cover " + "it — a living doc could describe it as current and nothing would object" + ) + + assert not _BANNED.search("FAO_PGMDataset._METADATA_COLS"), ( + "the ban caught `FAO_PGMDataset`, which is views-faoapi's live class and the " + "consumer contract our sidecar is written against — banning a true statement " + "about another repo's code is how a guard earns its own deletion" + ) + + +def test_the_ban_actually_fires_on_a_living_doc(tmp_path): + """The scan, not just the regex — a pattern nothing applies is decoration.""" + doc = tmp_path / "README.md" + doc.write_text("The manager joins metadata in `_append_metadata`.\n") + assert _deleted_symbol_offenders([doc]), ( + "the scan reported nothing on a doc naming a deleted method" + ) + + +def test_the_legacy_ok_marker_still_works(tmp_path): + """The escape hatch the retirement records depend on — line-scoped, deliberately. + + Four documents now name deleted methods in order to say where the work went. That is + better than a gap, and it only stays possible while this marker keeps working. It is + line-scoped: a marker on the following line does not excuse the mention above it. + """ + excused = tmp_path / "ok.md" + excused.write_text("`_append_metadata` was retired in #149. \n") + assert not _deleted_symbol_offenders([excused]) + + wrong_line = tmp_path / "wrong.md" + wrong_line.write_text("`_append_metadata` was retired.\n\n") + assert _deleted_symbol_offenders([wrong_line]), ( + "a marker on the NEXT line excused the mention — the opt-out must stay line-scoped, " + "or a single marker silently covers a whole document" + ) diff --git a/views_postprocessing/unfao/managers/README.md b/views_postprocessing/unfao/managers/README.md index 1812df1..abbd524 100644 --- a/views_postprocessing/unfao/managers/README.md +++ b/views_postprocessing/unfao/managers/README.md @@ -13,9 +13,17 @@ and uploads the result to the FAO Appwrite store. `UNFAOPostProcessorManager` is a **concrete pipeline-core postprocessor** — it subclasses `PostprocessorManager` + `ForecastingModelManager` (Template Method) and fills the -`read → transform → validate → save` lifecycle. It is a thin orchestrator: the -representation-free input-integrity rules live in `views_postprocessing/delivery/` and are -**called** by the manager (via the `unfao/extraction.py` seam), never inherited into it. +`read → transform → validate → save` lifecycle. The representation-free input-integrity +rules live in `views_postprocessing/delivery/` and are **called** by the manager (via the +`contract/frame_extraction.py` seam), never inherited into it. + +It is **406 lines**, against a 450-line budget enforced by +`tests/test_doc_accuracy.py::test_the_manager_stays_within_its_line_budget`. It was 636 +before #149. ADR-012 deliberately stopped calling it "thin" and states a number instead — +a word nobody can check became a bound a test can. + +It is also **the only module in this repository that imports `views_pipeline_core`**, and a +test keeps it that way (register C-40). It does **not** transform prediction values (no collapse, no reconciliation — those are downstream). It joins metadata, guards integrity, and delivers. @@ -24,14 +32,31 @@ downstream). It joins metadata, guards integrity, and delivers. | Stage | Method(s) | What happens | |-------|-----------|--------------| -| **Read** | `_read_historical_data`, `_read_forecast_data` | Historical actuals via the inherited `ViewsDataLoader`; the forecast file from the Appwrite prediction store. The selected forecast file's **identity** (name/loa) is asserted before download (`delivery.identity`, C-25). | -| **Transform** | `_transform` → `_append_metadata` | Joins the 9 GAUL metadata columns via `GaulLookupEnricher` (a precomputed parquet lookup, ADR-011). | -| **Validate** | `_validate`, `_check_coverage` | Null-gate on the 9 metadata columns; region **coverage** + GAUL-**excluded-cell** guards (`delivery.coverage`, C-34 / C-30). | -| **Clip** | `_clip_observed_history` | Drops fabricated zero-padded tail months from the *historical* actuals (`delivery.observed_range`, C-26); the forecast is untouched. The boundary (`last_valid_month_id`) is read from the producer (datafactory) via `unfao/source_metadata.py`. | -| **Save** | `_save` | Writes timestamped parquet and uploads to the FAO bucket, stamping each upload with **structured provenance** (`delivery.provenance`, C-15). | +| **Read** | `_read_historical_frame`, `_read_forecast_data_contract` | Historical actuals arrive **frame-native** (#126) via the inherited loader; the forecast run is resolved from the Appwrite store by its **run manifest**, and each shard's header is verified on load (`contract/wire/source_selection.py`, ADR-013 §4.3). | +| **Transform** | `_transform` | Resolution only. Metadata is no longer joined onto a pandas frame here — the GAUL columns are attached where each artifact is built. | +| **Validate** | `_validate`, `_check_coverage` | Asserts the read **resolved** (frame + forecast run present), then enforces the region coverage contract (`delivery/coverage.py`, C-34 / C-30). | +| **Save** | `_save` → `_save_contract` | Builds the ADR-013 wire — arrow shards, the §5 GAUL sidecar, the historical artifact — and commits the run **manifest last**. Each upload is stamped with structured provenance (`delivery/provenance.py`, C-15). | + +**Where the null-gate went.** It is no longer in `_validate`. The 9 metadata columns are +gated at artifact-build time by `contract/historical.assert_metadata_complete`, and the +forecast's guarantees are the wire's own verified chain: content hashes, header asserts, +per-target coverage inside each lease's `load()`, the §6 no-collapse gate, and gid parity +at `_save`. + +The 9 metadata columns are the single-source contract in `contract/gaul_schema.py` +(`COLUMNS` → `METADATA_COLS`, register C-70). + +**Retired in #149** (epic #148). Named here deliberately — older issues and PRs still +reference them, and a reader who greps for one deserves an answer rather than a gap: + +| retired | what it was | where the work went | +|---|---|---| +| `_append_metadata` | the pandas GAUL metadata join | attached per artifact at build time | +| `_clip_observed_history` | dropped zero-padded tail months | the rule survives as `delivery/observed_range.py` | +| `_delivery_description` | the store-document text | `delivery/provenance.py` | -The 9 metadata columns are the single-source contract in `unfao/gaul_schema.py` -(`METADATA_COLS`). +The pandas delivery path they belonged to is gone. There is one path, and a launcher that +has not declared it is refused by name (`contract/launch_config.py`, register C-63). ## Running it @@ -45,14 +70,16 @@ manager.execute() # read → transform → validate → save In production this is invoked by **views-models** (`postprocessors/un_fao/main.py`), not directly. The manager cannot be instantiated without `views-pipeline-core` + Appwrite env, -so its stage logic is covered by replica tests (`tests/test_validation.py`, -`tests/test_append_metadata.py`) and the input-integrity e2e suite -(`tests/test_input_integrity_e2e.py`) — see the CIC §10. +so its stage logic is covered by source-scan and seam tests rather than by instantiating +it — `tests/test_validation.py`, `tests/test_input_integrity_e2e.py`, +`tests/test_hop_b_sink_e2e.py` and the ADR-013 conformance suite. See the CIC §10. ## Configuration -Appwrite settings are read from the environment via `os.getenv` (no startup validation yet — -tracked in #11). The full variable set is documented in the repo +Appwrite settings are read from the environment and **validated fail-loud before any store +is constructed** — `unfao/appwrite_env.py` declares the required names and refuses a partial +environment, naming every missing variable (þing-01 D6, #134). This package loads no dotenv; +the launcher assembles the environment. The full variable set is documented in the repo [README](../../../README.md#configuration); note `APPWRITE_PROD_FORECASTS_COLLECTION_ID` is flagged there as needs-verify (the previously-documented value was found absent in live Appwrite).