From bcea29dd3fe31b66967498d3f80f05f1a9435942 Mon Sep 17 00:00:00 2001 From: Johannes Ott Date: Fri, 28 Aug 2026 17:38:57 +0200 Subject: [PATCH 1/2] docs(adr): record that hyperparameters outlive the search Training rebuilt the pipeline on every call and dropped the search result, so an untuned retraining silently reverted the model to the library defaults. Consumers retrain on their data cadence and want to search on a much slower one, which the old API could only express by flipping enable_hyperparameter_tuning from outside. ADR 0004 records the decision, why the parameters are training state rather than configuration, and why PFISelector keeps its own untuned clone of the base estimator. Co-Authored-By: Claude Opus 5 Signed-off-by: Johannes Ott --- ...0004-hyperparameters-outlive-the-search.md | 107 ++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 docs/adr/0004-hyperparameters-outlive-the-search.md diff --git a/docs/adr/0004-hyperparameters-outlive-the-search.md b/docs/adr/0004-hyperparameters-outlive-the-search.md new file mode 100644 index 0000000..897dd05 --- /dev/null +++ b/docs/adr/0004-hyperparameters-outlive-the-search.md @@ -0,0 +1,107 @@ +# 4. Found hyperparameters outlive the search that produced them + +- **Status:** accepted +- **Date:** 2026-08-28 + +## Context + +`Forecaster.train` builds a fresh pipeline through `_prepare_model_pipeline` on +every call. With `hyperparametertuning` enabled it replaced that pipeline with a +clone of `HalvingGridSearchCV.best_estimator_`; with tuning disabled it kept the +constructor defaults of `HistGradientBoostingRegressor`. Nothing carried the +search result from one call to the next — `best_params_` was logged and then +dropped. + +The two costs in a training run are not comparable. A single fit is one pass +over the plant's history; the search fits many candidates over the halving +rungs, and each candidate fit includes `PFISelector`'s permutation importance. +The repository does not measure the ratio, so no factor is claimed here — but +the search is a multiple of the fit by construction, since it contains many of +them. + +Consumers have moved to schedules that reflect that. `solaredge2mqtt` writes +training data hourly and rebuilds the model at most daily, and wants the search +on a slower cadence still. Under the old API the only way to express that was to +flip `Forecaster.enable_hyperparameter_tuning` between calls from outside, which +made the model alternate between tuned parameters and library defaults: the +tuning held until exactly the next retraining and was then discarded. A consumer +therefore had to choose between fresh data and tuned parameters. + +## Decision + +Tuning becomes a property of the run, and its result becomes state of the +forecaster. + +`train` takes `hyperparametertuning: bool | None = None`. `None` follows the +configured `enable_hyperparameter_tuning`; `True` and `False` decide for that +one call. Consumers stop mutating the attribute from outside. + +`_hyperparametertuning` returns the tuned pipeline **and** `best_params_`. After +a successful run `Forecaster.hyperparameters` and +`Forecaster.hyperparameters_tuned_at` hold them, published at the same point as +`model_pipeline` and `metadata`, so a failed run leaves the previous ones in +place. A run that does not tune applies the stored parameters to the fresh +pipeline with `Pipeline.set_params`. + +`ModelMetadata` carries both fields, so they survive a process restart, and +`Forecaster.load` restores them. `hyperparameters_tuned_at` is the timestamp of +the search, not of the training run, and is carried forward unchanged by untuned +runs — a consumer reads it to decide whether a new search is due. Neither field +takes part in `raise_on_mismatch`: they describe the model, they do not decide +whether it can be loaded, and ADR 0003 keeps that decision at the release +version alone. Both have defaults, so a sidecar written before this change still +validates. + +Applying stored parameters never fails a training run. `set_params` is wrapped, +and on `ValueError` the rejected keys are logged, the stored parameters are +dropped, and the run continues with the defaults. The parameter grid can change +between releases, and a stale key has to degrade into "train untuned" rather +than into an exception in the consumer's retraining loop. + +The keys are pipeline-scoped — `model__max_iter`, `model__max_depth`, +`model__learning_rate` — so they reach the `model` step only. `PFISelector` +holds its own clone of the base estimator and keeps the defaults. That is +deliberate: the selector's job is to rank features, not to be the best possible +regressor, and the search scores the pipeline end to end, so parameters tuned +through it were never measured against the selector's internal fit. + +## Consequences + +A consumer can retrain on its data cadence and search on a much slower one, and +every retraining in between fits with the parameters the last search found. + +With no stored parameters and no per-call override the code path is identical to +the previous one, which is what `tests/test_baseline_forecast.py` and +`tests/test_extraction_regression.py` hold in place: they were not modified by +this change and stay green. + +The stored parameters are training state, not configuration. They are not +readable from `ForecasterConfig` and cannot be pinned there; the only way to +change them is another search. + +A model persisted before this change loads with no stored parameters and trains +untuned with the defaults until the next search, which matches what it was +already doing. + +## Alternatives considered + +**Keeping tuning coupled to every training run.** The status quo. Rejected +because it forces the consumer to choose between fresh data and tuned +parameters: with the search on, every retraining pays for it; with it off, the +next retraining throws the previous search away. + +**Pinning the parameters in `ForecasterConfig`.** A consumer could copy the +logged `best_params_` into its configuration and get stable parameters without +any new state. Rejected because they are a training result, not a setting: they +are derived from the plant's own history, they change when that history changes, +and a value pinned in configuration would silently outlive the data it was +measured on. It also moves the responsibility for a correct parameter dictionary +to the consumer, where a typo becomes a `ValueError` at load time rather than a +dropped key in a log line. + +**Storing the tuned estimator itself instead of the parameters.** Persisting +`best_estimator_` and refitting it would carry more than the parameters — it +would carry a fitted state that a later run has to be careful to discard. +Parameters are the smaller, inspectable artefact, they survive in the JSON +sidecar next to the metrics, and a consumer can read them without unpickling a +model. From 4fef7bb5dfa9888e9b3cf01e49b69d4f1f624423 Mon Sep 17 00:00:00 2001 From: Johannes Ott Date: Fri, 28 Aug 2026 17:39:00 +0200 Subject: [PATCH 2/2] feat: let found hyperparameters outlive the search train() takes an optional hyperparametertuning argument that decides for one run alone: None follows the configured default, True searches, False skips the search and fits with the parameters the last search found. _hyperparametertuning now returns best_params_ alongside the tuned pipeline. They are published together with model_pipeline and metadata, so a failed run keeps the previous ones, and they are persisted in the sidecar so a restored model keeps them across a restart. hyperparameters_tuned_at is the timestamp of the search and is carried forward by untuned runs, so a consumer can decide when a new search is due. Neither field takes part in raise_on_mismatch. Applying stored parameters cannot fail a training run: on ValueError the rejected keys are logged, the parameters are dropped, and the run continues with the defaults, so a grid that changed between releases degrades into an untuned run rather than an exception. See ADR 0004. Co-Authored-By: Claude Opus 5 Signed-off-by: Johannes Ott --- pvlearn/forecaster.py | 68 ++++++++++-- pvlearn/metadata.py | 10 ++ tests/test_forecaster.py | 216 ++++++++++++++++++++++++++++++++++++++- tests/test_metadata.py | 57 +++++++++++ 4 files changed, 342 insertions(+), 9 deletions(-) diff --git a/pvlearn/forecaster.py b/pvlearn/forecaster.py index 6114556..1ff7db3 100644 --- a/pvlearn/forecaster.py +++ b/pvlearn/forecaster.py @@ -2,11 +2,12 @@ import logging import time from asyncio import Event +from datetime import datetime from json import JSONDecodeError from math import ceil from os import replace from pathlib import Path -from typing import cast +from typing import Any, cast from joblib import Memory, dump, load from numpy import ones_like @@ -78,6 +79,8 @@ def __init__( self.interval_minutes = config.interval_minutes self.model_pipeline: Pipeline | None = None self.metadata: ModelMetadata | None = None + self.hyperparameters: dict[str, Any] | None = None + self.hyperparameters_tuned_at: datetime | None = None self.training_completed: Event = Event() self.cache_size_limit_bytes = config.cache_size_limit_mb * 1024 * 1024 @@ -89,7 +92,20 @@ def __init__( def minimum_training_rows(self) -> int: return ceil(MINIMUM_TRAINING_HOURS * MINUTES_PER_HOUR / self.interval_minutes) - def train(self, data: DataFrame) -> None: + def train(self, data: DataFrame, hyperparametertuning: bool | None = None) -> None: + """Fit a model on `data`, searching for hyperparameters or reusing them. + + `hyperparametertuning` decides for this run alone: `None` follows + `enable_hyperparameter_tuning`, `True` searches, `False` skips the + search and fits with the parameters the last search found. See + ADR 0004. + """ + tuning = ( + self.enable_hyperparameter_tuning + if hyperparametertuning is None + else hyperparametertuning + ) + data_count = len(data) logger.info( "Training energy model with %d intervals of %d minutes", @@ -115,13 +131,19 @@ def train(self, data: DataFrame) -> None: pipeline = self._prepare_model_pipeline(data.columns.to_list()) - if self.enable_hyperparameter_tuning: + if tuning: # Tuned on the evaluation split's training part only. Searching # over the full dataset would pick the parameters with the # holdout's help and the metrics below would flatter the model. - pipeline = self._hyperparametertuning( + pipeline, hyperparameters = self._hyperparametertuning( data.iloc[train_index], y_vector.iloc[train_index], pipeline ) + hyperparameters_tuned_at = datetime.now().astimezone() + else: + hyperparameters = self._apply_hyperparameters(pipeline) + hyperparameters_tuned_at = ( + self.hyperparameters_tuned_at if hyperparameters else None + ) metrics = self._evaluate( data, y_vector, (train_index, test_index), pipeline @@ -154,12 +176,16 @@ def train(self, data: DataFrame) -> None: # the previously trained model in place rather than replacing it # with one that never finished fitting. self.model_pipeline = fitted_pipeline + self.hyperparameters = hyperparameters + self.hyperparameters_tuned_at = hyperparameters_tuned_at self.metadata = ModelMetadata.create( location=self.location, config=self.config, training_rows=data_count, selected_features=list(selected_features), metrics=metrics, + hyperparameters=hyperparameters, + hyperparameters_tuned_at=hyperparameters_tuned_at, ) finally: # Waiters must be released even when training failed, or every @@ -267,12 +293,37 @@ def _prepare_preprocessor(self, x_vector_columns: list[str]) -> ColumnTransforme ct.set_output(transform="pandas") return ct + def _apply_hyperparameters(self, pipeline: Pipeline) -> dict[str, Any] | None: + """Set the parameters the last search found on a fresh pipeline. + + Returns what was applied, or `None` when there is nothing to apply or + the pipeline no longer accepts it. A parameter grid that changed + between releases leaves stale keys behind, and those must degrade into + a run with the defaults rather than into a failed training — see + ADR 0004. + """ + if not self.hyperparameters: + return None + + try: + pipeline.set_params(**self.hyperparameters) + except ValueError: + logger.warning( + "Dropping stored hyperparameters the pipeline no longer " + "accepts (%s), training with the defaults instead", + ", ".join(sorted(self.hyperparameters)), + ) + return None + + logger.info("Training with stored hyperparameters: %s", self.hyperparameters) + return self.hyperparameters + def _hyperparametertuning( self, data: DataFrame, y_vector: Series, pipeline: Pipeline, - ) -> Pipeline: + ) -> tuple[Pipeline, dict[str, Any]]: param_grid = { "model__max_iter": [100, 200, 300], "model__max_depth": [None, 5, 10], @@ -299,7 +350,10 @@ def _hyperparametertuning( logger.info("Training with best parameters: %s", grid_search.best_params_) logger.info("Training with best score: %s", grid_search.best_score_) - return cast(Pipeline, clone(grid_search.best_estimator_)) + return ( + cast(Pipeline, clone(grid_search.best_estimator_)), + dict(grid_search.best_params_), + ) def _cleanup_cache(self) -> None: if self.memory is None: @@ -420,6 +474,8 @@ def load( ) from error forecaster.metadata = metadata + forecaster.hyperparameters = metadata.hyperparameters or None + forecaster.hyperparameters_tuned_at = metadata.hyperparameters_tuned_at forecaster.training_completed.set() logger.info( diff --git a/pvlearn/metadata.py b/pvlearn/metadata.py index ac9ca7f..dcf647e 100644 --- a/pvlearn/metadata.py +++ b/pvlearn/metadata.py @@ -47,6 +47,10 @@ class ModelMetadata(BaseModel): rather than a setting the whole model is pinned to. The pvlearn release is the single version this compares — see ADR 0003. + + `hyperparameters` and `hyperparameters_tuned_at` describe the model rather + than deciding whether it can be loaded, so they take no part in + `raise_on_mismatch` — see ADR 0004. """ pvlearn_version: str @@ -56,6 +60,8 @@ class ModelMetadata(BaseModel): training_rows: int selected_features: list[str] metrics: ModelMetrics + hyperparameters: dict[str, Any] = {} + hyperparameters_tuned_at: datetime | None = None @classmethod def create( @@ -66,6 +72,8 @@ def create( selected_features: list[str], metrics: ModelMetrics, trained_at: datetime | None = None, + hyperparameters: dict[str, Any] | None = None, + hyperparameters_tuned_at: datetime | None = None, ) -> "ModelMetadata": return cls( pvlearn_version=__version__, @@ -75,6 +83,8 @@ def create( training_rows=training_rows, selected_features=selected_features, metrics=metrics, + hyperparameters=hyperparameters or {}, + hyperparameters_tuned_at=hyperparameters_tuned_at, ) def raise_on_mismatch(self, location: Location, config: ForecasterConfig) -> None: diff --git a/tests/test_forecaster.py b/tests/test_forecaster.py index 95ec135..05a85f8 100644 --- a/tests/test_forecaster.py +++ b/tests/test_forecaster.py @@ -185,7 +185,9 @@ def test_train_uses_hyperparameter_tuning_when_enabled(self): forecaster, "_prepare_model_pipeline", return_value=MagicMock() ), patch.object( - forecaster, "_hyperparametertuning", return_value=tuned_pipeline + forecaster, + "_hyperparametertuning", + return_value=(tuned_pipeline, {"model__max_iter": 200}), ) as mock_tune, patch.object(forecaster, "_evaluate", return_value=make_metrics()), ): @@ -251,7 +253,7 @@ def test_tuning_never_sees_the_holdout(self): patch.object( forecaster, "_hyperparametertuning", - side_effect=lambda tuning_data, _y, pipeline: pipeline, + side_effect=lambda tuning_data, _y, pipeline: (pipeline, {}), ) as mock_tune, patch.object(forecaster, "_evaluate", return_value=make_metrics()), ): @@ -316,10 +318,218 @@ def test_hyperparametertuning_returns_cloned_best_estimator(self): steps=[("model", HistGradientBoostingRegressor(random_state=42))] ) - tuned = forecaster._hyperparametertuning(data, y_vector, pipeline) + tuned, parameters = forecaster._hyperparametertuning(data, y_vector, pipeline) assert isinstance(tuned, Pipeline) assert tuned is not pipeline + assert set(parameters) == { + "model__max_iter", + "model__max_depth", + "model__learning_rate", + } + + +class TestForecasterHyperparameters: + """The per-call tuning switch and the parameters it leaves behind.""" + + TUNED = { + "model__max_iter": 37, + "model__max_depth": 3, + "model__learning_rate": 0.05, + } + + @classmethod + def _tuning_stub(cls, parameters: dict | None = None): + """Stand in for the search: set the parameters, report them back.""" + found = cls.TUNED if parameters is None else parameters + + def tune(_data, _y_vector, pipeline): + pipeline.set_params(**found) + return pipeline, dict(found) + + return tune + + @classmethod + def _train_tuned(cls, forecaster: Forecaster, data: DataFrame) -> None: + with patch.object( + forecaster, "_hyperparametertuning", side_effect=cls._tuning_stub() + ): + forecaster.train(data, hyperparametertuning=True) + + @staticmethod + def _model_parameters(forecaster: Forecaster) -> dict: + assert forecaster.model_pipeline is not None + return forecaster.model_pipeline.named_steps["model"].get_params() + + def test_per_call_true_searches_although_the_config_disables_it(self): + forecaster = Forecaster(make_location(), make_config()) + + with patch.object( + forecaster, "_hyperparametertuning", side_effect=self._tuning_stub() + ) as mock_tune: + forecaster.train(make_training_data(), hyperparametertuning=True) + + mock_tune.assert_called_once() + + def test_per_call_false_skips_the_search_although_the_config_enables_it(self): + forecaster = Forecaster(make_location(), make_config(hyperparametertuning=True)) + + with patch.object(forecaster, "_hyperparametertuning") as mock_tune: + forecaster.train(make_training_data(), hyperparametertuning=False) + + mock_tune.assert_not_called() + + def test_none_follows_a_config_that_enables_tuning(self): + forecaster = Forecaster(make_location(), make_config(hyperparametertuning=True)) + + with patch.object( + forecaster, "_hyperparametertuning", side_effect=self._tuning_stub() + ) as mock_tune: + forecaster.train(make_training_data()) + + mock_tune.assert_called_once() + + def test_none_follows_a_config_that_disables_tuning(self): + forecaster = Forecaster(make_location(), make_config()) + + with patch.object(forecaster, "_hyperparametertuning") as mock_tune: + forecaster.train(make_training_data()) + + mock_tune.assert_not_called() + + def test_a_tuned_run_stores_what_the_search_found(self): + forecaster = Forecaster(make_location(), make_config()) + + self._train_tuned(forecaster, make_training_data()) + + assert forecaster.hyperparameters == self.TUNED + assert forecaster.hyperparameters_tuned_at is not None + assert forecaster.metadata is not None + assert forecaster.metadata.hyperparameters == self.TUNED + assert ( + forecaster.metadata.hyperparameters_tuned_at + == forecaster.hyperparameters_tuned_at + ) + + def test_an_untuned_run_fits_the_model_with_the_stored_parameters(self): + forecaster = Forecaster(make_location(), make_config()) + data = make_training_data() + self._train_tuned(forecaster, data) + + forecaster.train(data, hyperparametertuning=False) + + fitted = self._model_parameters(forecaster) + for key, value in self.TUNED.items(): + assert fitted[key.removeprefix("model__")] == value + + def test_an_untuned_run_keeps_the_timestamp_of_the_search(self): + forecaster = Forecaster(make_location(), make_config()) + data = make_training_data() + self._train_tuned(forecaster, data) + tuned_at = forecaster.hyperparameters_tuned_at + + forecaster.train(data, hyperparametertuning=False) + + assert forecaster.hyperparameters_tuned_at == tuned_at + assert forecaster.metadata is not None + assert forecaster.metadata.hyperparameters_tuned_at == tuned_at + assert forecaster.metadata.trained_at != tuned_at + + def test_a_later_search_replaces_the_stored_parameters(self): + forecaster = Forecaster(make_location(), make_config()) + data = make_training_data() + self._train_tuned(forecaster, data) + first_tuned_at = forecaster.hyperparameters_tuned_at + + with patch.object( + forecaster, + "_hyperparametertuning", + side_effect=self._tuning_stub({"model__max_iter": 42}), + ): + forecaster.train(data, hyperparametertuning=True) + + assert forecaster.hyperparameters == {"model__max_iter": 42} + assert forecaster.hyperparameters_tuned_at != first_tuned_at + + def test_stale_parameters_degrade_into_a_run_with_the_defaults(self): + """A parameter grid that changed between releases must not break + training in a consumer's retraining loop.""" + forecaster = Forecaster(make_location(), make_config()) + data = make_training_data() + forecaster.hyperparameters = {"model__no_such_parameter": 1} + forecaster.hyperparameters_tuned_at = datetime.now(timezone.utc) + + forecaster.train(data, hyperparametertuning=False) + + defaults = ( + forecaster._prepare_model_pipeline(data.columns.to_list()) + .named_steps["model"] + .get_params() + ) + assert self._model_parameters(forecaster) == defaults + assert forecaster.hyperparameters is None + assert forecaster.hyperparameters_tuned_at is None + assert forecaster.metadata is not None + assert forecaster.metadata.hyperparameters == {} + + def test_failed_training_keeps_the_previously_stored_parameters(self): + forecaster = Forecaster(make_location(), make_config()) + data = make_training_data() + self._train_tuned(forecaster, data) + tuned_at = forecaster.hyperparameters_tuned_at + + with patch.object(forecaster, "_evaluate", side_effect=RuntimeError("boom")): + with pytest.raises(RuntimeError): + forecaster.train(data, hyperparametertuning=False) + + assert forecaster.hyperparameters == self.TUNED + assert forecaster.hyperparameters_tuned_at == tuned_at + + def test_save_and_load_round_trip_the_stored_parameters(self, tmp_path): + location = make_location() + config = make_config() + forecaster = Forecaster(location, config) + self._train_tuned(forecaster, make_training_data()) + forecaster.save(tmp_path) + + restored = Forecaster.load(tmp_path, location, config) + + assert restored.hyperparameters == self.TUNED + assert restored.hyperparameters_tuned_at == forecaster.hyperparameters_tuned_at + + def test_a_restored_model_reuses_the_parameters_without_searching(self, tmp_path): + """Without this a process restart forces either a search or a run + with the library defaults.""" + location = make_location() + config = make_config() + data = make_training_data() + forecaster = Forecaster(location, config) + self._train_tuned(forecaster, data) + forecaster.save(tmp_path) + + restored = Forecaster.load(tmp_path, location, config) + restored.train(data, hyperparametertuning=False) + + assert self._model_parameters(restored)["max_iter"] == 37 + + def test_a_sidecar_without_the_new_fields_still_loads(self, tmp_path): + """A model persisted before the fields existed must not be rejected.""" + location = make_location() + config = make_config() + forecaster = Forecaster(location, config) + self._train_tuned(forecaster, make_training_data()) + forecaster.save(tmp_path) + + metadata_path = tmp_path / METADATA_FILENAME + sidecar = json.loads(metadata_path.read_text()) + del sidecar["hyperparameters"] + del sidecar["hyperparameters_tuned_at"] + metadata_path.write_text(json.dumps(sidecar)) + + restored = Forecaster.load(tmp_path, location, config) + + assert restored.hyperparameters is None + assert restored.hyperparameters_tuned_at is None class TestForecasterPredict: diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 3773619..8527f92 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -133,6 +133,63 @@ def test_reports_every_mismatch_at_once(self): assert "pvlearn_version" in message +class TestHyperparameters: + """They describe the model; they do not decide whether it loads.""" + + TUNED = {"model__max_iter": 200, "model__learning_rate": 0.01} + + def test_default_to_absent(self): + metadata = make_metadata() + + assert metadata.hyperparameters == {} + assert metadata.hyperparameters_tuned_at is None + + def test_create_records_them(self): + tuned_at = datetime(2026, 8, 28, 9, 0).astimezone() + + metadata = ModelMetadata.create( + location=make_location(), + config=make_config(), + training_rows=1440, + selected_features=["sun__time_elevation"], + metrics=ModelMetrics(mae=1.0, rmse=2.0, r2=0.9), + hyperparameters=self.TUNED, + hyperparameters_tuned_at=tuned_at, + ) + + assert metadata.hyperparameters == self.TUNED + assert metadata.hyperparameters_tuned_at == tuned_at + + def test_take_no_part_in_the_compatibility_check(self): + metadata = make_metadata( + hyperparameters={"model__gone_in_a_later_release": 1}, + hyperparameters_tuned_at=datetime(2020, 1, 1).astimezone(), + ) + + metadata.raise_on_mismatch(make_location(), make_config()) + + def test_round_trip_through_json(self): + metadata = make_metadata( + hyperparameters=self.TUNED, + hyperparameters_tuned_at=datetime(2026, 8, 28, 9, 0).astimezone(), + ) + + restored = ModelMetadata.model_validate_json(metadata.model_dump_json()) + + assert restored.hyperparameters == self.TUNED + assert restored.hyperparameters_tuned_at == metadata.hyperparameters_tuned_at + + def test_a_sidecar_written_without_them_still_validates(self): + sidecar = make_metadata().model_dump(mode="json") + del sidecar["hyperparameters"] + del sidecar["hyperparameters_tuned_at"] + + restored = ModelMetadata.model_validate(sidecar) + + assert restored.hyperparameters == {} + assert restored.hyperparameters_tuned_at is None + + class TestSerialization: def test_round_trips_through_json(self): metadata = make_metadata()