diff --git a/sagemaker-train/src/sagemaker/train/base_trainer.py b/sagemaker-train/src/sagemaker/train/base_trainer.py index 3aac3f476e..55623ba8fd 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,46 @@ 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, 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). + + Args: + user_hyperparameters: The hyperparameters dict captured from construction. + """ + if not user_hyperparameters: + return + 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.""" 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..af49481263 --- /dev/null +++ b/sagemaker-train/tests/unit/train/test_apply_user_hyperparameters.py @@ -0,0 +1,77 @@ +"""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_is_ignored_with_warning(caplog): + options = _make_options() + 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}) # overridable name, but 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..682ab8d1bc 100644 --- a/sagemaker-train/tests/unit/train/test_sft_trainer.py +++ b/sagemaker-train/tests/unit/train/test_sft_trainer.py @@ -25,6 +25,47 @@ 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_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( + {"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, "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') def test_init_with_full_training_type(self, mock_finetuning_options, mock_validate_group, mock_session):