Fix invalid indices bug for max_unpool2d/3d on MPS by can-gaa-hou · Pull Request #163036 · pytorch/pytorch

can-gaa-hou · 2025-09-16T02:28:34Z

Fixes #163035

pytorch-bot · 2025-09-16T02:28:37Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/163036

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 20e4e0c with merge base 456fbea ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

can-gaa-hou · 2025-09-16T16:00:49Z

@pytorchbot merge

pytorchmergebot · 2025-09-16T16:03:54Z

Merge failed

Reason: Approvers from one of the following sets are needed:

superuser (pytorch/metamates)
Core Reviewers (mruberry, lezcano, Skylion007, ngimel, peterbell10, ...)
Core Maintainers (soumith, gchanan, ezyang, dzhulgakov, malfet, ...)

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

aten/src/ATen/native/mps/operations/Pooling.mm

malfet

Thank you for working on the change, but I'm a bit suspicious why this is tested only for MPS?
Also, use simpler error checking message and print entire shape rather than having separate output for 2d and 3d cases, which would also eliminate the need for may-be unsued

test/nn/test_pooling.py

aten/src/ATen/native/mps/operations/Pooling.mm

Co-authored-by: Nikita Shulga <2453524+malfet@users.noreply.github.com>

malfet

Looks good to me, code looks much neater now

pytorch-bot · 2025-09-18T14:04:54Z

To add the ciflow label ciflow/mps please first approve the workflows that are awaiting approval (scroll to the bottom of this page).

This helps ensure we don't trigger CI on this PR until it is actually authorized to do so. Please ping one of the reviewers if you do not have access to approve and run workflows.

malfet · 2025-09-18T14:04:58Z

@pytorchbot merge

pytorch-bot · 2025-09-18T14:05:04Z

Pull workflow has not been scheduled for the PR yet. It could be because author doesn't have permissions to run those or skip-checks keywords were added to PR/commits, aborting merge. Please get/give approval for the workflows and/or remove skip ci decorators before next merge attempt. If you think this is a mistake, please contact PyTorch Dev Infra.

can-gaa-hou · 2025-09-19T02:32:27Z

Looks good to me, code looks much neater now

Thanks for the review!

can-gaa-hou · 2025-09-19T02:32:38Z

@pytorchbot merge

pytorchmergebot · 2025-09-19T02:34:29Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Fixes pytorch#163035 Pull Request resolved: pytorch#163036 Approved by: https://github.com/kulinseth, https://github.com/malfet Co-authored-by: Nikita Shulga <2453524+malfet@users.noreply.github.com>

malfet · 2026-01-09T00:51:20Z

I must have been blind when reviewing this PR:

Test only tests CPU behavior
And check in maxunpool introduces a sync point, which is a big no no

Introduced by #163036 I should have rejected that PR during the review ghstack-source-id: 75c86d2 Pull-Request: #172046

Introduced by #163036 I should have rejected that PR during the review ghstack-source-id: 770c1f1 Pull-Request: #172046

can-gaa-hou · 2026-01-09T02:26:55Z

Sorry about that. The check introduced a sync point on MPS, and the test only validated CPU behavior. I’ll be more careful about host-side reads/reductions on MPS paths and make sure tests cover the intended device next time. Thanks for catching and fixing this.

cc @malfet

Introduced by #163036 I should have rejected that PR during the review ghstack-source-id: ff2883b Pull-Request: #172046

Introduced by #163036, that I should have rejected, but ship have sailed Also, delete unnecessary `test_max_unpool_invalid_indices` as those checks are already covered by `test_MaxUnpool_index_errors` , but they were skipped in the past as test is marked slow (though it should have been marked as such only for CUDA) Pull Request resolved: #172046 Approved by: https://github.com/manuelcandales ghstack dependencies: #172051, #172052

Introduced by #163036, that I should have rejected, but ship have sailed Also, delete unnecessary `test_max_unpool_invalid_indices` as those checks are already covered by `test_MaxUnpool_index_errors` , but they were skipped in the past as test is marked slow (though it should have been marked as such only for CUDA) Pull Request resolved: #172046 Approved by: https://github.com/manuelcandales ghstack dependencies: #172051, #172052 (cherry picked from commit 9469ce4)

