Skip to content

feat: allow warm-start helper methods to set new hyperparameter_ranges (#1816) - #6352

Open
mohamedzeidan2021 wants to merge 1 commit into
aws:masterfrom
mohamedzeidan2021:fix/issue-1816-tuner-warmstart-ranges
Open

mohamedzeidan2021 wants to merge 1 commit into
aws:masterfrom
mohamedzeidan2021:fix/issue-1816-tuner-warmstart-ranges

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

Problem

The v3 warm-start helpers HyperparameterTuner.transfer_learning_tuner and _create_warm_start_tuner always reused the parent tuner's ranges (self._hyperparameter_ranges / self._hyperparameter_ranges_dict) with no way to override them. Users who wanted the warm-started child tuner to explore a different search space had to monkeypatch the private _hyperparameter_ranges attribute.

Fix

Added an optional, backward-compatible hyperparameter_ranges=None parameter to transfer_learning_tuner (forwarded to _create_warm_start_tuner). When provided it overrides the parent's ranges — used as hyperparameter_ranges in the single-trainer branch and as hyperparameter_ranges_dict in the .create()-based branch. When omitted (None), existing behavior is preserved exactly. Docstrings updated to document both accepted shapes.

Testing

Added 4 unit tests in test_tuner_phase5.py covering single-trainer override + fallback and dict-based override + fallback. Full tuner unit suites pass (65 tests). Verified the override tests fail when the source is reverted (negative control). black + flake8 clean.

Fixes #1816

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 1m 15s —— View job


PR Review — warm-start helper hyperparameter_ranges override (#1816)

  • Read PR diff
  • Read tuner.py context (warm-start helpers, create, constructor)
  • Assess correctness, API consistency, tests
  • Post review

Overall this is a clean, focused, backward-compatible change. The new hyperparameter_ranges=None keyword is threaded correctly from transfer_learning_tuner → _create_warm_start_tuner, and both branches (single-trainer and .create()-based) fall back to the parent's ranges when omitted, so existing callers are unaffected. Enabling range overrides is also semantically valid here specifically because this is a TransferLearning warm start (unlike identical-data-and-algorithm, transfer learning permits changing the search space). I confirmed there is no sibling identical-data helper in the v3 tuner, so transfer_learning_tuner is the only public entry point — good.

A few observations, none blocking:

1. One parameter overloaded to mean two different shapes (readability / API design) — tuner.py:889, tuner.py:937

The single hyperparameter_ranges argument means dict[str, ParameterRange] in the single-trainer branch but dict[str, dict[str, ParameterRange]] (a ranges dict) in the .create() branch. The docstrings document both shapes well, but overloading one name for two shapes is easy to misuse. Since the underlying constructor/create already distinguish hyperparameter_ranges vs hyperparameter_ranges_dict, consider mirroring that split (or at minimum keep the docstring caveat prominent). Not a blocker given it's documented.

2. No shape validation — tuner.py:969-976, tuner.py:1002-1009

If a user passes the single-trainer shape to a dict-based tuner (or vice-versa), the wrong shape is forwarded to HyperparameterTuner(...) / .create(...) and will fail later with a less obvious error, or silently build an incorrect warm-start config. A light guard (e.g. confirming the override is a dict, and that dict-branch values are themselves dicts) would give a clearer failure. Optional, but would improve the UX of the new feature.

3. Test coverage is good but leaves a small gap — test_tuner_phase5.py

The 4 added tests cover single-trainer override/fallback and dict override/fallback — nice, including the fallback (negative) cases. Two combinations aren't exercised: passing hyperparameter_ranges together with a model_trainer override, and the multi-trainer ValueError path is untouched by these ranges (it's raised before ranges are used, so fine). Consider adding the model_trainer + hyperparameter_ranges combined case since real warm-start usage often swaps the trainer too.

No correctness bugs, security concerns, or performance issues found. _hyperparameter_ranges / _hyperparameter_ranges_dict are stored verbatim by the constructor, so the equality assertions in the tests are valid.

Note: the inline-comment tool isn't available in this environment, so the file/line-anchored findings above are reported here in the summary instead.

@lucasjia-aws lucasjia-aws left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review: 1 non-blocking suggestion. See inline comment.


def transfer_learning_tuner(self, additional_parents=None, model_trainer=None):
def transfer_learning_tuner(
self, additional_parents=None, model_trainer=None, hyperparameter_ranges=None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] correctness

The new hyperparameter_ranges parameter is overloaded to carry two incompatible shapes depending on internal state the caller does not directly control: dict[str, ParameterRange] in the single-model_trainer branch, but dict[str, dict[str, ParameterRange]] in the .create()-based branch. It is forwarded without a shape check — as hyperparameter_ranges in one branch and hyperparameter_ranges_dict in the other — so passing the flat form to a dict-based parent tuner is silently accepted here and only fails later inside HyperparameterTuner/create with an error that does not point back to this argument. Validate the shape against the active branch and raise a ValueError naming the parameter and the expected shape, so misuse fails fast at this public API boundary.

This branch was successfully deployed

1 active deployment
auto-approve — 48cee8b1 Deployed Sep 28, 2026 by mohamedzeidan2021 via wait-for-approval #480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How to set hyperparameters for warm start jobs created from helper methods?

2 participants