Graph Safe support for TE Grouped linear Op#2923
Conversation
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
for more information, see https://pre-commit.ci
Greptile SummaryThis PR adds graph-safe (CUDA-graph-compatible) support for the
Confidence Score: 5/5The change is safe to merge. The new graph-safe forward/backward paths are well-structured, delay_wgrad=True correctly returns None for weight gradients and defers the GEMM to backward_dw, and the Megatron main-grad refactor is a clean consolidation with no functional regression. The core new code paths correctly handle all major branches: quantized vs. unquantized, single vs. per-group weights, delay_wgrad, and accumulate_into_main_grad. The shared _common.py helpers are straightforward extractions of duplicated inline logic. The unified saved-tensor layout for the fused MLP forward/backward is internally consistent and round-trips correctly through the unfused backward path. No files require special attention for merge safety. grouped_linear.py has pre-existing issues carried over from earlier reviews that the team is tracking separately. Important Files Changed
Reviews (10): Last reviewed commit: "fix linting errors" | Re-trigger Greptile |
| bias_scale: Optional[torch.Tensor] = None | ||
| if has_bias: | ||
| # Bias always needs to be passed as a GroupedTensor for the grouped GEMM. | ||
| grouped_bias = self._get_grouped_bias_for_gemm(dtype, device) | ||
| if self._scale_bias: | ||
| bias_scale = scales.reshape(-1) | ||
| if bias_scale.dtype != torch.float32: | ||
| bias_scale = bias_scale.to(dtype=torch.float32) | ||
|
|
||
| # Forward grouped GEMM (TN layout: out[i] = x[i] @ w[i]^T) | ||
| general_grouped_gemm_for_grouped_tensor( | ||
| grouped_weights, | ||
| grouped_x, | ||
| grouped_out, |
There was a problem hiding this comment.
Missing contiguity error handling for
main_grad.view(-1)
main_grad.view(-1) will raise a generic RuntimeError if main_grad is non-contiguous (e.g. when returned by get_main_grad() via __fsdp_param__). The equivalent code in backward_grouped_mlp.py wraps the reshape in try/except and re-raises with an actionable message that includes the shape and stride. Without that guard, users hitting this case will see an opaque PyTorch error instead of a clear diagnostic.
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: vthumbe1503 <vthumbe@nvidia.com>
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
Signed-off-by: vthumbe1503 <vthumbe@nvidia.com>
| grouped_wgrad = GroupedTensor.make_grouped_tensor_from_rowwise_data( | ||
| num_tensors=num_groups, | ||
| tensor_shape=weight_shape, | ||
| rowwise_data=main_grad.view(-1), | ||
| dtype=main_grad.dtype, | ||
| ) |
There was a problem hiding this comment.
Non-contiguous
main_grad causes opaque RuntimeError in graph-safe backward
main_grad.view(-1) will raise a plain RuntimeError whenever main_grad is non-contiguous (e.g., when FSDP returns a main_grad that is already shaped (num_groups, out_features, in_features) but lives as a non-unit-stride slice of a larger gradient buffer). view_main_grad_as_grouped_buffer only guards the reshape-to-grouped-shape step — once the helper returns the tensor unchanged (shape already matches), the subsequent .view(-1) is unprotected.
The equivalent code in backward_grouped_mlp.py avoids this by passing main_grad directly to make_grouped_tensor_from_rowwise_data, which internally calls .contiguous() when needed. grouped_linear.py should do the same:
| grouped_wgrad = GroupedTensor.make_grouped_tensor_from_rowwise_data( | |
| num_tensors=num_groups, | |
| tensor_shape=weight_shape, | |
| rowwise_data=main_grad.view(-1), | |
| dtype=main_grad.dtype, | |
| ) | |
| grouped_wgrad = GroupedTensor.make_grouped_tensor_from_rowwise_data( | |
| num_tensors=num_groups, | |
| tensor_shape=weight_shape, | |
| rowwise_data=main_grad, | |
| dtype=main_grad.dtype, | |
| ) |
timmoon10
left a comment
There was a problem hiding this comment.
This is an important PR and I think this looks good overall. My main thought is that we need to maintain the contract that unfused ops and fused ops are interchangeable.
Signed-off-by: vthumbe1503 <vthumbe@nvidia.com>
…t savings for fused/unfused paths Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
…3/TransformerEngine into grouped_linear_integration_v2
for more information, see https://pre-commit.ci
|
/te-ci pytorch |
Signed-off-by: Varun Thumbe <vthumbe@nvidia.com>
|
|
||
| return out, [()] | ||
|
|
||
| def fuser_forward_save_ctx( |
There was a problem hiding this comment.
This function is similar to the context setup stage in #2877. After this PR is merged, we should try to consolidate them together.
After that, it may be worthwhile calling this function from the fused ops so that we guarantee the unfused and fused ops have the same context format.
* starting effort Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * all tests seem to be working Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * cuda graph test + clean ups Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * clean up Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * cleanup Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * clean up main grad business Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * clean up a but Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * fix on l40/hopper to skip Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * address review comments + save activation in backward + common context savings for fused/unfused paths Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * cleanup Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * address review comments Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix linting errors Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> --------- Signed-off-by: Varun Thumbe <vthumbe@nvidia.com> Signed-off-by: vthumbe1503 <vthumbe@nvidia.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
Graph Safe Support for GroupedLinear Op for MXFP8/BF16/FP16:
Group Quantize + CUBLAS GGEMM for the above data types
Context Saving Refactoring for Graph Safe Paths
GroupedLinear and its Fused MOE Variants(GemmSwiglu based MLP) both have unified way of saving context and and hence using something like fused forward and unfused backward works without hassle.
Use save_for_backward for activations(GroupedTensorStorage) as well
GroupedTensorStorage now has prepare_for_saving and restore_from_saved functions defined
And this is used to save activations for both unfused GroupedLinear module and GEMM+Swiglu based MLP for mxfp8
Megatron Main Grad logic refactoring
All the repeated logic of using Megatrong DDP/FSDP specific attributes from parameters are abstracted out into reusable functions.
Fixes # (issue)
Type of change
Changes
Please list the changes introduced in this PR:
Checklist: