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