Add capture_time_hooks to make_graphed_callables for non-capturable per-callable hooks#2831
Add capture_time_hooks to make_graphed_callables for non-capturable per-callable hooks#2831buptzyb wants to merge 8 commits into
Conversation
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — hook dispatch, index aliasing, and is_training guards are consistent across both the _order and non-_order warmup/capture paths. All four hook invocation sites correctly fire outside the CUDA graph capture context in both warmup and capture, and are properly guarded by is_training for the backward hooks. The callable_idx / per_callable_fwd_idx aliasing is consistent. The new test covers both _order=None and _order=[1,2,-2,-1] paths and directly asserts hooks do not fire during graph replay. No files require special attention — the only finding is a naming inconsistency between the PR description and the actual key names used in the code. Important Files Changed
Sequence DiagramsequenceDiagram
participant MGC as make_graphed_callables
participant CTH as capture_time_hooks[i]
participant Graph as CUDA Graph Context
participant Module as Module
Note over MGC: Warmup iterations
MGC->>CTH: forward_pre_hooks[i](module)
MGC->>Module: "func(*args) [outside graph]"
MGC->>CTH: forward_hooks[i](module)
MGC->>CTH: backward_pre_hooks[i](module)
MGC->>Module: autograd.backward() [outside graph]
MGC->>CTH: backward_hooks[i](module)
Note over MGC: Capture phase
MGC->>CTH: forward_pre_hooks[i](module)
MGC->>Graph: enter capture
MGC->>Module: "func(*args) [RECORDED]"
MGC->>Graph: exit capture
MGC->>CTH: forward_hooks[i](module)
MGC->>CTH: backward_pre_hooks[i](module)
MGC->>Graph: enter capture
MGC->>Module: autograd.backward() [RECORDED]
MGC->>Graph: exit capture
MGC->>CTH: backward_hooks[i](module)
Note over MGC: Replay — no hooks fire
Reviews (5): Last reviewed commit: "Initialize CUDA graph grad inputs" | Re-trigger Greptile |
Signed-off-by: Robin Zhang <robinz@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Robin Zhang <robinz@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Robin Zhang <robinz@nvidia.com>
for more information, see https://pre-commit.ci
100b6e3 to
9ad5103
Compare
|
Hi @ksivaman could you help review? Thanks! |
timmoon10
left a comment
There was a problem hiding this comment.
We should add a test. make_graphed_callables has gotten too complicated for me to have any confidence of correctness just by looking at the code.
| and "backward_hooks" in capture_time_hooks[callable_idx] | ||
| ): | ||
| for hook in capture_time_hooks[callable_idx]["backward_hooks"].values(): | ||
| if hook(func, grad_inputs, grad_outputs) is not None: |
There was a problem hiding this comment.
Our behavior with grad_inputs doesn't match the torch.nn.Module backward hooks. Here we pass in the grads from both inputs and weights, ignoring non-tensor inputs and tensor inputs without grads. torch.nn.Module passes in the grads from the inputs, including non-tensor inputs and inputs without tensor inputs with grads.
There was a problem hiding this comment.
Addressed by adopting your later simplification suggestion. Capture-time hooks now have signature hook(module) and no longer expose grad_inputs or grad_outputs, so we no longer claim torch.nn.Module backward-hook semantics.
| else: | ||
| visited_te_modules[func_idx].update(modules) | ||
|
|
||
| if capture_time_hooks is not None and capture_time_hooks[callable_idx] is not None: |
There was a problem hiding this comment.
Having existence checks every time we access the hooks is quite burdensome. How about we preprocess capture_time_hooks so it has a fixed structure?
Function to preprocess `capture_time_hooks`
def _canonicalize_capture_time_hooks(
num_callables: int,
capture_time_hooks: Optional[List[Optional[Dict[str, Dict]]]],
) -> List[Dict[str, Dict]]:
"""Fill defaults in capture_time_hooks"""
expected_keys = {
"forward_pre_hooks",
"forward_pre_hooks_with_kwargs",
"forward_hooks",
"forward_hooks_with_kwargs",
"backward_pre_hooks",
"backward_hooks",
}
# Make sure there is one entry per callable
if capture_time_hooks is None:
capture_time_hooks = [None] * num_callables
if len(capture_time_hooks) != num_callables:
raise ValueError(
f"capture_time_hooks has {len(capture_time_hooks)} entries, "
f"but there are {num_callables} callables."
)
# Make sure each callable has a dict with expected keys
for callable_idx in range(num_callables):
if capture_time_hooks[callable_idx] is None:
capture_time_hooks[callable_idx] = {}
hooks = capture_time_hooks[callable_idx]
for key in expected_keys:
if key not in callable_hooks:
hooks[key] = {}
if set(hooks.keys()) != expected_keys:
raise ValueError(
"Found unexpected keys in capture_time_hooks "
f"({set(hooks.keys()) - expected_keys})."
)
return capture_time_hooksTriggering pre-forward hooks
# Call pre-forward capture-time hooks
hooks = capture_time_hooks[callable_idx]["forward_pre_hooks"]
hooks_with_kwargs = capture_time_hooks[callable_idx]["forward_pre_hooks_with_kwargs"]
for hook_id, hook in hooks.items():
hook_args = (
(func, args, kwargs) if hook_id in hooks_with_kwargs else (func, args)
)
if hook(*hook_args) is not None:
raise RuntimeError(
"capture_time_hooks forward_pre_hooks must not return a value "
"(args/kwargs must not be modified via hook return)"
)There was a problem hiding this comment.
Implemented _canonicalize_capture_time_hooks based on this suggestion, with the reduced four-key hook contract. It validates one entry per callable, fills missing hook maps, and rejects unsupported keys. Invocation and return-value validation are centralized in _run_capture_time_hooks.
| and "backward_pre_hooks" in capture_time_hooks[callable_idx] | ||
| ): | ||
| for hook in capture_time_hooks[callable_idx]["backward_pre_hooks"].values(): | ||
| if hook(func, grad_outputs) is not None: |
There was a problem hiding this comment.
We are treating grad_outputs inconsistently between warmup and capture. During warmup, we pass in only the required grads. During capture, we pass in grads for all of the outputs (matching torch.nn.Module pre-backward hooks).
See @greptile-apps's review in #2831 (comment).
There was a problem hiding this comment.
Yes, exactly right — this is the bug I flagged. The fix in _run_warmup_backward is to construct grad_outputs with the same structure-preserving convention used in both capture paths, then strip Nones only for the actual torch.autograd.backward call:
grad_outputs = tuple(
torch.empty_like(o) if o is not None and o.requires_grad else None
for o in outputs
)Then update the backward call to filter out Nones:
torch.autograd.backward(
outputs_requiring_grad,
grad_tensors=tuple(g for g in grad_outputs if g is not None),
)This ensures backward_pre_hooks and backward_hooks receive a grad_output tuple that positionally aligns with the full outputs tuple (with None for non-grad outputs), exactly matching what static_grad_outputs provides in both capture paths and matching PyTorch's register_full_backward_pre_hook / register_full_backward_hook convention.
There was a problem hiding this comment.
Now capture-time hooks no longer receive grad_outputs during either warmup or graph capture. So this problem disappears as well.
| A hook function that will be called after the warmup iterations. | ||
| A hook function that will be called once after all warmup iterations | ||
| (not once per callable). | ||
| capture_time_hooks: list of dict, optional |
There was a problem hiding this comment.
We put a lot of effort into matching the torch.nn.Module hook APIs, but they are causing correctness issues and we don't actually need them for our use-cases. Instead of making promises we can't fulfill, how about we simplify? We can just have "forward_pre_hooks", "forward_hooks", "backward_pre_hooks", "backward_hooks", each with signature hook(module).
There was a problem hiding this comment.
Implemented as suggested. Thanks!
Canonicalize each callable's hook maps to four supported phases and invoke every hook as hook(module) outside CUDA graph capture during warmup and graph construction. Reject non-None returns and keep callable mapping explicit across ordered and non-ordered paths. Add coverage for hook order, original module identity, graph-exterior execution, both capture paths, and exclusion from replay. This follows Tim Moon's review direction and incorporates his exploratory implementation from c95fb8d. Signed-off-by: Robin Zhang <robinz@nvidia.com>
Define grad_inputs on inference paths before conditional backward capture so static analysis can prove the value is always initialized. Apply the same behavior-preserving initialization to both ordered and non-ordered paths. Signed-off-by: Robin Zhang <robinz@nvidia.com>
Description
Summary
This PR adds
capture_time_hookstomake_graphed_callablesfor per-callable work that must run during warmup and CUDA Graph construction but remain outside the CUDA Graph capture context. This is needed for non-capturable operations such as CPU-side state updates and FSDP parameter un-shard/re-shard work that may allocate memory.These callbacks are graph-construction hooks, not PyTorch
Modulehooks: they receive only the original callable/module, must returnNone, are not recorded into the graph, and are not replayed.Changes
capture_time_hooksentry per callable, with support forforward_pre_hooks,forward_hooks,backward_pre_hooks, andbackward_hooks._orderacross chunks and microbatches. Existingbackward_dw()warmup behavior is unchanged.pre_warmup_hookandpost_warmup_hookonce around the complete warmup phase rather than once per callable.Modulehooks._order=Noneand_order=[1, 2, -2, -1].Hook contract
capture_time_hooksis a list with one entry per callable. Each entry may beNoneor a dictionary containing any subset of the four supported hook groups. Each group is a{hook_id: hook_fn}dictionary, and every hook has signaturehook(module) -> None.The list length must match the number of callables. Missing hook groups are treated as empty, unsupported group names raise
ValueError, and a non-Nonehook return raisesRuntimeError. The hooks cannot replace inputs, outputs,grad_inputs, orgrad_outputs.Normal PyTorch
Modulehooks may still be registered after graph construction.Execution order
For each callable, the order is:
forward_pre_hooksforward_hooksbackward_pre_hooks(training only)backward_hooks(training only)During graph construction, pre-hooks run before entering the corresponding capture context and post-hooks run after leaving it.
Tests and validation
The CUDA test verifies:
Validation performed:
2 passed;tests/pytorch/test_cuda_graphs.pyon H100:412 passed, 423 skipped;pylint==3.3.1, pre-commit, Python compilation, and diff checks passed.Type of change
Checklist