Skip to content

Commit cbadf8b

Browse files
rsareddy0329Roja Reddy Sareddy
andauthored
fix(train): raise on invalid constructor hyperparameters (#6306)
* 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.<name> = 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. * 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. * fix(train): raise on invalid constructor hyperparameters Change _apply_user_hyperparameters to route every constructor-supplied hyperparameter through FineTuningOptions.__setattr__ instead of skipping names that are not in the recipe spec. A dict passed at construction now behaves exactly like the trainer.hyperparameters.<name> = value path: - an unknown option name raises AttributeError - an out-of-spec or off-enum value raises ValueError Previously non-overridable names were ignored with a warning, which is inconsistent with the direct-assignment path and can still let a job run on a configuration the caller believed they set. Raising surfaces the mistake immediately. Update the helper and end-to-end SFT tests to assert the raise behavior. * test(train): add constructor-hyperparameter wiring tests for DPO/RLVR/RLAIF Each fine-tuning trainer calls _apply_user_hyperparameters at its own call-site, so add a focused end-to-end test per trainer asserting a constructor-supplied hyperparameter is applied onto the resolved FineTuningOptions (value set + tracked in _user_set). This guards each trainer's wiring against regressions; the apply/raise logic itself remains covered centrally in test_apply_user_hyperparameters.py. --------- Co-authored-by: Roja Reddy Sareddy <rsareddy@amazon.com>
1 parent b8237bf commit cbadf8b

6 files changed

Lines changed: 84 additions & 45 deletions

File tree

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

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -176,41 +176,35 @@ def _apply_user_hyperparameters(self, user_hyperparameters: Optional[Dict[str, A
176176
"""Apply constructor-supplied hyperparameters onto the resolved FineTuningOptions.
177177
178178
The fine-tuning trainers replace ``self.hyperparameters`` with a
179-
``FineTuningOptions`` built from the model's Hub spec, which would otherwise
180-
discard any ``hyperparameters`` dict passed at construction. This re-applies
181-
those user-provided values, but only for names that are overridable for the
182-
model (i.e. present in the options' ``_specs``). Each applied value goes through
183-
``FineTuningOptions.__setattr__``, so it is still validated against the spec and
184-
an out-of-spec value for an overridable name raises, exactly as
185-
``trainer.hyperparameters.<name> = value`` would.
179+
``FineTuningOptions`` built from the model's recipe override-params spec, which
180+
would otherwise discard any ``hyperparameters`` dict passed at construction. This
181+
re-applies those user-provided values by routing each one through
182+
``FineTuningOptions.__setattr__``, so a dict passed at construction behaves
183+
exactly like the ``trainer.hyperparameters.<name> = value`` path and is validated
184+
against the recipe spec:
186185
187-
Names that are not overridable are ignored (not applied), and a single warning
188-
lists them so the user knows those values will not take effect.
186+
* an unknown option name raises ``AttributeError``;
187+
* an out-of-spec or off-enum value for a known name raises ``ValueError``.
188+
189+
Values are never silently dropped in favor of the recipe default, which would
190+
otherwise launch a billable job on a configuration the caller did not set.
189191
190192
No-op when nothing was supplied or when ``self.hyperparameters`` is not a
191-
spec-backed ``FineTuningOptions`` (e.g. a plain dict).
193+
spec-backed ``FineTuningOptions`` (e.g. ``ModelTrainer``'s plain dict).
192194
193195
Args:
194196
user_hyperparameters: The hyperparameters dict captured from construction.
197+
198+
Raises:
199+
AttributeError: If a supplied name is not a valid option for the recipe.
200+
ValueError: If a supplied value fails the recipe spec (type/range/enum).
195201
"""
196202
if not user_hyperparameters:
197203
return
198-
specs = getattr(getattr(self, "hyperparameters", None), "_specs", None)
199-
if not isinstance(specs, dict):
204+
if not isinstance(getattr(getattr(self, "hyperparameters", None), "_specs", None), dict):
200205
return
201-
ignored = []
202206
for name, value in user_hyperparameters.items():
203-
if name not in specs:
204-
ignored.append(name)
205-
continue
206207
setattr(self.hyperparameters, name, value)
207-
if ignored:
208-
logger.warning(
209-
"Ignoring hyperparameters that are not overridable for this model: %s. "
210-
"These values will not take effect. Overridable hyperparameters: %s",
211-
ignored,
212-
list(specs.keys()),
213-
)
214208

215209
def _is_nova_model_for_telemetry(self) -> bool:
216210
"""Check if the model is a Nova model for telemetry tracking."""

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

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,18 +41,12 @@ def test_applies_valid_values_and_marks_user_set():
4141
assert trainer.hyperparameters._user_set == {"learning_rate", "epochs"}
4242

4343

44-
def test_invalid_option_name_is_ignored_with_warning(caplog):
44+
def test_invalid_option_name_raises():
4545
options = _make_options()
46-
with caplog.at_level("WARNING"):
47-
trainer = _apply(options, {"not_a_real_option": 1, "learning_rate": 0.001})
48-
49-
# The overridable value is applied; the non-overridable one is skipped.
50-
assert trainer.hyperparameters.learning_rate == 0.001
51-
assert not hasattr(trainer.hyperparameters, "not_a_real_option")
52-
assert trainer.hyperparameters._user_set == {"learning_rate"}
53-
# A warning names the ignored, non-overridable hyperparameter.
54-
assert "not_a_real_option" in caplog.text
55-
assert "not overridable" in caplog.text.lower()
46+
# An unknown option name raises (consistent with trainer.hyperparameters.x = v),
47+
# rather than being silently ignored.
48+
with pytest.raises(AttributeError):
49+
_apply(options, {"not_a_real_option": 1})
5650

5751

5852
def test_out_of_spec_value_raises():

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,24 @@ def test_init_with_defaults(self, mock_finetuning_options, mock_validate_group,
2424
assert trainer.training_type == TrainingType.LORA
2525
assert trainer.model == "test-model"
2626

27+
@patch('sagemaker.train.dpo_trainer._validate_and_resolve_model_package_group')
28+
@patch('sagemaker.train.dpo_trainer._get_fine_tuning_options_and_model_arn')
29+
def test_init_applies_constructor_hyperparameters(self, mock_finetuning_options, mock_validate_group, mock_session):
30+
"""Constructor hyperparameters are applied onto the resolved FineTuningOptions (wiring guard)."""
31+
from sagemaker.train.common import FineTuningOptions
32+
mock_validate_group.return_value = "test-group"
33+
options = FineTuningOptions(
34+
{"learning_rate": {"type": "float", "default": 0.0001, "min": 0.0, "max": 1.0}}
35+
)
36+
mock_finetuning_options.return_value = (options, "model-arn", False)
37+
trainer = DPOTrainer(
38+
model="test-model",
39+
model_package_group="test-group",
40+
hyperparameters={"learning_rate": 0.001},
41+
)
42+
assert trainer.hyperparameters.learning_rate == 0.001
43+
assert "learning_rate" in trainer.hyperparameters._user_set
44+
2745
@patch('sagemaker.train.dpo_trainer._validate_and_resolve_model_package_group')
2846
@patch('sagemaker.train.dpo_trainer._get_fine_tuning_options_and_model_arn')
2947
def test_init_with_full_training_type(self, mock_finetuning_options, mock_validate_group, mock_session):

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,24 @@ def test_init_with_defaults(self, mock_finetuning_options, mock_validate_group,
2424
assert trainer.training_type == TrainingType.LORA
2525
assert trainer.model == "test-model"
2626

27+
@patch('sagemaker.train.rlaif_trainer._validate_and_resolve_model_package_group')
28+
@patch('sagemaker.train.rlaif_trainer._get_fine_tuning_options_and_model_arn')
29+
def test_init_applies_constructor_hyperparameters(self, mock_finetuning_options, mock_validate_group, mock_session):
30+
"""Constructor hyperparameters are applied onto the resolved FineTuningOptions (wiring guard)."""
31+
from sagemaker.train.common import FineTuningOptions
32+
mock_validate_group.return_value = "test-group"
33+
options = FineTuningOptions(
34+
{"learning_rate": {"type": "float", "default": 0.0001, "min": 0.0, "max": 1.0}}
35+
)
36+
mock_finetuning_options.return_value = (options, "model-arn", False)
37+
trainer = RLAIFTrainer(
38+
model="test-model",
39+
model_package_group="test-group",
40+
hyperparameters={"learning_rate": 0.001},
41+
)
42+
assert trainer.hyperparameters.learning_rate == 0.001
43+
assert "learning_rate" in trainer.hyperparameters._user_set
44+
2745
@patch('sagemaker.train.rlaif_trainer._validate_and_resolve_model_package_group')
2846
@patch('sagemaker.train.rlaif_trainer._get_fine_tuning_options_and_model_arn')
2947
def test_init_with_full_training_type(self, mock_finetuning_options, mock_validate_group, mock_session):

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,24 @@ def test_init_with_defaults(self, mock_finetuning_options, mock_validate_group,
2424
assert trainer.training_type == TrainingType.LORA
2525
assert trainer.model == "test-model"
2626

27+
@patch('sagemaker.train.rlvr_trainer._validate_and_resolve_model_package_group')
28+
@patch('sagemaker.train.rlvr_trainer._get_fine_tuning_options_and_model_arn')
29+
def test_init_applies_constructor_hyperparameters(self, mock_finetuning_options, mock_validate_group, mock_session):
30+
"""Constructor hyperparameters are applied onto the resolved FineTuningOptions (wiring guard)."""
31+
from sagemaker.train.common import FineTuningOptions
32+
mock_validate_group.return_value = "test-group"
33+
options = FineTuningOptions(
34+
{"learning_rate": {"type": "float", "default": 0.0001, "min": 0.0, "max": 1.0}}
35+
)
36+
mock_finetuning_options.return_value = (options, "model-arn", False)
37+
trainer = RLVRTrainer(
38+
model="test-model",
39+
model_package_group="test-group",
40+
hyperparameters={"learning_rate": 0.001},
41+
)
42+
assert trainer.hyperparameters.learning_rate == 0.001
43+
assert "learning_rate" in trainer.hyperparameters._user_set
44+
2745
@patch('sagemaker.train.rlvr_trainer._validate_and_resolve_model_package_group')
2846
@patch('sagemaker.train.rlvr_trainer._get_fine_tuning_options_and_model_arn')
2947
def test_init_with_full_training_type(self, mock_finetuning_options, mock_validate_group, mock_session):

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

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,24 +47,21 @@ def test_init_applies_constructor_hyperparameters(self, mock_finetuning_options,
4747

4848
@patch('sagemaker.train.sft_trainer._validate_and_resolve_model_package_group')
4949
@patch('sagemaker.train.sft_trainer._get_fine_tuning_options_and_model_arn')
50-
def test_init_ignores_non_overridable_constructor_hyperparameter(self, mock_finetuning_options, mock_validate_group, mock_session):
51-
"""A non-overridable constructor hyperparameter is ignored (not applied, no raise)."""
50+
def test_init_invalid_constructor_hyperparameter_raises(self, mock_finetuning_options, mock_validate_group, mock_session):
51+
"""A non-overridable constructor hyperparameter raises rather than being dropped."""
5252
from sagemaker.train.common import FineTuningOptions
5353
mock_validate_group.return_value = "test-group"
5454
options = FineTuningOptions(
5555
{"learning_rate": {"type": "float", "default": 0.0001, "min": 0.0, "max": 1.0}}
5656
)
5757
mock_finetuning_options.return_value = (options, "model-arn", False)
5858

59-
trainer = SFTTrainer(
60-
model="test-model",
61-
model_package_group="test-group",
62-
hyperparameters={"learning_rate": 0.001, "not_a_real_option": 1},
63-
)
64-
65-
# Overridable value applied; non-overridable one ignored rather than raising.
66-
assert trainer.hyperparameters.learning_rate == 0.001
67-
assert not hasattr(trainer.hyperparameters, "not_a_real_option")
59+
with pytest.raises(AttributeError):
60+
SFTTrainer(
61+
model="test-model",
62+
model_package_group="test-group",
63+
hyperparameters={"learning_rate": 0.001, "not_a_real_option": 1},
64+
)
6865

6966
@patch('sagemaker.train.sft_trainer._validate_and_resolve_model_package_group')
7067
@patch('sagemaker.train.sft_trainer._get_fine_tuning_options_and_model_arn')

0 commit comments

Comments
 (0)