[Medium] Fix gradient checkpointing incompatibility with intervention hooks (#231) - #242
Open
pablocs116 wants to merge 2 commits into
Open
[Medium] Fix gradient checkpointing incompatibility with intervention hooks (#231)#242pablocs116 wants to merge 2 commits into
pablocs116 wants to merge 2 commits into
Conversation
…ointing Gradient checkpointing with `use_reentrant=False` recomputes each module's forward during `.backward()`. IntervenableModel.forward() removed the intervention hooks (and cleared their cached state) immediately after the forward, so the recomputation ran without them: fewer tensors were saved on the recompute than on the original forward and PyTorch raised `CheckpointError: A different number of tensors was saved during the original forward and recomputation` (issue stanfordnlp#231). This blocked PyReFT training with gradient checkpointing enabled. Defer the hook teardown (removing handles + `_cleanup_states`) to a post-backward callback whenever gradients are being tracked, so the hooks stay registered during recomputation and are torn down once the whole backward graph has run. When there is nothing to back-propagate through, teardown happens immediately, exactly as before. The teardown is scheduled by registering a hook on the grad-requiring output tensors; that hook fires at the start of backward and queues the real teardown via the autograd engine so it runs after every recomputation. A one-shot guard makes it idempotent across multiple output tensors. Adds GradientCheckpointingTestCase covering: forward+backward under `use_reentrant=False` no longer raises; loss and intervention gradients match a non-checkpointed run; and hooks/state are fully cleared after backward so nothing leaks across training steps. Closes stanfordnlp#231 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 071627946c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Addresses review feedback on stanfordnlp#231: when output_original_output=True the un-intervened base_outputs are computed before the intervention hooks are installed, so that graph is hook-free. Deferring teardown left the counterfactual hooks attached across a backward through base_outputs, which under non-reentrant gradient checkpointing recomputes the hook-free forward with hooks present -- re-triggering the tensor mismatch or corrupting the original gradients. Fall back to eager teardown on that path. Add a regression test (test_original_output_backward_is_hook_free) that backprops through original_outputs under checkpointing and asserts the base gradient matches an intervention-free reference and no hooks leak. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Description
Closes #231.
Training a PyReFT model with gradient checkpointing (
use_reentrant=False) failsin the backward pass with
CheckpointError: A different number of tensors was saved during the original forward and recomputation.IntervenableModel.forward()removed the intervention hooks and cleared theircached state immediately after the intervened forward — i.e. before the
Trainer calls
.backward(). Withuse_reentrant=False, PyTorch recomputes eachmodule's forward during backward; by then the hooks are gone, so the recompute
saves a different number of tensors than the original forward and the checkpoint
guard raises.
Fix: defer the hook teardown (removing the handles +
_cleanup_states) to apost-backward callback whenever gradients are being tracked, so the hooks stay
registered during recomputation and are cleaned up once the whole backward graph
has run. The teardown is scheduled via a hook on the grad-requiring output
tensors that queues the real cleanup on the autograd engine (one-shot guarded).
When there is nothing to back-propagate through, teardown happens immediately —
no behavior change off the training path.
Testing Done
Added
tests/integration_tests/GradientCheckpointingTestCase.py(3 tests):use_reentrant=Falseno longer raises;(checkpointing must not change numerics);
Confirmed the new regression test fails on
mainwith the reportedCheckpointErrorand passes with this change:Full existing suite (
python -m unittest discover -p '*TestCase.py'):Ran 75 tests — 74 pass. The single unrelated error
(
test_customized_intervention_function_get, a KeyError collecting attentionweights) also fails on a clean
mainwith currenttransformersand is nottouched by this PR.
Checklist:
[Your Priority] Your Title