[PyTorch] Fix shape and size() for columnwise-only quantized tensors - #3266
[PyTorch] Fix shape and size() for columnwise-only quantized tensors#3266pggPL wants to merge 4 commits into
Conversation
Float8BlockwiseQTensor.shape is a fast path added to avoid PyObject lookups. For a columnwise-only tensor it returned the raw columnwise buffer shape, but blockwise stores columnwise data transposed, so the property disagreed with both Float8BlockwiseQTensorStorage.size() and the wrapper's own metadata: make_empty((128, 256)) yielded .shape == (256, 128) while .size() == (128, 256). Apply the same reorder size() does. Float8Tensor and NVFP4Tensor already de-transpose in their equivalent fast paths; MXFP8Tensor needs no change because it stores columnwise data untransposed. Cover the invariant for every quantization scheme and usage combination with test_shape_matches_size, rather than only for blockwise. Introduced in 9dac78e ("CPU Overhead Optimizations", NVIDIA#2559). Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com>
Greptile SummaryCorrects logical shape reporting for columnwise-only FP8 tensors.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "Merge branch 'main' into quantized_tenso..." | Re-trigger Greptile |
…path Row-scaled NVFP4 accepts set_usage(rowwise=False) but its allocator asserts on rowwise usage, so test_shape_matches_size hit an NVTE_CHECK failure instead of skipping. Filter that combination out before set_usage. Also give Float8BlockwiseQTensor.shape a 2D fast path: indexing torch.Size directly measures ~186 ns/call against ~311 ns for building an intermediate list, on the columnwise-only branch. Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com>
Both storages forwarded *args straight to the underlying buffer, then reordered the result. That works for the rowwise branch, where the buffer matches the logical shape, but not for the transposed one: size(dim) returned an int from the buffer, which the reorder then tried to index, so every size(dim) call raised TypeError. Forwarding dim is also wrong in principle there, since buffer dim i is not logical dim i. Rebuild the logical shape in full first, then index into it. Float8TensorStorage also flattened the transpose-only shape to 2D, which disagreed with both dim() and Float8Tensor.shape for rank >= 3; it now applies the same rotation the shape property does. Reachable from quantize() followed by update_usage(rowwise_usage=False). NVFP4 is left alone: it reports columnwise-only tensors flattened on purpose and warns about it, and shape and size() agree there. Extend test_shape_matches_size over 2D and 3D shapes, asserting that shape and size() describe the same tensor and that size(dim) agrees with it for positive and negative dims. Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com>
There was a problem hiding this comment.
LGTM. Thanks for catching and fixing this bug. We probably need a wider test suite for FP8 BS. I had found similar code coverage missing for FP8 BS missing w.r.t dequantize not being autograd aware in this PR
Also, a minor note based on comments in the description, CPU overhead issues are more pronounced on Grace Systems, but indeed can be a problem in other systems as well as demonstrated in the description
| # NVFP4 deliberately reports columnwise-only tensors flattened to 2D and | ||
| # warns about it, so only the ranks it preserves are checked here. | ||
| if not (isinstance(quantizer, NVFP4Quantizer) and not rowwise and len(shape) > 2): | ||
| assert tuple(x.shape) == tuple(shape), f"{name}.shape is {tuple(x.shape)}" |
There was a problem hiding this comment.
We might want to dig deep into why we dont need to apply the same shape preservation for NVFP4 tensors. But given it already exists in the code base, makes sense to defer it to a different PR.
|
/te-ci pytorch |
Description
Float8BlockwiseQTensor.shape,Float8TensorStorage.size()andFloat8BlockwiseQTensorStorage.size()all report the logical shape of a tensor whose data is only stored columnwise/transposed. Each of the three gets it wrong in a different way.1.
Float8BlockwiseQTensor.shapereturns the transposed buffer shape. Blockwise stores columnwise data transposed, and the fast path added in 9dac78e ("CPU Overhead Optimizations", #2559) does not undo that, so the property disagrees with bothsize()and the wrapper metadata:That commit added the equivalent property to four classes at once;
Float8TensorandNVFP4Tensorundo the reordering.MXFP8Tensorreads the same but is already correct, since it stores columnwise data untransposed.2.
size(dim)raisesTypeErroron the columnwise-only path, for FP8 and blockwise alike. Both storages forward*argsto the underlying buffer and then reorder the result.size(dim)returns anint, which the reorder then indexes:Forwarding
dimis also wrong in principle here, because buffer dimiis not logical dimiunder a transposed layout. This predates #2559 — it came in with the classes themselves (#1505, #1513).3.
Float8TensorStorage.size()flattens to 2D, which contradictsdim()andFloat8Tensor.shapefor rank ≥ 3:All of this is reachable from
quantize()followed byupdate_usage(rowwise_usage=False), which is the normal way to release rowwise data once it is no longer needed.NVFP4 is deliberately left alone: it reports columnwise-only tensors flattened and warns about it, and its
shapeandsize()agree with each other.Type of change
Changes
Float8BlockwiseQTensor.shapeundoes the columnwise transpose, with a 2D fast path.Float8BlockwiseQTensorStorage.size()andFloat8TensorStorage.size()rebuild the full logical shape before applying adimargument, instead of forwarding it to the buffer.Float8TensorStorage.size()applies the same rotation asFloat8Tensor.shaperather than flattening to 2D.test_shape_matches_sizeintests/pytorch/test_quantized_tensor.pycovers every entry of_quantization_list, three usage combinations and 2D/3D shapes, asserting thatshapeandsize()describe the same tensor and thatsize(dim)agrees with it for positive and negative dims.Note on CPU overhead
#2559 introduced the
shapeproperty specifically to cut CPU overhead, so the fix keeps the rowwise path untouched and adds a 2D fast path on the columnwise-only branch. Measured on an RTX Ada,make_empty((128, 256)), columnwise-only, ns/call:torch.Sizetorch.Tensor.size(self)(wrapper metadata)list()+ slicesFloat8BlockwiseQTensorStorage.size()before this PR.shapetherefore stays cheaper than thesize()that already computed the same rotation. Reading the wrapper metadata benchmarks similarly but was avoided: buffers are reassigned after construction in a number of places, and the metadata would not follow.Checklist:
Note on local verification: my workstation is an RTX Ada, so
fp8_blockwise,mxfp8andnvfp4are not in_quantization_listthere and only thefp8variants of the new test ran locally. The other paths were verified by direct measurement across all seven quantizers, three usage combinations and both ranks, usingmake_empty, which allocates regardless of arch support.test_numericsshows 28 failures both with and without these changes, so they are pre-existing on this GPU.One thing worth watching in CI: making
Float8TensorStorage.size()rank-preserving changes the rank it returns for transpose-only tensors of rank ≥ 3. If anything in the stack relied on the 2D flattening, it would only surface on Hopper+.🤖 Generated with Claude Code