correctly set strides for expanded/unsqueezed dimensions #90341
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/90341
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 FailuresAs of commit 6519637: The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a labelIf your changes are user facing and intended to be a part of release notes, please use a label starting with If not, please add the For more information, see https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work. |
test/test_meta.py
Outdated
| @@ -288,6 +288,11 @@ def test_tensor_outlives_converter(self): | |||
| torch.Tensor.__getitem__, | |||
| } | |||
|
|
|||
| CHECK_ALL_STRIDES = { | |||
| torch.Tensor.expand, | |||
There was a problem hiding this comment.
torch.expand.Tensor ?
There was a problem hiding this comment.
Should be aten.expand.default.
test/test_meta.py
Outdated
| if should_check_strides(func) == CheckStrides.ALL: | ||
| same_strides, _ = torch._prims_common.check_all_strides(meta_r, r) | ||
| test_assert(same_strides, f"but real stride was {r.stride()}") | ||
| if should_check_strides(func) == CheckStrides.SIGNIFICANT: |
There was a problem hiding this comment.
nit. maybe use elif, as these branch should be exclusive.
|
@pytorchbot merge -f "test failures unrelated" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Fixes pytorch/torchdynamo#1959, pytorch#90260 However, I wasn't able to make existing stride tests fail before the fix, even though I'm comparing all, not just significant strides. Separately running refs on meta tensors produces wrong strides as shown in pytorch#90260, however, it looks like in meta tests some other way of computing meta info is used (I've been running ``` pytest -s -v test/test_meta.py -k test_meta_outplace_expand_cuda_float64 ``` and verified that it has sample input that should fail, and that it indeed compares all the strides, but the produced `meta_rs` results somehow still had correct strides). Edit: @SherlockNoMad helped me figure out how to fail the tests, and now I've set the correct ops for checking. `expand` fails for some test inputs because it special-cases 0-dim input case, correctly modeling it in prims would require a lot of changes, so skipping that for now. Pull Request resolved: pytorch#90341 Approved by: https://github.com/SherlockNoMad
Fixes pytorch/torchdynamo#1959, #90260
However, I wasn't able to make existing stride tests fail before the fix, even though I'm comparing all, not just significant strides.
Separately running refs on meta tensors produces wrong strides as shown in #90260, however, it looks like in meta tests some other way of computing meta info is used (I've been running
and verified that it has sample input that should fail, and that it indeed compares all the strides, but the produced
meta_rsresults somehow still had correct strides).Edit: @SherlockNoMad helped me figure out how to fail the tests, and now I've set the correct ops for checking.
expandfails for some test inputs because it special-cases 0-dim input case, correctly modeling it in prims would require a lot of changes, so skipping that for now.cc @SherlockNoMad