From 1543ef95afc0ab480a0567217eb6f5f58e8ce786 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 10:47:29 +0200 Subject: [PATCH 1/2] docs(register): C-103..C-107 from the repo-assimilation pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five entries, none of which duplicated an existing one — checked against all 22 open and 80 resolved concerns before writing: - C-103 (Tier 2) the observed-range clip depends on `datafactory_query`, which is declared nowhere and is not installed; its absence is swallowed by a bare `except Exception`, so a missing dependency and "the producer publishes no boundary" leave through the same branch and ship fabricated months either way. Carries the verification question it cannot answer from this seat. - C-104 (Tier 3) a stale virtualenv takes 25 tests red, 20 of them the only tests that import either manager. Measured: 433 passed / 25 failed against pipeline-core 2.3.0 + pyarrow 23.0.1 where the lock pins 3.0.1 + 16.1.0. - C-105 (Tier 3) a run uploads file-by-file with no rollback and no idempotency. Added to Cluster J. - C-106 (Tier 4) `wire/header.build_header` is reachable only from tests; so is `gaul_schema.colrow`. C-100's shape, one module over. - C-107 (Tier 4) the doc-accuracy scan reads markdown only, so two docstrings still point at modules deleted in #151 and moved in #153. Header counts and Cluster J updated; test_register_integrity green (40 tests). Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 103 +++++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 4 deletions(-) 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 From 08548aace18ea5f187c0485cbd0df9df3b4b3855 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 10:51:43 +0200 Subject: [PATCH 2/2] =?UTF-8?q?docs(adr-013):=20=C2=A75.1a=20=E2=80=94=20n?= =?UTF-8?q?ullable=20int64=20was=20considered=20and=20rejected=20(#278)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the loop the partner has now opened twice (#278, views-postprocessing#272): they ask for integer GAUL codes, we say no, and the reason we give does not survive contact with the format we actually deliver. The old reason was wrong. "Codes are always float64" was justified by an integer column being unable to carry a missing value — a property of NumPy-backed pandas, not of parquet. Arrow and parquet carry nullable integers natively, and this repo's own `data/gaul_lookup.parquet` stores all three code columns as `int64` with zero nulls; the float is introduced by `contract/wire/sidecar.py`, not by the source. The 2026-07-19 ruling weighed "int64 when complete" and rejected it for making the schema depend on the data. It never weighed nullable int64. The rule survives on a measured ground instead. Measured 2026-08-17, pyarrow 23.0.1 / pandas 3.0.5: an int64 parquet column with no null reads back `int64` under a default `pd.read_parquet`; the same column with one null reads back `float64`. So nullable int64 does not remove the float — it moves it from our writer to the consumer's reader and makes it appear only sometimes, which is the data-dependent schema §5.1 exists to prevent. Independently, faoapi's reader `reindex`es the sidecar and calls `.to_numpy()`; both yield float64 from a nullable integer column, so the change would not reach them as integers anyway. The useful half is recorded too: because the delivered region drops the GAUL-uncovered cells, no delivered code is ever missing — held in CI by `tests/test_gaul_lookup_fidelity.py::test_lookup_has_no_nulls` — so a consumer's `astype("int64")` is lossless for this product. That is a property of the region, not the contract, which is why the type stays float64. No behaviour change, no `contract_version` bump: §5.1a records a rejected alternative, it does not alter the rule. `gaul_schema.py`'s docstring now says so at the declaration and points at §5.1a, so the next person to be asked finds the answer where they are standing rather than reopening it. Co-Authored-By: Claude Opus 5 (1M context) --- .../013_sampled_forecast_wire_contract.md | 34 +++++++++++++++++++ views_postprocessing/contract/gaul_schema.py | 16 +++++++++ 2 files changed, 50 insertions(+) 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/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