fix(flux): clone the validation loss out of Megatron's rescale path - #1079
Open
olehtika wants to merge 2 commits into
Open
fix(flux): clone the validation loss out of Megatron's rescale path#1079olehtika wants to merge 2 commits into
olehtika wants to merge 2 commits into
Conversation
Megatron's forward_step takes the first element of what a loss function returns as the tensor to backpropagate and rescales it IN PLACE -- `output_tensor *= cp_group_size`, then `output_tensor /= num_microbatches` -- and only then stores the reported dict. The diffusion validation path put `loss_sum.detach()` in that dict, and a detached tensor shares storage with the one it came from, so the reported loss was rescaled along with it. What reached the caller was therefore the true validation loss divided by the number of microbatches. At one microbatch per rank per step the divisor is 1 and nothing shows, which is why every shape measured so far looked right. At two it halves, and under mlperf_mode the halved value is what the convergence gate is compared against -- so a run reports convergence at roughly half the samples it actually needed, and reports it as a pass. Measured on Flux 12B at matched global batch 512: micro batch 32 reported 0.630950 at step 100 where micro batch 64 reported 1.266634, a ratio of 2.007. With this fix the two agree to 0.7%, and to 0.01% by step 200. Context parallelism has the mirror problem, inflating the reported loss by cp_group_size. The training path a few lines below already clones for this reason.
olehtika
requested review from
Xiaoming-AMD,
limou102 and
wenxie-amd
as code owners
September 2, 2026 04:28
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix directly addresses a confirmed aliasing bug and is covered by a focused regression test that fails pre-fix and passes post-fix.
Pull request overview
Fixes incorrect validation-loss reporting in Megatron diffusion training by preventing Megatron’s in-place rescale of the backprop tensor from mutating the reported validation numerator (which previously shared storage via detach()).
Changes:
- Clone the detached validation-loss sum before returning it in the reporting dict so Megatron’s in-place rescale cannot affect the reported metric.
- Add a unit test that reproduces Megatron’s in-place rescale behavior and asserts the reported
(loss_sum, sample_count)pair remains stable.
File summaries
| File | Description |
|---|---|
primus/backends/megatron/diffusion_trainer.py |
Prevents validation metric aliasing by returning a detached clone for the reported loss numerator. |
tests/unit_tests/backends/megatron/diffusion/training/test_diffusion_trainer.py |
Adds a regression test ensuring reported validation loss is invariant to Megatron’s in-place rescale of the backprop tensor. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix addresses the confirmed tensor-aliasing root cause and is covered by a targeted regression unit test that fails without the change.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
What is wrong
DiffusionPretrainTrainer.forward_step's validation loss function returnsMegatron's
forward_steptakes the first element as the tensor to backpropagate and rescales it in place before storing the reported dict:loss_sum.detach()shares storage withloss_sum, so the reported numerator is rescaled along with the backpropagated tensor. What reaches the caller is the true validation loss divided by the number of microbatches (and, with context parallelism, multiplied bycp_group_size).Why it matters
At one microbatch per rank per step the divisor is 1, so nothing is visible — which is why every shape measured until now looked correct. At two microbatches the reported validation loss halves, and under
mlperf_modethat halved value is what the convergence gate is compared against. A run then reports convergence at roughly half the samples it actually needed, and reports it as a pass. Any Flux MLPerf configuration that reaches its global batch through gradient accumulation is affected.Measured on Flux 12B (MI355X), matched global batch 512, same stack, same node:
Before the fix the ratio between the two configurations is 2.007 at step 100 and 1.972 at step 200. After it they agree to 0.7% at step 100 and 0.01% at step 200. The same check on two nodes at global batch 1024: micro batch 32 now reports 0.767331 at step 256 against micro batch 64's 0.766881, where before the fix it reported 0.383666 and tripped the 0.586 gate at its first evaluation.
Evaluation coverage was never at fault — both configurations report
covered 29696 samples— only the reported value.The fix
Clone rather than alias, which is what the training path a few lines below already does for
reduced_train_loss.Test plan
test_validation_report_survives_megatrons_in_place_rescalereproduces Megatron's in-place rescale and asserts the reported pair does not move. It fails withassert 8.0 == 16.0without the fix.tests/unit_tests/backends/megatron/diffusion/training/test_diffusion_trainer.pypasses in full (8 passed).Made with Cursor