run_megatron: dump parameter grads, fix CP loss normalisation, skip DDP on forward-only runs - #1823
Open
yueming-yuan wants to merge 1 commit into
Open
run_megatron: dump parameter grads, fix CP loss normalisation, skip DDP on forward-only runs#1823yueming-yuan wants to merge 1 commit into
yueming-yuan wants to merge 1 commit into
Conversation
…DP on forward-only runs Three model-agnostic fixes to the standalone run_megatron worker. 1. --run-backward produced no parameter gradients at all. The worker registered the non-intrusive (activation) dumper but never called dump_model, and DUMPER_ENABLE_MODEL_GRAD is only consumed inside it. The training path reaches dump_model via DumperMegatronUtil.finalize; the worker had no equivalent. get_grad is required alongside: the dumper's default is a bare param.grad read, and under Megatron DDP the gradient lives in param.main_grad with param.grad left None, so calling dump_model without it still yields a silently empty dump. The distributed-optimizer shard gather in dumper_utils is only needed for the FT trainer. 2. loss_func used reduction='mean', which under context parallelism divides by each rank's own valid-token count rather than the global one. Every activation gradient comes out scaled by global/local -- about 2x at CP=2 -- so a CP run compared against CP=1 shows a spurious ~0.5 relative difference that swamps any real discrepancy. 3. get_model always wrapped in DDP, allocating per-parameter grad buffers even for forward-only runs, which OOMs on large models when only logits/logprobs are wanted. Also mirror the miles backend's variable_seq_lengths, which Megatron's validate_args unconditionally resets to False.
yueming-yuan
requested review from
Shi-Dong,
Zhichenzzz,
fzyzcjy,
guapisolo,
jybsuper and
maocheng23
as code owners
July 27, 2026 14:55
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
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.
Three model-agnostic fixes to the standalone
run_megatronworker, found whileusing it for cross-stack forward/backward alignment.
1.
--run-backwardproduced no parameter gradients at allThe worker called
dumper.register_non_intrusive_dumper(m)(activations) and_finalize_dumper()(step()+configure(enable=False)), but neverdump_model.DUMPER_ENABLE_MODEL_GRADis only consumed insidedump_model(
enable_curr_grad=self._config.enable_model_grad), so setting it producedactivations and zero parameter gradients — no error, no warning, empty dump.
The training path reaches
dump_modelthroughDumperMegatronUtil.finalize(
miles/utils/dumper_utils.py); the standalone worker had no equivalent.get_gradis required alongside, not optional. Onsglang-miles,dumper.py:509reads:a bare
.gradwith nomain_gradfallback. Under Megatron DDP the gradientlives in
param.main_gradwithparam.gradleftNone, so callingdump_modelwithout a getter still yields a silently empty dump. The heavierdistributed-optimizer shard gather in
dumper_utils._build_full_grad_getterisonly needed for the FT trainer, whose reduce-scatter leaves each rank holding a
partial buffer — the standalone worker has no reduce-scatter, so the cheap
.grad or main_gradread is sufficient.step=is deliberately not passed:dump_modelonsglang-mileshas nostepparameter, so it lands in
**kwargs→extra_kwargs→tags, and_dump_singlethen doesdict(step=..., rank=..., **tags)→TypeError: dict() got multiple values for keyword argument 'step'.2.
loss_funcnormalised by the local valid-token count under CPreduction="mean"divides by each rank's own valid count, but under contextparallelism the labels are sharded. Every activation gradient comes out scaled
by global/local — about 2x at CP=2 — so a CP run compared against CP=1 shows a
spurious ~0.5 relative difference that swamps any real discrepancy.
Summing locally and dividing by the CP-reduced count makes each rank contribute
its true share of the global mean: the per-rank losses add up to the CP=1 loss
and the gradients match position for position.
3. Forward-only runs still allocated DDP grad buffers
get_modelwas always called with the defaultwrap_with_ddp=True, so aforward-only run paid for per-parameter grad buffers it never used, OOMing on
large models when only logits/logprobs were wanted. Now keyed off
script.run_backward, withm.train(script.run_backward)to match.Also mirrors the miles backend's
variable_seq_lengths, which Megatron'svalidate_argsunconditionally resets toFalse.Adjacent issue, not fixed here
By the same
step=mechanism,miles/utils/dumper_utils.pycallingdumper.dump_model(extracted_model, get_grad=get_grad, step=0)would raiseTypeErroragainstsglang-miles. Left alone since it is a different codepath; flagging it because anyone enabling
DUMPER_ENABLE_MODEL_GRADon thetraining path will hit it. Happy to fix in a follow-up.
Note also that
dumper_utils.finalizeonly builds aget_gradwhenenable_experimental_ft_trainer()is true and passesNoneotherwise — which,per the bare-
.graddefault above, means non-FT Megatron DDP runs dump emptygradients there too.
_log_model_grad_coverageuses the correct.grad or main_gradread, so the two disagree.Testing
pre-commitandpy_compilepass. I could not run the fast tests locally — nosglang/megatronon this host — so CI is the first real exercise.