[MPS] Remove error-checking sync point from MaxUnpool (#172046) Introduced by #163036, that I should have rejected, but ship have sailed Also, delete unnecessary `test_max_unpool_invalid_indices` as those checks are already covered by `test_MaxUnpool_index_errors` , but they were skipped in the past as test is marked slow (though it should have been marked as such only for CUDA) Pull Request resolved: #172046 Approved by: https://github.com/manuelcandales ghstack dependencies: #172051, #172052 (cherry picked from commit 9469ce4) Co-authored-by: Nikita Shulga <nshulga@meta.com>

Introduced by pytorch#163036, that I should have rejected, but ship have sailed Also, delete unnecessary `test_max_unpool_invalid_indices` as those checks are already covered by `test_MaxUnpool_index_errors` , but they were skipped in the past as test is marked slow (though it should have been marked as such only for CUDA) Pull Request resolved: pytorch#172046 Approved by: https://github.com/manuelcandales ghstack dependencies: pytorch#172051, pytorch#172052

Introduced by pytorch/pytorch#163036 I should have rejected that PR during the review ghstack-source-id: 770c1f1 Pull-Request: pytorch/pytorch#172046 ghstack-source-id: c97ccf1 Pull Request resolved: pytorch/pytorch#172077

Introduced by pytorch/pytorch#163036 I should have rejected that PR during the review ghstack-source-id: 770c1f1 Pull-Request: pytorch/pytorch#172046 ghstack-source-id: 8fccd95 Pull Request resolved: pytorch/pytorch#172080

Fix invalid indices bug for max_unpool2d/3d on MPS

5336eba

can-gaa-hou requested review from kulinseth and malfet as code owners September 16, 2025 02:28

pytorch-bot bot added the release notes: mps Release notes category label Sep 16, 2025

pytorchbot added the open source label Sep 16, 2025

kulinseth approved these changes Sep 16, 2025

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 16, 2025

pytorchmergebot added the merging label Sep 16, 2025

pytorchmergebot removed the merging label Sep 16, 2025

Skylion007 reviewed Sep 16, 2025

View reviewed changes

aten/src/ATen/native/mps/operations/Pooling.mm Outdated Show resolved Hide resolved

malfet requested changes Sep 16, 2025

View reviewed changes

test/nn/test_pooling.py Outdated Show resolved Hide resolved

aten/src/ATen/native/mps/operations/Pooling.mm Outdated Show resolved Hide resolved

aten/src/ATen/native/mps/operations/Pooling.mm Outdated Show resolved Hide resolved

Update aten/src/ATen/native/mps/operations/Pooling.mm

0eb78e6

Co-authored-by: Nikita Shulga <2453524+malfet@users.noreply.github.com>

pytorch-bot bot removed the ciflow/trunk Trigger trunk jobs on your pull request label Sep 17, 2025

Fix some issues in comments

20e4e0c

can-gaa-hou requested review from Skylion007 and malfet September 17, 2025 07:19

mikaylagawarecki added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 18, 2025

malfet approved these changes Sep 18, 2025

View reviewed changes

malfet added topic: bug fixes topic category ciflow/mps Run MPS tests (subset of trunk) labels Sep 18, 2025

pytorch-bot bot removed the ciflow/mps Run MPS tests (subset of trunk) label Sep 18, 2025

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 19, 2025

pytorchmergebot added the merging label Sep 19, 2025

pytorchmergebot added the Merged label Sep 19, 2025

pytorchmergebot closed this in ce5637b Sep 19, 2025

pytorchmergebot removed the merging label Sep 19, 2025

malfet mentioned this pull request Jan 9, 2026

[MPS] Remove error-checking sync point from MaxUnpool #172046

Closed

malfet added a commit that referenced this pull request Jan 9, 2026

[MPS] Remove sync-point from MaxUnpool

2429a3a

Introduced by #163036 I should have rejected that PR during the review ghstack-source-id: 75c86d2 Pull-Request: #172046

malfet added a commit that referenced this pull request Jan 9, 2026

[MPS] Remove sync-point from MaxUnpool

3de03eb

Introduced by #163036 I should have rejected that PR during the review ghstack-source-id: 770c1f1 Pull-Request: #172046

This was referenced Jan 9, 2026

[MPS] Remove sync-point from MaxUnpool #172077

Closed

[MPS] Remove sync-point from MaxUnpool #172080

Closed

malfet added a commit that referenced this pull request Jan 9, 2026

[MPS] Remove sync-point from MaxUnpool

8e1767c

Introduced by #163036 I should have rejected that PR during the review ghstack-source-id: ff2883b Pull-Request: #172046

pytorchbot mentioned this pull request Jan 9, 2026

[MPS] Remove error-checking sync point from MaxUnpool #172111

Merged

Conversation

can-gaa-hou commented Sep 16, 2025

Uh oh!

pytorch-bot bot commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/163036

✅ No Failures

Uh oh!

can-gaa-hou commented Sep 16, 2025

Uh oh!

pytorchmergebot commented Sep 16, 2025

Merge failed

Uh oh!

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

pytorch-bot bot commented Sep 18, 2025

Uh oh!

malfet commented Sep 18, 2025

Uh oh!

pytorch-bot bot commented Sep 18, 2025

Uh oh!

can-gaa-hou commented Sep 19, 2025

Uh oh!

can-gaa-hou commented Sep 19, 2025

Uh oh!

pytorchmergebot commented Sep 19, 2025

Merge started

Uh oh!

malfet commented Jan 9, 2026

Uh oh!

can-gaa-hou commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pytorch-bot bot commented Sep 16, 2025 •

edited

Loading