Fix doubled lm_head gradient in Unsloth_Offloaded_Log_Softmax backward#892
Fix doubled lm_head gradient in Unsloth_Offloaded_Log_Softmax backward#892danielhanchen wants to merge 4 commits intomainunslothai/unsloth-zoo:mainfrom fix/offloaded-logsoftmax-gradunslothai/unsloth-zoo:fix/offloaded-logsoftmax-gradCopy head branch name to clipboard
Conversation
The offloaded log-softmax Function recomputes its output in backward, then called torch.autograd.backward(output, grad_output) and returned lm_head.grad. That writes the leaf .grad and returns it as the op's gradient contribution, so the outer engine's AccumulateGrad adds it again: lm_head gradients came out 2x too large for a single use, and more when lm_head is shared across chunks (the normal GRPO path), and any grad already accumulated from a previous micro-batch was corrupted. Switch to torch.autograd.grad, which returns each input's local gradient without touching leaf .grad. hidden_states.grad was already correct (its reloaded tensor is a private leaf) and is unchanged. Only reachable on the offload path (long-completion / large-batch GRPO) with a trainable lm_head or tied embeddings; frozen lm_head (standard LoRA) returned None and was unaffected.
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request replaces the use of torch.autograd.backward with torch.autograd.grad in the backward pass of Unsloth_Offloaded_Log_Softmax to correctly compute and return local gradient contributions, preventing gradient double-counting on lm_head. It also adds comprehensive unit tests to verify this behavior. The reviewer recommends adding a check to handle cases where grad_output is None in the backward method to prevent potential runtime errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| grad_inputs = torch.autograd.grad( | ||
| output, | ||
| (hidden_states, lm_head) if ctx.lm_head_requires_grad else (hidden_states,), | ||
| grad_output, | ||
| ) |
There was a problem hiding this comment.
In PyTorch custom autograd functions, always check if the incoming gradient tensor (grad_output) is None in the backward method and return early if so. This prevents runtime AttributeError or other errors when downstream layers or base models are frozen and do not require gradients.
if grad_output is None:
return (None, None, None, None, None, None, None, None)
grad_inputs = torch.autograd.grad(
output,
(hidden_states, lm_head) if ctx.lm_head_requires_grad else (hidden_states,),
grad_output,
)References
- In PyTorch custom autograd functions, always check if the incoming gradient tensor is None in the backward method and return early if so. This prevents runtime AttributeError when downstream layers or base models are frozen and do not require gradients.
There was a problem hiding this comment.
This Function keeps the default materialize_grads=True (it never calls ctx.set_materialize_grads(False)), so PyTorch materializes any unused-output gradient as a zero tensor before calling backward. grad_output is therefore never None here, and the single output (the per-token logprobs) always feeds the loss regardless. A None guard would be unreachable dead code, so leaving it out.
Optimize the CPU-offloading log-softmax Function used by the large-batch or long-completion GRPO logprob path, on top of the local-input-grads fix: - Offload hidden_states through a pinned host buffer on a side CUDA stream so the D2H copy runs asynchronously and overlaps the no_grad forward; backward waits on the recorded copy event before the H2D reload, which is required for correctness with pinned non_blocking copies. - Skip the CPU round-trip entirely when the GPU has generous headroom (free memory at least 4x the tensor size); the detached alias adds no allocation and the big logits are still checkpointed via recompute. - Drop the dead post-reload dtype cast: the saved tensor already keeps the forward dtype. - Fall back to the original pageable copy on CPU tensors, missing CUDA, or pinned allocation failure, keeping Windows/Mac/CPU behavior unchanged. Outputs and gradients are bitwise identical to the previous implementation for both frozen and trainable lm_head, and fwd+bwd time on B=16,T=2048,H=2048,V=32000 bf16 with 256 chunks is equal or slightly better in all measured cases. Add CPU tests pinning the fallback numerics and the event-sync and is_cuda-guard source invariants.
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Commit 5c9f364 is a comment-only cleanup (verified AST-identical code, tests unchanged at 18 passing). |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c9f3646a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if tensor_bytes * 4 <= free_bytes: | ||
| # Enough headroom: keep the alias on GPU (mem_get_info failure defaults to offload). | ||
| saved_hidden_states = detached_hidden_states |
There was a problem hiding this comment.
Keep offloaded hidden states off GPU between chunks
In the padded GRPO path with multiple zipped_inputs chunks, this branch retains each saved hidden-state tensor on CUDA until backward. At the call site the input is a [:, :-1, :] slice, so detached_hidden_states = hidden_states.detach().contiguous() has usually just allocated a full contiguous GPU copy; keeping it here means those copies accumulate across chunks instead of being released after the CPU offload as before. Long-completion/large-batch runs that relied on this offloaded path to fit can now OOM even when each individual tensor satisfies the 4x-free-memory check, because the recomputed logits still need additional temporary memory. Please keep this path offloaded, or make the GPU-retention path opt-in with a check that accounts for all retained chunks and recompute temporaries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 05d9c63: the keep decision now budgets cumulatively, 4 * (tensor_bytes + retained_this_step) <= free_bytes, capping retention at a quarter of measured free memory with at least three quarters left for the backward recompute. The counter lives in grpo_accumulated_loss scope and is recreated on every call, so no state can leak across steps. Verified with a 16 chunk retention test (old head kept 11 of 16 chunks, fixed head kept 3, all gradients bitwise exact) and the keep-on-GPU perf win is unchanged within noise.
The padded GRPO path calls Unsloth_Offloaded_Log_Softmax once per zipped_inputs chunk before any backward runs. The per-call check tensor_bytes * 4 <= free_bytes only sees current free memory, so with N chunks each sized at the threshold the retained total approaches free * (1 - (3/4)^N), i.e. nearly all initially free memory, starving backward's logit recompute. Track bytes kept this step in a function-local counter and require 4 * (tensor_bytes + retained) <= free_bytes, capping retention at 1/4 of measured free memory. The counter is recreated on every grpo_accumulated_loss call, so no state can leak across steps, eval passes, or exceptions. Single-chunk behavior is unchanged.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
What
Unsloth_Offloaded_Log_Softmax(the CPU-offloading / checkpointed log-softmax used for the large / long-completion GRPO logprob path viaefficient_log_softmax) recomputes its output in backward, then did:torch.autograd.backwardwrites into the leaf.gradattributes. Returninglm_head.gradas this op's gradient contribution then gets accumulated again by the outer engine'sAccumulateGrad, so:lm_headis shared across chunks (the normal GRPO path -- the same output embedding feeds everycompute_logprobs_chunkcall), andlm_headfrom a previous micro-batch is corrupted (returned as2*pre + 2*ginstead ofpre + g).Fix
Use
torch.autograd.grad(output, inputs, grad_output), which returns each input's local gradient contribution without touching leaf.grad.hidden_states.gradwas already correct (its reloaded tensor is a private leaf) and is unchanged.Reachability
Only on the offload path (
index.shape[1] > 1024orbatch_size > 8, i.e. long-completion / large-batch GRPO) and with a trainablelm_head/ tied embeddings (full fine-tuning, or LoRA withmodules_to_save=["lm_head"]). Standard LoRA freezeslm_head, so the old path returnedNoneand was unaffected.Verification
Reproduced against the non-offloaded twin
chunked_hidden_states_selective_log_softmaxas oracle: before the fix,|lm_head.grad| / |ref|was exactly 2.0 (single use) and 3.0 (two shared uses), with pre-existing grad corrupted to2*pre + 2*g; after the fix all cases match the oracle exactly (ratio 1.0, max abs diff 0.0), andhidden_states.gradstays exact. Added a CPU regression test that pins the pattern (fixed matches plain-autograd oracle for single / shared / pre-existing-grad; the old pattern provably diverges) plus a source guard.