Skip to content

fix(fsdp): restore full-layer activation checkpointing for KV-shared models - #3677

Draft
athitten wants to merge 1 commit into
mainfrom
athitten/fix/gemma4-kv-shared-ac
Draft

fix(fsdp): restore full-layer activation checkpointing for KV-shared models#3677
athitten wants to merge 1 commit into
mainfrom
athitten/fix/gemma4-kv-shared-ac

Conversation

@athitten

Copy link
Copy Markdown
Contributor

What does this PR do ?

Fixes a 16% memory regression on the nightly Gemma4-4B benchmark by putting KV-shared
models (Gemma4 E2B/E4B) back on full-layer activation checkpointing.

The bug

Activation checkpointing can be applied two ways:

  • whole block — wrap the entire decoder layer, so attention is recomputed in backward
  • sub-module — wrap mlp and the norms individually

The sub-module path deliberately skips self_attn for KV-shared models.

PR #3513 added and not _has_kv_sharing to the whole-block branch. Gemma4 E4B has
num_kv_shared_layers: 18, so it started taking the sub-module path instead. Attention
activations across all 42 language layers stopped being recomputed and stayed resident:

Applied submodule activation checkpointing to 42 layers:
  {'mlp': 42, 'attention': 0, 'pre_norm': 42, 'post_norm': 42, 'mot': 0}
                ^^^^^^^^^^^^^

Nothing crashed and the loss did not move, so only the memory metric caught it.

Why the guard is not needed

The guard exists because recomputing attention could write K/V into the cache a second
time. That does not happen here: Gemma4 passes its shared layers _Gemma4KVShareHolder,
whose update() just returns what it was given. Replaying a block writes nothing.

Evidence

2x GB200, examples/vlm_finetune/gemma4/gemma4_4b_mock.yaml, activation checkpointing on:

peak memory loss / grad_norm
this PR's parent 87.37 GiB 6.0085 / 125.5791 …
before #3513 70.06 GiB identical
this PR 70.06 GiB identical

70.06 GiB matches the nightly's pre-regression value of 7.005e+01 exactly, and losses
and gradient norms agree to four decimals across all 8 steps. This was purely a memory
regression, not a numerics change.

Changelog

Verified the new test fails on the regressed code:

AssertionError: KV-shared Gemma4 decoder blocks [0, 1, 2, 3] are not checkpoint-wrapped;
activation checkpointing fell back to sub-module wrapping, which excludes self_attn

and passes with the fix (OK (63 gradients compared)).

Peak-memory magnitude stays with the nightly benchmark — a proxy this small can't measure
it without a threshold that says more about the proxy than about the code.

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?

Additional Information

…models

PR #3513 added a `not _has_kv_sharing` guard to the full-layer activation
checkpointing branch. Gemma4 E2B/E4B have `num_kv_shared_layers > 0`, so they
now fall back to `apply_submodule_checkpointing`, which deliberately skips
`self_attn`. Attention activations across all 42 language layers stopped being
recomputed and peak memory rose 16% on the nightly Gemma4-4B benchmark.

The guard is not needed here. Gemma4 hands its shared layers
`_Gemma4KVShareHolder`, whose `update()` is a pass-through, so replaying a whole
block during backward does not write K/V twice.

Measured on 2x GB200 with examples/vlm_finetune/gemma4/gemma4_4b_mock.yaml and
activation checkpointing on: 87.37 GiB before, 70.06 GiB after, matching the
pre-regression nightly value. Losses and gradient norms are identical to four
decimals over 8 steps.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Abhishree <abhishreetm@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant