Skip to content

Commit ba04040

Browse files
mohamedzeidan2021Mohamed Zeidan
andauthored
fix(train): assign SDK-managed channels to instance groups on heterog… (#6116)
* fix(train): assign SDK-managed channels to instance groups on heterogeneous clusters ModelTrainer injects code/sm_drivers/recipe channels that users cannot configure. On a heterogeneous cluster, SageMaker enforces an all-or-nothing rule: if any channel is assigned to instance groups, every channel must be. So setting instance_group_names on any user channel caused CreateTrainingJob to fail with "Some channels have assigned instance groups ... while others not: [sm_drivers, code]", making instance_group_names unusable with ModelTrainer. When a user assigns instance groups to any channel, assign the SDK-managed channels to the full set of instance groups (code/drivers must be present on every node). Behavior is unchanged when instance groups are not in use. Fixes #6089 * chore(train): shorten inline comments --------- Co-authored-by: Mohamed Zeidan <zeidmo@amazon.com>
1 parent 0da8c7c commit ba04040

2 files changed

Lines changed: 212 additions & 1 deletion

File tree

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

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,11 @@ def _create_training_job_args(
594594

595595
final_input_data_config = list(existing_channels.values()) + new_channels
596596

597+
# Assign SDK-managed channels to instance groups when the user uses them (see helper).
598+
managed_channel_instance_group_names = self._resolve_managed_channel_instance_groups(
599+
final_input_data_config
600+
)
601+
597602
if self._is_nova_recipe or self._is_llmft_recipe:
598603
for input_data in final_input_data_config:
599604
if input_data.channel_name == SM_RECIPE:
@@ -606,6 +611,7 @@ def _create_training_job_args(
606611
channel_name=SM_RECIPE,
607612
data_source=recipe_file_path,
608613
key_prefix=input_data_key_prefix,
614+
instance_group_names=managed_channel_instance_group_names,
609615
)
610616
final_input_data_config.append(recipe_channel)
611617
if self._is_nova_recipe or self._is_llmft_recipe:
@@ -664,6 +670,7 @@ def _create_training_job_args(
664670
data_source=self.source_code.source_dir,
665671
key_prefix=input_data_key_prefix,
666672
ignore_patterns=self.source_code.ignore_patterns,
673+
instance_group_names=managed_channel_instance_group_names,
667674
)
668675
final_input_data_config.append(source_code_channel)
669676

@@ -686,6 +693,7 @@ def _create_training_job_args(
686693
data_source=self._temp_code_dir.name,
687694
key_prefix=input_data_key_prefix,
688695
ignore_patterns=self.source_code.ignore_patterns,
696+
instance_group_names=managed_channel_instance_group_names,
689697
)
690698
final_input_data_config.append(sm_drivers_channel)
691699

@@ -891,12 +899,52 @@ def _resolve_staging_bucket(self) -> tuple[str,str]:
891899

892900
return default_bucket, None
893901

902+
def _resolve_managed_channel_instance_groups(
903+
self, input_data_config: List[Union[Channel, InputData]]
904+
) -> Optional[List[str]]:
905+
"""Resolve the instance groups to assign to SDK-managed channels.
906+
907+
SageMaker requires that, on a heterogeneous cluster, either all channels are
908+
assigned to instance groups or none are. The SDK injects channels (recipe, code,
909+
sm_drivers) that users cannot configure, so when a user assigns instance groups to
910+
any of their channels, the managed channels must be assigned too. We assign them to
911+
the full set of instance groups defined on the compute config, since the code and
912+
drivers must be present on every node regardless of instance group.
913+
914+
Args:
915+
input_data_config (List[Union[Channel, InputData]]): The user-provided channels.
916+
917+
Returns:
918+
Optional[List[str]]: The full list of instance group names to assign to
919+
managed channels, or ``None`` if instance groups are not in use.
920+
"""
921+
instance_groups = getattr(self.compute, "instance_groups", None) if self.compute else None
922+
if not instance_groups:
923+
return None
924+
925+
def _channel_has_instance_groups(channel: Union[Channel, InputData]) -> bool:
926+
# Channel nests its S3DataSource under data_source; InputData may hold it directly.
927+
s3_data_source = None
928+
if isinstance(channel, Channel):
929+
if channel.data_source:
930+
s3_data_source = channel.data_source.s3_data_source
931+
elif isinstance(channel, InputData):
932+
if isinstance(channel.data_source, S3DataSource):
933+
s3_data_source = channel.data_source
934+
return bool(getattr(s3_data_source, "instance_group_names", None))
935+
936+
if not any(_channel_has_instance_groups(channel) for channel in input_data_config):
937+
return None
938+
939+
return [group.instance_group_name for group in instance_groups]
940+
894941
def create_input_data_channel(
895942
self,
896943
channel_name: str,
897944
data_source: DataSourceType,
898945
key_prefix: Optional[str] = None,
899946
ignore_patterns: Optional[List[str]] = None,
947+
instance_group_names: Optional[List[str]] = None,
900948
) -> Channel:
901949
"""Create an input data channel for the training job.
902950
@@ -915,9 +963,21 @@ def create_input_data_channel(
915963
ignore_patterns: (Optional[List[str]]) :
916964
The ignore patterns to ignore specific files/folders when uploading to S3.
917965
If not specified, default to: ['.env', '.git', '__pycache__', '.DS_Store', '.cache', '.ipynb_checkpoints'].
966+
instance_group_names: (Optional[List[str]]) :
967+
The names of the instance groups (for heterogeneous clusters) that this
968+
channel's data should be assigned to. Only applied when the channel is
969+
built from a URI/local-path data source (not a caller-supplied
970+
``S3DataSource``/``FileSystemDataSource``, which the caller controls).
918971
"""
919972
from sagemaker.core.helper.pipeline_variable import PipelineVariable
920-
973+
974+
# Pass the field only when provided, so it stays unset (Unassigned) by default.
975+
instance_group_kwargs = (
976+
{"instance_group_names": instance_group_names}
977+
if instance_group_names is not None
978+
else {}
979+
)
980+
921981
channel = None
922982
if isinstance(data_source, PipelineVariable):
923983
channel = Channel(
@@ -927,6 +987,7 @@ def create_input_data_channel(
927987
s3_data_type="S3Prefix",
928988
s3_uri=data_source,
929989
s3_data_distribution_type="FullyReplicated",
990+
**instance_group_kwargs,
930991
),
931992
),
932993
input_mode="File",
@@ -940,6 +1001,7 @@ def create_input_data_channel(
9401001
s3_data_type="S3Prefix",
9411002
s3_uri=data_source,
9421003
s3_data_distribution_type="FullyReplicated",
1004+
**instance_group_kwargs,
9431005
),
9441006
),
9451007
input_mode="File",
@@ -996,6 +1058,7 @@ def create_input_data_channel(
9961058
s3_data_type="S3Prefix",
9971059
s3_uri=s3_uri,
9981060
s3_data_distribution_type="FullyReplicated",
1061+
**instance_group_kwargs,
9991062
),
10001063
),
10011064
input_mode="File",

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

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
Channel,
7474
DataSource,
7575
MetricDefinition,
76+
InstanceGroup,
7677
)
7778
from sagemaker.train.distributed import Torchrun, SMP, MPI
7879
from sagemaker.train.sm_recipes.utils import _load_recipes_cfg, _is_nova_recipe, _get_args_from_nova_recipe
@@ -472,6 +473,153 @@ def test_create_input_data_channel(mock_default_bucket, mock_upload_data, model_
472473
assert channel.data_source.s3_data_source.s3_uri == expected_s3_uri
473474

474475

476+
def test_create_input_data_channel_with_instance_group_names(model_trainer):
477+
"""instance_group_names is propagated onto the channel's S3DataSource."""
478+
channel = model_trainer.create_input_data_channel(
479+
channel_name="code",
480+
data_source=f"s3://{DEFAULT_BUCKET}/{DEFAULT_BASE_NAME}-job/input/code",
481+
instance_group_names=["head-instance-group", "worker-instance-group-1"],
482+
)
483+
assert channel.data_source.s3_data_source.instance_group_names == [
484+
"head-instance-group",
485+
"worker-instance-group-1",
486+
]
487+
488+
489+
HETEROGENEOUS_INSTANCE_GROUPS = [
490+
InstanceGroup(
491+
instance_type="ml.t3.large", instance_count=1, instance_group_name="head-instance-group"
492+
),
493+
InstanceGroup(
494+
instance_type="ml.m5.2xlarge",
495+
instance_count=2,
496+
instance_group_name="worker-instance-group-1",
497+
),
498+
]
499+
500+
501+
def _instance_group_names(channel):
502+
"""Return the channel's assigned instance_group_names as a list (or None)."""
503+
s3_data_source = channel.data_source.s3_data_source if channel.data_source else None
504+
names = getattr(s3_data_source, "instance_group_names", None) if s3_data_source else None
505+
return names if isinstance(names, list) else None
506+
507+
508+
def _managed_channel_names(input_data_config):
509+
return {
510+
channel.channel_name: _instance_group_names(channel) for channel in input_data_config
511+
}
512+
513+
514+
@patch("sagemaker.train.model_trainer.Session.upload_data")
515+
@patch("sagemaker.train.model_trainer.Session.default_bucket")
516+
def test_managed_channels_assigned_instance_groups_when_user_channel_assigns(
517+
mock_default_bucket, mock_upload_data
518+
):
519+
"""Regression test for issue #6089.
520+
521+
On a heterogeneous cluster, when a user assigns instance_group_names to any of their
522+
channels, the SDK-managed ``code``/``sm_drivers`` channels must also be assigned to the
523+
full set of instance groups, otherwise CreateTrainingJob fails validation with
524+
"Some channels have assigned instance groups ... while others not".
525+
"""
526+
mock_upload_data.return_value = f"s3://{DEFAULT_BUCKET}/{DEFAULT_BASE_NAME}-job/input/code"
527+
mock_default_bucket.return_value = DEFAULT_BUCKET
528+
expected_names = ["head-instance-group", "worker-instance-group-1"]
529+
530+
trainer = ModelTrainer(
531+
training_image=DEFAULT_IMAGE,
532+
role=DEFAULT_ROLE,
533+
source_code=DEFAULT_SOURCE_CODE,
534+
compute=Compute(instance_groups=HETEROGENEOUS_INSTANCE_GROUPS),
535+
stopping_condition=DEFAULT_STOPPING_CONDITION,
536+
output_data_config=DEFAULT_OUTPUT_DATA_CONFIG,
537+
)
538+
539+
user_channel = InputData(
540+
channel_name="processing",
541+
data_source=S3DataSource(
542+
s3_data_type="S3Prefix",
543+
s3_uri=f"s3://{DEFAULT_BUCKET}/data/",
544+
s3_data_distribution_type="FullyReplicated",
545+
instance_group_names=expected_names,
546+
),
547+
)
548+
549+
args = trainer._create_training_job_args(input_data_config=[user_channel])
550+
channels = _managed_channel_names(args["input_data_config"])
551+
552+
assert channels["processing"] == expected_names
553+
assert channels["code"] == expected_names
554+
assert channels["sm_drivers"] == expected_names
555+
556+
557+
@patch("sagemaker.train.model_trainer.Session.upload_data")
558+
@patch("sagemaker.train.model_trainer.Session.default_bucket")
559+
def test_managed_channels_not_assigned_when_user_channel_unassigned(
560+
mock_default_bucket, mock_upload_data
561+
):
562+
"""Managed channels stay unassigned when the user does not use instance groups.
563+
564+
Even on a heterogeneous cluster, if no user channel assigns instance_group_names, the
565+
SDK must not assign them to managed channels (which would itself violate the
566+
all-or-nothing rule against the unassigned user channel).
567+
"""
568+
mock_upload_data.return_value = f"s3://{DEFAULT_BUCKET}/{DEFAULT_BASE_NAME}-job/input/code"
569+
mock_default_bucket.return_value = DEFAULT_BUCKET
570+
571+
trainer = ModelTrainer(
572+
training_image=DEFAULT_IMAGE,
573+
role=DEFAULT_ROLE,
574+
source_code=DEFAULT_SOURCE_CODE,
575+
compute=Compute(instance_groups=HETEROGENEOUS_INSTANCE_GROUPS),
576+
stopping_condition=DEFAULT_STOPPING_CONDITION,
577+
output_data_config=DEFAULT_OUTPUT_DATA_CONFIG,
578+
)
579+
580+
user_channel = InputData(
581+
channel_name="processing",
582+
data_source=S3DataSource(
583+
s3_data_type="S3Prefix",
584+
s3_uri=f"s3://{DEFAULT_BUCKET}/data/",
585+
s3_data_distribution_type="FullyReplicated",
586+
),
587+
)
588+
589+
args = trainer._create_training_job_args(input_data_config=[user_channel])
590+
channels = _managed_channel_names(args["input_data_config"])
591+
592+
assert channels["code"] is None
593+
assert channels["sm_drivers"] is None
594+
595+
596+
@patch("sagemaker.train.model_trainer.Session.upload_data")
597+
@patch("sagemaker.train.model_trainer.Session.default_bucket")
598+
def test_managed_channels_not_assigned_on_homogeneous_cluster(
599+
mock_default_bucket, mock_upload_data
600+
):
601+
"""No instance groups configured -> managed channels are never assigned."""
602+
mock_upload_data.return_value = f"s3://{DEFAULT_BUCKET}/{DEFAULT_BASE_NAME}-job/input/code"
603+
mock_default_bucket.return_value = DEFAULT_BUCKET
604+
605+
trainer = ModelTrainer(
606+
training_image=DEFAULT_IMAGE,
607+
role=DEFAULT_ROLE,
608+
source_code=DEFAULT_SOURCE_CODE,
609+
compute=DEFAULT_COMPUTE_CONFIG,
610+
stopping_condition=DEFAULT_STOPPING_CONDITION,
611+
output_data_config=DEFAULT_OUTPUT_DATA_CONFIG,
612+
)
613+
614+
user_channel = InputData(channel_name="train", data_source=f"s3://{DEFAULT_BUCKET}/train/")
615+
616+
args = trainer._create_training_job_args(input_data_config=[user_channel])
617+
channels = _managed_channel_names(args["input_data_config"])
618+
619+
assert channels["code"] is None
620+
assert channels["sm_drivers"] is None
621+
622+
475623
@pytest.mark.parametrize(
476624
"test_case",
477625
[

0 commit comments

Comments
 (0)