From 8bda95f537a52d0b287991e82a444d905f5d38b7 Mon Sep 17 00:00:00 2001 From: Roja Reddy Sareddy Date: Fri, 18 Sep 2026 14:48:33 -0700 Subject: [PATCH 1/2] fix(train): apply constructor hyperparameters in fine-tuning trainers Passing hyperparameters={...} when constructing a fine-tuning trainer (SFT, DPO, RLVR, RLAIF, MultiTurnRL) was silently ignored: BaseTrainer stored the dict, but each trainer then unconditionally replaced self.hyperparameters with a fresh FineTuningOptions built from the model's Hub spec, discarding the user's values. The only way to set values was post-construction via trainer.hyperparameters. = value. Capture the constructor-supplied hyperparameters in BaseTrainer and add a shared _apply_user_hyperparameters helper that re-applies them onto the rebuilt FineTuningOptions through its validating __setattr__. Each fine-tuning trainer calls it after building options, so a dict passed at construction is honored and validated against the model spec. Invalid option names or out-of-spec values raise (consistent with the direct attribute-assignment path) instead of being dropped. Add unit tests for the helper (valid apply, invalid name, out-of-spec value, empty/None no-op, non-FineTuningOptions no-op) and end-to-end SFTTrainer tests covering the construction path. --- .../src/sagemaker/train/base_trainer.py | 29 ++++++++ .../src/sagemaker/train/dpo_trainer.py | 4 ++ .../sagemaker/train/multi_turn_rl_trainer.py | 4 ++ .../src/sagemaker/train/rlaif_trainer.py | 4 ++ .../src/sagemaker/train/rlvr_trainer.py | 4 ++ .../src/sagemaker/train/sft_trainer.py | 4 ++ .../train/test_apply_user_hyperparameters.py | 69 +++++++++++++++++++ .../tests/unit/train/test_sft_trainer.py | 38 ++++++++++ 8 files changed, 156 insertions(+) create mode 100644 sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py diff --git a/sagemaker-train/src/sagemaker/train/base_trainer.py b/sagemaker-train/src/sagemaker/train/base_trainer.py index 3aac3f476e..0fde6099f9 100644 --- a/sagemaker-train/src/sagemaker/train/base_trainer.py +++ b/sagemaker-train/src/sagemaker/train/base_trainer.py @@ -153,6 +153,11 @@ def __init__( self.base_job_name = base_job_name self.tags = tags self.hyperparameters = hyperparameters or {} + # Preserve the constructor-supplied hyperparameters. The fine-tuning trainers + # replace ``self.hyperparameters`` with a spec-backed FineTuningOptions after + # this runs; they re-apply these captured values via _apply_user_hyperparameters + # so a dict passed at construction is not silently dropped. + self._constructor_hyperparameters = hyperparameters or {} self.output_data_config = output_data_config self.input_data_config = input_data_config self.environment = environment or {} @@ -167,6 +172,30 @@ def __init__( self.notification_rule_arn = self._setup_notifications(notifications) self._checkpoint_s3_uri = None + def _apply_user_hyperparameters(self, user_hyperparameters: Optional[Dict[str, Any]]) -> None: + """Apply constructor-supplied hyperparameters onto the resolved FineTuningOptions. + + The fine-tuning trainers replace ``self.hyperparameters`` with a + ``FineTuningOptions`` built from the model's Hub spec, which would otherwise + discard any ``hyperparameters`` dict passed at construction. This re-applies + those user-provided values through ``FineTuningOptions.__setattr__`` so each is + validated against the model spec, exactly as ``trainer.hyperparameters. = + value`` would be. Invalid option names or out-of-spec values therefore raise + (rather than being silently dropped). + + No-op when nothing was supplied or when ``self.hyperparameters`` is not a + spec-backed ``FineTuningOptions`` (e.g. a plain dict). + + Args: + user_hyperparameters: The hyperparameters dict captured from construction. + """ + if not user_hyperparameters: + return + if not hasattr(getattr(self, "hyperparameters", None), "_specs"): + return + for name, value in user_hyperparameters.items(): + setattr(self.hyperparameters, name, value) + def _is_nova_model_for_telemetry(self) -> bool: """Check if the model is a Nova model for telemetry tracking.""" model_name = getattr(self, "_model_name", None) diff --git a/sagemaker-train/src/sagemaker/train/dpo_trainer.py b/sagemaker-train/src/sagemaker/train/dpo_trainer.py index 159e7b4230..e128b549f8 100644 --- a/sagemaker-train/src/sagemaker/train/dpo_trainer.py +++ b/sagemaker-train/src/sagemaker/train/dpo_trainer.py @@ -196,6 +196,10 @@ def __init__( # Process hyperparameters self._process_hyperparameters() + # Re-apply any hyperparameters passed at construction (see BaseTrainer), + # which the FineTuningOptions rebuild above would otherwise drop. + self._apply_user_hyperparameters(self._constructor_hyperparameters) + # Validate and set EULA acceptance self.accept_eula = _validate_eula_for_gated_model(model, accept_eula, is_gated_model) diff --git a/sagemaker-train/src/sagemaker/train/multi_turn_rl_trainer.py b/sagemaker-train/src/sagemaker/train/multi_turn_rl_trainer.py index 2ab58b1cbf..7768312034 100644 --- a/sagemaker-train/src/sagemaker/train/multi_turn_rl_trainer.py +++ b/sagemaker-train/src/sagemaker/train/multi_turn_rl_trainer.py @@ -262,6 +262,10 @@ def __init__( ) self.accept_eula = _validate_eula_for_gated_model(model, accept_eula, is_gated_model) self._process_hyperparameters() + + # Re-apply any hyperparameters passed at construction (see BaseTrainer), + # which the FineTuningOptions rebuild above would otherwise drop. + self._apply_user_hyperparameters(self._constructor_hyperparameters) self._latest_job: AgentRFTJob | None = None @_telemetry_emitter( diff --git a/sagemaker-train/src/sagemaker/train/rlaif_trainer.py b/sagemaker-train/src/sagemaker/train/rlaif_trainer.py index 3a8197d74b..bc77bba1ee 100644 --- a/sagemaker-train/src/sagemaker/train/rlaif_trainer.py +++ b/sagemaker-train/src/sagemaker/train/rlaif_trainer.py @@ -195,6 +195,10 @@ def __init__( # Process reward_prompt parameter self._process_hyperparameters() + # Re-apply any hyperparameters passed at construction (see BaseTrainer), + # which the FineTuningOptions rebuild above would otherwise drop. + self._apply_user_hyperparameters(self._constructor_hyperparameters) + def _validate_reward_model_id(self, reward_model_id): """Validate reward_model_id is one of the allowed values.""" if not reward_model_id: diff --git a/sagemaker-train/src/sagemaker/train/rlvr_trainer.py b/sagemaker-train/src/sagemaker/train/rlvr_trainer.py index a182b1a581..52b050a25b 100644 --- a/sagemaker-train/src/sagemaker/train/rlvr_trainer.py +++ b/sagemaker-train/src/sagemaker/train/rlvr_trainer.py @@ -230,6 +230,10 @@ def __init__( # Remove constructor-handled hyperparameters self._process_hyperparameters() + # Re-apply any hyperparameters passed at construction (see BaseTrainer), + # which the FineTuningOptions rebuild above would otherwise drop. + self._apply_user_hyperparameters(self._constructor_hyperparameters) + # Validate and set EULA acceptance self.accept_eula = _validate_eula_for_gated_model(model, accept_eula, is_gated_model) diff --git a/sagemaker-train/src/sagemaker/train/sft_trainer.py b/sagemaker-train/src/sagemaker/train/sft_trainer.py index e810bdec52..821ff38f13 100644 --- a/sagemaker-train/src/sagemaker/train/sft_trainer.py +++ b/sagemaker-train/src/sagemaker/train/sft_trainer.py @@ -227,6 +227,10 @@ def __init__( # Process hyperparameters self._process_hyperparameters() + + # Re-apply any hyperparameters passed at construction (see BaseTrainer), + # which the FineTuningOptions rebuild above would otherwise drop. + self._apply_user_hyperparameters(self._constructor_hyperparameters) # Validate and set EULA acceptance self.accept_eula = _validate_eula_for_gated_model(model, accept_eula, is_gated_model) diff --git a/sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py b/sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py new file mode 100644 index 0000000000..f658e1046b --- /dev/null +++ b/sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py @@ -0,0 +1,69 @@ +"""Unit tests for BaseTrainer._apply_user_hyperparameters. + +These lock in that hyperparameters supplied at trainer construction are re-applied +onto the spec-backed FineTuningOptions (through its validating __setattr__) instead of +being silently dropped when the trainer rebuilds hyperparameters from the model spec. +""" +import pytest + +from sagemaker.train.base_trainer import BaseTrainer +from sagemaker.train.common import FineTuningOptions + + +def _make_options(): + return FineTuningOptions( + { + "learning_rate": {"type": "float", "default": 0.0001, "min": 0.0, "max": 1.0}, + "epochs": {"type": "integer", "default": 1, "min": 1, "max": 10}, + } + ) + + +class _DummyTrainer: + """Minimal stand-in exposing only what the helper touches.""" + + +def _apply(hyperparameters, user_hyperparameters): + trainer = _DummyTrainer() + trainer.hyperparameters = hyperparameters + # Call the unbound helper; it only depends on self.hyperparameters. + BaseTrainer._apply_user_hyperparameters(trainer, user_hyperparameters) + return trainer + + +def test_applies_valid_values_and_marks_user_set(): + options = _make_options() + trainer = _apply(options, {"learning_rate": 0.001, "epochs": 3}) + + assert trainer.hyperparameters.learning_rate == 0.001 + assert trainer.hyperparameters.epochs == 3 + # Values applied via __setattr__ are tracked as explicitly user-set. + assert trainer.hyperparameters._user_set == {"learning_rate", "epochs"} + + +def test_invalid_option_name_raises(): + options = _make_options() + with pytest.raises(AttributeError): + _apply(options, {"not_a_real_option": 1}) + + +def test_out_of_spec_value_raises(): + options = _make_options() + with pytest.raises(ValueError): + _apply(options, {"learning_rate": 5.0}) # exceeds max of 1.0 + + +def test_empty_user_hyperparameters_is_noop(): + options = _make_options() + trainer = _apply(options, {}) + assert trainer.hyperparameters._user_set == set() + + trainer_none = _apply(options, None) + assert trainer_none.hyperparameters._user_set == set() + + +def test_non_finetuning_options_container_is_noop(): + # A plain dict has no ``_specs``; the helper must not raise or mutate it. + plain = {"existing": 1} + trainer = _apply(plain, {"learning_rate": 0.001}) + assert trainer.hyperparameters == {"existing": 1} diff --git a/sagemaker-train/tests/unit/train/test_sft_trainer.py b/sagemaker-train/tests/unit/train/test_sft_trainer.py index 8229f5203a..00d6e3f6e9 100644 --- a/sagemaker-train/tests/unit/train/test_sft_trainer.py +++ b/sagemaker-train/tests/unit/train/test_sft_trainer.py @@ -25,6 +25,44 @@ def test_init_with_defaults(self, mock_finetuning_options, mock_validate_group, assert trainer.training_type == TrainingType.LORA assert trainer.model == "test-model" + @patch('sagemaker.train.sft_trainer._validate_and_resolve_model_package_group') + @patch('sagemaker.train.sft_trainer._get_fine_tuning_options_and_model_arn') + def test_init_applies_constructor_hyperparameters(self, mock_finetuning_options, mock_validate_group, mock_session): + """Hyperparameters passed at construction are applied onto FineTuningOptions.""" + from sagemaker.train.common import FineTuningOptions + mock_validate_group.return_value = "test-group" + options = FineTuningOptions( + {"learning_rate": {"type": "float", "default": 0.0001, "min": 0.0, "max": 1.0}} + ) + mock_finetuning_options.return_value = (options, "model-arn", False) + + trainer = SFTTrainer( + model="test-model", + model_package_group="test-group", + hyperparameters={"learning_rate": 0.001}, + ) + + assert trainer.hyperparameters.learning_rate == 0.001 + assert "learning_rate" in trainer.hyperparameters._user_set + + @patch('sagemaker.train.sft_trainer._validate_and_resolve_model_package_group') + @patch('sagemaker.train.sft_trainer._get_fine_tuning_options_and_model_arn') + def test_init_invalid_constructor_hyperparameter_raises(self, mock_finetuning_options, mock_validate_group, mock_session): + """An invalid constructor hyperparameter surfaces instead of being silently dropped.""" + from sagemaker.train.common import FineTuningOptions + mock_validate_group.return_value = "test-group" + options = FineTuningOptions( + {"learning_rate": {"type": "float", "default": 0.0001, "min": 0.0, "max": 1.0}} + ) + mock_finetuning_options.return_value = (options, "model-arn", False) + + with pytest.raises(AttributeError): + SFTTrainer( + model="test-model", + model_package_group="test-group", + hyperparameters={"not_a_real_option": 1}, + ) + @patch('sagemaker.train.sft_trainer._validate_and_resolve_model_package_group') @patch('sagemaker.train.sft_trainer._get_fine_tuning_options_and_model_arn') def test_init_with_full_training_type(self, mock_finetuning_options, mock_validate_group, mock_session): From cd174e4f82bd1a124f760d8c441583c6e7c54fdd Mon Sep 17 00:00:00 2001 From: Roja Reddy Sareddy Date: Fri, 18 Sep 2026 14:59:26 -0700 Subject: [PATCH 2/2] fix(train): only apply overridable constructor hyperparameters, warn on rest Refine _apply_user_hyperparameters to apply a constructor-supplied hyperparameter only when its name is overridable for the model (present in the FineTuningOptions _specs). Non-overridable names are ignored rather than raising, and a single warning lists them so the user knows those values will not take effect. Overridable names are still applied through __setattr__, so an out-of-spec value for an overridable name still raises. Update tests: a non-overridable name is now skipped with a warning (instead of raising); out-of-spec value for an overridable name still raises. --- .../src/sagemaker/train/base_trainer.py | 26 +++++++++++++++---- .../train/test_apply_user_hyperparameters.py | 16 +++++++++--- .../tests/unit/train/test_sft_trainer.py | 19 ++++++++------ 3 files changed, 44 insertions(+), 17 deletions(-) diff --git a/sagemaker-train/src/sagemaker/train/base_trainer.py b/sagemaker-train/src/sagemaker/train/base_trainer.py index 0fde6099f9..55623ba8fd 100644 --- a/sagemaker-train/src/sagemaker/train/base_trainer.py +++ b/sagemaker-train/src/sagemaker/train/base_trainer.py @@ -178,10 +178,14 @@ def _apply_user_hyperparameters(self, user_hyperparameters: Optional[Dict[str, A The fine-tuning trainers replace ``self.hyperparameters`` with a ``FineTuningOptions`` built from the model's Hub spec, which would otherwise discard any ``hyperparameters`` dict passed at construction. This re-applies - those user-provided values through ``FineTuningOptions.__setattr__`` so each is - validated against the model spec, exactly as ``trainer.hyperparameters. = - value`` would be. Invalid option names or out-of-spec values therefore raise - (rather than being silently dropped). + those user-provided values, but only for names that are overridable for the + model (i.e. present in the options' ``_specs``). Each applied value goes through + ``FineTuningOptions.__setattr__``, so it is still validated against the spec and + an out-of-spec value for an overridable name raises, exactly as + ``trainer.hyperparameters. = value`` would. + + Names that are not overridable are ignored (not applied), and a single warning + lists them so the user knows those values will not take effect. No-op when nothing was supplied or when ``self.hyperparameters`` is not a spec-backed ``FineTuningOptions`` (e.g. a plain dict). @@ -191,10 +195,22 @@ def _apply_user_hyperparameters(self, user_hyperparameters: Optional[Dict[str, A """ if not user_hyperparameters: return - if not hasattr(getattr(self, "hyperparameters", None), "_specs"): + specs = getattr(getattr(self, "hyperparameters", None), "_specs", None) + if not isinstance(specs, dict): return + ignored = [] for name, value in user_hyperparameters.items(): + if name not in specs: + ignored.append(name) + continue setattr(self.hyperparameters, name, value) + if ignored: + logger.warning( + "Ignoring hyperparameters that are not overridable for this model: %s. " + "These values will not take effect. Overridable hyperparameters: %s", + ignored, + list(specs.keys()), + ) def _is_nova_model_for_telemetry(self) -> bool: """Check if the model is a Nova model for telemetry tracking.""" diff --git a/sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py b/sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py index f658e1046b..af49481263 100644 --- a/sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py +++ b/sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py @@ -41,16 +41,24 @@ def test_applies_valid_values_and_marks_user_set(): assert trainer.hyperparameters._user_set == {"learning_rate", "epochs"} -def test_invalid_option_name_raises(): +def test_invalid_option_name_is_ignored_with_warning(caplog): options = _make_options() - with pytest.raises(AttributeError): - _apply(options, {"not_a_real_option": 1}) + with caplog.at_level("WARNING"): + trainer = _apply(options, {"not_a_real_option": 1, "learning_rate": 0.001}) + + # The overridable value is applied; the non-overridable one is skipped. + assert trainer.hyperparameters.learning_rate == 0.001 + assert not hasattr(trainer.hyperparameters, "not_a_real_option") + assert trainer.hyperparameters._user_set == {"learning_rate"} + # A warning names the ignored, non-overridable hyperparameter. + assert "not_a_real_option" in caplog.text + assert "not overridable" in caplog.text.lower() def test_out_of_spec_value_raises(): options = _make_options() with pytest.raises(ValueError): - _apply(options, {"learning_rate": 5.0}) # exceeds max of 1.0 + _apply(options, {"learning_rate": 5.0}) # overridable name, but exceeds max of 1.0 def test_empty_user_hyperparameters_is_noop(): diff --git a/sagemaker-train/tests/unit/train/test_sft_trainer.py b/sagemaker-train/tests/unit/train/test_sft_trainer.py index 00d6e3f6e9..682ab8d1bc 100644 --- a/sagemaker-train/tests/unit/train/test_sft_trainer.py +++ b/sagemaker-train/tests/unit/train/test_sft_trainer.py @@ -47,8 +47,8 @@ def test_init_applies_constructor_hyperparameters(self, mock_finetuning_options, @patch('sagemaker.train.sft_trainer._validate_and_resolve_model_package_group') @patch('sagemaker.train.sft_trainer._get_fine_tuning_options_and_model_arn') - def test_init_invalid_constructor_hyperparameter_raises(self, mock_finetuning_options, mock_validate_group, mock_session): - """An invalid constructor hyperparameter surfaces instead of being silently dropped.""" + def test_init_ignores_non_overridable_constructor_hyperparameter(self, mock_finetuning_options, mock_validate_group, mock_session): + """A non-overridable constructor hyperparameter is ignored (not applied, no raise).""" from sagemaker.train.common import FineTuningOptions mock_validate_group.return_value = "test-group" options = FineTuningOptions( @@ -56,12 +56,15 @@ def test_init_invalid_constructor_hyperparameter_raises(self, mock_finetuning_op ) mock_finetuning_options.return_value = (options, "model-arn", False) - with pytest.raises(AttributeError): - SFTTrainer( - model="test-model", - model_package_group="test-group", - hyperparameters={"not_a_real_option": 1}, - ) + trainer = SFTTrainer( + model="test-model", + model_package_group="test-group", + hyperparameters={"learning_rate": 0.001, "not_a_real_option": 1}, + ) + + # Overridable value applied; non-overridable one ignored rather than raising. + assert trainer.hyperparameters.learning_rate == 0.001 + assert not hasattr(trainer.hyperparameters, "not_a_real_option") @patch('sagemaker.train.sft_trainer._validate_and_resolve_model_package_group') @patch('sagemaker.train.sft_trainer._get_fine_tuning_options_and_model_arn')