From ab14545fe9f29c129dd6e2ee6598e2ebaac690a6 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Sun, 21 Jun 2026 11:59:26 +0200 Subject: [PATCH] feat(manager): ADR-003 E2 load-time stamp guard (D-18, #86) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unpickling a model artifact that predates the target-transform contract (no `_target_transform_name` stamp) previously proceeded silently, with an unknown prediction scale that cannot be safely inverted — the frozen-artifact ambiguity ADR-003 E2 was written to close. Add `StepshifterManager._guard_loaded_artifact`, called after each `pickle.load` in `_evaluate_model_artifact` and `_forecast_model_artifact` (placed after the FileNotFoundError handler so the guard-raise is not swallowed). A missing stamp raises the new `PreContractArtifactError`. The guard is presence-only, not value-policing: a legitimate artifact carrying a non-identity transform (e.g. `log1p`) loads fine. Tests: guard fires on a pre-contract artifact on both load paths and predict is never reached; a stamped artifact loads and predicts normally. Resolves D-18 — its last residual was exactly E2 (the gate `target_transform` key and the E1 characterization test already shipped). Register updated: D-18 moved to Resolved. Co-Authored-By: Claude Opus 4.8 (1M context) --- reports/technical_risk_register.md | 29 +++++------ tests/test_stepshifter_manager.py | 52 +++++++++++++++++++ .../infrastructure/exceptions.py | 12 +++++ .../manager/stepshifter_manager.py | 29 +++++++++++ 4 files changed, 107 insertions(+), 15 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 3e7a51b..26f4c52 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -1,9 +1,9 @@ # Technical Risk Register — views-stepshifter -**Last updated:** 2026-06-14 +**Last updated:** 2026-06-21 **Governing ADR:** Pending (will be added when base docs are adopted) **Total entries:** 35 -**Concerns:** Open 32 | Resolved 1 | Invalidated 2 +**Concerns:** Open 31 | Resolved 2 | Invalidated 2 > **ID convention:** This register uses the `D-xx` (Debt) prefix for all concern entries; there are no disagreement entries. IDs are permanent and sequential. @@ -141,19 +141,6 @@ --- -### D-18 — Raw-I/O contract is unfalsifiable in CI until guards E1/E2 and the gate key are implemented - -| Field | Value | -|---|---| -| **Tier** | 2 | -| **Trigger** | When a contributor reintroduces or adds an internal target transform to `StepshifterModel` before the ADR-003 characterization test (E1), load-time guard (E2), and the gate `target_transform` key are implemented. | -| **Source** | expert-review (2026-06-08) | -| **Status** | Partially resolved (2026-06-13) — gate key + E1-equivalent test shipped; **E2 load-time guard still Open** | -| **Location** | ADR-003 §"Enforcement required at ratification"; ~~`reproducibility_gate.py:24` (`CORE_GENOME` lacks `target_transform`)~~ → now `:25` (key present, validated steps 5–7); E1-equivalent test now in `tests/test_target_transforms.py`; **E2 still absent** (no load-time guard) | -| **Notes** | ADR-003 ratifies a raw-in/raw-out contract but is documentation-only until guards E1 (fit→predict characterization test asserting raw-space output) and E2 (load-time transform-name guard) ship and `target_transform` is added to the gate. Until then, a future silent-transform commit — a `5fcfe43` rhyme — passes CI green, which is exactly the regression that produced D-17. The fix is cheap (E1 needs no registry) and should not wait for the full declarative `TRANSFORMS` mechanism. See also D-09 (gate omits keys models dereference), D-10 (gate runs only on the train path), and D-13 (existing tests verify wiring, not numerical behavior). **Update (2026-06-13, repo-assimilation):** this entry's premise is now **substantially stale**. `target_transform` IS in `CORE_GENOME` (`reproducibility_gate.py:25`) and is validated against the closed registry (steps 5–7), and an **E1-equivalent characterization test** ships and is green (`tests/test_target_transforms.py::test_predict_routes_through_inverse_exactly_once_end_to_end`, plus the `_process_data`-applies-forward-once and identity-noop tests). A silent-transform commit no longer passes CI unflagged. **What remains Open:** the ADR-003 **E2 load-time guard** (fail-loud on a missing `_target_transform_name` stamp when unpickling a pre-contract artifact) is still unimplemented — this is the residual gap and the one still tied to D-17. The doc drift this staleness reflects (ADR-001/CIC under-describe the shipped contract) is tracked separately in **D-31**. | - ---- - ### D-19 — ADR-003 depends on a not-yet-ratified views-pipeline-core platform ADR (dangling `⟨PENDING⟩` reference) | Field | Value | @@ -464,3 +451,15 @@ | **Source** | Tech debt audit (2026-04-07); confirmed resolved by repo-assimilation (2026-06-08) | | **Status** | Resolved (2026-06-08) | | **Resolution** | The dependency is now declared: `pyproject.toml:18` reads `darts = "^0.40.0"` (uncommented), satisfying the five runtime import sites (`stepshifter.py:5`, `stepshifter.py:54,57` lazy, `hurdle_model.py:45` lazy, `darts_model.py:2`). The original entry flagged this as "likely already resolved" pending verification; verified on the 2026-06-08 assimilation audit. | + +--- + +### D-18 — Raw-I/O contract is unfalsifiable in CI until guards E1/E2 and the gate key are implemented — RESOLVED + +| Field | Value | +|---|---| +| **Tier** | 2 | +| **Original trigger** | When a contributor reintroduces or adds an internal target transform to `StepshifterModel` before the ADR-003 characterization test (E1), load-time guard (E2), and the gate `target_transform` key are implemented. | +| **Source** | expert-review (2026-06-08) | +| **Status** | Resolved (2026-06-21) — all three ADR-003 enforcement obligations now ship | +| **Resolution** | ADR-003 mandated three enforcement artifacts, all now present: (1) the gate `target_transform` key is in `CORE_GENOME` (`reproducibility_gate.py:25`), validated against the closed registry (steps 5–7); (2) the **E1** characterization test ships and is green (`tests/test_target_transforms.py::test_predict_routes_through_inverse_exactly_once_end_to_end`, plus `_process_data`-applies-forward-once and identity-noop tests); (3) the **E2** load-time guard now ships (Story A1, #86) — `StepshifterManager._guard_loaded_artifact` raises `PreContractArtifactError` after each `pickle.load` (`stepshifter_manager.py:29-49`, called at the evaluate/forecast load sites) when a loaded artifact lacks the `_target_transform_name` stamp, closing the frozen-pre-contract-artifact ambiguity that tied this to D-17. The guard is presence-only (a legit `log1p` artifact loads fine), per ADR-003 E2. The ADR-001/CIC doc-drift this entry once noted is tracked separately in **D-31** (Workstream C). | diff --git a/tests/test_stepshifter_manager.py b/tests/test_stepshifter_manager.py index 445428f..d931d21 100644 --- a/tests/test_stepshifter_manager.py +++ b/tests/test_stepshifter_manager.py @@ -4,6 +4,7 @@ from pathlib import Path from unittest.mock import MagicMock, patch, mock_open from views_stepshifter.manager.stepshifter_manager import StepshifterManager +from views_stepshifter.infrastructure.exceptions import PreContractArtifactError from views_pipeline_core.managers.model import ModelPathManager from views_pipeline_core.cli.args import ForecastingModelArgs @@ -354,6 +355,57 @@ def test_forecast_model_artifact(stepshifter_manager): assert path_artifact == Path("/path/to/artifacts/non_default_artifact.pkl") mock_logger.exception.assert_called_once_with(f"Model artifact not found at {path_artifact}") +def test_load_guard_rejects_pre_contract_artifact_on_evaluate(stepshifter_manager): + """ADR-003 E2: an artifact missing `_target_transform_name` (a pre-contract + artifact) has an unknown prediction scale and MUST fail loud on load — never be + silently assumed to be raw space.""" + # spec without `_target_transform_name` => hasattr(...) is False, unlike a bare MagicMock + pre_contract_model = MagicMock(spec=["predict"]) + with patch("builtins.open", mock_open(read_data=b"mocked_data")), \ + patch("pickle.load", MagicMock(return_value=pre_contract_model)): + + args = ForecastingModelArgs(run_type="test_run_type", evaluate=True, saved=True) + stepshifter_manager.configs = vars(args) + + with pytest.raises(PreContractArtifactError): + stepshifter_manager._evaluate_model_artifact("test_eval_type", None) + # the guard fires before prediction is attempted + pre_contract_model.predict.assert_not_called() + + +def test_load_guard_rejects_pre_contract_artifact_on_forecast(stepshifter_manager): + """Same E2 guard on the forecast path (`_forecast_model_artifact`).""" + pre_contract_model = MagicMock(spec=["predict"]) + with patch("builtins.open", mock_open(read_data=b"mocked_data")), \ + patch("pickle.load", MagicMock(return_value=pre_contract_model)): + + args = ForecastingModelArgs(run_type="forecasting", forecast=True, saved=True) + stepshifter_manager.configs = vars(args) + + with pytest.raises(PreContractArtifactError): + stepshifter_manager._forecast_model_artifact(None) + pre_contract_model.predict.assert_not_called() + + +def test_load_guard_accepts_stamped_artifact(stepshifter_manager): + """The guard checks for the *presence* of the stamp, not its value: a legitimate + artifact carrying a non-identity stamp (e.g. `log1p`) loads and predicts normally.""" + stamped_model = MagicMock(spec=["predict", "_target_transform_name"]) + stamped_model._target_transform_name = "log1p" + stamped_model.predict.return_value = ["mock_df"] + with patch("builtins.open", mock_open(read_data=b"mocked_data")), \ + patch("pickle.load", MagicMock(return_value=stamped_model)), \ + patch.object(StepshifterManager, "_get_standardized_df", return_value="standardized_df"): + + args = ForecastingModelArgs(run_type="forecasting", forecast=True, saved=True) + stepshifter_manager.configs = vars(args) + + result = stepshifter_manager._forecast_model_artifact(None) + + stamped_model.predict.assert_called_once_with("forecasting") + assert result == "standardized_df" + + def test_evaluate_sweep(stepshifter_manager): """ Test the _evaluate_sweep method. diff --git a/views_stepshifter/infrastructure/exceptions.py b/views_stepshifter/infrastructure/exceptions.py index ba3eee8..fbd1bc4 100644 --- a/views_stepshifter/infrastructure/exceptions.py +++ b/views_stepshifter/infrastructure/exceptions.py @@ -8,3 +8,15 @@ class MissingHyperparameterError(ReproducibilityError): """Raised when a mandatory hyperparameter is missing from the config.""" pass + + +class PreContractArtifactError(ReproducibilityError): + """Raised when a loaded model artifact predates the target-transform contract. + + Per ADR-003 E2 (raw-target-space I/O contract): an artifact missing the + ``_target_transform_name`` stamp has an unknown prediction scale that cannot + be safely inverted, so it must be rejected loudly at load time rather than + silently assumed to be raw space. + """ + + pass diff --git a/views_stepshifter/manager/stepshifter_manager.py b/views_stepshifter/manager/stepshifter_manager.py index fcc12fa..610f355 100644 --- a/views_stepshifter/manager/stepshifter_manager.py +++ b/views_stepshifter/manager/stepshifter_manager.py @@ -4,6 +4,7 @@ from views_stepshifter.models.hurdle_model import HurdleModel from views_stepshifter.models.shurf_model import ShurfModel from views_stepshifter.infrastructure.reproducibility_gate import ReproducibilityGate +from views_stepshifter.infrastructure.exceptions import PreContractArtifactError import logging import pickle import pandas as pd @@ -24,6 +25,30 @@ def __init__( self._is_hurdle = self._config_meta["algorithm"] == "HurdleModel" self._is_shurf = self._config_meta["algorithm"] == "ShurfModel" + @staticmethod + def _guard_loaded_artifact(model: Any, path_artifact) -> None: + """ADR-003 E2 load-time guard. + + A loaded artifact must declare the target-transform space it predicts in. + A pre-contract artifact (missing ``_target_transform_name``) has an unknown + prediction scale that cannot be safely inverted, so it is rejected loudly + rather than silently assumed to be raw space. + + Note: the guard checks for the *presence* of the stamp, not its value — a + legitimate plain-model artifact may carry a non-identity transform (e.g. + ``log1p``) and must load fine. + """ + if not hasattr(model, "_target_transform_name"): + msg = ( + f"Model artifact at {path_artifact} predates the target-transform " + f"contract (ADR-003 E2): it carries no `_target_transform_name` stamp, " + f"so its prediction space is unknown and cannot be safely inverted. " + f"Re-train and re-save the artifact under the current contract before " + f"evaluating or forecasting." + ) + logger.error(msg) + raise PreContractArtifactError(msg) + @staticmethod def _get_standardized_df(df: pd.DataFrame) -> pd.DataFrame: """ @@ -166,6 +191,8 @@ def _evaluate_model_artifact( logger.exception(f"Model artifact not found at {path_artifact}") raise + StepshifterManager._guard_loaded_artifact(stepshift_model, path_artifact) + df_predictions = stepshift_model.predict(run_type, eval_type) df_predictions = [ StepshifterManager._get_standardized_df(df) for df in df_predictions @@ -209,6 +236,8 @@ def _forecast_model_artifact(self, artifact_name: str) -> pd.DataFrame: logger.exception(f"Model artifact not found at {path_artifact}") raise + StepshifterManager._guard_loaded_artifact(stepshift_model, path_artifact) + df_prediction = stepshift_model.predict(run_type) df_prediction = StepshifterManager._get_standardized_df(df_prediction)