Repository navigation
Conversation
Contributor
|
@q10 has imported this pull request. If you are a Meta employee, you can view this in D118492251. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The eager paths now reject malformed offsets, but batched_unary_embeddings_forward_meta still floor-divides (offsets.sym_numel() - 1) / T without enforcing the new T > 0 / divisibility invariant. FakeTensor/export can therefore accept an input shape that eager now rejects and report a misleading B. Could the Meta implementation mirror the new validation?
|
|
Contributor
Author
|
q10
added a commit
to q10/FBGEMM
that referenced
this pull request
Sep 17, 2026
Summary: Pull Request resolved: pytorch#6318 X-link: https://github.com/facebookresearch/FBGEMM/pull/3192 NOTE: No linked task. Please associate a task with this diff. ## Motivation `batched_unary_embeddings_test.py::test_gpu` was disabled on ROCm after its permute case triggered a GPU memory access fault. The failure came from malformed test input, but the operator accepted that input instead of reporting an error, allowing it to reach the backward kernel and access memory out of bounds. ## Root cause The operator uses two offset tensors for different purposes. `table_offsets` separates the embedding tables in the concatenated weight tensor and determines the number of tables, `T`. `offsets` separates the input bags for every `(table, batch)` pair, so it must contain exactly `T * B + 1` entries. The failing test created a module with `T = 2` and intended `B = 4`, but supplied only 8 offsets instead of 9. The forward path calculated `B` using integer division: `(8 - 1) / 2 = 3`. This silently discarded the incomplete final table row. The backward path still processed all seven encoded segments; with the truncated `B`, the final segment decoded to table index `t = 2`, outside the valid table range `[0, 2)`, and wrote beyond `grad_weight`. ROCm surfaced that invalid access as a memory fault. ## Fix - Correct the test to provide offsets `[0, ..., 8]`, giving the required 9 entries for `T = 2` and `B = 4`. - Validate positive dimensions and the `T * B + 1` offsets contract before deriving `B` in the CPU and CUDA forward paths. Mirror the contract in the Meta implementation so eager execution and tracing reject the same malformed shapes. - Validate tensor sizes before narrowing them to the existing 32-bit kernel parameters, while keeping flattened address calculations in 64-bit arithmetic where the output can exceed `2^32` elements. - Preserve CUDA backward compatibility with both `[N, sum_E]` and `[N, sum_E, 1]` weights, with a focused gradient regression for the 2-D layout. - Re-enable the GPU test on ROCm and add CPU/GPU invalid-input, dynamic-shape, Meta, and large-grid regression coverage. Imported from [facebookresearch/FBGEMM#6261](pytorch#6261), authored by Andrey Bokovoy. Reviewed By: spcyppt, yvonne-lab Differential Revision: D118492251
meta-codesync Bot
pushed a commit
that referenced
this pull request
Sep 17, 2026
Summary: Pull Request resolved: #6318 X-link: https://github.com/facebookresearch/FBGEMM/pull/3192 NOTE: No linked task. Please associate a task with this diff. ## Motivation `batched_unary_embeddings_test.py::test_gpu` was disabled on ROCm after its permute case triggered a GPU memory access fault. The failure came from malformed test input, but the operator accepted that input instead of reporting an error, allowing it to reach the backward kernel and access memory out of bounds. ## Root cause The operator uses two offset tensors for different purposes. `table_offsets` separates the embedding tables in the concatenated weight tensor and determines the number of tables, `T`. `offsets` separates the input bags for every `(table, batch)` pair, so it must contain exactly `T * B + 1` entries. The failing test created a module with `T = 2` and intended `B = 4`, but supplied only 8 offsets instead of 9. The forward path calculated `B` using integer division: `(8 - 1) / 2 = 3`. This silently discarded the incomplete final table row. The backward path still processed all seven encoded segments; with the truncated `B`, the final segment decoded to table index `t = 2`, outside the valid table range `[0, 2)`, and wrote beyond `grad_weight`. ROCm surfaced that invalid access as a memory fault. ## Fix - Correct the test to provide offsets `[0, ..., 8]`, giving the required 9 entries for `T = 2` and `B = 4`. - Validate positive dimensions and the `T * B + 1` offsets contract before deriving `B` in the CPU and CUDA forward paths. Mirror the contract in the Meta implementation so eager execution and tracing reject the same malformed shapes. - Validate tensor sizes before narrowing them to the existing 32-bit kernel parameters, while keeping flattened address calculations in 64-bit arithmetic where the output can exceed `2^32` elements. - Preserve CUDA backward compatibility with both `[N, sum_E]` and `[N, sum_E, 1]` weights, with a focused gradient regression for the 2-D layout. - Re-enable the GPU test on ROCm and add CPU/GPU invalid-input, dynamic-shape, Meta, and large-grid regression coverage. Imported from [facebookresearch/FBGEMM#6261](#6261), authored by Andrey Bokovoy. Reviewed By: spcyppt, yvonne-lab Differential Revision: D118492251 fbshipit-source-id: c00c9d4cb5a06af93f8d724f427272d2be685eb5
Contributor
|
Closed in 4f8c2ef |
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.
Problem
batched_unary_embeddings_test.py::test_gpu crashed with a GPU memory access fault on ROCm. The permute sub-test passed
offsets of length 8 (7 segments) for a T=2 module, violating the op's offsets.numel() == T*B+1 contract. Forward silently truncated
B=3 and dropped the trailing segment; backward walked all 7 segments, deriving an out-of-range table id t=2 that indexed
grad_weight out of bounds.
Fix