[PyTorch] Preserve FP8 recompute state for inner autocast - #3284
[PyTorch] Preserve FP8 recompute state for inner autocast#3284nvegesna-netizen wants to merge 13 commits into
Conversation
839178b to
06dc465
Compare
Greptile SummaryUpdates FP8 activation-recompute tracking to use the live autocast state within checkpoint regions.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant Checkpoint
participant Region as Recompute region state
participant Callable
participant Autocast as Inner FP8 autocast
participant Module as FP8 module
Checkpoint->>Region: Enter checkpoint region
Checkpoint->>Callable: Execute forward
Callable->>Autocast: Enable FP8
Autocast->>Module: Run module
Module->>Region: Query region and live FP8 state
Region-->>Module: FP8 recompute enabled
Module->>Module: Stash FP8 metadata
Checkpoint->>Region: Enter recompute phase
Checkpoint->>Callable: Replay forward
Callable->>Autocast: Enable FP8
Autocast->>Module: Run module
Module->>Module: Restore stashed FP8 metadata
Reviews (14): Last reviewed commit: "Merge branch 'main' into fix/fp8-inner-a..." | Re-trigger Greptile |
|
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.