From 237378d780e2a90f085fc3f5f5b0ed8a86c6984f Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 5 Aug 2026 11:58:28 +0200 Subject: [PATCH] =?UTF-8?q?refactor(managers):=20C-40=20=E2=80=94=20store?= =?UTF-8?q?=20construction=20off=20the=20manager=20class?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-40's residual read "gated on views-pipeline-core 3.0.0, which is a release signal rather than engineering work". 3.0.0 shipped 2026-08-03 and nothing became possible, because two different gates had been conflated: - the pandas gate (consequence b) was gated on pipeline-core's DataFrame retirement, an epic, not a version — and it lifted on its own on 2026-07-27. - the de-inheritance was never gated on any pipeline-core release. It is gated on views-models: postprocessors/un_fao/main.py:27 constructs our manager directly and the framework's Template Method drives _read/_transform/_validate/_save. The inheritance IS the integration contract, so removing it is a two-repo change. Nobody had written that down, which is how the entry sat parked reading as unblocked. What was actually available is the inbound half of this entry's own prescribed DIP mitigation. The outbound half landed in July as _ContractStorePort; the inbound half never did. _prod_forecasts_datastore, __datastore and __appwrite_config were methods constructing AppwriteConfig and DatastoreModule inline. They are now module-level functions taking declared arguments. No delivered byte moves. The gain is C-40's consequence (a): store construction, its refusals and their ordering are testable with no manager instance, no views-models path manager and no Appwrite environment — tests/test_store_construction.py, 14 tests, which could not have been written a day ago. First part of the manager seam reachable without the framework. Two things found while in there, both removed: - self.ensemble_path_manager: assigned in __init__ and in the builder, read once four lines later, and nowhere else in either partner package or in views-models. - loa = "pgm" followed by `if not loa: raise` — an unreachable branch guarding a variable never used again, beside the commented-out block that once computed it. Manager class 351 -> 272 lines (16 -> 14 methods); files 440 against the 450 directory budget. New ratchet test_the_manager_class_itself_stays_thin bounds the class at 300 — the existing budget counts the directory and so cannot see a class re-absorbing logic, which is exactly the shape of this refactor. Both bounds stand; neither was relaxed. Four mutations proven: env assert moved after construction, a builder given back its self, a coordinate value pasted in, the class padded past the ratchet. C-40 re-scoped with the corrected gate and trigger; Location rows on C-33 and C-40 updated; the dated ADR-011 assessment banners the two claims this change invalidated. Suite 377 passed / 40 xfailed; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- docs/ADR-011_implementation_assessment.md | 8 + reports/technical_risk_register.md | 25 ++- tests/test_doc_accuracy.py | 35 ++++ tests/test_store_construction.py | 173 +++++++++++++++++++ views_postprocessing/crafd/managers/crafd.py | 172 +++++++++--------- views_postprocessing/unfao/managers/unfao.py | 172 +++++++++--------- 6 files changed, 420 insertions(+), 165 deletions(-) create mode 100644 tests/test_store_construction.py diff --git a/docs/ADR-011_implementation_assessment.md b/docs/ADR-011_implementation_assessment.md index 83e3714..9259901 100644 --- a/docs/ADR-011_implementation_assessment.md +++ b/docs/ADR-011_implementation_assessment.md @@ -1,6 +1,14 @@ # ADR-011 Implementation Assessment **Last updated:** 2026-06-12 +> **This is a dated snapshot (2026-06-12), not a description of the code today.** It is +> kept as a record of what was known when ADR-011 was assessed. Several things it +> describes as current have since been removed — notably the `.env` credential borrow +> (killed by þing-01 #134; the environment is now declared and validated fail-loud in +> each partner's `appwrite_env.py`) and `self.ensemble_path_manager`, which stopped +> existing on 2026-08-05 when store construction moved off the manager class (register +> C-40). Read §8 in particular as history. + **Status:** Data prerequisites MET — see §10. Implementation unblocked on the data side; verification infrastructure (Appwrite, pipeline-core E2E) still required before switching the pipeline. --- diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index a1e6879..0dcfc30 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -345,7 +345,7 @@ See also D-10 (handling decision), C-43 (the *value*-correctness sibling — run | Source | `expert-code-review` (2026-06-12) | | Trigger | **Fired 2026-08-03 — see the update below.** The remaining trigger is the *extraction* one, and it is now named: a **third** in-repo partner package, **or** the first bug that must be hand-patched identically in both manager files — whichever comes first. | | Owner | Whoever adds the third partner package, or hits the first double-patch. Until one of those happens the duplication is the deliberate WET position, not a task anyone is behind on. | -| Location | `views_postprocessing//managers/.py` — `_prod_forecasts_datastore`, `__datastore`, `__appwrite_config`, and the four hardcoded `os.getenv("APPWRITE__*")` literals inside the last of those; declared names in each partner's `appwrite_env.py`. **Symbols, not line numbers** — see the note under the measurement below. | +| Location | `views_postprocessing//managers/.py` — the module-level `_build_prod_forecasts_store`, `_build_partner_store`, `_partner_appwrite_config` (methods until 2026-08-05), and the four hardcoded `os.getenv("APPWRITE__*")` names inside the last of those; declared names in each partner's `appwrite_env.py`. **Symbols, not line numbers** — see the note under the measurement below. | Mitigation: a small `DeliveryProfile` (bucket/collection/database ids, category, targets) passed to the manager — one manager class, N store configs. Scheduled **after** the FAO global delivery ships (D-09); the only immediate action is deleting the commented-out config blocks at lines 80-107, which are a mis-uncomment hazard during deadline work. @@ -398,7 +398,7 @@ See also C-24 (schema contract per store), C-77 (the fourth home for partner ide | Tier | 2 | | Source | `expert-code-review` (2026-06-24) | | Trigger | **(a) Upstream change:** when pipeline-core changes `PGMDataset` / the data loader / the postprocessor base (mid-migration: their #186/#188/#161), verify the inherited surface this repo depends on still holds. **(b) Standing work item:** the input-side de-inheritance (the sink side landed — see the 2026-07-31 update) — schedule it, don't wait for a trigger. | -| Location | `views_postprocessing//managers/.py` — the `class PostProcessorManager(PostprocessorManager, ForecastingModelManager)` statement (double inheritance); `_prod_forecasts_datastore`, `__datastore`, `__appwrite_config` (inline env/AppwriteConfig/DatastoreModule); `_validate` and `_check_coverage`; the DIP sink adapter `_ContractStorePort`. **Since 2026-08-03 all of it exists twice** — `unfao` and `crafd` are the same file with the partner name changed (C-33). Symbols rather than lines, deliberately: an earlier version of this row was invalidated by a comment edit four lines long. | +| Location | `views_postprocessing//managers/.py` — the `class PostProcessorManager(PostprocessorManager, ForecastingModelManager)` statement (double inheritance); `_build_prod_forecasts_store`, `_build_partner_store`, `_partner_appwrite_config` (env/AppwriteConfig/DatastoreModule — moved OFF the class 2026-08-05, see the update below); `_validate` and `_check_coverage`; the DIP sink adapter `_ContractStorePort`. **Since 2026-08-03 all of it exists twice** — `unfao` and `crafd` are the same file with the partner name changed (C-33). Symbols rather than lines, deliberately: an earlier version of this row was invalidated by a comment edit four lines long. | `UNFAOPostProcessorManager` subclasses **two concrete** pipeline-core base classes (`PostprocessorManager`, `ForecastingModelManager`) and **interleaves infrastructure** (env reading, `AppwriteConfig` construction, `DatastoreModule`, path resolution) with the FAO **business logic** (GAUL enrichment, the 9-column null gate) inside the lifecycle hooks. Consequences: (a) the FAO logic cannot be instantiated or unit-tested without the full framework + Appwrite env + viewser; (b) **pandas cannot leave the delivery path** because the inherited data loader and `PGMDataset` are pandas — gated on pipeline-core's own DataFrame retirement; (c) **SDP exposure** — heavy *inheritance* coupling to a pipeline-core that is itself unstable (mid-migration), so upstream changes break far from their cause (cf. C-27, C-29); (d) it's the repo's only composition-over-inheritance violation. The dependency itself is correct (`unfao.py` genuinely *is* a pipeline-core postprocessor) — the issue is its **blast radius**. Mitigation (does **not** fight the Template-Method framework): keep the subclass as a **thin shell** but extract `enrich` + `validate` + the 9-column contract into a pipeline-core-free core object the manager *calls*, and wrap the Appwrite I/O behind a small delivery-sink adapter (DIP). This makes the FAO logic testable standalone and insulates it from pipeline-core churn. @@ -436,7 +436,26 @@ See also C-24 (schema contract per store), C-77 (the fourth home for partner ide **On þing-02 S24(5).** `docs/CLONING.md` cited that verdict as forbidding these imports outright. Reading it directly (`þingit/02_credential_identity_key_ownership/sáttmál.md:240-242` — precondition (5) itself; the section opens at `:232` under the heading *"§5 — The clone (`un-crafdapi`)"* — and `orð_dómr.md:418-441`), it binds *"the clone"* — `un-crafdapi` and `views-productionapi`, repositories **git-cloned from views-faoapi** — and does not reach an in-repo partner package of the producer. CLONING.md over-claimed; PR #211 corrects the citation rather than weakening the rule. This entry's own scope is unaffected: the coupling is a design concern here regardless of what the verdict binds, and issue **#146**'s deferred unwind now covers two files instead of one. -Tier held at 2. The residual scope — the double inheritance and the framework-bound instantiation — is unchanged, and is still gated on views-pipeline-core 3.0.0 (C-44/C-62). +**Update 2026-08-05 — the stated gate was wrong, and the source half of the mitigation has now landed.** + +*The gate first, because it is what kept this entry parked.* The line below said the residual was *"gated on views-pipeline-core 3.0.0, which is a release signal rather than engineering work"*. 3.0.0 shipped on 2026-08-03 and nothing became possible. Two different gates had been conflated: + +- **Consequence (b), the pandas gate** — genuinely gated on pipeline-core's DataFrame retirement, which is an epic (#186/#207), not a version number. It lifted on its own on 2026-07-27 when the frame-native fetch shipped, and 3.0.0 had nothing to do with it. +- **The de-inheritance** — never gated on any pipeline-core release. **It is gated on views-models**, which is the thing nobody had written down. `postprocessors/un_fao/main.py:27` constructs `UNFAOPostProcessorManager(...)` directly, and the framework's Template Method drives `_read`/`_transform`/`_validate`/`_save`. **The inheritance *is* the integration contract with views-models.** Removing it means writing a different launcher there, so it is a two-repository change and an operator decision — not, as this entry implied for two days, unblocked work waiting on nobody. + +*What landed.* This entry's own prescribed mitigation was to wrap the Appwrite I/O behind a DIP adapter. The **outbound** half landed in July as `_ContractStorePort`. The **inbound** half never did: `_prod_forecasts_datastore`, `__datastore` and `__appwrite_config` were still methods on the manager, constructing `AppwriteConfig` and `DatastoreModule` inline. They are now module-level functions taking declared arguments — `_build_prod_forecasts_store`, `_build_partner_store`, `_partner_appwrite_config`. + +That is a smaller change than it sounds and a bigger one than it looks. Smaller: no delivered byte moves, and the three functions are the same code with their inputs declared. Bigger: **consequence (a) is substantially discharged for this surface.** Store construction, its refusals and their ordering are now testable with no manager instance, no views-models path manager and no Appwrite environment — `tests/test_store_construction.py`, 14 tests, which could not have been written at any price a day ago. That is the first time any part of the manager seam has been reachable without the framework. + +*Two things found while in there.* `self.ensemble_path_manager` was assigned in `__init__` and in the store builder, read exactly once four lines after being set, and read nowhere else in either partner package or in views-models — a method-local value wearing the costume of manager state. And a `loa = "pgm"` assignment followed by `if not loa: raise`, an unreachable branch guarding a variable never used again, beside a commented-out block that had computed it for real. Both gone. + +*Measured.* The manager **class** went 351 → **272 lines** (16 → 14 methods); both partner files are 440 lines against a 450 directory budget. A new ratchet, `test_the_manager_class_itself_stays_thin`, bounds the class at 300 — added because the existing budget counts the *directory*, so it cannot see a class re-absorbing logic, and this refactor is precisely the shape it is blind to. Both bounds now stand; neither was relaxed. + +**Residual scope, stated honestly.** The double inheritance itself, and with it consequences (c) SDP exposure and (d) composition-over-inheritance. Not unilaterally actionable, and arguably not a defect: this entry has always conceded that *"the dependency itself is correct — `unfao.py` genuinely **is** a pipeline-core postprocessor"*, and ADR-015 §1 argues there is nothing to unwind to. What remains is exposure to an upstream that moves, which 3.0.0 demonstrated by moving. + +**Trigger, corrected.** Not a pipeline-core release. Revisit when **views-models changes how postprocessors are launched** — at which point de-inheritance becomes a one-repo change rather than two — or when an upstream base-class change actually breaks a delivery, which `tests/test_framework_contract.py` now catches here rather than on the partner's run. + +Tier held at 2: the blast-radius argument is unchanged for what remains, and containment is not removal. *Did not:* the double inheritance (the `class UNFAOPostProcessorManager(...)` statement — this row cited `unfao.py:80` when written, and that number has moved twice since) stands, and so do consequences (a) — the FAO logic still cannot be instantiated without the framework — and (c)/(d). **This entry remains open on exactly that scope.** Its remaining fix is gated on views-pipeline-core's 3.0.0 (C-44/C-62), which is a release signal rather than engineering work. diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index 250eec9..1c1c87d 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -367,6 +367,10 @@ def test_internal_doc_links_resolve(): #: previously unbudgeted, which is the same regrowth wearing a different filename. _MANAGER_LINE_BUDGET = 450 +#: The manager CLASS, separately (C-40). 351 before the 2026-08-05 extraction, 272 after. +#: A ratchet — see `test_the_manager_class_itself_stays_thin` for why it is not a target. +_MANAGER_CLASS_BUDGET = 300 + def _is_type_checking(test: ast.expr) -> bool: """`TYPE_CHECKING` or `typing.TYPE_CHECKING`, and nothing else. @@ -488,6 +492,37 @@ def test_the_manager_stays_within_its_line_budget(managers_dir): ) +@pytest.mark.parametrize("partner", _PARTNER_PACKAGES) +def test_the_manager_class_itself_stays_thin(partner): + """The directory budget above is anti-regrowth. This one is anti-*fusion*. + + They measure different things and both are needed. A file can sit comfortably under + 450 lines while the manager **class** absorbs store construction, env assembly and + business logic — which is exactly what register C-40 is about, and exactly what the + directory count cannot see. Conversely a thin class proves nothing if 800 lines + moved to a sibling module, which is why the directory count exists. + + Added 2026-08-05 alongside the C-40 extraction that took the class from 351 lines to + 272. The bound is a **ratchet, not a target**: it is the measured value plus modest + headroom, and the right response to it failing is to move something out of the class, + not to raise the number. If it is ever raised, the raise belongs in a commit message + that says what was added and why it had to live on the manager. + """ + source = (_PKG / partner / "managers" / f"{partner}.py").read_text() + cls = next( + n for n in ast.walk(ast.parse(source)) + if isinstance(n, ast.ClassDef) and n.name.endswith("PostProcessorManager") + ) + lines = cls.end_lineno - cls.lineno + 1 + assert lines <= _MANAGER_CLASS_BUDGET, ( + f"{cls.name} is {lines} lines, over the {_MANAGER_CLASS_BUDGET} ratchet. The " + "manager is the Template-Method shell: it orchestrates hooks and adapts the " + "framework. Logic that can be called with declared arguments belongs in a " + "module-level function or in contract/ — where it can be tested without a " + "manager, an Appwrite environment or a views-models path manager (C-40 (a))." + ) + + def _imported_subpackages(source: str, module_path: Path) -> set[str]: """Every ``views_postprocessing.`` this module imports — all four spellings. diff --git a/tests/test_store_construction.py b/tests/test_store_construction.py new file mode 100644 index 0000000..1abd151 --- /dev/null +++ b/tests/test_store_construction.py @@ -0,0 +1,173 @@ +"""Store construction, called without a manager — register C-40's consequence (a). + +C-40's first stated consequence is that *"the FAO logic cannot be instantiated or +unit-tested without the full framework + Appwrite env + viewser."* For the store +construction that was literally true until 2026-08-05: building the production-forecasts +datastore meant calling ``self._prod_forecasts_datastore()``, which meant having a +manager, which meant a views-models path manager and a live environment. + +Nothing about that code needed any of it. It read one value out of ``self.configs`` and +assigned a local to ``self.ensemble_path_manager`` that nothing ever read back. Turning +the three store builders into module-level functions with declared arguments removed the +requirement entirely — and this file is the proof, because every test here runs with no +manager instance and with the Appwrite variables explicitly *removed* from the +environment. + +**What this does not test.** That a real store connects, or that the credentials work. +That needs the production Appwrite project, which þing-02 D2 forbids testing against and +which is the only project that exists (A3(h), answered 2026-08-05). What is testable +offline is the part that was previously untestable at any price: the refusals, their +ordering, and the fact that the environment contract is checked *before* anything is +constructed. +""" + +from __future__ import annotations + +import pytest + +from tests.conftest import PARTNER_PACKAGES + +#: Every Appwrite name any partner's builders read. Cleared before each test so a +#: maintainer's populated shell cannot make these pass for the wrong reason — the +#: failure mode a laptop-only run has already produced twice in this repo (C-81). +_APPWRITE_PREFIX = "APPWRITE_" + + +@pytest.fixture +def no_appwrite_env(monkeypatch): + """A deliberately empty Appwrite environment.""" + import os + + for name in [k for k in os.environ if k.startswith(_APPWRITE_PREFIX)]: + monkeypatch.delenv(name, raising=False) + + +def _managers(partner: str): + pytest.importorskip( + "views_pipeline_core", + reason="the builders construct pipeline-core objects; it is a declared " + "dependency, so this skip means a broken environment rather than a valid one", + ) + return __import__( + f"views_postprocessing.{partner}.managers.{partner}", + fromlist=["_build_prod_forecasts_store"], + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_builders_are_functions_not_methods(partner): + """The structural claim C-40's fix rests on. Assert it, do not assume it. + + If these ever migrate back onto the class, every test below would still pass while + silently requiring a manager again — the guard has to check the shape, not just the + behaviour. + """ + import inspect + + module = _managers(partner) + for name in ("_build_prod_forecasts_store", "_build_partner_store", + "_partner_appwrite_config"): + fn = getattr(module, name, None) + assert fn is not None, ( + f"[{partner}] {name} is gone from the module namespace. If it moved back " + "onto the manager class, C-40's consequence (a) is back with it." + ) + assert inspect.isfunction(fn), f"[{partner}] {name} is not a module-level function" + params = list(inspect.signature(fn).parameters) + assert "self" not in params, ( + f"[{partner}] {name} takes `self` — it is a method wearing a function's " + "name, and still cannot be called without a manager." + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_a_missing_ensemble_name_is_refused_before_anything_is_built( + partner, no_appwrite_env +): + """The launch-config error, raised with no manager and no environment. + + This is the whole point of the extraction in one assertion: the refusal is + observable by calling a function with an argument. + """ + module = _managers(partner) + with pytest.raises(ValueError, match="ensemble"): + module._build_prod_forecasts_store(None) + + +@pytest.mark.parametrize("bad", ["", None], ids=["empty-string", "none"]) +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_an_empty_ensemble_name_is_refused_too(partner, bad, no_appwrite_env): + """`""` and `None` are both "not configured", and both used to reach `configs.get`. + + A launch config with `ensemble: ""` is a plausible typo and must not be treated as + a name — `EnsemblePathManager("")` would resolve to something, and what it resolves + to is not this delivery's ensemble. + """ + module = _managers(partner) + with pytest.raises(ValueError, match="ensemble"): + module._build_prod_forecasts_store(bad) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_environment_contract_is_checked_before_a_store_is_constructed( + partner, no_appwrite_env +): + """Ordering, not just presence — and the ordering is the fail-loud guarantee. + + ``assert_env_declared`` must run before ``AppwriteConfig``/``DatastoreModule`` are + built, so a missing coordinate produces the declared error naming what is absent + rather than whatever the client raises when handed ``None`` for an endpoint. þing-01 + #134 turned this repo's environment handling from an implicit borrow into a declared + contract; an unchecked construction path would quietly undo that. + + The error must also name the store, because two different stores read two different + variable sets and "some Appwrite variable is missing" sends an operator to the wrong + half of the environment. + """ + module = _managers(partner) + with pytest.raises(Exception) as excinfo: + module._build_prod_forecasts_store("some_ensemble") + message = str(excinfo.value) + assert "production_forecasts" in message, ( + f"[{partner}] the refusal does not name which store's environment is " + f"incomplete: {message!r}" + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_partner_store_names_its_own_environment_when_it_refuses( + partner, no_appwrite_env +): + """The outbound store reads a different variable set from the inbound one.""" + module = _managers(partner) + with pytest.raises(Exception) as excinfo: + module._partner_appwrite_config(model_path=None) + assert "datastore" in str(excinfo.value).lower() + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_no_coordinate_value_is_baked_into_the_builders(partner): + """The repo is PUBLIC and the registry is referenced, never copied (þing-01 S6). + + The builders read every coordinate through ``os.getenv`` by declared NAME. A literal + bucket id or endpoint appearing here would be a value copy in a public repository — + the platform's original failure. The repo-wide guard covers this too; asserted at + the seam as well because this is the file where such a literal would be most + tempting to paste while debugging. + """ + import inspect + + module = _managers(partner) + source = "".join( + inspect.getsource(getattr(module, name)) + for name in ("_build_prod_forecasts_store", "_partner_appwrite_config") + ) + for line in source.splitlines(): + if "os.getenv(" in line: + assert "os.getenv(\"APPWRITE_" in line or "os.getenv('APPWRITE_" in line, ( + f"[{partner}] a coordinate is read by something other than a declared " + f"APPWRITE_ name: {line.strip()!r}" + ) + assert "https://" not in line or "#" in line.split("https://")[0], ( + f"[{partner}] a literal endpoint appears in the builders: {line.strip()!r}" + ) diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py index 87c09d2..903e47b 100644 --- a/views_postprocessing/crafd/managers/crafd.py +++ b/views_postprocessing/crafd/managers/crafd.py @@ -77,6 +77,95 @@ def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, ) +def _build_prod_forecasts_store(ensemble_name: str | None) -> DatastoreModule: + """The shared internal store (ADR-013's "shared shelf"), built from the + launcher-assembled environment (validated fail-loud; þing-01 #134 — no dotenv is + loaded here). + + pipeline-core's ``DatastoreModule.get_predictions_by_metadata`` injects an automatic + ``name == model_name`` filter on every lookup. The contract read must **not** have + it: ADR-013 artifacts are named ``{run_id}__{target}__m{month}.arrow.parquet`` + (never the bare ensemble name), so an injected ``name == "rusty_bucket"`` matches + nothing and also clobbers the wire layer's own run-id / target / name filters. + Suppressed unconditionally below — the retired legacy reader was the only caller + that needed it on (#149). + + **A function, not a method (register C-40).** It reads no manager state: the + ensemble name arrives as an argument rather than through ``self.configs``, and the + ``EnsemblePathManager`` is local — it used to be assigned to + ``self.ensemble_path_manager`` and then read exactly once, four lines later, in this + same body. Nothing else in either partner package, or in views-models, ever read it. + A method-local value wearing the costume of manager state. + + The gain is C-40's consequence (a): this is callable, and its refusals observable, + with **no manager instance, no views-models path manager and no Appwrite + environment** — see ``tests/test_store_construction.py``. + """ + if not ensemble_name: + err_msg = "Ensemble name must be provided in configs with the `ensemble` key for forecasting. Cannot proceed." + logger.error(err_msg) + raise ValueError(err_msg) + path_manager = EnsemblePathManager(ensemble_name_or_path=ensemble_name, validate=False) + + appwrite_env.assert_env_declared( + appwrite_env.CONNECTION_ENV + appwrite_env.PROD_FORECASTS_ENV, + store="production_forecasts datastore", + ) + appwrite_config = AppwriteConfig( + path_manager=path_manager, + endpoint=os.getenv("APPWRITE_ENDPOINT"), + project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), + credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), + auth_method="api_key", + cache_ttl_hours=24, + bucket_id=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_ID"), + bucket_name=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_NAME"), + collection_id=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_ID"), + collection_name=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_NAME"), + database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), + database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), + ) + datastore = DatastoreModule(appwrite_file_manager_config=appwrite_config) + # Suppress the automatic name==model_name filter (see docstring). model_path is used + # by DatastoreModule only for that injection and for uploads; the contract read + # neither uploads nor performs any model-scoped lookup. + datastore.model_path = None + return datastore + + +def _build_partner_store(model_path) -> DatastoreModule: + """The partner-facing store (`crafd_bucket`) — a function for the same reason as above. + + ``model_path`` is the framework's own path manager, so it is passed in rather than + reached for. That is the whole difference between this and a method, and it is what + lets the environment contract be checked without standing up a manager. + """ + return DatastoreModule(appwrite_file_manager_config=_partner_appwrite_config(model_path)) + + +def _partner_appwrite_config(model_path) -> AppwriteConfig: + """Env → ``AppwriteConfig`` for the partner bucket. Declared names only; the values + live in the environment and are never read into this repository's source.""" + appwrite_env.assert_env_declared( + appwrite_env.CONNECTION_ENV + appwrite_env.CRAFD_ENV, + store="crafd_bucket datastore", + ) + return AppwriteConfig( + path_manager=model_path, + endpoint=os.getenv("APPWRITE_ENDPOINT"), + project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), + credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), + auth_method="api_key", + cache_ttl_hours=24, + bucket_id=os.getenv("APPWRITE_CRAFD_BUCKET_ID"), + bucket_name=os.getenv("APPWRITE_CRAFD_BUCKET_NAME"), + collection_id=os.getenv("APPWRITE_CRAFD_COLLECTION_ID"), + collection_name=os.getenv("APPWRITE_CRAFD_COLLECTION_NAME"), + database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), + database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), + ) + + class CRAFDPostProcessorManager(PostprocessorManager, ForecastingModelManager): def __init__( self, @@ -90,7 +179,6 @@ def __init__( logger.info(f"Initializing {self.__class__.__name__}") self._forecast_resolution = None # {target: TargetLease}, set by _read self._historical_frame = None # views_frames.FeatureFrame, set by _read - self.ensemble_path_manager = None def _read_historical_frame(self): """#126: historical actuals as a views_frames.FeatureFrame — the first @@ -136,61 +224,6 @@ def _read_historical_data(self): self._initialize_data_loader() self._read_historical_frame() - def _prod_forecasts_datastore(self) -> DatastoreModule: - """The shared internal store (ADR-013's 'shared shelf'), configured from the - launcher-assembled environment (validated fail-loud; þing-01 #134 — no dotenv - is loaded here). - - pipeline-core's ``DatastoreModule.get_predictions_by_metadata`` injects an - automatic ``name == model_name`` filter on every lookup. The contract read - must **not** have it: ADR-013 artifacts are named - ``{run_id}__{target}__m{month}.arrow.parquet`` (never the bare ensemble - name), so an injected ``name == "rusty_bucket"`` matches nothing and also - clobbers the wire layer's own run-id / target / name filters. Suppressed - unconditionally below — the retired legacy reader was the only caller that - needed it on (#149).""" - ensemble_name = self.configs.get("ensemble", None) - if not ensemble_name: - err_msg = "Ensemble name must be provided in configs with the `ensemble` key for forecasting. Cannot proceed." - logger.error(err_msg) - raise ValueError(err_msg) - self.ensemble_path_manager = EnsemblePathManager(ensemble_name_or_path=ensemble_name, validate=False) - # ensemble_configs = EnsembleManager( - # ensemble_path=self.ensemble_path_manager, - # ).configs - - # loa = ensemble_configs.get("level", None) - loa = "pgm" - if not loa: - err_msg = "level must be defined in the ensemble configurations (e.g, pgm, cm). Cannot proceed." - logger.error(err_msg) - raise ValueError(err_msg) - - appwrite_env.assert_env_declared( - appwrite_env.CONNECTION_ENV + appwrite_env.PROD_FORECASTS_ENV, - store="production_forecasts datastore", - ) - appwrite_config = AppwriteConfig( - path_manager=self.ensemble_path_manager, - endpoint=os.getenv("APPWRITE_ENDPOINT"), - project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), - credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), - auth_method="api_key", - cache_ttl_hours=24, - bucket_id=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_ID"), - bucket_name=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_NAME"), - collection_id=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_ID"), - collection_name=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_NAME"), - database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), - database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), - ) - datastore = DatastoreModule(appwrite_file_manager_config=appwrite_config) - # Suppress the automatic name==model_name filter (see docstring). model_path - # is used by DatastoreModule only for that injection and for uploads; the - # contract read neither uploads nor performs any model-scoped lookup. - datastore.model_path = None - return datastore - def _read_forecast_data_contract(self): """ADR-013 contract inbound (epic #105; streaming since the run-0 OOM fix): RESOLVE the newest fully-manifested run — manifests + pinned shard @@ -199,7 +232,7 @@ def _read_forecast_data_contract(self): released). The `wire/` package owns the policy; this method only adapts the store (DIP) and declares the product facts (region curation + coverage expectations live in the lease, where frames exist).""" - port = _ContractStorePort(self._prod_forecasts_datastore()) + port = _ContractStorePort(_build_prod_forecasts_store(self.configs.get("ensemble"))) region = self.configs.get("region") self._forecast_resolution = source_selection.resolve_run( port, @@ -319,7 +352,7 @@ def _save_contract(self) -> dict: # file itself. lookup = gaul_lookup.load() upload_enabled = bool(self.configs.get("wire_upload_enabled", product.UPLOAD_ENABLED)) - store = _ContractStorePort(self._crafd_datastore()) if upload_enabled else None + store = _ContractStorePort(_build_partner_store(self._model_path)) if upload_enabled else None # The wire is partner-neutral (#153): the manager supplies CRAF'd's product # facts explicitly rather than the mechanism reaching for them. summary = wire_sink.deliver_run( @@ -369,29 +402,6 @@ def _save_contract(self) -> dict: summary["historical"] = hist_path.name return summary - def _crafd_datastore(self) -> DatastoreModule: - """The CRAF'd-facing store (`crafd_bucket`).""" - return DatastoreModule(appwrite_file_manager_config=self._crafd_appwrite_config()) - - def _crafd_appwrite_config(self) -> AppwriteConfig: - appwrite_env.assert_env_declared( - appwrite_env.CONNECTION_ENV + appwrite_env.CRAFD_ENV, store="crafd_bucket datastore" - ) - return AppwriteConfig( - path_manager=self._model_path, - endpoint=os.getenv("APPWRITE_ENDPOINT"), - project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), - credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), - auth_method="api_key", - cache_ttl_hours=24, - bucket_id=os.getenv("APPWRITE_CRAFD_BUCKET_ID"), - bucket_name=os.getenv("APPWRITE_CRAFD_BUCKET_NAME"), - collection_id=os.getenv("APPWRITE_CRAFD_COLLECTION_ID"), - collection_name=os.getenv("APPWRITE_CRAFD_COLLECTION_NAME"), - database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), - database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), - ) - def _save(self) -> dict: """Deliver the run — ADR-013 contract only (#149).""" return self._save_contract() diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 36388e2..e787bcd 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -77,6 +77,95 @@ def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, ) +def _build_prod_forecasts_store(ensemble_name: str | None) -> DatastoreModule: + """The shared internal store (ADR-013's "shared shelf"), built from the + launcher-assembled environment (validated fail-loud; þing-01 #134 — no dotenv is + loaded here). + + pipeline-core's ``DatastoreModule.get_predictions_by_metadata`` injects an automatic + ``name == model_name`` filter on every lookup. The contract read must **not** have + it: ADR-013 artifacts are named ``{run_id}__{target}__m{month}.arrow.parquet`` + (never the bare ensemble name), so an injected ``name == "rusty_bucket"`` matches + nothing and also clobbers the wire layer's own run-id / target / name filters. + Suppressed unconditionally below — the retired legacy reader was the only caller + that needed it on (#149). + + **A function, not a method (register C-40).** It reads no manager state: the + ensemble name arrives as an argument rather than through ``self.configs``, and the + ``EnsemblePathManager`` is local — it used to be assigned to + ``self.ensemble_path_manager`` and then read exactly once, four lines later, in this + same body. Nothing else in either partner package, or in views-models, ever read it. + A method-local value wearing the costume of manager state. + + The gain is C-40's consequence (a): this is callable, and its refusals observable, + with **no manager instance, no views-models path manager and no Appwrite + environment** — see ``tests/test_store_construction.py``. + """ + if not ensemble_name: + err_msg = "Ensemble name must be provided in configs with the `ensemble` key for forecasting. Cannot proceed." + logger.error(err_msg) + raise ValueError(err_msg) + path_manager = EnsemblePathManager(ensemble_name_or_path=ensemble_name, validate=False) + + appwrite_env.assert_env_declared( + appwrite_env.CONNECTION_ENV + appwrite_env.PROD_FORECASTS_ENV, + store="production_forecasts datastore", + ) + appwrite_config = AppwriteConfig( + path_manager=path_manager, + endpoint=os.getenv("APPWRITE_ENDPOINT"), + project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), + credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), + auth_method="api_key", + cache_ttl_hours=24, + bucket_id=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_ID"), + bucket_name=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_NAME"), + collection_id=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_ID"), + collection_name=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_NAME"), + database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), + database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), + ) + datastore = DatastoreModule(appwrite_file_manager_config=appwrite_config) + # Suppress the automatic name==model_name filter (see docstring). model_path is used + # by DatastoreModule only for that injection and for uploads; the contract read + # neither uploads nor performs any model-scoped lookup. + datastore.model_path = None + return datastore + + +def _build_partner_store(model_path) -> DatastoreModule: + """The partner-facing store (`unfao_bucket`) — a function for the same reason as above. + + ``model_path`` is the framework's own path manager, so it is passed in rather than + reached for. That is the whole difference between this and a method, and it is what + lets the environment contract be checked without standing up a manager. + """ + return DatastoreModule(appwrite_file_manager_config=_partner_appwrite_config(model_path)) + + +def _partner_appwrite_config(model_path) -> AppwriteConfig: + """Env → ``AppwriteConfig`` for the partner bucket. Declared names only; the values + live in the environment and are never read into this repository's source.""" + appwrite_env.assert_env_declared( + appwrite_env.CONNECTION_ENV + appwrite_env.UNFAO_ENV, + store="unfao_bucket datastore", + ) + return AppwriteConfig( + path_manager=model_path, + endpoint=os.getenv("APPWRITE_ENDPOINT"), + project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), + credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), + auth_method="api_key", + cache_ttl_hours=24, + bucket_id=os.getenv("APPWRITE_UNFAO_BUCKET_ID"), + bucket_name=os.getenv("APPWRITE_UNFAO_BUCKET_NAME"), + collection_id=os.getenv("APPWRITE_UNFAO_COLLECTION_ID"), + collection_name=os.getenv("APPWRITE_UNFAO_COLLECTION_NAME"), + database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), + database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), + ) + + class UNFAOPostProcessorManager(PostprocessorManager, ForecastingModelManager): def __init__( self, @@ -90,7 +179,6 @@ def __init__( logger.info(f"Initializing {self.__class__.__name__}") self._forecast_resolution = None # {target: TargetLease}, set by _read self._historical_frame = None # views_frames.FeatureFrame, set by _read - self.ensemble_path_manager = None def _read_historical_frame(self): """#126: historical actuals as a views_frames.FeatureFrame — the first @@ -136,61 +224,6 @@ def _read_historical_data(self): self._initialize_data_loader() self._read_historical_frame() - def _prod_forecasts_datastore(self) -> DatastoreModule: - """The shared internal store (ADR-013's 'shared shelf'), configured from the - launcher-assembled environment (validated fail-loud; þing-01 #134 — no dotenv - is loaded here). - - pipeline-core's ``DatastoreModule.get_predictions_by_metadata`` injects an - automatic ``name == model_name`` filter on every lookup. The contract read - must **not** have it: ADR-013 artifacts are named - ``{run_id}__{target}__m{month}.arrow.parquet`` (never the bare ensemble - name), so an injected ``name == "rusty_bucket"`` matches nothing and also - clobbers the wire layer's own run-id / target / name filters. Suppressed - unconditionally below — the retired legacy reader was the only caller that - needed it on (#149).""" - ensemble_name = self.configs.get("ensemble", None) - if not ensemble_name: - err_msg = "Ensemble name must be provided in configs with the `ensemble` key for forecasting. Cannot proceed." - logger.error(err_msg) - raise ValueError(err_msg) - self.ensemble_path_manager = EnsemblePathManager(ensemble_name_or_path=ensemble_name, validate=False) - # ensemble_configs = EnsembleManager( - # ensemble_path=self.ensemble_path_manager, - # ).configs - - # loa = ensemble_configs.get("level", None) - loa = "pgm" - if not loa: - err_msg = "level must be defined in the ensemble configurations (e.g, pgm, cm). Cannot proceed." - logger.error(err_msg) - raise ValueError(err_msg) - - appwrite_env.assert_env_declared( - appwrite_env.CONNECTION_ENV + appwrite_env.PROD_FORECASTS_ENV, - store="production_forecasts datastore", - ) - appwrite_config = AppwriteConfig( - path_manager=self.ensemble_path_manager, - endpoint=os.getenv("APPWRITE_ENDPOINT"), - project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), - credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), - auth_method="api_key", - cache_ttl_hours=24, - bucket_id=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_ID"), - bucket_name=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_NAME"), - collection_id=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_ID"), - collection_name=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_NAME"), - database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), - database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), - ) - datastore = DatastoreModule(appwrite_file_manager_config=appwrite_config) - # Suppress the automatic name==model_name filter (see docstring). model_path - # is used by DatastoreModule only for that injection and for uploads; the - # contract read neither uploads nor performs any model-scoped lookup. - datastore.model_path = None - return datastore - def _read_forecast_data_contract(self): """ADR-013 contract inbound (epic #105; streaming since the run-0 OOM fix): RESOLVE the newest fully-manifested run — manifests + pinned shard @@ -199,7 +232,7 @@ def _read_forecast_data_contract(self): released). The `wire/` package owns the policy; this method only adapts the store (DIP) and declares the product facts (region curation + coverage expectations live in the lease, where frames exist).""" - port = _ContractStorePort(self._prod_forecasts_datastore()) + port = _ContractStorePort(_build_prod_forecasts_store(self.configs.get("ensemble"))) region = self.configs.get("region") self._forecast_resolution = source_selection.resolve_run( port, @@ -319,7 +352,7 @@ def _save_contract(self) -> dict: # file itself. lookup = gaul_lookup.load() upload_enabled = bool(self.configs.get("wire_upload_enabled", product.UPLOAD_ENABLED)) - store = _ContractStorePort(self._unfao_datastore()) if upload_enabled else None + store = _ContractStorePort(_build_partner_store(self._model_path)) if upload_enabled else None # The wire is partner-neutral (#153): the manager supplies FAO's product # facts explicitly rather than the mechanism reaching for them. summary = wire_sink.deliver_run( @@ -369,29 +402,6 @@ def _save_contract(self) -> dict: summary["historical"] = hist_path.name return summary - def _unfao_datastore(self) -> DatastoreModule: - """The FAO-facing store (`unfao_bucket`).""" - return DatastoreModule(appwrite_file_manager_config=self._unfao_appwrite_config()) - - def _unfao_appwrite_config(self) -> AppwriteConfig: - appwrite_env.assert_env_declared( - appwrite_env.CONNECTION_ENV + appwrite_env.UNFAO_ENV, store="unfao_bucket datastore" - ) - return AppwriteConfig( - path_manager=self._model_path, - endpoint=os.getenv("APPWRITE_ENDPOINT"), - project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), - credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), - auth_method="api_key", - cache_ttl_hours=24, - bucket_id=os.getenv("APPWRITE_UNFAO_BUCKET_ID"), - bucket_name=os.getenv("APPWRITE_UNFAO_BUCKET_NAME"), - collection_id=os.getenv("APPWRITE_UNFAO_COLLECTION_ID"), - collection_name=os.getenv("APPWRITE_UNFAO_COLLECTION_NAME"), - database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), - database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), - ) - def _save(self) -> dict: """Deliver the run — ADR-013 contract only (#149).""" return self._save_contract()