From f883b2faef8744e5295cf759fb313c5d206fd981 Mon Sep 17 00:00:00 2001 From: Deep Shah Date: Tue, 15 Sep 2026 18:20:48 +0000 Subject: [PATCH 1/2] fix(instance-preferences): reject preference lists on recipe trainers with a clear error Recipe-based trainers (SFTTrainer, DPOTrainer, RLVRTrainer) accept a Compute for serverful training, and Compute now carries instance_preferences. Recipes are rendered for one instance type: the device class selects the image and launcher, and the type is validated against the model's allowed-type list. A service-chosen type cannot apply, so instance_preferences is unsupported on this path, as documented. Today the unsupported case surfaces indirectly. With no instance_type set, the allowed-type check reports "Instance type 'None' is not supported" before ModelTrainer.from_recipe can raise its explicit message. _train_serverful_smtj now rejects a preference list up front, before the recipe fetch, with the same wording from_recipe uses. Two existing tests modelled Compute as a bare MagicMock, whose auto-created instance_preferences attribute is truthy; they now pin it to None, as a real Compute without preferences reports. --- .../src/sagemaker/train/base_trainer.py | 9 +++++ .../unit/train/test_base_trainer_compute.py | 3 ++ .../unit/train/test_base_trainer_serverful.py | 34 +++++++++++++++++++ .../train/test_serverful_recipe_validation.py | 1 + 4 files changed, 47 insertions(+) diff --git a/sagemaker-train/src/sagemaker/train/base_trainer.py b/sagemaker-train/src/sagemaker/train/base_trainer.py index 3aac3f476e..ba99255dea 100644 --- a/sagemaker-train/src/sagemaker/train/base_trainer.py +++ b/sagemaker-train/src/sagemaker/train/base_trainer.py @@ -901,6 +901,15 @@ def _train_serverful_smtj(self, training_dataset=None, validation_dataset=None, role = self.role compute = self.compute + # Recipes are rendered for one instance type (device class, image, + # launcher), so a service-chosen type cannot apply. Fail before the + # recipe fetch, and before the type-enum check reports a confusing + # "Instance type 'None' is not supported". + if getattr(compute, "instance_preferences", None): + raise ValueError( + "Training recipes do not support ``instance_preferences``. " + "Set a single ``instance_type`` in Compute when using a recipe-based trainer." + ) customization_technique = self._customization_technique # Resolve the recipe S3 URI from hub metadata diff --git a/sagemaker-train/tests/unit/train/test_base_trainer_compute.py b/sagemaker-train/tests/unit/train/test_base_trainer_compute.py index c9bb4cf8aa..4098635216 100644 --- a/sagemaker-train/tests/unit/train/test_base_trainer_compute.py +++ b/sagemaker-train/tests/unit/train/test_base_trainer_compute.py @@ -112,6 +112,7 @@ def test_compute_attributes_forwarded(self): trainer = _ConcreteTrainer() trainer.compute = MagicMock( instance_type="ml.p4d.24xlarge", + instance_preferences=None, instance_count=4, volume_size_in_gb=300, keep_alive_period_in_seconds=1200, @@ -131,6 +132,7 @@ def test_training_plan_arn_forwarded(self): trainer = _ConcreteTrainer() trainer.compute = MagicMock( instance_type="ml.p5.48xlarge", + instance_preferences=None, instance_count=2, volume_size_in_gb=500, keep_alive_period_in_seconds=0, @@ -147,6 +149,7 @@ def test_training_plan_arn_none_when_not_set(self): trainer = _ConcreteTrainer() trainer.compute = MagicMock( instance_type="ml.p4d.24xlarge", + instance_preferences=None, instance_count=1, volume_size_in_gb=30, keep_alive_period_in_seconds=0, diff --git a/sagemaker-train/tests/unit/train/test_base_trainer_serverful.py b/sagemaker-train/tests/unit/train/test_base_trainer_serverful.py index 590a5cc987..f37590367f 100644 --- a/sagemaker-train/tests/unit/train/test_base_trainer_serverful.py +++ b/sagemaker-train/tests/unit/train/test_base_trainer_serverful.py @@ -29,6 +29,7 @@ def __init__(self, **kwargs): self.compute = MagicMock( instance_type="ml.p4d.24xlarge", instance_count=1, + instance_preferences=None, volume_size_in_gb=100, keep_alive_period_in_seconds=None, training_plan_arn=None, @@ -388,3 +389,36 @@ def test_skips_validation_when_enum_unavailable(self, mock_enum): # Any instance type is accepted; the method returns None to signal skip. assert trainer._validate_instance_type("ml.g5.xlarge", MagicMock()) is None + + +class TestInstancePreferencesRejected: + """Recipes are rendered for one instance type, so a preference list must be + refused up front -- before the recipe fetch, and before the instance-type + enum check can report a misleading "Instance type 'None' is not supported".""" + + @pytest.fixture + def trainer(self): + trainer = _ConcreteTrainer() + trainer.compute = MagicMock( + instance_type=None, + instance_count=2, + instance_preferences=[MagicMock(instance_type="ml.p5.48xlarge")], + ) + return trainer + + def test_rejected_before_any_recipe_or_network_access(self, trainer): + with patch("sagemaker.train.base_trainer.get_recipe_s3_uri") as fetch, pytest.raises( + ValueError, match="Training recipes do not support ``instance_preferences``" + ): + trainer._train_serverful_smtj(training_dataset="s3://bucket/train.jsonl") + fetch.assert_not_called() + + def test_message_names_the_fix(self, trainer): + with pytest.raises(ValueError, match="Set a single ``instance_type`` in Compute"): + trainer._train_serverful_smtj(training_dataset="s3://bucket/train.jsonl") + + def test_single_instance_type_is_unaffected(self): + trainer = _ConcreteTrainer() + _, kwargs = _run_serverful(trainer) + assert kwargs["compute"].instance_type == "ml.p4d.24xlarge" + assert not kwargs["compute"].instance_preferences diff --git a/sagemaker-train/tests/unit/train/test_serverful_recipe_validation.py b/sagemaker-train/tests/unit/train/test_serverful_recipe_validation.py index 2c568e63b3..0ee72c55f0 100644 --- a/sagemaker-train/tests/unit/train/test_serverful_recipe_validation.py +++ b/sagemaker-train/tests/unit/train/test_serverful_recipe_validation.py @@ -38,6 +38,7 @@ def __init__(self, **kwargs): self.validation_dataset = None self.compute = MagicMock( instance_type="ml.p5.48xlarge", + instance_preferences=None, instance_count=4, volume_size_in_gb=100, keep_alive_period_in_seconds=None, From edc851c6e299a624b3458b4e5020032189f9ce7d Mon Sep 17 00:00:00 2001 From: Deep Shah Date: Tue, 15 Sep 2026 18:21:11 +0000 Subject: [PATCH 2/2] fix(instance-preferences): stop pydantic serializer warnings for unset preference fields Compute's validator converts Unassigned() to None on its own fields but not on the nested InstancePreference objects, so every submit with a preference list emitted one PydanticSerializationUnexpectedValue warning per element (for the unset training_plan_arns list) when _to_resource_config() called model_dump(). The request payload was already correct; the noise was the only effect. Both Compute classes now normalise each preference the same way. The serialized ResourceConfig is unchanged. A test asserts _to_resource_config() completes with warnings turned into errors. --- .../src/sagemaker/core/modules/configs.py | 4 +++ .../src/sagemaker/core/training/configs.py | 4 +++ .../tests/unit/test_compute_configs.py | 26 +++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/sagemaker-core/src/sagemaker/core/modules/configs.py b/sagemaker-core/src/sagemaker/core/modules/configs.py index 865018f50c..8d5af5d374 100644 --- a/sagemaker-core/src/sagemaker/core/modules/configs.py +++ b/sagemaker-core/src/sagemaker/core/modules/configs.py @@ -166,6 +166,10 @@ class Compute(shapes.ResourceConfig): def _model_validator(self) -> "Compute": """Convert Unassigned values to None and validate instance_preferences.""" converted = convert_unassigned_to_none(self) + # Nested preferences keep Unassigned() on unset fields, which model_dump + # flags per element; normalise them the same way as the top level. + for preference in converted.instance_preferences or (): + convert_unassigned_to_none(preference) validate_instance_preferences(converted) return converted diff --git a/sagemaker-core/src/sagemaker/core/training/configs.py b/sagemaker-core/src/sagemaker/core/training/configs.py index 6ba49005a9..f434f1e46d 100644 --- a/sagemaker-core/src/sagemaker/core/training/configs.py +++ b/sagemaker-core/src/sagemaker/core/training/configs.py @@ -194,6 +194,10 @@ class Compute(shapes.ResourceConfig): def _model_validator(self) -> "Compute": """Convert Unassigned values to None and validate instance_preferences.""" converted = convert_unassigned_to_none(self) + # Nested preferences keep Unassigned() on unset fields, which model_dump + # flags per element; normalise them the same way as the top level. + for preference in converted.instance_preferences or (): + convert_unassigned_to_none(preference) validate_instance_preferences(converted) return converted diff --git a/sagemaker-core/tests/unit/test_compute_configs.py b/sagemaker-core/tests/unit/test_compute_configs.py index bf3b6c2981..e5384a1dc2 100644 --- a/sagemaker-core/tests/unit/test_compute_configs.py +++ b/sagemaker-core/tests/unit/test_compute_configs.py @@ -1,6 +1,10 @@ """Unit tests for Compute and HyperPodCompute config classes.""" +import warnings + import pytest +from sagemaker.core.modules.configs import Compute as ModulesCompute +from sagemaker.core.shapes import InstancePreference from sagemaker.core.training.configs import Compute, HyperPodCompute @@ -337,3 +341,25 @@ def test_processing_cluster_config_single_type_still_works(self): ) assert pcc.instance_type == "ml.m5.xlarge" assert pcc.instance_count == 1 + + +class TestInstancePreferencesSerialization: + """Unset fields on nested preferences must not surface as pydantic + serializer warnings on every submit, and the request payload must omit them.""" + + @pytest.mark.parametrize("compute_cls", [Compute, ModulesCompute]) + def test_to_resource_config_emits_no_serializer_warning(self, compute_cls): + compute = compute_cls( + instance_preferences=[ + InstancePreference(instance_type="ml.m5.large"), + InstancePreference(instance_type="ml.m5.xlarge"), + ], + instance_count=1, + ) + with warnings.catch_warnings(): + warnings.simplefilter("error") + resource_config = compute._to_resource_config() + prefs = resource_config.instance_preferences + assert [p.instance_type for p in prefs] == ["ml.m5.large", "ml.m5.xlarge"] + assert all(p.training_plan_arns is None and p.instance_count is None for p in prefs) + assert resource_config.instance_count == 1