Refactor/hpo from model to protocol

[zlilu]

refactor: hpo as meta learner protocol with tuning inside Evaluate.create, hpo metadata included as field in Evaluation.

[knutdrand]

Review of the HPO protocol refactor

Reviewed at faf296ca. ruff, mypy and the touched tests pass; everything below is behavioural. The direction of the refactor is good — replacing the HpoModel train/predict facade with an explicit MetaLearner protocol removes a real abstraction mismatch. Two issues need fixing before merge, plus some smaller cleanups.

1. The tuned configuration no longer reaches the NetCDF (medium)

hyperparameter_optimizer.py:36

On master, get_hpo_estimator passed the caller's ModelConfiguration through without copying, and HpoModel.train mutated it in place:

self._configuration.user_option_values = dict(self._best_config["user_option_values"])

train ran during Evaluation.create, so by the time evaluate.py reached evaluation.to_file(..., model_configuration=configuration.model_dump()) the object had already been updated and the NetCDF recorded the tuned values. The master comment says so explicitly: "updates the original configuration for outer evaluation logging as long as user_option_values stays mutable".

This PR replaces that with self._base_config = deepcopy(configuration) and mutates only the copy. That is the right call — smuggling a result out by mutating a caller's argument is exactly what a protocol refactor should remove. But evaluate.py:320 still dumps the caller's configuration, which is now pristine. After chap eval --estimator-options.mode hpo:

• ds.attrs["model_configuration"] holds the input config, not the tuned one
• generate_modelcard.py:189 reads that attr and renders an untuned config into the model card

The tuned config does survive, in the new hpo attribute via to_flat() (types.py:68), but no consumer reads it — Evaluation.from_file parses ds.attrs["hpo"] and the model card never looks there.

Suggested fix in _run_eval: prefer the tuned config when one exists, so model_configuration keeps a single meaning — "the config the reported scores came from":

model_configuration=(
    evaluation.hpo.model_configuration
    if evaluation.hpo is not None
    else (configuration.model_dump() if configuration else {})
)

(MLflow params are unaffected — eval_tracking reads the YAML separately in the outer wrapper before _run_eval starts, so those were untuned on master too.)

2. user_option_values is replaced rather than merged (medium)

hyperparameter_optimizer.py:82

self._base_config.user_option_values = best_model_config["user_option_values"]

This drops any user-supplied option that is not part of the search space. With ModelConfiguration(user_option_values={"x": 9, "keep_me": "yes"}) and a search space over x only, the result is user_option_values={'x': 2} — keep_me is gone and the tuned model trains with the template default instead of the user's value.

The correct merge is sitting commented out three lines below:

configuration.user_option_values = {
    **(configuration.user_option_values or {}),
    **deepcopy(best_candidate["config"]),
}

Pre-existing behaviour, but this is the line being rewritten, and it compounds #1: even once the tuned config is recorded, it would be recorded wrong.

3. Eager chap_core.hpo import pulls optuna onto the CLI path (medium)

assessment/evaluation.py:43

The new top-level chap_core.hpo.* imports pull in hpo/searcher.py, which imports optuna at module scope. import chap_core.assessment.evaluation went from 0.58s to 1.01s on my machine. This module otherwise lazy-imports carefully (backtest and train_test_generator are still function-local inside create), and it sits on the chap eval and REST-API startup paths. HyperparameterOptimizer / MetaLearner are only needed for the isinstance checks inside create, so a function-local import restores the old cost.

4. Root logger mutated at import time (medium)

hyperparameter_optimizer.py:13 (and objective.py:10)

logger = logging.getLogger()
logger.setLevel(logging.INFO)

getLogger() with no name is the root logger. Carried over from the deleted hpoModel.py, where it was harmless because the module was only imported lazily inside get_hpo_estimator. Now that evaluation.py imports it eagerly, merely importing chap_core.assessment.evaluation resets the host application's root log level: logging.basicConfig(level=WARNING) then import gives root level 30 on master, 20 on this branch. The CLI masks it because initialize_logging runs afterwards, but library and REST-API consumers get their logs flooded. Should be logging.getLogger(__name__) with no setLevel.

5. No tests for the new code (medium)

tests/hpo/test_hpo_model.py was deleted with no replacement, and grep finds nothing referencing HyperparameterOptimizer, meta_learn or MetaLearner. That leaves the max_trials must be specified for non-exhaustive searchers guard, leaderboard ordering, to_flat(), and the NetCDF hpo round-trip uncovered. I exercised the round-trip manually and it works, but nothing locks it in.

6. Wrapping on a max-only model hits a bare AssertionError (low/medium)

assessment/evaluation.py:490

The wrap condition checks only max_prediction_periods, but ExtendedPredictor.predict asserts min_pred_length is not None (ExtendedPredictor.py:43-46). The new test test_create_wraps_when_only_max_set_and_below_n_periods documents min=None, max=2 as a wrap case and cites a real model (chap-models/Vietnam-dengue-superensemble, max=1 / no min) — that model now gets wrapped and dies with a bare AssertionError at predict time. Latent on master too, but this refactor spreads it from chap eval alone to preference_learn, evaluate-ensemble and every inner HPO trial. Either guard on min_prediction_periods is not None or default it to 1 in ExtendedPredictor.

Smaller points

• hyperparameter_optimizer.py:75 — the incremental _is_better tracking and the assert best_params == leaderboard[0]["config"] check were replaced by a bare list.sort. Objective.__call__ only rejects a None metric, so a NaN score passes float(score), and NaN comparisons make sort order arbitrary — a degenerate trial can silently land at leaderboard[0] and become the chosen config. Worth rejecting non-finite scores before appending.
• cli_endpoints/_common.py:351 — the ValueError still points users at --estimator-options.search-space-yaml, which is not a flag (the field is search_space, so --help shows --estimator-options.search-space). The PR fixed exactly this wording in evaluate.py:133 and :172 but missed this message.
• hpo/types.py:52 — HyperparameterOptimization.write_best_config has no callers anywhere. HpoModel.write_best_config had the same problem, so the refactor carried a never-invoked method across. Either wire it up (there is currently no way to emit the tuned config as YAML) or drop it.
• hpo/README.md — still documents HpoModel implements HpoModelInterface ... train / predict / get_leaderboard, all deleted here. It is the last remaining reference to the removed API.
• tests/evaluation/test_evaluation.py:192 — test_create_wraps_when_only_max_set_and_below_n_periods is byte-for-byte identical to the test above it apart from min_prediction_periods=None, which Evaluation.create never reads. The min/max split mattered when the logic lived in evaluate.py and consulted both; as written the new test cannot fail independently. Parametrize the two, or drop one.