[PyTorch] Preserve FP8 recompute state for inner autocast - #3284
[PyTorch] Preserve FP8 recompute state for inner autocast#3284nvegesna-netizen wants to merge 12 commits into
Conversation
839178b to
06dc465
Compare
Greptile SummaryPreserves FP8 activation-recompute bookkeeping when autocast begins inside a checkpointed callable.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (13): Last reviewed commit: "Merge branch 'main' into fix/fp8-inner-a..." | Re-trigger Greptile |
| # example, to select precision per layer). Delayed-scaling modules in | ||
| # that inner context must still save their scale and amax metadata for | ||
| # the recompute forward. | ||
| _FP8_ACTIVATION_RECOMPUTE_ENABLED = self.activation_recompute |
There was a problem hiding this comment.
With the gate moved to the getter, _FP8_ACTIVATION_RECOMPUTE_ENABLED no longer has anything to do with FP8 — it now just means "inside an activation recompute region". Worth renaming to _IN_ACTIVATION_RECOMPUTE_REGION; it's free, the global only appears in this file, and is_fp8_activation_recompute_enabled() keeps its name since it now returns the conjunction.
There was a problem hiding this comment.
Done in 281e97f — renamed to _IN_ACTIVATION_RECOMPUTE_REGION across all six references (the declaration, both global statements, both writes, and the read in the getter). I confirmed the global appears in no other file in the repo and isn't in __all__, so nothing external breaks. is_fp8_activation_recompute_enabled() keeps its name and now returns the conjunction, as you suggested.
Two adjacent things I deliberately left alone — happy to fold either in if you'd prefer:
_FP8_ACTIVATION_RECOMPUTE_PHASEhas the same misnomer:in_fp8_activation_recompute_phase()returns it ungated, with callers supplying the FP8 gate themselves. Renaming it to_ACTIVATION_RECOMPUTE_PHASEwould keep the pair symmetric — they sit on adjacent lines and share bothglobalstatements — but it's pre-existing and outside the scope of this fix, so the asymmetry here is intentional rather than an oversight.__exit__resets both globals toFalserather than restoring the previous value. Also pre-existing, but the new name makes the nested-checkpointcase more visible: an inner region's exit clears the flag while still lexically inside the outer region. This PR neither introduces nor fixes that; say the word if you'd rather see save/restore semantics while we're here.
There was a problem hiding this comment.
Follow-up: I went ahead and did the symmetric rename too, in 199aa6f — _FP8_ACTIVATION_RECOMPUTE_PHASE is now _ACTIVATION_RECOMPUTE_PHASE, so the pair reads consistently.
Same verification as the first one: 6 occurrences, all in this file, nothing external and no dynamic access. Both public getters are untouched — in_fp8_activation_recompute_phase() keeps its name, and I confirmed all 18 call sites across base.py, linear.py, grouped_linear.py, layernorm_linear.py and layernorm_mlp.py are unchanged. The rename is confined to the private global, which is consistent with those callers already supplying their own FP8 gate.
|
Could the tests go into |
Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
bdfc6c3 to
174cc88
Compare
Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
174cc88 to
00c727f
Compare
The global no longer encodes FP8 state now that the FP8 gate lives in is_fp8_activation_recompute_enabled(); it only marks the checkpoint region. Rename it to match. The public getter keeps its name since it now returns the conjunction of the region flag and the current FP8 state. Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
_FP8_ACTIVATION_RECOMPUTE_PHASE carries no FP8 state either - in_fp8_activation_recompute_phase() returns it ungated and callers apply their own FP8 gate. Rename to _ACTIVATION_RECOMPUTE_PHASE to match _IN_ACTIVATION_RECOMPUTE_REGION. Both public getters keep their names. Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
|
Done — moved into What changed in the tests
I also restored two tests that my earlier move had silently dropped:
ValidationRan on an H100, pinned to the branch commit: Locally: Note the last commit (assertion rework + a One limitation worth statingThe numeric comparison in the inner-autocast test cannot currently catch a wrong-scale restore — only a missing one. The minimal way to give the numeric arm teeth is Separately, could you kick off Scope and a known remaining gapWriting this out because the change is easy to under-describe. What it makes reachable. Previously, delayed-scaling recipes crashed with Known remaining gap. With This gating predates the change and is untouched by it — it was simply unreachable while the configuration crashed. It also needs qualifying: because How much it costs, if hit. The quantization stays self-consistent —
So: negligible for activations, a few percent for typical weights, and it degrades sharply only for very small-magnitude tensors. I would call it suboptimal FP8 range utilization rather than an accuracy bug — but it is entirely silent, with no warning or diagnostic anywhere. I have not fixed it here. The phase-1 gate is deliberate, and the update it guards triggers a cross-rank amax collective, so relaxing it changes when a collective fires — not something to guess at without multi-GPU validation. I would rather disclose it than paper over it. Happy to follow up with either the gate relaxation or a one-time warning when a delayed-scaling module is stashed in a recompute region that cannot reach depth 0, alongside two other pre-existing items I found while reviewing this (the non-restoring One more scope note. |
…ping Moving the regression into test_numerics.py silently dropped two of the three original tests. Restore both: the non-FP8 negative case (which needs no FP8 hardware and is the direct guard for making the region flag FP8-agnostic) and the mixed FP8/non-FP8 region case. The inner-autocast test could not observe a missing stash: the outer autocast is disabled, so autocast_depth never returns to 0 with FP8 enabled, reduce_and_update_fp8_tensors is never called, and the forward scale stays at 1.0 - making stashed and unstashed recompute identical. Record the stash/restore of the forward scale and assert every stash is restored exactly once, and use an inner-autocast reference so the two runs differ only by recompute. Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
The scale equality assertion was vacuous: record_restore read the scale after the real restore had already copied the stashed value into it, so it compared a tensor with itself, and every scale is 1.0 here anyway. Count stash and restore per module instead, and check a module was stashed before the real restore runs so a regression reports that rather than a KeyError from the recompute buffer lookup. Also skip the non-FP8 checkpoint test when bf16 is unavailable (it has no FP8 skipif, so it would otherwise run on pre-Ampere), and correct the is_fp8_activation_recompute_enabled docstring, which still claimed to return a bare global. Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
Counting stashes and restores catches a missing stash but not a wrong one. The outputs cannot catch either: the forward scale never moves here (autocast_depth stays >= 1 so reduce_and_update_fp8_tensors never fires), and a wrong scale would only perturb FP8 rounding regardless, since scale_inv is derived from the same scale at cast time. Compare the state each restore installs against a clone captured at stash time, for both the scale and the amax history, and assert at least one restore actually moved the live state so the comparison is not vacuous. This is exact rather than tolerance-based and works for both reentrant and non-reentrant checkpointing. Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
Comparing the restored state against a clone taken in the stash wrapper could not fail: the real stash clones the same tensors at the same instant, and the restore copies that entry back, so both sides derive from one snapshot. It exercised the stash/restore plumbing rather than anything this change touches. Keep the checks that do have teeth - that some module stashed, and that stashes and restores balance per module - and skip the bookkeeping for modules whose recipe is not delayed scaling, since the restore site is not gated on it while the stash site is. Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Replace test_checkpoint_without_fp8_does_not_save_fp8_recompute_state, which could not fail: with no autocast self.fp8 is False, so the stash site is unreachable before the region flag is ever consulted, and the assertion holds no matter what the getter returns. The mixed-region test already makes the same negative claim in an FP8-enabled session. Assert instead on is_fp8_activation_recompute_enabled() itself from inside a checkpointed callable that opens its own autocast: True in both phases inside the autocast, False outside it. The first half fails before the fix; the second pins the FP8 term against a later over-correction that drops it. Also move the fp8_meta key constant up to the other module constants. Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com>
Description
The FP8 activation-recompute stash and its restore are gated on conditions
sampled at different times, so they can disagree.
The stash in
prepare_forwardis gated onis_fp8_activation_recompute_enabled(),which was latched from
FP8GlobalStateManager.is_fp8_enabled()at the momentte.checkpointwas entered. The matching restore is gated onself.fp8, whichis read when the module actually runs. If the checkpointed callable opens its own
autocast— selecting precision per layer, for example — FP8 is off at checkpointentry and on at module execution. Phase 1 then skips the stash while the recompute
phase still restores, and
get_old_fp8_meta_tensors_for_recomputeraisesKeyError: 'global_fp8_buffer_pos_fwd_recompute'.This is delayed-scaling only: the stash helpers early-return for every other recipe.
Megatron already works around this rather than hitting it.
transformer_block.pyroutes delayed scaling to an outer context and everything else to an inner one, and
multi_token_prediction.pycarries an eleven-line comment naming this exact failure(
2b77d32b1, May 2026) explaining why the outer context is required there. The trapis real enough that someone paid for it; this change removes it rather than requiring
callers to route around it.
Changes
the FP8 state in
is_fp8_activation_recompute_enabled()instead, so both gatesderive from the same live autocast state.
getters keep their names.
checkpoint regions in
test_numerics.py, which is already run by L0.A behaviour change worth calling out
is_fp8_activation_recompute_enabled()also drivescolumnwise_usageinlinear.pyandgrouped_linear.py, and that path is not delayed-scaling only.In the inner-autocast pattern the predicate now returns
Truefor every recipe, sothe weight quantizer is asked for a columnwise copy in phase 1 where it previously
was not. That matches what the outer-autocast pattern already does, and phase 1 runs
under
no_grad, so this is the value the recompute needs — but it is a memory andwork change on a recipe-independent path, and it is not covered by the tests here.
Flagging it explicitly for review.
Validation
use_reentrantvalues.-k activation_recomputefamily passes on both: 218 tests on H100, 416on GB200 (the Blackwell recipe list is longer, so MXFP8, block-scaling and NVFP4
recompute paths are exercised there too).
black,pylint 3.3.1(10.00/10 on the changed source file) and the license checkpass locally.
Known limitation, not addressed here
The stash is gated on
self.trainingand the restores are not, so a module left in.eval()inside a grad-enabled checkpointed region still hits the sameKeyError.That is the other half of the same asymmetry and needs a different fix — pairing the
restore to a decision recorded at stash time rather than re-deriving it in a later
autograd phase — so it is left for a follow-up rather than bundled here.