From 5f83c237edf625bbc157158e1f3f55fa4c25ce99 Mon Sep 17 00:00:00 2001 From: Johannes Ott Date: Mon, 10 Aug 2026 23:43:07 +0200 Subject: [PATCH 1/4] Reduce model invalidation to a single version: the pvlearn release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sidecar compared three version fields — feature_schema_version, pipeline_version and sklearn_version — while pvlearn_version was recorded but excluded. That split was both redundant and incomplete. Redundant: runtime dependencies are pinned with ==, so neither a dependency, nor the schema, nor the pipeline can change without a pvlearn release. Every event the three fields detect is already a new release. Incomplete, which matters more: the three cover scikit-learn but not numpy or joblib, and the persisted artefact depends on those too — joblib's pickle path is numpy-version-sensitive, visible today as a DeprecationWarning from joblib/numpy_pickle.py. The claim that numpy/pandas/scipy do not affect reproduction turned out to rest on a single manual run recorded only as prose in a comment; no CI job varies a dependency version, so it cannot fail if it stops being true. The pyproject comment is narrowed to what is actually tested. Only the release segment is compared, so a model survives the dev builds between two releases: setuptools_scm uses local_scheme = "node-and-date", and comparing the full string would invalidate every locally trained model on every commit and again on every new day. Reasoning and the alternatives in docs/adr/0003. Signed-off-by: Johannes Ott Co-Authored-By: Claude Opus 5 Signed-off-by: Johannes Ott --- AGENTS.md | 11 +-- ...e-aware-chronological-feature-selection.md | 1 + ...one-version-decides-model-compatibility.md | 90 +++++++++++++++++++ pvlearn/metadata.py | 61 ++++++------- pvlearn/schema.py | 6 +- pyproject.toml | 10 +-- tests/test_forecaster.py | 21 +---- tests/test_metadata.py | 78 ++++++++-------- 8 files changed, 178 insertions(+), 100 deletions(-) create mode 100644 docs/adr/0003-one-version-decides-model-compatibility.md diff --git a/AGENTS.md b/AGENTS.md index 16e6872..865bd13 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -42,7 +42,7 @@ The weather feature schema, time/sun-position features, target variable, and mod invalidation rules are specified in `pvlearn/schema.py` and `pvlearn/metadata.py` — the code is the canonical spec, there is no separate document restating it. Do not invent field names or diverge from that schema — every trained model becomes invalid if it changes, so a change there -is a deliberate, versioned decision (`feature_schema_version`), not a normal refactor. +is a deliberate decision carried by the next release, not a normal refactor (ADR 0003). ### Decisions @@ -109,10 +109,11 @@ the reasoning that survives into `main`'s history. - **Custom sklearn transformers** (encoders, selectors) must support `sklearn.clone()` and pickling — constructor arguments must be primitive and serializable, not settings objects. - **Model persistence:** joblib/pickle with a metadata sidecar, see `pvlearn/metadata.py`. Loading - a model whose `feature_schema_version`, `pipeline_version`, sklearn minor version, interval, or - location does not match current config means hard rejection and retraining — never a - best-effort load. The weather provider is not part of that check: it is a per-row categorical - feature in the training data now, not a training-run setting. + a model whose pvlearn release, interval, or location does not match current config means hard + rejection and retraining — never a best-effort load. The release is the only version compared, + and it covers schema, pipeline and dependency changes alike (ADR 0003). The weather provider is + not part of the check: it is a per-row categorical feature in the training data now, not a + training-run setting. ### Testing diff --git a/docs/adr/0002-noise-aware-chronological-feature-selection.md b/docs/adr/0002-noise-aware-chronological-feature-selection.md index 658b2b2..6f8b756 100644 --- a/docs/adr/0002-noise-aware-chronological-feature-selection.md +++ b/docs/adr/0002-noise-aware-chronological-feature-selection.md @@ -4,6 +4,7 @@ - **Date:** 2026-08-06 - **Phase:** 1c (feature-selection correction) - **Supersedes the threshold decided in:** [ADR 0001](0001-feature-selection-threshold.md) +- **Its `pipeline_version` mechanism is superseded by:** [ADR 0003](0003-one-version-decides-model-compatibility.md). The selection decision below stands. ## Context diff --git a/docs/adr/0003-one-version-decides-model-compatibility.md b/docs/adr/0003-one-version-decides-model-compatibility.md new file mode 100644 index 0000000..6178d8f --- /dev/null +++ b/docs/adr/0003-one-version-decides-model-compatibility.md @@ -0,0 +1,90 @@ +# 3. The pvlearn release alone decides model compatibility + +- **Status:** accepted +- **Date:** 2026-08-10 +- **Supersedes the versioning mechanism introduced in:** [ADR 0002](0002-noise-aware-chronological-feature-selection.md) + +## Context + +`ModelMetadata.raise_on_mismatch` compared three version fields — the feature +vocabulary (`feature_schema_version`), the shape of the pipeline +(`pipeline_version`), and the installed scikit-learn minor version — while +`pvlearn_version` was recorded but deliberately excluded, on the grounds that +"not every release changes how features are built". + +Two problems with that split. + +**It is redundant.** Runtime dependencies are pinned with `==`, so no dependency +can move without a pvlearn release, and neither the schema nor the pipeline can +change without one either. Every event the three fields detect is therefore +already a new release. The fields are a strictly finer partition of something +the release number alone separates. + +**It is also incomplete**, which matters more. The three fields cover +scikit-learn but not numpy, joblib, or pydantic, and the persisted artefact +depends on those too: joblib's pickle path is numpy-version-sensitive, visible +today as a `DeprecationWarning` from `joblib/numpy_pickle.py` about NumPy 2.5 +changing how an array's shape may be set. A model pickled under one numpy and +unpickled under another is exactly the silent-failure case the sidecar exists to +prevent, and the three fields do not see it. + +The claim that numpy, pandas and scipy do not affect reproduction — recorded in +a `pyproject.toml` comment and in the message of commit `ad94aac` — is not +backed by anything executable. There is no dependency matrix in CI: the two jobs +differ only in Python version, and because dependencies are pinned exactly, both +install identical numpy, pandas, scipy and scikit-learn. No script in the +repository reproduces the check. It rests on a single manual run recorded only +as prose, and cannot fail if it stops being true. + +## Decision + +Compare one version: the release segment of `pvlearn_version`. +`feature_schema_version`, `pipeline_version` and `sklearn_version` are removed +from the sidecar, along with `FEATURE_SCHEMA_VERSION`, `PIPELINE_VERSION` and +`sklearn_minor_version()`. + +Only the release segment takes part — `0.3.0` out of +`0.3.0.post2+gd22c402c0.d20260807`. `setuptools_scm` is configured with +`local_scheme = "node-and-date"`, so the full version string carries the commit +hash and, on a dirty tree, the date. Comparing it whole would invalidate every +locally trained model on every commit and again on every new day. + +This is the conservative direction. The release is a superset trigger: it never +invalidates too little, only sometimes too much. + +## Consequences + +A release that changes neither schema, pipeline, nor dependencies — a +documentation or CI release — now invalidates every persisted model. That cost +is real and accepted: training needs 60 hours of history the caller already +holds, hyperparameter tuning is opt-in and off by default, and the alternative +is the known gap above. The project's stated order of preference is unchanged — +a needless retrain is cheap, a silently wrong forecast is not. + +Between two releases the version does not move, so a developer who changes the +pipeline locally keeps loading the model trained before the change. Deleting the +persisted pair is the remedy; this is a development-time concern only, since +released versions differ by construction. + +Every persisted model is invalidated once more by this change itself, but +cleanly: pydantic ignores unknown fields, so a sidecar carrying the three +removed ones still parses and is then rejected on its release with a precise +reason, rather than being reported as unreadable metadata. + +## Alternatives considered + +**Keeping the three fields and adding more.** Fields for numpy and joblib would +close the specific gap named above. Rejected because it treats the symptom: the +list is only ever as complete as the last audit of which dependency touches the +artefact, and each addition is another thing to remember to bump. The release +number needs no audit — it moves whenever anything ships. + +**Keeping the three fields as they are.** The status quo optimises for avoiding +unnecessary retrains, at the price of a gap it cannot close without becoming the +alternative above. Rejected because the thing being optimised — retraining cost +— is the cheap side of the trade. + +**Comparing the full version string.** Strictly the most conservative option and +the simplest to implement. Rejected for the `local_scheme` reason above: it +makes every local model unusable after any commit, which trains developers to +work around the check rather than trust it. diff --git a/pvlearn/metadata.py b/pvlearn/metadata.py index 2211510..ac9ca7f 100644 --- a/pvlearn/metadata.py +++ b/pvlearn/metadata.py @@ -1,32 +1,30 @@ +import re +from collections.abc import Callable from datetime import datetime +from typing import Any -import sklearn from pydantic import BaseModel from pvlearn import __version__ from pvlearn.config import ForecasterConfig from pvlearn.exceptions import SchemaMismatchError from pvlearn.location import Location -from pvlearn.schema import FEATURE_SCHEMA_VERSION -#: Version of how the training pipeline is built — preprocessing steps, the -#: feature-selection rule and the estimator. Distinct from -#: `FEATURE_SCHEMA_VERSION`, which versions the feature vocabulary in -#: `pvlearn.schema`: the same columns can be assembled into a different model. -#: Bump it in the same commit as any change to `Forecaster._prepare_model_pipeline` -#: or to `PFISelector`. -PIPELINE_VERSION = 2 +#: Leading release segment of a PEP 440 version — `0.3.0` out of +#: `0.3.0.post3.dev1+g527ccef`. +_RELEASE_SEGMENT = re.compile(r"^\d+(?:\.\d+)*") -def sklearn_minor_version() -> str: - """The `major.minor` part of the installed scikit-learn version. +def release_version(version: str = __version__) -> str: + """The release part of a version, without pre/post/dev/local segments. - Patch releases do not change model behaviour, minor ones can — and a model - unpickled across a minor bump either warns or predicts differently, both of - which are worse than retraining. + See ADR 0003 for why the release alone decides model compatibility. """ - major, minor = sklearn.__version__.split(".")[:2] - return f"{major}.{minor}" + match = _RELEASE_SEGMENT.match(version) + if match is None: + raise ValueError(f"Version {version!r} does not start with a release segment") + + return match.group() class ModelMetrics(BaseModel): @@ -47,15 +45,11 @@ class ModelMetadata(BaseModel): is deliberately not part of this: it is the `weather_provider` categorical feature in `pvlearn.schema` now, a per-row fact the model can learn from rather than a setting the whole model is pinned to. + + The pvlearn release is the single version this compares — see ADR 0003. """ pvlearn_version: str - feature_schema_version: int - #: Defaulted so that sidecars written before this field existed are read as - #: version 1 — which is what they are — and rejected with a precise reason - #: rather than as unreadable metadata. - pipeline_version: int = 1 - sklearn_version: str interval_minutes: int location: Location trained_at: datetime @@ -75,9 +69,6 @@ def create( ) -> "ModelMetadata": return cls( pvlearn_version=__version__, - feature_schema_version=FEATURE_SCHEMA_VERSION, - pipeline_version=PIPELINE_VERSION, - sklearn_version=sklearn_minor_version(), interval_minutes=config.interval_minutes, location=location, trained_at=trained_at or datetime.now().astimezone(), @@ -89,14 +80,13 @@ def create( def raise_on_mismatch(self, location: Location, config: ForecasterConfig) -> None: """Reject the model unless it was trained under the given setup. - `pvlearn_version` deliberately does not take part: not every release - changes how features are built, and the parts that do are covered by - `feature_schema_version` and `pipeline_version`. + Only the release segment of `pvlearn_version` takes part, so a model + survives the dev builds between two releases — see ADR 0003. """ mismatches = [ - self._compare("feature_schema_version", FEATURE_SCHEMA_VERSION), - self._compare("pipeline_version", PIPELINE_VERSION), - self._compare("sklearn_version", sklearn_minor_version()), + self._compare( + "pvlearn_version", release_version(), transform=release_version + ), self._compare("interval_minutes", config.interval_minutes), self._compare("location", location), ] @@ -108,9 +98,14 @@ def raise_on_mismatch(self, location: Location, config: ForecasterConfig) -> Non f"and has to be retrained: {'; '.join(reasons)}" ) - def _compare(self, attribute: str, expected: object) -> str | None: + def _compare( + self, + attribute: str, + expected: object, + transform: Callable[[Any], Any] = lambda value: value, + ) -> str | None: persisted = getattr(self, attribute) - if persisted == expected: + if transform(persisted) == expected: return None return f"{attribute} is {persisted!r}, expected {expected!r}" diff --git a/pvlearn/schema.py b/pvlearn/schema.py index 9452f73..13405a3 100644 --- a/pvlearn/schema.py +++ b/pvlearn/schema.py @@ -5,12 +5,10 @@ optional — a provider that has no UV index simply yields a smaller feature set, never an error. -Changing anything here invalidates every persisted model, which is what -`FEATURE_SCHEMA_VERSION` exists for. Bump it in the same commit as the change. +Changing anything here invalidates every persisted model. The pvlearn release +in the model metadata carries that — see ADR 0003. """ -FEATURE_SCHEMA_VERSION = 2 - #: Column holding the timezone-aware timestamp of an interval's start. TIME_FEATURE = "time" diff --git a/pyproject.toml b/pyproject.toml index fd41808..cbff019 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,11 +25,11 @@ classifiers = [ # they shape how the wheel is built, not how the installed package behaves, and # pinning them breaks builds on newer setuptools for no reproducibility gain. # -# scikit-learn is the one pin that is load-bearing rather than merely tidy: the -# frozen Phase 0 baseline is only reproducible against 1.9.0. Verified that the -# baseline reproduces bit-identically across numpy 2.4.6/2.5.1, pandas -# 3.0.3/3.0.5 and scipy 1.17.1/1.18.0 as long as scikit-learn does not move, so -# a scikit-learn bump must regenerate the baseline and say so in its changelog. +# scikit-learn is load-bearing rather than merely tidy: the frozen Phase 0 +# baseline is only reproducible against 1.9.0, so a scikit-learn bump must +# regenerate the baseline and say so in its changelog. Whether the other pins +# are equally load-bearing is untested — nothing in CI varies a dependency +# version, so treat any bump as capable of moving the baseline until measured. # # numpy 2.5 and scipy 1.18 require Python 3.12, which is why 3.11 is not # supported. diff --git a/tests/test_forecaster.py b/tests/test_forecaster.py index 4d507f3..95ec135 100644 --- a/tests/test_forecaster.py +++ b/tests/test_forecaster.py @@ -491,32 +491,17 @@ def test_save_leaves_the_previous_pair_intact_when_it_fails(self, tmp_path): assert restored.metadata == first.metadata assert not list(tmp_path.glob("*.tmp")) - def test_load_rejects_an_outdated_feature_schema(self, tmp_path): + def test_load_rejects_a_model_from_an_older_release(self, tmp_path): forecaster = Forecaster(make_location(), make_config()) forecaster.train(make_training_data()) forecaster.save(tmp_path) metadata_path = tmp_path / METADATA_FILENAME metadata = json.loads(metadata_path.read_text()) - metadata["feature_schema_version"] = 0 + metadata["pvlearn_version"] = "0.0.1" metadata_path.write_text(json.dumps(metadata)) - with pytest.raises(SchemaMismatchError, match="feature_schema_version"): - Forecaster.load(tmp_path, make_location(), make_config()) - - def test_load_rejects_a_model_from_an_older_pipeline(self, tmp_path): - """A sidecar written before `pipeline_version` existed reads as 1 and - has to be retrained, not loaded.""" - forecaster = Forecaster(make_location(), make_config()) - forecaster.train(make_training_data()) - forecaster.save(tmp_path) - - metadata_path = tmp_path / METADATA_FILENAME - metadata = json.loads(metadata_path.read_text()) - del metadata["pipeline_version"] - metadata_path.write_text(json.dumps(metadata)) - - with pytest.raises(SchemaMismatchError, match="pipeline_version is 1"): + with pytest.raises(SchemaMismatchError, match="pvlearn_version"): Forecaster.load(tmp_path, make_location(), make_config()) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index b5ed3f0..2871533 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -1,19 +1,12 @@ from datetime import datetime import pytest -import sklearn from pvlearn import __version__ from pvlearn.config import ForecasterConfig from pvlearn.exceptions import SchemaMismatchError from pvlearn.location import Location -from pvlearn.metadata import ( - PIPELINE_VERSION, - ModelMetadata, - ModelMetrics, - sklearn_minor_version, -) -from pvlearn.schema import FEATURE_SCHEMA_VERSION +from pvlearn.metadata import ModelMetadata, ModelMetrics, release_version def make_location(**overrides) -> Location: @@ -46,10 +39,24 @@ def make_metadata(**overrides) -> ModelMetadata: return metadata.model_copy(update=overrides) -def test_sklearn_minor_version_drops_the_patch_level(): - major, minor = sklearn.__version__.split(".")[:2] +@pytest.mark.parametrize( + ("version", "expected"), + [ + ("0.3.0", "0.3.0"), + ("0.3.0.post3+g527ccef", "0.3.0"), + ("0.3.0.post3.dev1+g527ccef.d20260810", "0.3.0"), + ("1.0.0rc1", "1.0.0"), + ("1.0.0.dev0", "1.0.0"), + ("2", "2"), + ], +) +def test_release_version_keeps_only_the_release_segment(version: str, expected: str): + assert release_version(version) == expected + - assert sklearn_minor_version() == f"{major}.{minor}" +def test_release_version_rejects_a_version_without_a_release_segment(): + with pytest.raises(ValueError, match="does not start with a release segment"): + release_version("unknown") class TestCreate: @@ -57,9 +64,6 @@ def test_stamps_the_current_environment(self): metadata = make_metadata() assert metadata.pvlearn_version == __version__ - assert metadata.feature_schema_version == FEATURE_SCHEMA_VERSION - assert metadata.pipeline_version == PIPELINE_VERSION - assert metadata.sklearn_version == sklearn_minor_version() assert metadata.interval_minutes == 60 assert metadata.training_rows == 1440 @@ -85,23 +89,19 @@ class TestRaiseOnMismatch: def test_accepts_the_setup_it_was_created_from(self): make_metadata().raise_on_mismatch(make_location(), make_config()) - def test_rejects_a_different_feature_schema(self): - metadata = make_metadata(feature_schema_version=FEATURE_SCHEMA_VERSION + 1) - - with pytest.raises(SchemaMismatchError, match="feature_schema_version"): - metadata.raise_on_mismatch(make_location(), make_config()) - - def test_rejects_a_different_pipeline_version(self): - metadata = make_metadata(pipeline_version=PIPELINE_VERSION - 1) + def test_rejects_a_model_from_another_release(self): + metadata = make_metadata(pvlearn_version="0.0.1") - with pytest.raises(SchemaMismatchError, match="pipeline_version"): + with pytest.raises(SchemaMismatchError, match="pvlearn_version"): metadata.raise_on_mismatch(make_location(), make_config()) - def test_rejects_a_different_sklearn_minor_version(self): - metadata = make_metadata(sklearn_version="0.1") + def test_accepts_a_dev_build_of_the_same_release(self): + """Commits between two releases must not invalidate a model.""" + metadata = make_metadata( + pvlearn_version=f"{release_version()}.post3.dev1+g527ccef" + ) - with pytest.raises(SchemaMismatchError, match="sklearn_version"): - metadata.raise_on_mismatch(make_location(), make_config()) + metadata.raise_on_mismatch(make_location(), make_config()) def test_rejects_a_different_interval(self): metadata = make_metadata(interval_minutes=15) @@ -123,20 +123,14 @@ def test_rejects_a_different_timezone_at_the_same_coordinates(self): ) def test_reports_every_mismatch_at_once(self): - metadata = make_metadata(interval_minutes=15, sklearn_version="0.1") + metadata = make_metadata(interval_minutes=15, pvlearn_version="0.0.1") with pytest.raises(SchemaMismatchError) as error: metadata.raise_on_mismatch(make_location(), make_config()) message = str(error.value) assert "interval_minutes" in message - assert "sklearn_version" in message - - def test_ignores_the_pvlearn_version(self): - """Not every release changes how features are built.""" - metadata = make_metadata(pvlearn_version="0.0.1") - - metadata.raise_on_mismatch(make_location(), make_config()) + assert "pvlearn_version" in message class TestSerialization: @@ -146,3 +140,17 @@ def test_round_trips_through_json(self): restored = ModelMetadata.model_validate_json(metadata.model_dump_json()) assert restored == metadata + + def test_a_sidecar_from_before_adr_0003_is_a_mismatch_not_unreadable(self): + """The dropped version fields must degrade into a precise reason.""" + sidecar = make_metadata(pvlearn_version="0.3.0").model_dump(mode="json") + sidecar |= { + "feature_schema_version": 2, + "pipeline_version": 2, + "sklearn_version": "1.9", + } + + restored = ModelMetadata.model_validate(sidecar) + + with pytest.raises(SchemaMismatchError, match="pvlearn_version is '0.3.0'"): + restored.raise_on_mismatch(make_location(), make_config()) From 8178bacc43fb3dc3d7b17ea537de0ba81afacf27 Mon Sep 17 00:00:00 2001 From: Johannes Ott Date: Tue, 11 Aug 2026 08:31:05 +0200 Subject: [PATCH 2/4] Stop pinning a sidecar test to a concrete release number test_a_sidecar_from_before_adr_0003_is_a_mismatch_not_unreadable built its sidecar with pvlearn_version="0.3.0" and asserted a mismatch. That holds only while the release under test is something other than 0.3.0: it passed locally against a stale editable install (0.2.0.post2) and failed in CI, where setuptools_scm resolves the v0.3.0 tag and the two sides agree. 0.0.1 is below every published release, so the assertion no longer depends on which version the test run was built from and needs no upkeep per release. The sibling test derives its version from release_version() and already moved along on its own. Signed-off-by: Johannes Ott Co-Authored-By: Claude Opus 5 Signed-off-by: Johannes Ott --- tests/test_metadata.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 2871533..3773619 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -142,8 +142,12 @@ def test_round_trips_through_json(self): assert restored == metadata def test_a_sidecar_from_before_adr_0003_is_a_mismatch_not_unreadable(self): - """The dropped version fields must degrade into a precise reason.""" - sidecar = make_metadata(pvlearn_version="0.3.0").model_dump(mode="json") + """The dropped version fields must degrade into a precise reason. + + `0.0.1` is below every published release, so this stays a mismatch + whatever version the test run itself was built from. + """ + sidecar = make_metadata(pvlearn_version="0.0.1").model_dump(mode="json") sidecar |= { "feature_schema_version": 2, "pipeline_version": 2, @@ -152,5 +156,5 @@ def test_a_sidecar_from_before_adr_0003_is_a_mismatch_not_unreadable(self): restored = ModelMetadata.model_validate(sidecar) - with pytest.raises(SchemaMismatchError, match="pvlearn_version is '0.3.0'"): + with pytest.raises(SchemaMismatchError, match="pvlearn_version is '0.0.1'"): restored.raise_on_mismatch(make_location(), make_config()) From 4594600f7ebcc4c8f03253a3b05435245d9122b0 Mon Sep 17 00:00:00 2001 From: Johannes Ott Date: Tue, 11 Aug 2026 17:59:23 +0200 Subject: [PATCH 3/4] State only what is measured about dependency reproducibility The comment calling scikit-learn's pin load-bearing claimed the frozen Phase 0 baseline "is only reproducible against 1.9.0". Same provenance as the numpy/pandas/scipy claim already narrowed in this branch -- prose in commit ad94aac, nothing executable behind it -- and this one is also contradicted by the project's own Phase 1a measurement, recorded in tests/test_extraction_regression.py: the baseline does not reproduce bit-for-bit across CPUs even with every version held fixed, with CI landing 5.26% off the MAE. The binding variable that was actually measured is the machine, not scikit-learn. test_records_the_versions_it_depends_on looks like it guards the claim but asserts only that the frozen sidecar records 1.9.0; it would pass unchanged if a newer scikit-learn reproduced the baseline perfectly. The real guard is the tolerance comparison in test_extraction_regression.py, which does run in CI -- the slow marker is declared but nothing deselects it. Also drops the references to chapters of the deleted Umsetzungsplan that the previous cleanup missed by grepping for the document name rather than "chapter". One of them, in freeze_baseline_forecast.py, pointed at the chapter requiring sklearn_version in the model metadata, which ADR 0003 has since removed. Signed-off-by: Johannes Ott Co-Authored-By: Claude Opus 5 Signed-off-by: Johannes Ott --- pvlearn/config.py | 2 +- pvlearn/forecaster.py | 3 +-- pvlearn/result.py | 4 ++-- pyproject.toml | 13 ++++++++----- scripts/freeze_baseline_forecast.py | 17 ++++++++++------- tests/conftest.py | 2 +- tests/test_baseline_forecast.py | 9 +++++---- tests/test_extraction_regression.py | 7 +++---- tests/test_result.py | 2 +- 9 files changed, 32 insertions(+), 27 deletions(-) diff --git a/pvlearn/config.py b/pvlearn/config.py index 6135f5b..ea6fd70 100644 --- a/pvlearn/config.py +++ b/pvlearn/config.py @@ -2,7 +2,7 @@ #: Interval lengths the library trains and predicts on today. The field exists #: so no code path silently assumes an hour; finer resolutions need measurement -#: data and a provider that delivers them natively (chapter 3.5). +#: data and a provider that delivers them natively. SUPPORTED_INTERVAL_MINUTES: tuple[int, ...] = (60,) diff --git a/pvlearn/forecaster.py b/pvlearn/forecaster.py index 1c024ee..6114556 100644 --- a/pvlearn/forecaster.py +++ b/pvlearn/forecaster.py @@ -59,8 +59,7 @@ class Forecaster: There is exactly one model. The former power model trained on identical features and, at hourly resolution, produced the same number in a different - unit — on the reference dataset the two were less than a percent apart - (chapter 3.3 of the Umsetzungsplan). + unit — on the reference dataset the two were less than a percent apart. """ #: Splits used to carve a holdout out of the training data for the metrics diff --git a/pvlearn/result.py b/pvlearn/result.py index 169c1f0..bfb0374 100644 --- a/pvlearn/result.py +++ b/pvlearn/result.py @@ -52,8 +52,8 @@ class ForecastResult(BaseModel): next hour is the one that actually follows. `interval_minutes` does not drive any of that — the timestamps do. It is - carried because the interval belongs in the forecast response (chapter 3.5) - and a consumer should not have to infer it from key spacing. + carried because the interval belongs in the forecast response and a + consumer should not have to infer it from key spacing. """ interval_minutes: int diff --git a/pyproject.toml b/pyproject.toml index cbff019..06e5eb8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,11 +25,14 @@ classifiers = [ # they shape how the wheel is built, not how the installed package behaves, and # pinning them breaks builds on newer setuptools for no reproducibility gain. # -# scikit-learn is load-bearing rather than merely tidy: the frozen Phase 0 -# baseline is only reproducible against 1.9.0, so a scikit-learn bump must -# regenerate the baseline and say so in its changelog. Whether the other pins -# are equally load-bearing is untested — nothing in CI varies a dependency -# version, so treat any bump as capable of moving the baseline until measured. +# Which of these pins actually moves a forecast is not known: nothing in CI +# varies a dependency version, and the pins themselves make it unmeasurable. +# What is measured is narrower — the frozen baseline does not reproduce +# bit-for-bit across CPUs even with every version held fixed, so +# tests/test_extraction_regression.py compares quality within a tolerance +# rather than the raw numbers. That test is the only guard a bump has to pass; +# treat any bump as capable of moving the baseline until it is measured +# otherwise. # # numpy 2.5 and scipy 1.18 require Python 3.12, which is why 3.11 is not # supported. diff --git a/scripts/freeze_baseline_forecast.py b/scripts/freeze_baseline_forecast.py index 98d86f4..5dc624d 100644 --- a/scripts/freeze_baseline_forecast.py +++ b/scripts/freeze_baseline_forecast.py @@ -21,11 +21,13 @@ --solaredge2mqtt /path/to/solaredge2mqtt \\ --out tests/fixtures/baseline_forecast -Determinism: training is reproducible because `HistGradientBoostingRegressor`, -`train_test_split` and `permutation_importance` all pin `random_state=42`. It is -*not* reproducible across scikit-learn versions, which is why the versions are -recorded alongside the predictions and why chapter 3.4 makes them part of the -model metadata. +Determinism: training pins `random_state=42` in +`HistGradientBoostingRegressor`, `train_test_split` and +`permutation_importance`, which makes a rerun on the same machine and the same +versions reproducible. It does not survive a different CPU — see +`tests/test_extraction_regression.py`. Whether a different scikit-learn also +breaks it has never been measured; the versions are recorded alongside the +predictions so that a future divergence can at least be attributed. """ from __future__ import annotations @@ -76,8 +78,9 @@ def assert_process_timezone_matches(dataset_timezone: str) -> None: solaredge2mqtt reads the timezone once at import time (`encoders.py`: `LOCAL_TZ = get_localzone_name()`) and SunEncoder builds its location from it. Baselines frozen under a different process timezone are - silently not comparable. Removing this global is chapter 3.2 of the roadmap; - until then it has to be asserted. + silently not comparable. pvlearn itself has no such global — `SunEncoder` + takes the timezone as a parameter — but the frozen baseline predates that, + so until it is regenerated this has to be asserted. """ from tzlocal import get_localzone_name # pyright: ignore[reportMissingImports] diff --git a/tests/conftest.py b/tests/conftest.py index c9ed3f5..95ca839 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,7 +11,7 @@ BASELINE_FORECAST = FIXTURES_DIR / "baseline_forecast.parquet" BASELINE_METADATA = FIXTURES_DIR / "baseline_forecast.json" -#: OpenWeatherMap One Call field names to the canonical schema of chapter 3.1. +#: OpenWeatherMap One Call field names to the canonical schema in `pvlearn.schema`. #: The real adapter lives in the consuming application (solaredge2mqtt today, #: learninghouse in the future); this one only exists to feed the frozen #: reference export, which was recorded before the schema was canonical, to the diff --git a/tests/test_baseline_forecast.py b/tests/test_baseline_forecast.py index 9a70e5c..12afb6a 100644 --- a/tests/test_baseline_forecast.py +++ b/tests/test_baseline_forecast.py @@ -21,9 +21,10 @@ "power_published", } -# The baseline is only reproducible against the scikit-learn version that produced -# it; a different one silently shifts every prediction. Chapter 3.4 makes this part -# of the persisted model metadata for the same reason. +# The version the frozen artifact was produced under. Pinning it here keeps the +# artifact and the pin in pyproject.toml from drifting apart unnoticed; it says +# nothing about whether another version would reproduce the baseline, which has +# never been measured. BASELINE_SKLEARN_VERSION = "1.9.0" WH_PER_KWH = 1000 @@ -98,7 +99,7 @@ def test_energy_model_explains_most_variance(self, baseline_metadata: dict): def test_energy_and_power_metrics_agree(self, baseline_metadata: dict): """At hourly resolution mean power in W equals energy in Wh. - Chapter 3.3 drops the power model on exactly this basis, so the two MAEs + The power model was dropped on exactly this basis, so the two MAEs should stay within a few percent of one another. """ energy_mae = baseline_metadata["metrics"]["energy"]["mae"] diff --git a/tests/test_extraction_regression.py b/tests/test_extraction_regression.py index 16a4cfe..8e2c020 100644 --- a/tests/test_extraction_regression.py +++ b/tests/test_extraction_regression.py @@ -1,9 +1,8 @@ """Phase 1b's acceptance test: the consolidated model against the baseline. -The Umsetzungsplan requires that forecast quality on the reference dataset is -not worse than the frozen baseline, which was produced by solaredge2mqtt's -original two-model code. What is compared is predictive quality, not the raw -numbers. +Forecast quality on the reference dataset must not be worse than the frozen +baseline, which was produced by solaredge2mqtt's original two-model code. What +is compared is predictive quality, not the raw numbers. Bit-for-bit comparison was tried in Phase 1a and does not hold up: predictions reproduce exactly on the machine that trains them, but diverge across different diff --git a/tests/test_result.py b/tests/test_result.py index 12ae578..32efaff 100644 --- a/tests/test_result.py +++ b/tests/test_result.py @@ -92,7 +92,7 @@ def test_missing_period_is_zero_rather_than_an_error(self): class TestIntervalIndependence: def test_hourly_figures_sum_sub_hour_intervals(self): - """Nothing assumes one row equals one hour (chapter 3.5).""" + """Nothing assumes one row equals one hour.""" start = datetime(2024, 6, 15, 0, 0, tzinfo=BERLIN) result = make_result(start, 96, interval_minutes=15, wh_per_period=25) From 906547be3e75a2480c911d8afa93b786c546ec60 Mon Sep 17 00:00:00 2001 From: Johannes Ott Date: Tue, 11 Aug 2026 19:03:42 +0200 Subject: [PATCH 4/4] Forbid citing implementation plans in ADRs and comments Records the convention that produced the previous two commits: no ADR, code comment or docstring may point at an implementation plan, roadmap or phase document. Those are written for one migration and then deleted, and every reference to one rots into a pointer at nothing -- deleting pvlearn-umsetzungsplan.md left nine dangling citations, one of them requiring a metadata field ADR 0003 had already removed. An ADR has to stand alone, so where a plan carried reasoning the record needs, the passage is copied or summarised into the ADR instead. ADR 0001's last citation is replaced that way: "the canonical, provider-independent schema in chapter 3.1" now states what that schema actually guarantees and points at pvlearn/schema.py, which moves with the code and breaks loudly when it stops existing. Naming a phase as the provenance of a measurement stays allowed -- it dates a result rather than sending the reader somewhere. The test is whether removing the document would leave the sentence broken. ADR 0003 also picks up the withdrawn scikit-learn reproducibility claim, so the rule's pointer to it covers both cases rather than one. Signed-off-by: Johannes Ott Co-Authored-By: Claude Opus 5 Signed-off-by: Johannes Ott --- AGENTS.md | 29 ++++++++++++++++--- docs/adr/0001-feature-selection-threshold.md | 11 +++---- ...one-version-decides-model-compatibility.md | 13 +++++++++ scripts/export_reference_dataset.py | 6 ++-- scripts/freeze_baseline_forecast.py | 7 ++--- tests/test_reference_dataset.py | 4 +-- 6 files changed, 52 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 865bd13..334d6b1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,6 +50,27 @@ A decision that needed weighing — and especially one whose reasoning would oth comment in the code — goes into `docs/adr/` as a numbered record, with the alternatives and the numbers that settled it. The code links to the record rather than restating it. +**Never cite an implementation plan, roadmap, or phase document** — not in an ADR, not in a code +comment, not in a docstring. No "chapter 3.4", no "point 7 of the roadmap", no "the Umsetzungsplan +requires". Such documents are written for one migration and then deleted, and every reference to +one rots into a pointer at nothing: `pvlearn-umsetzungsplan.md` was removed once its extraction +phase was done and left nine dangling citations behind, one of them requiring a metadata field +that had since been deleted. + +An ADR must stand alone. When a plan carries reasoning the record needs, **copy the relevant +passage or summarise it into the ADR itself** rather than pointing at it. For anything that lives +in the code — a schema, a threshold, a metadata field — cite the module or symbol +(`pvlearn/schema.py`, `ModelMetadata.raise_on_mismatch`), which moves with the code and breaks +loudly when it stops existing. + +Naming a past phase as the *provenance of a measurement* — "bit-for-bit comparison was tried in +Phase 1a and does not hold up" — is fine and stays: it dates a result rather than sending the +reader to a document. The line is whether removing the document would leave the sentence broken. + +The same applies to claims: an ADR or comment states what was measured and what backs it. Prose +asserting an empirical result that no test, script, or CI job reproduces is worse than no claim at +all, because it reads as evidence — see ADR 0003 for two that had to be withdrawn. + --- ## Developer Commands @@ -79,10 +100,10 @@ This repository enforces the Developer Certificate of Origin. Every commit needs To repair a branch where it is missing: `git rebase --signoff` followed by `git push --force-with-lease`. -**One pull request per phase or sub-phase of the roadmap, not per commit.** A phase lands as -a single branch with however many commits it takes, reviewed and merged as one unit. Phase 0 -was built the other way round — a pull request per step — which produced eight of them for -what is really one deliverable and made the phase impossible to review as a whole. +**One pull request per deliverable, not per commit.** A deliverable lands as a single branch +with however many commits it takes, reviewed and merged as one unit. The earliest work here was +built the other way round — a pull request per step — which produced eight of them for what was +really one deliverable and made it impossible to review as a whole. Commits within a branch stay individually meaningful, so the branch reads as a sequence rather than one opaque change. Merges are squashed, which is why the pull request description carries diff --git a/docs/adr/0001-feature-selection-threshold.md b/docs/adr/0001-feature-selection-threshold.md index c2ea31c..d3683ac 100644 --- a/docs/adr/0001-feature-selection-threshold.md +++ b/docs/adr/0001-feature-selection-threshold.md @@ -50,11 +50,12 @@ cannot be fitted at all. ## Consequences The rule no longer depends on how many candidates exist. This matters beyond -the numbers above: Phase 2 makes Open-Meteo the default provider, which adds -`ghi`, `dni` and `dhi`. Under a quantile rule those three would shift the cut -for every existing feature, so the same plant would select differently per -provider — directly against the point of the canonical, provider-independent -schema in chapter 3.1. +the numbers above: a provider that delivers irradiance — `ghi`, `dni`, `dhi` — +adds three candidates that a quantile rule would let shift the cut for every +existing feature, so the same plant would select differently per provider. +That runs against what the canonical schema in `pvlearn/schema.py` is for: +provider-specific field names never reach the library, and a provider that +delivers less yields a smaller feature set rather than a different model. Forecast quality lands at 641.54 Wh MAE, 3.3% above the baseline instead of 7.67%, with R² (0.8831) close to the baseline's 0.8859. The remaining gap is a diff --git a/docs/adr/0003-one-version-decides-model-compatibility.md b/docs/adr/0003-one-version-decides-model-compatibility.md index 6178d8f..cc54f50 100644 --- a/docs/adr/0003-one-version-decides-model-compatibility.md +++ b/docs/adr/0003-one-version-decides-model-compatibility.md @@ -36,6 +36,19 @@ install identical numpy, pandas, scipy and scikit-learn. No script in the repository reproduces the check. It rests on a single manual run recorded only as prose, and cannot fail if it stops being true. +Its counterpart from the same commit — that the frozen Phase 0 baseline "is +only reproducible against 1.9.0", which is what made scikit-learn's pin +load-bearing — fails harder. `test_records_the_versions_it_depends_on` looks +like it guards the claim but asserts only that the frozen sidecar records +`1.9.0`; it would pass unchanged if a newer scikit-learn reproduced the +baseline perfectly. And the claim is contradicted by a measurement the project +did make, recorded in `tests/test_extraction_regression.py`: the baseline does +not reproduce bit-for-bit across CPUs even with every version held fixed, CI +landing 5.26% off the MAE with `random_state=42` pinned throughout. The +variable that was actually measured to break reproduction is the machine, not +scikit-learn. Both comments now state only what is measured; the pins stay, as +a precaution rather than a finding. + ## Decision Compare one version: the release segment of `pvlearn_version`. diff --git a/scripts/export_reference_dataset.py b/scripts/export_reference_dataset.py index b75ab1c..d09bcff 100755 --- a/scripts/export_reference_dataset.py +++ b/scripts/export_reference_dataset.py @@ -1,8 +1,8 @@ #!/usr/bin/env python3 """Export a reference training dataset from a solaredge2mqtt InfluxDB instance. -Phase 0 of the roadmap needs a frozen dataset that the extraction work in Phase 1a -can be verified against. This script pulls the `forecast_training` measurement, +The extraction is verified against a frozen dataset rather than live data. This +script produces that dataset: it pulls the `forecast_training` measurement, pivots it into the same wide shape that `Forecaster.train()` consumes, and writes it as Parquet next to a JSON sidecar describing the export. @@ -78,7 +78,7 @@ def parse_args() -> argparse.Namespace: def query_training_data(args: argparse.Namespace, token: str): # influxdb-client is deliberately not a project dependency: pvlearn itself must stay - # free of I/O clients. This one-off Phase 0 script asks you to install it ad hoc. + # free of I/O clients. This one-off script asks you to install it ad hoc. from influxdb_client import ( # pyright: ignore[reportMissingImports] InfluxDBClient, ) diff --git a/scripts/freeze_baseline_forecast.py b/scripts/freeze_baseline_forecast.py index 5dc624d..00a9cf8 100644 --- a/scripts/freeze_baseline_forecast.py +++ b/scripts/freeze_baseline_forecast.py @@ -1,10 +1,9 @@ #!/usr/bin/env python3 """Freeze baseline forecasts from the current solaredge2mqtt production code. -Phase 0 of the roadmap needs the forecasts the existing implementation produces on -the reference dataset, recorded before any code is moved. Phase 1a is accepted only -if the extracted pvlearn code reproduces them, so this artifact is what makes the -extraction verifiable at all. +These are the forecasts the existing implementation produced on the reference +dataset, recorded before any code was moved. The extraction was accepted only +once pvlearn reproduced them, so this artifact is what made it verifiable at all. The script trains solaredge2mqtt's own `Forecaster` on the dataset minus a holdout tail, predicts that tail, and writes the predictions plus metrics and the exact diff --git a/tests/test_reference_dataset.py b/tests/test_reference_dataset.py index e985ec6..c5a253c 100644 --- a/tests/test_reference_dataset.py +++ b/tests/test_reference_dataset.py @@ -1,7 +1,7 @@ """Guards on the frozen reference dataset. -Every phase of the roadmap is verified by reproducing forecasts from this dataset, -so it has to stay exactly what it is. These tests do not check pvlearn behaviour; +Model changes are verified by reproducing forecasts from this dataset, so it has +to stay exactly what it is. These tests do not check pvlearn behaviour; they fail loudly if the fixture is ever silently replaced with a different export that no longer covers the cases later tests depend on. """