diff --git a/docs/ADRs/013_sampled_forecast_wire_contract.md b/docs/ADRs/013_sampled_forecast_wire_contract.md index 6911d44..3c3fd91 100644 --- a/docs/ADRs/013_sampled_forecast_wire_contract.md +++ b/docs/ADRs/013_sampled_forecast_wire_contract.md @@ -661,6 +661,40 @@ geography. **Source:** the sidecar is built from this repo's ADR-011 GAUL lookup (`views_postprocessing/data/gaul_lookup.parquet`, area-majority cell→region mapping sourced from views-datafactory); the #91 sink leg attaches it per run. +**§5.1a Nullable int64 was considered and rejected** *(clarification 2026-08-17, +MINOR — no change to the rule, and no `contract_version` bump; this records an +alternative the 2026-07-19 ruling did not weigh).* The partner has now twice asked +for the `*_code` columns as integers (#278; views-postprocessing#272), and the +justification given each time — that an integer column cannot carry a missing +value — is a property of NumPy-backed pandas, **not** of Parquet. Parquet and Arrow +both carry nullable integers natively, and this repo's own lookup stores all three +code columns as `int64` with zero nulls; the float is introduced by our writer +(`contract/wire/sidecar.py`), not by the source. So the alternative is real and the +old reason for dismissing it was wrong. + +It is rejected anyway, on a measured ground rather than that one. What §5.1 requires +is a schema that does not depend on the data. Nullable int64 does not deliver that +at the layer the consumer observes — it relocates the dependence. Measured +2026-08-17 (pyarrow 23.0.1, pandas 3.0.5): an int64 Parquet column containing **no** +null reads back as `int64` under a default `pd.read_parquet`, and the same column +containing **one** null reads back as `float64`. Under float64 the consumer sees one +dtype always; under nullable int64 they would see `int64` usually and `float64` +whenever a run happened to contain a missing code — which is the data-dependent +schema the 2026-07-19 ruling rejected, moved from our writer to their reader. The +escape (`dtype_backend="numpy_nullable"`) is a consumer-side commitment this repo +can neither verify nor enforce (cf. C-87, C-92). Independently, faoapi's reader +`reindex`es the sidecar onto the forecast's gids and then calls `.to_numpy()`, both +of which return float64 from a nullable integer column — so the change would not +even reach the consumer as integers. + +**Consequence for the partner, and it is the useful half:** because the delivered +region excludes the GAUL-uncovered cells (`delivery/coverage.py`), no delivered code +is ever missing — `tests/test_gaul_lookup_fidelity.py::test_lookup_has_no_nulls` +holds this in CI — so `astype("int64")` on read is lossless for this product. That +is a property of the delivered **region**, not of the contract: a future region with +no exclusion list could carry genuinely missing codes, which is exactly why the +column type stays float64. + **§5.2 Consistency.** The sidecar's cell-id set must equal the forecast's cell-id set (views-postprocessing's existing coverage/identity invariants, to be extended to the sidecar in the #91 leg — not yet built as of 2026-07-19). The sidecar hash is pinned in **the Hop-B run manifest diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index f90ef8d..c6a0890 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -4,9 +4,9 @@ |-------------------|--------------------------------------| | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | -| Last Updated | 2026-08-15 | -| Total Concerns | 102 | -| Open Concerns | 22 | +| Last Updated | 2026-08-16 | +| Total Concerns | 107 | +| Open Concerns | 27 | | Resolved Concerns | 80 | --- @@ -54,7 +54,7 @@ covered a single open entry (see Historical clusters below). ### Cluster J: Delivery aftercare has no mechanism **Root cause:** the delivery pipeline is write-only — nothing exists downstream of upload for correction, recall, or provenance audit. -**Entries:** C-22 (acute), C-15, C-24 +**Entries:** C-22 (acute), C-15, C-24, C-105 (added 2026-08-16 — a torn upload attempt is aftercare the write-only path has no answer for) **Highest tier:** 3 **Fix strategy:** the C-22 correction procedure (issue #15) plus pipeline-core #245's structured metadata field to retire the description-as-carrier abuse. **Resolution scope:** Partial (process, not code). @@ -1053,6 +1053,101 @@ CI now checks that repository out and those seven run on every pull request — Recorded rather than left implicit because "the operator did a console session" is exactly the kind of adjacent fact that gets mistaken for progress on this entry. It is not. What it does establish is that the session is a thing that happens, and these two items are small enough to ride along with the next one. +--- + +### C-103: The clip that keeps fabricated months off the wire depends on a package this repo does not declare, and its absence is swallowed + +| Field | Value | +|-------|-------| +| ID | C-103 | +| Tier | 2 — structural fragility with a named scenario: the guard's dependency is absent, the absence is caught by a bare `except Exception`, and the delivery proceeds unclipped. Held at 2 rather than 1 because of a gap, not a judgement — see the verification question below. | +| Source | `/repo-assimilation` (2026-08-16), measured | +| Trigger | When the launcher's environment is next built or changed — a views-models deploy, a container rebuild, a `poetry install` on the delivery host — verify `import datafactory_query` succeeds there. The delivery will not tell you if it does not. | +| Owner | This repository, for the swallow and the declaration. The producer owns the fact itself. | +| Location | `views_postprocessing/contract/source_metadata.py:37`; `views_postprocessing/unfao/managers/unfao.py:137-142`, `views_postprocessing/crafd/managers/crafd.py:137-142`; `pyproject.toml` (the dependency is absent) | + +`source_metadata.last_valid_month_id` lazily imports `datafactory_query.defaults`. Measured 2026-08-16: that package is in neither `pyproject.toml` nor `poetry.lock`, and `import datafactory_query` raises `ModuleNotFoundError` in the project venv. Its only caller wraps the call in `except Exception: lv = None` and then returns the historical frame **unclipped**, logging one WARNING — so "the dependency is missing" and "the producer publishes no boundary attribute" leave through the same branch with the same outcome, and that outcome is unobserved zero-padded months shipping to the partner as observed history. The lazy import states its own reason — *"so this module loads without the heavy datafactory dependency present (e.g. in unit-test environments)"* — but nothing at the call site distinguishes a unit-test environment from a delivery. + +**Verification question, stated as a gap rather than dressed as a finding.** Whether the production launcher's environment supplies `datafactory_query` could not be established from this repository; views-models builds that environment. If it does, this is a declaration gap and the swallow is the whole risk. If it does not, C-26's fabrication has been shipping unclipped since #126. **Do not downgrade on inspection of this repo alone** — the instruction C-26 already carries, for the same reason. + +**C-60 is this shape, and it was resolved by deleting the degradation.** There, a provenance stamp reached into the producer's ledger schema inside a bare `except … pass` and returned `"unknown"`; the fix was to raise. The difference here is that the degradation is deliberate and documented ("degrade-open, C-26") — which makes the question *whether the open side is still the right one*, not whether someone forgot. + +Cross-refs: **C-26** (the fabrication this clip exists to prevent), **C-07** (undeclared runtime dependencies, the same class, resolved), **C-60** (bare-except degradation, resolved by raising), **C-27** (a swallowed failure surfacing far from its cause), **D-07** (the decision that data facts come from the producer, which created this import). + +--- + +### C-104: A stale virtualenv turns 25 tests red, and 20 of them are the only tests that import either manager + +| Field | Value | +|-------|-------| +| ID | C-104 | +| Tier | 3 — no production impact. The cost is that a red suite stops carrying signal, on precisely the two modules with the thinnest coverage. | +| Source | `/repo-assimilation` (2026-08-16), measured | +| Trigger | When `pytest` reports failures in `tests/test_framework_contract.py` or `tests/test_store_construction.py`, check `pip show views-pipeline-core` against `poetry.lock` before reading them as defects. | +| Owner | This repository. | +| Location | `tests/test_framework_contract.py`, `tests/test_store_construction.py` (20 failures); `tests/test_wire_shard.py`, `tests/test_wire_sidecar.py`, `tests/test_hop_b_sink_e2e.py` (5 failures); `poetry.lock` versus the project venv | + +Measured 2026-08-16 in the project venv: 458 collected, **433 passed, 25 failed**, 39 xfailed, in 14.85s. The venv holds `views-pipeline-core 2.3.0` and `pyarrow 23.0.1`; `poetry.lock` pins **3.0.1** and **16.1.0**. The pyarrow half is known and predicted: 5 byte-parity failures reporting *"pinned toolchain violated: byte-parity oracle requires pyarrow 16.1.0, found 23.0.1"*, exactly what `tests/fixtures/wire_contract/README.md` says will happen under **C-72**. The pipeline-core half is documented nowhere: `ModuleNotFoundError: No module named 'views_pipeline_core.modules.dataloaders.datafactory_contract'`, raised at import of both managers, which takes out every test that constructs or inspects one. + +The consequence is that in a drifted checkout the two largest modules in the package — 387 lines each, 21% of the source — are not merely under-covered but **entirely unexercised**, and the suite reports that in a form indistinguishable from a real break. CI runs `poetry install` and gets the locked versions, so this is a local condition rather than a CI one — **C-81**'s asymmetry running in the other direction, with the laptop the weaker seat rather than the stronger. **C-36** is the precedent for what a suite that is red for a known reason costs: it stops being read. + +Cross-refs: **C-72** (owns the pyarrow half — that half is not re-registered here), **C-81** (CI-versus-local coverage asymmetry), **C-36** (a permanently-red suite cannot detect new regressions). + +--- + +### C-105: A run is uploaded file-by-file with no rollback and no idempotency — a mid-run failure leaves orphans and the retry adds more + +| Field | Value | +|-------|-------| +| ID | C-105 | +| Tier | 3 — no delivered value is corrupted. The store accumulates unreferenced objects that no artifact describes, in a bucket with no named retention owner. | +| Source | `/repo-assimilation` (2026-08-16) | +| Trigger | When the upload interlock is first opened for a live run (`wire_upload_enabled: True`), or when the retention owner D-12 defers is named — whichever comes first — decide what a torn attempt leaves behind and who removes it. | +| Owner | This repository for the mechanism; the operator for retention. | +| Location | `views_postprocessing/contract/wire/sink.py:167-171` | + +`deliver_run` uploads every shard, then the sidecar, then the run manifest, each through `_ContractStorePort.upload`, which raises on anything but explicit success (**C-79**). A raise at shard *k* of *n* is therefore correct in the one dimension the contract governs — no manifest means the run is invisible to the consumer, which is the §4.2 commit-marker design working — and silent in every other: the *k* uploaded objects remain, nothing records that they exist, and nothing removes them. Re-running the delivery re-uploads all *n* under the same names, and whether that supersedes or duplicates is a store semantic this repository asserts nowhere. At run-0 scale that is roughly 110 objects per attempt. + +`docs/operations/correction_procedure.md` covers the *wrong value* case — the contract has no retraction primitive, so a correction is a new complete run, manifest last. A torn attempt is a different case and is not covered by it. + +Cross-refs: **C-94** (nothing observes the outcome of an upload at the time it happens), **C-79** (the single-file orphan this generalises), **D-12** (the unnamed retention owner this compounds with). Part of causal cluster: **Cluster J — Delivery aftercare has no mechanism**. + +--- + +### C-106: The §2 header builder — the module that owns the contract version — is reachable only from tests + +| Field | Value | +|-------|-------| +| ID | C-106 | +| Tier | 4 — unreached, not wrong. Registered because the identical shape has been closed four times here by deletion, and because this instance sits in a module whose *other* export is live on every delivery. | +| Source | `/repo-assimilation` (2026-08-16), measured | +| Trigger | When someone proposes changing `CONTRACT_VERSION`, or when this repository first acts as a Hop-A *producer* rather than only a consumer — at that point `build_header` acquires the caller it was written for and this entry is discharged. | +| Owner | This repository. | +| Location | `views_postprocessing/contract/wire/header.py:32` (`build_header`); `views_postprocessing/contract/gaul_schema.py:87` (`colrow`) | + +Measured: `build_header` is called from `tests/test_wire_header.py` and `tests/test_wire_shard.py`, and nowhere else. On the delivery path the sink re-embeds the producer's Hop-A header untouched (`contract/wire/sink.py:111-113`, §10.2 *"the sink mints nothing"*), so the builder never runs in a delivery. The module is half-reached rather than dead: `CONTRACT_VERSION = "1.5"` is imported by `contract/wire/run_manifest.py:19` and written into every run manifest, so deletion is not the question — what `build_header` is *for* is. Separately, `gaul_schema.colrow` has zero callers anywhere, tests and build scripts included. Neither is a defect; both are surface a reader must make a decision about, and neither currently has one recorded. + +Cross-refs: **C-100** (the live sibling — a four-method port with three used methods), **C-64**, **C-75**, **C-45** (three prior instances of unreached declared surface, all resolved by deleting). + +--- + +### C-107: The doc-accuracy scan reads markdown only, so a docstring pointing at a moved file rots unwatched — two already have + +| Field | Value | +|-------|-------| +| ID | C-107 | +| Tier | 4 — navigational, with no correctness or reliability impact. Registered because this repository's stated discipline is that a docstring points at the one home of a fact, which makes a broken pointer a failure of the discipline rather than a typo. | +| Source | `/repo-assimilation` (2026-08-16), measured | +| Trigger | When the next module moves under `views_postprocessing/`, check its inbound docstring references as well as its markdown ones — or when someone proposes widening `tests/test_doc_accuracy.py`'s corpus, at which point C-97's objection applies and this entry states what the gap actually is. | +| Owner | This repository. | +| Location | `tests/test_doc_accuracy.py:76-78,133-134` (the scanned corpus); `views_postprocessing/delivery/coverage.py:5`; `views_postprocessing/delivery/observed_range.py:6` | + +`test_doc_accuracy` scans `README.md`, `docs/architecture/*.md`, package `README.md` files, ADRs and CICs. Python docstrings are outside that corpus. Two are already stale, and both point at files that moved in exactly the refactors whose *markdown* fallout the same test was extended to catch: `delivery/coverage.py` sends the reader to `views_postprocessing/unfao/extraction.py`, deleted in #151, and `delivery/observed_range.py` to `views_postprocessing/unfao/source_metadata.py`, moved to `contract/` in #153. + +**This is not a proposal to scan docstrings.** C-97 measured what that costs for the no-copy scan and argued it down under ADR-014 §3 — a guard that fires on ordinary prose gets deleted, after which the real rule is unguarded. The two scans are not the same (a deleted symbol or a repo-relative module path is a far narrower pattern than a store name in a sentence), so the objection is not decisive here — but it is the reason this is registered as a measured gap rather than fixed on sight. + +Cross-refs: **C-80** (the same guard, the adjacent corpus gap, resolved by widening), **C-97** (why widening a scan into docstrings is not automatic). + --- ## Disagreements diff --git a/views_postprocessing/contract/gaul_schema.py b/views_postprocessing/contract/gaul_schema.py index 1a49e49..e51f6da 100644 --- a/views_postprocessing/contract/gaul_schema.py +++ b/views_postprocessing/contract/gaul_schema.py @@ -19,6 +19,22 @@ `COLUMNS` below states each column's role and wire dtype once; everything else is derived from it. Reordering `COLUMNS` **is a wire change** and will fail the §10 byte-parity fixture — which is the intended consequence, not an accident. + +**Before proposing integer code columns, read ADR-013 §5.1a.** The partner has asked +twice (#278, #272), and will ask again; the reason recorded here until 2026-08-17 was +not a good one. "Codes are always float64" is *not* because an integer column cannot +hold a missing value — parquet and arrow carry nullable integers natively, and the +lookup this module describes stores all three code columns as `int64` with zero +nulls. The float is introduced by the builders below, not by the data. + +The rule survives on a different, measured ground: an int64 parquet column reads back +as `int64` under a default pandas read when it holds no null, and as `float64` when it +holds one, so nullable int64 would move the dtype instability from our writer to the +consumer's reader and make it depend on what a given run contained. §5.1a carries the +measurement, the consumer-side evidence, and the one useful consequence — that because +the delivered region excludes the GAUL-uncovered cells, no delivered code is ever +missing (`tests/test_gaul_lookup_fidelity.py::test_lookup_has_no_nulls`), so a +consumer's `astype("int64")` on read is lossless for this product. """ from __future__ import annotations