Add optional gradient clipping and non-finite guard to the LoRA trainer - #1659
Add optional gradient clipping and non-finite guard to the LoRA trainer#1659axiom-of-choice wants to merge 1 commit into
Conversation
The trainer had no bound on gradient size and no guard against non-finite values, so a single overflowing batch left the weights in NaN permanently and the rest of the run was lost. Add `grad_clip` to TrainingArgs and `--grad-clip` to mlx_lm.lora. When set, the accumulated and averaged gradient is clipped to that global norm just before the optimizer update, so it composes with grad_accumulation_steps and average_gradients rather than clipping each micro-batch. clip_grad_norm propagates non-finite values instead of removing them: a NaN in one entry makes the norm NaN and scales every parameter to NaN, and an inf norm makes the normalizer zero, so inf * 0 is also NaN. Clipping alone therefore does not protect against the failure above. Zero the gradient for that step as well, keeping the check in the compiled graph with mx.where so it costs no device sync. This is not a full no-op: a momentum optimizer still moves the weights from its existing state, but the non-finite values never reach them. Both are off unless the flag is passed; the default trajectory is unchanged. Steps with a non-finite norm are reported as a warning and surfaced to training callbacks as max_grad_norm and non_finite_grad_steps.
|
Additional diagnostics, because I wanted to rule out the obvious competing explanation before leaning on "a safety net is the right fix". The batch that overflowed was 4x2048 with all four sequences at ~2030 tokens, i.e. about as long as this dataset gets. So "long sequences overflow" was the natural alternative. It does not hold up. Re-running forward and backward on those exact rows with weights at init and no optimizer updates, batch size 1: Two conclusions. The culprit rows are perfectly healthy in isolation: the row that produced a NaN mid-run gives norm 4.06 at init. And length does not correlate in the direction the hypothesis needs, in fact it inverts, the shortest rows produce the highest gradient norms here. Not a context-window effect either: So the overflow requires the accumulated weight state of a real training run. It's an interaction between the adapted LoRA weights and a batch, not a property of any batch or of any sequence length. No single row reproduces it in isolation. That is the case for a guard rather than for finding and removing the offending data: there is nothing wrong with the data to remove. The run that skipped the batch kept training and the loss kept falling. I mention this mainly because #361 was closed after moving toward data format, and #49 after an mlx upgrade. This failure mode is neither, and without something in the trainer there is nothing a user can do about it beyond lowering the learning rate and hoping, which as noted in #1658 did not work here either (NaN at lr 4.5e-7). |
|
Note: this and #1665 both touch |
Closes #1658 (opened for discussion; opening the PR since the implementation is done and tested, happy to change either design decision).
Summary
The LoRA trainer has no bound on gradient size and no guard against non-finite gradients. One batch whose gradient overflows puts the LoRA weights into NaN permanently, and every later iteration reports
nanwith no warning and no recovery path.This adds
grad_cliptoTrainingArgsand--grad-cliptomlx_lm.lora. Off by default, so no existing run changes behavior.High-level changes
mlx_lm/tuner/trainer.py:grad_clip: Optional[float] = NoneonTrainingArgs; clipping plus a non-finite guard inside the compiledstep();max_grad_normandnon_finite_grad_stepssurfaced to training callbacks; a warning when a step is dropped.mlx_lm/lora.py:--grad-clipflag and itsCONFIG_DEFAULTSentry.mlx_lm/LORA.md: a "NaN Loss" section.mlx_lm/examples/lora_config.yaml:grad_clip: null.tests/test_tuner_grad_clip.py: 8 tests driving the realtrain().Clipping is applied to the already-accumulated and averaged gradient, immediately before
optimizer.update, so it composes withgrad_accumulation_stepsandaverage_gradientsrather than clipping each micro-batch separately.Why clipping alone is not enough
clip_grad_normpropagates non-finite values rather than removing them. A NaN in one entry makes the norm NaN, which scales every parameter to NaN. Aninfnorm makes the normalizer0, andinf * 0is NaN, so infinite gradients also poison every parameter:[nan, 1.0],[2.0, 3.0][inf, 1.0],[2.0, 3.0][nan, 0.0],[0.0, 0.0]So clipping covers large-but-finite gradients, a different failure from the one that motivated the issue. The guard zeroes the gradient when the norm is not finite:
mx.wherekeeps the check inside the compiled graph. A Python branch ongrad_normwould force a device sync every iteration.This is not a full no-op: a momentum optimizer still moves the weights from its existing state, and AdamW applies weight decay independently of the gradient (measured 1.07e-07 relative at lr=5e-6; exactly 0 with Adam or
weight_decay=0.0). The non-finite values never reach the weights, which is the guarantee. Documented inLORA.mdand in a code comment.Evaluation
The default is inert. 30 iterations, same seed,
mainat e5baded vs this branch with no--grad-clip:Losses bit-identical. Weights differ by 2.98e-08, but
maindiffers from itself across two runs by exactly the same 2.98e-08, so that is run-to-run nondeterminism, not this change.It catches a real overflow and the run recovers. Qwen3-1.7B, LoRA rank 8 over 16 layers, batch 4, max_seq 2048, AdamW, cosine + warmup 100. This config previously died at iter 10 with
Train loss nanand never recovered. With--grad-clip 1.0:Two things worth noting. The loss was finite (1.2309) while the norm was NaN, so the overflow is in the backward pass and is invisible to loss-watching; by the time the loss prints
nanthe weights are already lost. And the norm escalated4.2 → 5044 → 1.78e8 → nanwith clipping active the whole time, so clipping alone would not have saved this run. One skip in 60 iterations, and the lowest loss of the run came after it.Per-iteration cost is below measurement noise. 5 interleaved pairs of 40 iterations on a synthetic model: median 32.09 ms without, 33.78 ms with, stdev 15.33 and 10.19. I am not claiming a number from that; the difference is inside the noise floor. The added work is one
square().sum()tree-reduce plus awhereper parameter, all inside the existing compiled graph.Environment: mlx 0.32.0, M2 Pro 32 GB, macOS 26.5.2.
Risks
clip_grad_normsurvivesmx.compile, since it introduces a data-dependent value. Verified it does, including across a step with a 1000x gradient spike, and the real run above is 60+ compiled iterations with no tracing error.--grad-clip <= 0raisesValueErrorrather than silently doing nothing.Tests
The new tests drive the real
train()rather than a reimplementation, and include a control asserting that without the flag the weights do go non-finite, so the guard is tested against the actual current behavior. Note that a bad batch has to be marked in the batch data rather than by a Python-side counter, sincemx.compiletraces the loss once and would otherwise bake the overflow into every later iteration.Open questions
Happy to split, rename, or drop either half.
Pre-merge
Nothing required beyond review. No new dependencies, no migration, no config changes for existing users.
Post-merge
If you want,
--grad-clip 1.0could be mentioned in the LoRA docs as a first thing to try when a fine-tune reportsnan, since #49 and #361 both drew several reports of that symptom.Not in this PR
Two adjacent things I found while diagnosing this and deliberately left out:
iterate_batchesgates its seeding onif seed:, so an explicitseed=0is silently ignored, and0is the default inCONFIG_DEFAULTS. Filed as iterate_batches ignores seed=0 due toif seed:, and 0 is the default seed #1660. Note batch order for a normal CLI run is still reproducible, sincerun()seeds global numpy before training; this only bites when calling the API directly withseed=0, or when something else draws from numpy in between.CacheDataset.itemlendoeslen(self._data[idx]), and all three dataset classes return the raw record from__getitem__, so for{"text": ...}it returns 1 for every row and the length sort initerate_batchesnever sorts. That is a throughput and memory issue, not a correctness one. I measured 31.6% extra padded compute on one 8786-row dataset and will open it separately.