Skip to content

Commit aa37a9b

Browse files
mohamedzeidan2021Mohamed Zeidan
andauthored
fix: keep job name in pipeline request for ModelTrainer and HyperparameterTuner (#6323)
Both unconditionally popped training_job_name / hyper_parameter_tuning_job_name from the request for a PipelineSession. The TrainingStep/TuningStep already strip it via trim_request_dict (dropped by default, prefix preserved when PipelineDefinitionConfig(use_custom_job_prefix=True)), so popping upstream left use_custom_job_prefix nothing to preserve and made base_job_name have no effect in pipelines. Stop popping; let trim_request_dict handle it. Fixes #5776, #6299 Co-authored-by: Mohamed Zeidan <zeidmo@amazon.com>
1 parent 5285726 commit aa37a9b

4 files changed

Lines changed: 109 additions & 11 deletions

File tree

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
from sagemaker.core.shapes import AlgorithmSpecification, ModelPackageConfig
3434
from sagemaker.core.utils.utils import serialize
3535
from sagemaker.core.apiutils._boto_functions import to_pascal_case
36+
from sagemaker.core.common_utils import name_from_base
3637

3738
from pydantic import BaseModel, ConfigDict, PrivateAttr, validate_call
3839
from sagemaker.core.config.config_schema import (
@@ -76,7 +77,6 @@
7677
from sagemaker.train.distributed import Torchrun, DistributedConfig
7778
from sagemaker.train.utils import (
7879
_default_s3_uri,
79-
_get_unique_name,
8080
_is_valid_path,
8181
_is_valid_s3_uri,
8282
safe_serialize,
@@ -679,7 +679,13 @@ def _create_training_job_args(
679679
Dict[str, Any]: The training job arguments.
680680
"""
681681
self._populate_intelligent_defaults()
682-
current_training_job_name = _get_unique_name(self.base_job_name)
682+
# Use the shared name_from_base timestamp format so that
683+
# sagemaker.core.common_utils.base_from_name can strip it back to base_job_name.
684+
# trim_request_dict relies on base_from_name to preserve the prefix when a pipeline
685+
# sets PipelineDefinitionConfig(use_custom_job_prefix=True) (issues #5776, #6299).
686+
# Underscores are replaced because SageMaker job names disallow them; _get_unique_name
687+
# used to do this and name_from_base does not.
688+
current_training_job_name = name_from_base(self.base_job_name.replace("_", "-"))
683689
# Use input_s3_key_prefix (e.g. a pipeline name) as the leading key prefix when set;
684690
# otherwise fall back to base_job_name to preserve the default upload layout.
685691
input_data_key_prefix_root = self.input_s3_key_prefix or self.base_job_name
@@ -880,8 +886,10 @@ def _create_training_job_args(
880886
training_request["model_package_config"] = self.model_package_config
881887

882888
if boto3 or isinstance(self.sagemaker_session, PipelineSession):
883-
if isinstance(self.sagemaker_session, PipelineSession):
884-
training_request.pop("training_job_name", None)
889+
# Keep training_job_name in the request. The TrainingStep strips it via
890+
# trim_request_dict, which drops it by default but preserves the base_job_name
891+
# prefix when PipelineDefinitionConfig(use_custom_job_prefix=True). Popping it here
892+
# unconditionally left use_custom_job_prefix nothing to preserve (issue #5776, #6299).
885893
# ``serialize`` renames the fields of Tag models to the boto-style
886894
# {"Key": ..., "Value": ...} form the service requires, but passes plain dicts
887895
# through untouched, so hand it the validated Tag objects. ``TrainingJob.create``

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1321,8 +1321,10 @@ def _start_tuning_job(self, inputs):
13211321
from sagemaker.core.utils.utils import serialize
13221322
from sagemaker.core.apiutils._boto_functions import to_pascal_case
13231323

1324-
# Remove job name for pipeline as it's auto-generated at execution time
1325-
tuning_request.pop("hyper_parameter_tuning_job_name", None)
1324+
# Keep hyper_parameter_tuning_job_name in the request. The TuningStep strips it via
1325+
# trim_request_dict, which drops it by default but preserves the base_tuning_job_name
1326+
# prefix when PipelineDefinitionConfig(use_custom_job_prefix=True). Popping it here
1327+
# unconditionally left use_custom_job_prefix nothing to preserve (issue #6299, #5776).
13261328
# Convert snake_case to PascalCase for AWS API
13271329
pipeline_request = {to_pascal_case(k): v for k, v in tuning_request.items()}
13281330
serialized_request = serialize(pipeline_request)

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

Lines changed: 59 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,60 @@ def test_model_trainer_param_validation(test_case, modules_session):
257257
assert trainer.base_job_name == DEFAULT_BASE_NAME
258258

259259

260+
def test_pipeline_session_request_keeps_training_job_name(modules_session):
261+
"""Regression for #5776 / #6299.
262+
263+
Under a PipelineSession the request must KEEP TrainingJobName. The TrainingStep strips it via
264+
trim_request_dict (dropped by default, prefix preserved when use_custom_job_prefix=True);
265+
popping it here left use_custom_job_prefix nothing to preserve.
266+
"""
267+
from unittest.mock import Mock
268+
from sagemaker.core.workflow.pipeline_context import PipelineSession
269+
270+
session = Mock(spec=PipelineSession)
271+
session.default_bucket.return_value = DEFAULT_BUCKET
272+
session.default_bucket_prefix = DEFAULT_BUCKET_PREFIX
273+
session.boto_region_name = DEFAULT_REGION
274+
session.sagemaker_config = {}
275+
276+
trainer = ModelTrainer(
277+
training_image=DEFAULT_IMAGE,
278+
role=DEFAULT_ROLE,
279+
compute=DEFAULT_COMPUTE_CONFIG,
280+
stopping_condition=DEFAULT_STOPPING_CONDITION,
281+
output_data_config=DEFAULT_OUTPUT_DATA_CONFIG,
282+
base_job_name="my-prefix",
283+
sagemaker_session=session,
284+
)
285+
286+
args = trainer._create_training_job_args(input_data_config=[])
287+
288+
assert "TrainingJobName" in args, "TrainingJobName must survive into the pipeline request"
289+
assert args["TrainingJobName"].startswith("my-prefix")
290+
291+
# The generated name must use a timestamp format base_from_name can strip, otherwise
292+
# trim_request_dict bakes a definition-time timestamp into the pipeline definition
293+
# instead of preserving the base prefix.
294+
from sagemaker.core.common_utils import base_from_name
295+
296+
assert base_from_name(args["TrainingJobName"]) == "my-prefix"
297+
298+
# End-to-end: with use_custom_job_prefix=True, trim_request_dict must leave exactly the base.
299+
from types import SimpleNamespace
300+
from sagemaker.core.workflow.utilities import trim_request_dict
301+
from sagemaker.core.workflow.pipeline_definition_config import PipelineDefinitionConfig
302+
303+
custom_prefix_config = SimpleNamespace(
304+
pipeline_definition_config=PipelineDefinitionConfig(use_custom_job_prefix=True)
305+
)
306+
trimmed = trim_request_dict(dict(args), "TrainingJobName", custom_prefix_config)
307+
assert trimmed["TrainingJobName"] == "my-prefix"
308+
309+
# And by default (no config) the key is dropped entirely.
310+
dropped = trim_request_dict(dict(args), "TrainingJobName", None)
311+
assert "TrainingJobName" not in dropped
312+
313+
260314
@patch("sagemaker.train.model_trainer.TrainingJob")
261315
@patch("sagemaker.train.model_trainer.ModelTrainer.create_input_data_channel")
262316
def test_sm_drivers_channel_ignores_user_ignore_patterns(
@@ -1105,7 +1159,7 @@ def test_remote_debug_config(mock_training_job, modules_session):
11051159
)
11061160

11071161

1108-
@patch("sagemaker.train.model_trainer._get_unique_name")
1162+
@patch("sagemaker.train.model_trainer.name_from_base")
11091163
@patch("sagemaker.train.model_trainer.TrainingJob")
11101164
def test_model_trainer_full_init(mock_training_job, mock_unique_name, modules_session):
11111165
def mock_upload_data(path, bucket, key_prefix, extra_args=None):
@@ -1381,7 +1435,7 @@ def mock_upload_data(path, bucket, key_prefix, extra_args=None):
13811435

13821436
# TODO: Re-Enable after local mode fully migrated to v3
13831437
# @patch("sagemaker.train.model_trainer._LocalContainer")
1384-
# @patch("sagemaker.train.model_trainer._get_unique_name")
1438+
# @patch("sagemaker.train.model_trainer.name_from_base")
13851439
# @patch("sagemaker.train.local.local_container.download_folder")
13861440
# def test_model_trainer_local_full_init(
13871441
# mock_download_folder, mock_unique_name, mock_local_container, modules_session
@@ -1637,7 +1691,7 @@ def test_hyperparameters_invalid(mock_exists, modules_session):
16371691
)
16381692

16391693

1640-
@patch("sagemaker.train.model_trainer._get_unique_name")
1694+
@patch("sagemaker.train.model_trainer.name_from_base")
16411695
@patch("sagemaker.train.model_trainer.TrainingJob")
16421696
def test_model_trainer_default_paths(mock_training_job, mock_unique_name, modules_session):
16431697
def mock_upload_data(path, bucket, key_prefix, extra_args=None):
@@ -1828,7 +1882,7 @@ def test_metric_definitions(mock_training_job, modules_session):
18281882
)
18291883

18301884

1831-
@patch("sagemaker.train.model_trainer._get_unique_name")
1885+
@patch("sagemaker.train.model_trainer.name_from_base")
18321886
@patch("sagemaker.core.resources.TrainingJob")
18331887
def test_nova_recipe(mock_training_job, mock_unique_name, modules_session):
18341888
def mock_upload_data(path, bucket, key_prefix, extra_args=None):
@@ -2203,7 +2257,7 @@ def test_nova_recipe_model_package_config_only_mpg_from_recipe(modules_session):
22032257
os.unlink(recipe.name)
22042258

22052259

2206-
@patch("sagemaker.train.model_trainer._get_unique_name")
2260+
@patch("sagemaker.train.model_trainer.name_from_base")
22072261
@patch("sagemaker.train.model_trainer.TrainingJob")
22082262
def test_llmft_recipe(mock_training_job, mock_unique_name, modules_session):
22092263
def mock_upload_data(path, bucket, key_prefix, extra_args=None):

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -704,6 +704,40 @@ def test_build_training_job_definition_includes_spot_params(self):
704704
definition.stopping_condition.max_wait_time_in_seconds, int
705705
), "Max wait time should be set"
706706

707+
def test_pipeline_session_request_keeps_tuning_job_name(self):
708+
"""Regression for #6299 / #5776.
709+
710+
Under a PipelineSession the tuning request must KEEP HyperParameterTuningJobName. The
711+
TuningStep strips it via trim_request_dict (dropped by default, prefix preserved when
712+
use_custom_job_prefix=True); popping it here left use_custom_job_prefix nothing to preserve.
713+
"""
714+
from unittest.mock import Mock
715+
from sagemaker.core.workflow.pipeline_context import PipelineSession
716+
717+
pipeline_session = Mock(spec=PipelineSession)
718+
mock_trainer = _create_mock_model_trainer()
719+
mock_trainer.sagemaker_session = pipeline_session
720+
721+
tuner = HyperparameterTuner(
722+
model_trainer=mock_trainer,
723+
objective_metric_name="accuracy",
724+
hyperparameter_ranges=_create_single_hp_range(),
725+
)
726+
tuner._current_job_name = "my-prefix-2026-01-01-00-00-00-000"
727+
728+
tuner._start_tuning_job(inputs=None)
729+
730+
pipeline_session._intercept_create_request.assert_called_once()
731+
serialized_request = pipeline_session._intercept_create_request.call_args.args[0]
732+
assert "HyperParameterTuningJobName" in serialized_request
733+
assert serialized_request["HyperParameterTuningJobName"].startswith("my-prefix")
734+
735+
# The name must be strippable back to the base, or trim_request_dict would preserve a
736+
# definition-time timestamp instead of the prefix when use_custom_job_prefix=True.
737+
from sagemaker.core.common_utils import base_from_name
738+
739+
assert base_from_name(serialized_request["HyperParameterTuningJobName"]) == "my-prefix"
740+
707741
def test_build_training_job_definition_includes_environment_variables(self):
708742
"""Test that _build_training_job_definition includes environment variables.
709743

0 commit comments

Comments
 (0)