Skip to content

Commit f2c42b4

Browse files
mohamedzeidan2021Mohamed Zeidan
andauthored
fix: allow PipelineVariable keys in HyperparameterTuner hyperparameter_ranges annotation (#6327)
The HyperparameterTuner constructor annotated hyperparameter_ranges as Dict[str, ParameterRange], but the tuner accepts a PipelineVariable (e.g. a pipeline ParameterString) as a dict key -- the hyperparameter name -- which is a documented, working pattern when building a tuning step inside a pipeline. The narrow annotation makes mypy report a false positive (dict-item) on valid code. Broaden the key type to Union[str, PipelineVariable] (Union and PipelineVariable are already imported) and update the __init__ and create() docstrings. Annotation/docstring-only: runtime behavior is unchanged and ParameterString subclasses PipelineVariable, so the union is exact. Adds an annotation regression guard plus a runtime sanity test. Fixes #5243 Co-authored-by: Mohamed Zeidan <zeidmo@amazon.com>
1 parent ef40811 commit f2c42b4

2 files changed

Lines changed: 48 additions & 6 deletions

File tree

‎src/sagemaker/tuner.py‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -594,7 +594,7 @@ def __init__(
594594
self,
595595
estimator: EstimatorBase,
596596
objective_metric_name: Union[str, PipelineVariable],
597-
hyperparameter_ranges: Dict[str, ParameterRange],
597+
hyperparameter_ranges: Dict[Union[str, PipelineVariable], ParameterRange],
598598
metric_definitions: Optional[List[Dict[str, Union[str, PipelineVariable]]]] = None,
599599
strategy: Union[str, PipelineVariable] = "Bayesian",
600600
objective_type: Union[str, PipelineVariable] = "Maximize",
@@ -624,10 +624,11 @@ def __init__(
624624
instance.
625625
objective_metric_name (str or PipelineVariable): Name of the metric for evaluating
626626
training jobs.
627-
hyperparameter_ranges (dict[str, sagemaker.parameter.ParameterRange]): Dictionary of
628-
parameter ranges. These parameter ranges can be one
627+
hyperparameter_ranges (dict[str or PipelineVariable, sagemaker.parameter.ParameterRange]):
628+
Dictionary of parameter ranges. These parameter ranges can be one
629629
of three types: Continuous, Integer, or Categorical. The keys of
630-
the dictionary are the names of the hyperparameter, and the
630+
the dictionary are the names of the hyperparameter (a str, or a
631+
PipelineVariable such as a pipeline ParameterString), and the
631632
values are the appropriate parameter range class to represent
632633
the range.
633634
metric_definitions (list[dict[str, str] or list[dict[str, PipelineVariable]]): A list of
@@ -1906,8 +1907,9 @@ def create(
19061907
names as in estimator_dict, and there must be one entry for each estimator in
19071908
estimator_dict. Each value is a dictionary of sagemaker.parameter.ParameterRange
19081909
instance, which can be one of three types: Continuous, Integer, or Categorical.
1909-
The keys of each ParameterRange dictionaries are the names of the hyperparameter,
1910-
and the values are the appropriate parameter range class to represent the range.
1910+
The keys of each ParameterRange dictionary are the names of the hyperparameter
1911+
(a str, or a PipelineVariable such as a pipeline ParameterString), and the values
1912+
are the appropriate parameter range class to represent the range.
19111913
metric_definitions_dict (dict(str, list[dict]]): Dictionary of metric definitions.
19121914
The keys are the same set or a subset of estimator names as in estimator_dict,
19131915
and there must be one entry for each estimator in estimator_dict. Each value is

‎tests/unit/test_tuner.py‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import copy
1616
import os
1717
import re
18+
import typing
1819

1920
import pytest
2021
from mock import Mock, patch
@@ -42,6 +43,7 @@
4243
create_transfer_learning_tuner,
4344
HyperparameterTuner,
4445
)
46+
from sagemaker.workflow.entities import PipelineVariable
4547
from sagemaker.workflow.functions import JsonGet, Join
4648
from sagemaker.workflow.parameters import ParameterString, ParameterInteger
4749

@@ -2230,3 +2232,41 @@ def test_create_tuner_with_grid_search_strategy():
22302232

22312233
assert tuner is not None
22322234
assert tuner.max_jobs is None
2235+
2236+
2237+
def test_hyperparameter_ranges_annotation_allows_pipeline_variable_keys():
2238+
"""Regression for #5243.
2239+
2240+
The ``hyperparameter_ranges`` key type must allow PipelineVariable (e.g. a pipeline
2241+
ParameterString), not only ``str``, since the tuner accepts pipeline variables as
2242+
hyperparameter names. This asserts the resolved annotation, so it fails if the type
2243+
is narrowed back to ``Dict[str, ParameterRange]``.
2244+
"""
2245+
annotation = HyperparameterTuner.__init__.__annotations__["hyperparameter_ranges"]
2246+
2247+
key_type, value_type = typing.get_args(annotation)
2248+
key_options = typing.get_args(key_type) # (str, PipelineVariable)
2249+
2250+
assert str in key_options, f"str must remain a valid key type, got {key_options}"
2251+
assert (
2252+
PipelineVariable in key_options
2253+
), f"PipelineVariable must be an allowed key type, got {key_options}"
2254+
assert value_type is ParameterRange
2255+
2256+
2257+
def test_init_with_pipeline_variable_hyperparameter_key(estimator):
2258+
"""A pipeline ParameterString used as a hyperparameter-range key is accepted (#5243).
2259+
2260+
NOTE: this is a runtime sanity check, not the regression guard -- annotations are not
2261+
enforced at runtime, so this passes with or without the fix. The guard against
2262+
re-narrowing the type is test_hyperparameter_ranges_annotation_allows_pipeline_variable_keys.
2263+
"""
2264+
hparam_name = ParameterString(name="HParamName", default_value="hparam")
2265+
2266+
tuner = HyperparameterTuner(
2267+
estimator=estimator,
2268+
objective_metric_name=OBJECTIVE_METRIC_NAME,
2269+
hyperparameter_ranges={hparam_name: CategoricalParameter([1, 2])},
2270+
)
2271+
2272+
assert hparam_name in tuner._hyperparameter_ranges

0 commit comments

Comments
 (0)