fix(train): raise on invalid constructor hyperparameters - #6306
rsareddy0329 wants to merge 5 commits into
Conversation
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.
…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.
…ctor-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.
|
Claude finished @rsareddy0329's task in 1m 0s —— View job Code Review —
|
…/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.
|
Claude finished @rsareddy0329's task in 44s —— View job Code Review —
|
This branch was successfully deployed
1 participant
Issue #, if available: N/A
Description of changes:
Follow-up to #6293. That change made a
hyperparameters={...}dict passed to a fine-tuning trainer constructor take effect, but it ignored (with a warning) any name that is not overridable for the model. This PR changes that to raise, so the constructor path behaves exactly like the direct-assignment path (trainer.hyperparameters.<name> = value)._apply_user_hyperparametersnow routes every constructor-supplied hyperparameter straight throughFineTuningOptions.__setattr__:AttributeError(naming the key and listing valid options),ValueError(naming the key, value, and constraint).Rationale: silently skipping an override the caller explicitly set is inconsistent with the direct-assignment path and can still let a job run on a configuration the caller believed they had set. Raising surfaces the mistake at construction time.
Unchanged: no-op when nothing was supplied, and no-op when
self.hyperparametersis not a spec-backedFineTuningOptions(e.g.ModelTrainer's plain dict), so unaffected trainers are not touched.Testing:
test_apply_user_hyperparameters.py: unknown option name now assertsAttributeError; out-of-spec value still assertsValueError; valid-apply, empty/None, and non-FineTuningOptionsno-op cases unchanged.test_sft_trainer.py: end-to-endSFTTrainerconstruction with an invalid constructor hyperparameter now asserts a raise.DescribeHubContent); dpo/rlvr/rlaif/multi-turn trainer suites pass.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.