Skip to content

Commit dec47ab

Browse files
committed
fix: address review comments (iteration #1)
1 parent f1ea9d5 commit dec47ab

File tree

2 files changed

+11
-7
lines changed

2 files changed

+11
-7
lines changed

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1504,10 +1504,10 @@ def _build_training_job_definition(self, inputs):
15041504
model_trainer.stopping_condition.max_wait_time_in_seconds
15051505
)
15061506

1507-
# Get environment variables from model_trainer
1508-
env = getattr(model_trainer, "environment", None)
1509-
if not env or not isinstance(env, dict):
1510-
env = None
1507+
# Get environment variables from model_trainer.
1508+
# environment is a defined attribute on ModelTrainer (dict | None).
1509+
# We pass it through as-is; even an empty dict is valid for the API.
1510+
env = model_trainer.environment
15111511

15121512
definition = HyperParameterTrainingJobDefinition(
15131513
algorithm_specification=algorithm_spec,

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -639,7 +639,11 @@ def test_build_training_job_definition_with_none_environment(self):
639639
assert definition.environment is None, "Environment should be None when not set"
640640

641641
def test_build_training_job_definition_with_empty_environment(self):
642-
"""Test that _build_training_job_definition handles empty environment gracefully."""
642+
"""Test that _build_training_job_definition passes through empty environment.
643+
644+
An empty dict is valid for the SageMaker API, so we pass it through as-is
645+
rather than silently converting it to None.
646+
"""
643647
mock_trainer = _create_mock_model_trainer()
644648
mock_trainer.environment = {}
645649

@@ -651,6 +655,6 @@ def test_build_training_job_definition_with_empty_environment(self):
651655

652656
definition = tuner._build_training_job_definition(None)
653657

654-
assert definition.environment is None, (
655-
"Environment should be None when empty dict is provided"
658+
assert definition.environment == {}, (
659+
"Empty dict environment should be passed through as-is"
656660
)

0 commit comments

Comments
 (0)