KEMBAR78
Fixing MPS conv1d error message for output 2**16 by jhavukainen · Pull Request #134770 · pytorch/pytorch · GitHub
Skip to content

Conversation

@jhavukainen
Copy link
Collaborator

Fixes #134416 by removing the misleading message.

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 29, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit ddae7f1 with merge base 4fcd15a (image):
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added ciflow/mps Run MPS tests (subset of trunk) release notes: mps Release notes category labels Aug 29, 2024
Copy link
Contributor

@hvaara hvaara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/pytorch/pytorch/pull/134770/files#diff-2b02a4fc9ee55c0e5e6c3ca50de5211843685b34b7f3fd709710b09504ccca78L381-L387

Should this be updated as well?

edit: Reviewed on mobile. Not sure why the code snippet didn't get included.

@qqaatw
Copy link
Collaborator

qqaatw commented Aug 29, 2024

Another approach would be emitting a one-time warning and re-dispatch to cpu here :)

@hvaara
Copy link
Contributor

hvaara commented Aug 29, 2024

Another approach would be emitting a one-time warning and re-dispatch to cpu here :)

I like this option. This is what I was going to do. It's actually quite easy to do in aten/src/ATen/native/Convolution.cpp.

@jhavukainen
Copy link
Collaborator Author

https://github.com/pytorch/pytorch/pull/134770/files#diff-2b02a4fc9ee55c0e5e6c3ca50de5211843685b34b7f3fd709710b09504ccca78L381-L387

Should this be updated as well?

edit: Reviewed on mobile. Not sure why the code snippet didn't get included.

Yeah that as well. Updated the PR so we can close the original issue of misleading error message while the work for a proper workaround happens.

I like this option. This is what I was going to do. It's actually quite easy to do in aten/src/ATen/native/Convolution.cpp.

That sounds good to me. Let me know if you'd still like to make a PR for that. Otherwise I'll add it to my todo list.

@hvaara
Copy link
Contributor

hvaara commented Aug 29, 2024

That sounds good to me. Let me know if you'd still like to make a PR for that. Otherwise I'll add it to my todo list.

I don't want to add to your workload if you're busy. Current solution proposed in this PR also LGTM. I can make a quick attempt and we can see if special casing is a solution we want to move forward with.

@jhavukainen
Copy link
Collaborator Author

@pytorchbot merge

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 29, 2024

This PR needs to be approved by an authorized maintainer before merge.

@janeyx99 janeyx99 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 30, 2024
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@malfet
Copy link
Contributor

malfet commented Oct 18, 2024

@pytorchbot merge -f "Lint is green and this is a error only change"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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

@github-actions github-actions bot deleted the dev/joona/fixConv1DErrorMsg branch November 18, 2024 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/mps Run MPS tests (subset of trunk) Merged open source release notes: mps Release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PYTORCH_ENABLE_MPS_FALLBACK does not appear to work for nn.Conv1d

7 participants