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()