Skip to content

Commit 9d819b4

Browse files
fix: preserve volume KMS key in HyperparameterTuner training job definition (#6522)
HyperparameterTuner._build_training_job_definition() built the tuning job's ResourceConfig from ModelTrainer.compute but never copied volume_kms_key_id. HPO child training jobs therefore dropped the configured customer-managed KMS key for their compute volumes, and the request succeeded without error, so the loss was silent. Copy volume_kms_key_id from the trainer's compute when it is assigned, and leave the field Unassigned otherwise so it stays out of the serialized CreateHyperParameterTuningJob request. Explicit keys, plain strings, and PipelineVariable values are all passed through unchanged. Add unit tests covering explicit keys, plain-string keys, pipeline variables, and omitted keys. Closes #6521 Co-authored-by: Mufaddal Rohawala <mufi@amazon.com>
1 parent 510e390 commit 9d819b4

3 files changed

Lines changed: 73 additions & 0 deletions

File tree

‎sagemaker-train/src/sagemaker/train/tuner.py‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1413,6 +1413,7 @@ def _build_training_job_definition(self, inputs):
14131413
DataSource,
14141414
S3DataSource,
14151415
)
1416+
from sagemaker.core.utils.utils import Unassigned
14161417

14171418
model_trainer = self.model_trainer
14181419

@@ -1520,6 +1521,16 @@ def _build_training_job_definition(self, inputs):
15201521
),
15211522
)
15221523

1524+
# Preserve the volume KMS key configured on the ModelTrainer's compute so that
1525+
# HPO child training jobs encrypt their storage volumes with the customer-managed
1526+
# key. Omit the field entirely when no key is configured; leaving it Unassigned
1527+
# keeps it out of the serialized request (issue #6521). A PipelineVariable or a
1528+
# plain string is passed through unchanged.
1529+
if model_trainer.compute:
1530+
volume_kms_key_id = model_trainer.compute.volume_kms_key_id
1531+
if volume_kms_key_id is not None and not isinstance(volume_kms_key_id, Unassigned):
1532+
resource_config.volume_kms_key_id = volume_kms_key_id
1533+
15231534
# Build stopping condition
15241535
stopping_condition = StoppingCondition()
15251536
if model_trainer.stopping_condition:

‎sagemaker-train/tests/unit/train/test_tuner.py‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ def _create_mock_model_trainer(with_internal_channels=False, with_spot_training=
7070
trainer.compute.instance_type = "ml.m5.xlarge"
7171
trainer.compute.instance_count = 1
7272
trainer.compute.volume_size_in_gb = 30
73+
trainer.compute.volume_kms_key_id = None
7374
trainer.stopping_condition = MagicMock()
7475
trainer.stopping_condition.max_runtime_in_seconds = 3600
7576
trainer.input_data_config = None
@@ -865,6 +866,66 @@ def test_omitted_values_stay_unassigned(self):
865866
assert not isinstance(definition.retry_strategy, RetryStrategy)
866867

867868

869+
class TestBuildTrainingJobDefinitionVolumeKmsKey:
870+
"""Regression for #6521.
871+
872+
``_build_training_job_definition`` dropped ``volume_kms_key_id`` from
873+
``ModelTrainer.compute``, so HPO child training jobs silently failed to
874+
encrypt their storage volumes with the configured customer-managed KMS key.
875+
"""
876+
877+
@staticmethod
878+
def _tuner(trainer):
879+
return HyperparameterTuner(
880+
model_trainer=trainer,
881+
objective_metric_name="accuracy",
882+
hyperparameter_ranges=_create_single_hp_range(),
883+
)
884+
885+
def test_explicit_key_is_propagated(self):
886+
trainer = _create_mock_model_trainer()
887+
trainer.compute.volume_kms_key_id = (
888+
"arn:aws:kms:us-east-2:123456789012:key/00000000-0000-0000-0000-000000000000"
889+
)
890+
891+
definition = self._tuner(trainer)._build_training_job_definition(None)
892+
893+
assert definition.resource_config.volume_kms_key_id == (
894+
"arn:aws:kms:us-east-2:123456789012:key/00000000-0000-0000-0000-000000000000"
895+
)
896+
897+
def test_plain_string_key_is_propagated(self):
898+
trainer = _create_mock_model_trainer()
899+
trainer.compute.volume_kms_key_id = "1234abcd-12ab-34cd-56ef-1234567890ab"
900+
901+
definition = self._tuner(trainer)._build_training_job_definition(None)
902+
903+
assert (
904+
definition.resource_config.volume_kms_key_id == "1234abcd-12ab-34cd-56ef-1234567890ab"
905+
)
906+
907+
def test_pipeline_variable_key_is_propagated(self):
908+
trainer = _create_mock_model_trainer()
909+
kms_param = ParameterString(name="VolumeKmsKeyId")
910+
trainer.compute.volume_kms_key_id = kms_param
911+
912+
definition = self._tuner(trainer)._build_training_job_definition(None)
913+
914+
assert definition.resource_config.volume_kms_key_id is kms_param
915+
916+
def test_omitted_key_stays_unassigned(self):
917+
from sagemaker.core.utils.utils import Unassigned
918+
919+
trainer = _create_mock_model_trainer()
920+
trainer.compute.volume_kms_key_id = None
921+
922+
definition = self._tuner(trainer)._build_training_job_definition(None)
923+
924+
# Leaving the field Unassigned keeps VolumeKmsKeyId out of the
925+
# serialized CreateHyperParameterTuningJob request.
926+
assert isinstance(definition.resource_config.volume_kms_key_id, Unassigned)
927+
928+
868929
class TestHyperparameterTunerTagInheritance:
869930
"""Test tag inheritance from ModelTrainer (issue #6472)."""
870931

‎sagemaker-train/tests/unit/train/test_tuner_driver_channels.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ def _mock_model_trainer(**overrides):
7979
trainer.compute.instance_type = "ml.m5.xlarge"
8080
trainer.compute.instance_count = 1
8181
trainer.compute.volume_size_in_gb = 30
82+
trainer.compute.volume_kms_key_id = None
8283
trainer.stopping_condition = MagicMock()
8384
trainer.stopping_condition.max_runtime_in_seconds = 3600
8485
trainer.input_data_config = None

0 commit comments

Comments
 (0)