fix(optim): reject nesterov and dampening in SGD and LARS - #2085
Open
ConnorMoss02 wants to merge 1 commit into
Open
ConnorMoss02 wants to merge 1 commit into
ConnorMoss02 wants to merge 1 commit into
Conversation
All six SGD and LARS classes accepted `nesterov` and `dampening`, and the SGD docs advertised Nesterov momentum, but neither argument reached any update path. `nesterov` was never referenced after the signature and `dampening` was passed as betas[1], which no momentum kernel reads, so both were silently ignored. Reject non-default values instead, mirroring the existing Adam8bit and AdamW8bit guards, and correct the docs sentence. PytorchLARS implements both and is unchanged.
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
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.
Problem
SGD,SGD8bit,SGD32bit,LARS,LARS8bitandLARS32bitacceptnesterovanddampening, but neither reaches any update path:nesterovis never referenced after the constructor signature.dampeningis passed to the base optimizer asbetas[1], which no momentum path reads.So both are silently ignored.
bnb.optim.SGD(..., nesterov=True)is bit-identical to plain momentum, which is a different update rule:Both are 0.000000 for all six classes.
docs/source/reference/optim/sgd.mdxalso stated that bitsandbytes supports Nesterov momentum.Fix
nesterov=Trueand ondampening != 0in all six classes, mirroring the existingAdam8bit/AdamW8bitguards (relates to Wrong doc and function signature for 8-bit optim #1261). Add docstring notes.Notes
PytorchLARSis unchanged: it implementsnesterovitself and already validates it (lars.py,"Nesterov momentum requires a momentum and zero dampening").Tests (
tests/test_optim.py)test_sgd_lars_reject_nesterov,test_sgd_lars_reject_dampening— parametrized over all six classes; defaults still construct.test_pytorch_lars_still_supports_nesterov— behavioral, guards the boundary of this change.pre-commit run --all-filespasses. 12 of the 13 new cases fail with the source reverted; thePytorchLARScase passes either way, as it should. On this machinetests/test_optim.py -k "momentum or lars or sgd"gives 54 passed / 9 failed against 41 passed / 9 failed on main: the same 9 aretest_optimizer8bit, failing withNotImplementedError: The operator 'bitsandbytes::optimizer_update_8bit_blockwise' is not implementedbecause there is no CUDA device here.Relates to #1261