[MPS] Fix placeholder case for missing gather graph #83744
Conversation
🔗 Helpful links
✅ 1 Base FailuresAs of commit 8792ef3 (more details on the Dr. CI page): Expand to see more✅ None of the CI failures appear to be your fault 💚
🚧 1 fixed upstream failure:These were probably caused by upstream breakages that were already fixed.
Please rebase on the
|
mikaylagawarecki
added
the
triaged
|
@pytorchbot merge |
|
@pytorchbot successfully started a merge job. Check the current status here. |
|
Merge failed |
|
@pytorchbot merge -f "All the checks are passing." |
|
@pytorchbot successfully started a merge job. Check the current status here. |
|
Hey @kulinseth. |
Summary: Fixes #82543, #83230 The current Placeholder code relies to find a gather graph in order to make the data contiguous, otherwise we'll try calling into tensor.contiguous() directly, which for slice elements, won't do anything. E.g consider the following basic case where we index a 2 element tensor: ``` tensor_list = torch.tensor([1.2, 1.0], device="mps") for scalar in tensor_list: r_mps = torch.ceil(scalar) r_cpu = torch.ceil(scalar.to("cpu")) self.assertEqual(r_mps.cpu(), r_cpu) ``` The second element 1.0 is a contiguous view tensor (similar to slicing), but it has no gather graph created behind. In the placeholder, we won't be able to find the graph, thus relying on the fallback case where we call _tensor = src.contiguous();. For an already contiguous tensor, this won't do anything, thus we end up creating the NDArray with all the values of the tensor (1.2 and 1.0 instead of just 1.0). Doing clone instead of contiguous will actually perform a blit behind and take into consideration the storage_offset of the view when performing the copy. Similarly, the following basic case is also failing because of this issue: ``` x = torch.tensor([1.0, 0.49], device="mps") print(x) # prints 1.0 and 0.0 ``` Pull Request resolved: #83744 Approved by: https://github.com/razarmehr Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/a6b75bb0990f2c949bf6de4e6ae58f019d61c0ac Reviewed By: weiwangmeta Differential Revision: D38946708 Pulled By: weiwangmeta fbshipit-source-id: ae93ee9d8f581ae3cf9e8ed5fba159d5dc7f6cdc
Fixes #82543, #83230
The current Placeholder code relies to find a gather graph in order to make the data contiguous, otherwise we'll try calling into tensor.contiguous() directly, which for slice elements, won't do anything.
E.g consider the following basic case where we index a 2 element tensor:
The second element 1.0 is a contiguous view tensor (similar to slicing), but it has no gather graph created behind. In the placeholder, we won't be able to find the graph, thus relying on the fallback case where we call _tensor = src.contiguous();. For an already contiguous tensor, this won't do anything, thus we end up creating the NDArray with all the values of the tensor (1.2 and 1.0 instead of just 1.0). Doing clone instead of contiguous will actually perform a blit behind and take into consideration the storage_offset of the view when performing the copy.
Similarly, the following basic case is also failing because of this issue: