add FSDP and TP tests for Float8BlockwiseLinear - #4295
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/4295
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 New Failure, 8 PendingAs of commit 96989e1 with merge base 15f2e97 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
||
|
|
||
| @torch.library.custom_op("torchao::blockwise_scaled_mm", mutates_args=()) | ||
| def blockwise_scaled_mm( |
There was a problem hiding this comment.
moved here because i wanted to register sharding rules for the scaled_mm. felt it made sense to move these definitions in kernel.py
danielvegamyhre
left a comment
There was a problem hiding this comment.
LGTM with some minor comments
|
|
||
| EPS = 1e-12 | ||
|
|
||
| _SCALING_TYPE = getattr(F, "ScalingType", getattr(torch._C, "_ScalingType", None)) |
There was a problem hiding this comment.
why is this necessary? torch changes between versions?
There was a problem hiding this comment.
yeah, this is to guard against different torch versions. older pytorch has torch._C._ScalingType and newer has torch.nn.functional.ScalingType
|
|
||
| _SCALING_TYPE = getattr(F, "ScalingType", getattr(torch._C, "_ScalingType", None)) | ||
| BLOCKWISE_1X128_SCALING_TYPE = ( | ||
| _SCALING_TYPE.BlockWise1x128 if _SCALING_TYPE is not None else 4 |
There was a problem hiding this comment.
what are the magic variables 4/5? can we define them as a constant or at least add a comment to make it more easily interpretable
|
|
||
|
|
||
| def _pad_blockwise_128x128_scale_k_major(scale: torch.Tensor) -> torch.Tensor: | ||
| # cuBLASLt requires BLK128x128 scales to be K-major with the K stride padded to a multiple of 4. |
There was a problem hiding this comment.
Does the multiple of 4 requirement stem from TMA where the tensor's global stride must be a multiple of 16 bytes? i.e., 4 fp32 scales = 16 bytes
If possible, we should explain why this requirement exists in the comment so we have a full understanding
There was a problem hiding this comment.
i'm unsure if this is because of TMA but i've updated it to add more detail directly from NVIDIA's cuBLAS docs:
CUBLASLT_MATMUL_MATRIX_SCALE_BLK128x128_32F scaling mode, the scaling factors are
-major and the stride between the consecutive columns must be a multiple of 4. Let
, where the denotes rounding up to the nearest multiple of 4.
reference:
https://docs.nvidia.com/cuda/cublas/index.html#scaling-factors-layouts
| if not _is_row_major(a): | ||
| a = a.contiguous() | ||
| if not _is_column_major(b): | ||
| b = b.t().contiguous().t() |
There was a problem hiding this comment.
can we just assert the data is in the expected layout, instead of silently doing these expensive copies to transform it (which hurts perf)?
i.e., if someone imports and uses blockwise_scaled_mm and calls it with inputs in the wrong layouts, it should fail loudly with clear error messages describing the necessary layouts, rather than "succeed" with bad perf, leaving the user potentially confused why fp8 is not producing speedup vs bf16 when rooflines for the shapes suggest it should.
There was a problem hiding this comment.
yup agreed and changed to assertions. oversight on my part where it's unclear to user/hurts perf
| scale_recipe_a, | ||
| scale_b: torch.Tensor, | ||
| scale_recipe_b, | ||
| triton_scale_b: torch.Tensor | None = None, |
There was a problem hiding this comment.
is the new triton_scale_b arg necessary because the Triton gem requires that the B tensor scale be in a different layout than what torch _scaled_mm requires?
if so, in a follow up, we should try to address that - having 2 separate scale_b params for different kernels is pretty gnarly
alternatively, if the fixes in core _scaled_mm are going to land soon (that will enable to stop using the triton kernel at all), then we can leave this triton_scale_b arg for now and just delete all the triton gemm stuff once the fix in core lands.
There was a problem hiding this comment.
yeah, agreed its not great. the triton and scaled_mm paths expect the RHS scale tensor in different physical layouts for the grad_weight case. the triton 1x128_128x1 kernel consumes b_s in row-major layout whereas scaled_mm expects the transposed scale layout, so with the arg we let each path receive the layout it expects without adding a copy/transpose on the hot path
i believe the changes in core will be landed soon so we can get rid of the triton gemm work when it does. pytorch/pytorch#180668
There was a problem hiding this comment.
ok sounds good. please make sure this is explained in the docstring somewhere.
There was a problem hiding this comment.
yup added a comment explaining it
Summary
This adds distributed coverage for Float8BlockwiseLinear on top of the DTensor sharding rules we already added for the blockwise FP8 kernels. The goal is to validate the full module-level training path before wiring the kernels into a real torchtitan run.
Added a
torchao::blockwise_scaled_mmwrapper around the fallback_scaled_mmpath and registered DTensor sharding for it. This was necessary because TP onFloat8BlockwiseLinear(use_triton=False)routes throughaten._scaled_mm_v2, and that op did not have the DTensor sharding propagation we needed. Without this, the Triton path worked under TP, but the_scaled_mmfallback path failed.Added the following tests to the following files:
test/prototype/blockwise_fp8_training/test_dtensor.pytest_linear_tp_parityToyModel.w1: colwisew2: colwiseout_proj: rowwise_scaled_mmtest/prototype/blockwise_fp8_training/test_fsdp2.pytest_fsdp2_parityToyModel.fully_shard()is applied on the FFN submodules plus the parent module.test/prototype/blockwise_fp8_training/test_fsdp2_tp.pyFSDP2 + TPparity test on a 2D mesh with(dp, tp) = (2, 2).All of the three tests use the same testing metholdogy:
ToyModelinstancesFloat8BlockwiseLinearConfig()use_tritonTesting
2-GPU DTensor + TP coverage:
2-GPU FSDP2 coverage:
4-GPU FSDP2 + TP coverage: