[Bugfix][Training] Avoid entropy grad graph when entropy_coef is zero#330
Open
Henry-Jessie wants to merge 1 commit into
Open
[Bugfix][Training] Avoid entropy grad graph when entropy_coef is zero#330Henry-Jessie wants to merge 1 commit into
Henry-Jessie wants to merge 1 commit into
Conversation
Skip entropy autograd graph and logits clone when entropy_coef is zero, while preserving the existing entropy path when entropy gradients are needed. Also keep allgather-CP zero fills and empty slices aligned with the reference tensor autograd state, following the corresponding slime fixes. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Henry <henryen.jessie@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request optimizes memory and computation during entropy calculation by introducing a with_entropy_grad flag. This flag conditionally disables gradient tracking (using torch.no_grad()) and avoids cloning logits when the entropy term does not affect the loss. Additionally, it optimizes empty tensor creation in _extract_per_sample by slicing existing tensors instead of calling torch.zeros, and ensures requires_grad is correctly propagated in _allgather_cp_redistribute. There are no review comments, so I have no feedback to provide.
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.
Summary
This PR reduces actor-update memory usage when entropy is logged but does not contribute to the loss.
Long-context actor updates can become memory-bound because entropy is evaluated over the full vocabulary. When
entropy_coef == 0, VIME still computes entropy metrics, but the entropy tensor is only used for reporting and has no gradient contribution. The current implementation always builds the entropy autograd graph and clones the full logits tensor before entropy computation, which increases peak memory without changing the loss.The change keeps the existing VIME log-probability and entropy implementation, but adds an explicit
with_entropy_gradgate:get_log_probs_and_entropy()computeswith_entropy_grad = with_entropy and getattr(args, "entropy_coef", 0.0) != 0.calculate_log_probs_and_entropy()computes entropy undertorch.no_grad()whenwith_entropy_gradis false.The PR also includes two allgather-CP graph fixes from slime #2123:
requires_grad=ref_value.requires_gradinstead of always requiring gradients;log_prob_full[:0]andentropy_full[:0]instead of newly allocated detached zero tensors.This keeps entropy values available for metrics while avoiding unnecessary entropy-backward activations when the entropy loss coefficient is zero.
Upstream references
This PR follows recent slime changes, but only applies the pieces that fit VIME's separate log-probability and entropy paths.
slime #2130 (open, not merged): proposed computing entropy under
torch.no_grad()and skipping the defensivelogits.clone()whenentropy_coef == 0on the separate_VocabParallelEntropypath. This is the closest conceptual match to VIME's current implementation.slime #2144 / slime #2152: slime later moved to a fused
_VocabParallelLogProbEntropyfunction and added an internalwith_entropy_gradgate. This PR borrows the same gate derivation,with_entropy_grad = with_entropy and getattr(args, "entropy_coef", 0.0) != 0, but does not port the fused function because VIME still uses separatecompute_log_probsand_VocabParallelEntropypaths.slime #2123: fixed allgather-CP graph participation issues by using
requires_grad=ref_value.requires_gradfor redistributed zero fills andlog_prob_full[:0]/entropy_full[:0]for empty slices. This PR includes those small fixes to keep allgather-CP tensors' autograd state consistent when entropy may be computed without gradients.Not included: the fused
_VocabParallelLogProbEntropyrefactor. Porting it would replace Megatron'sfused_vocab_parallel_cross_entropywith a custom vocab-parallel softmax, which is a larger behavioral change than this bug fix needs.Correctness
The no-grad branch is safe because
_VocabParallelEntropy.forward()does not modify the input logits in place. Its in-placeexp_()anddiv_()operations are applied to tensors derived fromvocab_parallel_logits - logits_max, not to the original logits tensor.The clone is still kept when entropy gradients are required, preserving the original behavior for nonzero entropy coefficients.
Using
getattr(args, "entropy_coef", 0.0) != 0keeps the gradient path for any nonzero coefficient, including negative values.The allgather-CP changes keep empty or missing response pieces aligned with the reference tensor's autograd state. This avoids creating artificial gradient paths for metric-only tensors, while still preserving graph connectivity for tensors that need to participate in distributed backward.
Validation
The profiling run followed the official GRPO training script structure in
scripts/run-qwen3.5-27B.sh: Ray job submission totrain.py, colocated actor/rollout, DAPO-style math data, GRPO withentropy_coef=0, dynamic batching, recompute, and Megatron context parallelism. The run was adapted to Qwen3.5-2B.Run setup:
entropy_coef=0,TP=2,CP=2, rollout max response length 32krollout-num-gpus-per-engine=4,global-batch-size=128,max-tokens-per-gpu=16384, andlog-probs-chunk-size=1024For matched ~32k-token actor microbatches, the internal loss-segment
torch.cuda.max_memory_allocatedpeak was:a reduction of ~15 GiB (~37%). This matches expectations: at ~32k tokens with vocab 248320 and
TP=2(V_local = 124160, fp32), one full[T, V_local]tensor is ~15 GiB, and the patch removes the entropy autograd graph and defensive logits clone that contribute that order of magnitude to the loss-segment peak.AI Assistance
This PR was prepared with AI assistance from OpenAI Codex. I reviewed the changed code, compared it against the relevant slime changes, and validated the patch locally.