Fix lr_warmup_fraction silently rejected by default lr_warmup_steps=100#2243
Closed
alvinttang wants to merge 1 commit into
Closed
Fix lr_warmup_fraction silently rejected by default lr_warmup_steps=100#2243alvinttang wants to merge 1 commit into
alvinttang wants to merge 1 commit into
Conversation
…mup_steps
The default value of `lr_warmup_steps` was 100, which was treated as user-set
by `__post_init__`'s mutually-exclusive validation. As a result, passing
`--train.lr_warmup_fraction 0.1` (e.g. for pretraining) raised:
ValueError: Can't provide both `--train.lr_warmup_fraction` and
`--train.lr_warmup_steps`. Choose one.
This forced users to also pass `--train.lr_warmup_steps 0` explicitly. The bug
was reported in issue Lightning-AI#2116 (comment): "lr_warmup_fraction can be set, but is
not used. Only lr_warmup_steps is used."
Fix: change the default of `lr_warmup_steps` to `None` and resolve it inside
`__post_init__`:
* if the user provides `lr_warmup_fraction`, default `lr_warmup_steps` to 0
(so it does not conflict and is ignored downstream)
* otherwise, default to 100 (preserving prior behavior)
The mutually-exclusive validation still raises when both are provided
explicitly. All existing semantics (truthy fraction wins in `warmup_iters`,
`lr_warmup_fraction=0` means disabled) are preserved.
Tests added to `tests/test_args.py`:
* `test_lr_warmup_fraction_works_without_overriding_default_steps`
* `test_lr_warmup_conflict_only_when_both_explicitly_set`
alvinttang
requested review from
KaelanDt,
andyland,
k223kim,
lianakoleva and
t-vi
as code owners
April 25, 2026 14:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the bug surfaced in #2116 (and #2212 discussions, tutorial issues):
TrainArgs.lr_warmup_fractionis silently unusable becauselr_warmup_steps: int | None = 100(truthy default) collides withlr_warmup_fractionin__post_init__'s mutually-exclusive validator (if self.lr_warmup_fraction and self.lr_warmup_steps: raise). User passing only--train.lr_warmup_fraction 0.1immediately hitsValueError, forcing--train.lr_warmup_steps 0as a workaround.Classic mutually-exclusive-defaults anti-pattern.
Fix
litgpt/args.py(+9/-2):lr_warmup_stepsdefault from100toNone.__post_init__, after the conflict check, materialize the default:0whenlr_warmup_fractionis truthy, else100. Preserves prior behaviour for all existing call sites andlr_warmup_fraction=0semantics.Test
tests/test_args.py(+18 LOC, TDD RED→GREEN):test_lr_warmup_fraction_works_without_overriding_default_steps— was RED before (raisedValueError), now GREEN; assertswarmup_iters() == 30for fraction-only input.test_lr_warmup_conflict_only_when_both_explicitly_set— confirms validation still raises when both are explicitly set.pytest tests/test_args.py3/3 pass. Broadertest_args.py + test_config.py + test_utils.py + test_types.py→ 265 passed, 6 skipped (pre-existing missing-deps failures unrelated).Risk notes
adapter,adapter_v2,lora,lora_legacy,full) explicitly passlr_warmup_steps=100to their per-scriptTrainArgs(...)— unchanged behaviour.pretrain.pypasseslr_warmup_steps=2000explicitly — unchanged.TrainArgs()(no args) →lr_warmup_steps == 100, identical to before.lr_warmup_fractionin theunsupportedset, so the user-observable behaviour change is in pretraining only — exactly where the bug manifests.