diff --git a/AGENTS.md b/AGENTS.md index 16e6872..334d6b1 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 @@ -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 @@ -109,10 +130,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/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/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..cc54f50 --- /dev/null +++ b/docs/adr/0003-one-version-decides-model-compatibility.md @@ -0,0 +1,103 @@ +# 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. + +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`. +`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/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/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/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/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..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 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. +# 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/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 98d86f4..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 @@ -21,11 +20,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 +77,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_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..3773619 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,21 @@ 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. + + `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, + "sklearn_version": "1.9", + } + + restored = ModelMetadata.model_validate(sidecar) + + with pytest.raises(SchemaMismatchError, match="pvlearn_version is '0.0.1'"): + restored.raise_on_mismatch(make_location(), make_config()) 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. """ 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)