Fix duplicate FP8 state updates during activation checkpoint recomputation#3213
Fix duplicate FP8 state updates during activation checkpoint recomputation#3213AlbertYang514 wants to merge 5 commits into
Conversation
…compute Signed-off-by: AlbertYang514 <201034045+AlbertYang514@users.noreply.github.com>
Signed-off-by: AlbertYang514 <201034045+AlbertYang514@users.noreply.github.com>
Greptile SummaryThis PR fixes a real bug where
Confidence Score: 5/5The change is safe to merge: it fixes a well-documented bookkeeping bug without altering any tensor math, CUDA kernels, or distributed communication primitives. The fix is carefully structured with validation before any global mutation, exception-safe rollback via try/finally throughout, and monotonic AND-merge to prevent ownership revival. The new test suite covers all six checkpoint configurations from the PR description plus five exception-path edge cases. No files require special attention; the state transitions in distributed.py and quantization.py have been verified against all tested configurations. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User training loop
participant AC as te.autocast
participant ARF as activation_recompute_forward
participant RAC as autocast(_recompute=True)
participant Mod as TE Module (forward)
participant MB as Module backward
User->>AC: "__enter__ depth 0 to 1, is_first=True"
Note over ARF: Original forward (reentrant, no_grad)
User->>ARF: "__enter__ reserve=True, state.is_first=True, global=False"
User->>Mod: "forward() is_first()=False reserved, no bwd owner"
User->>ARF: __exit__ success global stays False
AC->>AC: "__exit__ depth 1 to 0, reduce_and_update(forward=True)"
Note over ARF,RAC: Backward / recompute phase
User->>ARF: "__enter__ recompute_phase=True, prev=False, global=True"
User->>RAC: "__enter__ _recompute=True, depth 0 to 1, no reset"
User->>Mod: "forward() is_first()=True, bwd_owner=True"
User->>RAC: "__exit__ _recompute=True, no forward update"
User->>ARF: "__exit__ merge prev(False) AND cur = False"
User->>MB: "backward() reduce_and_update(forward=False)"
Reviews (3): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile |
Signed-off-by: AlbertYang514 <201034045+AlbertYang514@users.noreply.github.com>
|
Hi @ksivaman and @ptrendx gentle ping when either of you has a chance. This PR has been open for about a week and is currently mergeable. It fixes duplicate FP8 global state updates under activation checkpoint recomputation, with regression coverage for reentrant, non-reentrant, per-layer, and nested checkpoint paths. Could either of you please take a look, or point me to the current owner of the activation-recompute / FP8 state-management path? I’m happy to adjust or split the change if preferred. |
|
Hi @vthumbe1503 — I revised #3213 to cover nested-autocast ownership restoration and transactional recovery from original-forward, recompute-entry, and recompute-body exceptions. The updated candidate passes the existing 8 tests, 6 focused edge tests, and all 49 cases in the local checkpoint audit suite. The residual SM120 RTC cast-transpose failure is still reproducible with correct 1/1 ownership/update counts and is being treated separately in #3215. When convenient, could you please take another look at the revised ownership and rollback logic? |
Summary
This change fixes FP8 first-module ownership across activation-checkpoint forward and recompute. The checkpoint frame reserves ownership for the original forward and reuses the captured state during recompute, preventing duplicate FP8 update ownership while preserving the expected single update.
v2 edge-case handling
The revised candidate also covers three state-transition edges:
Ownership merging is monotonic (available may become consumed, but consumed is not restored to available from an older snapshot). The nested-autocast path explicitly reacquires quantization state after restoring the outer autocast state rather than relying on object identity.
Validation
Scope
This PR is limited to checkpoint/autocast state ownership and rollback. A residual SM120 RTC cast-transpose failure can still be reproduced with correct 1/1 ownership/update counts; its investigation and workaround belong to PR #3215 and are not part of this change.