Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inconsistent torch.nn.MaxPool1d output on cpu and gpu #99843

Closed
wants to merge 5 commits into from
Closed

Fix inconsistent torch.nn.MaxPool1d output on cpu and gpu #99843

wants to merge 5 commits into from

Conversation

tfsingh
Copy link
Contributor

@tfsingh tfsingh commented Apr 23, 2023

Fixes #99412 , correctly raising an error when an output of invalid size is calculated.

Would be happy to iterate on this if there are any issues.

cc @malfet @jbschlosser

@pytorch-bot
Copy link

pytorch-bot bot commented Apr 23, 2023

🔗 Helpful Links

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

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

❌ 2 New Failures

As of commit 7025c3d:

NEW FAILURES - The following jobs have failed:

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

@mikaylagawarecki mikaylagawarecki added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 25, 2023
@tfsingh
Copy link
Contributor Author

tfsingh commented May 4, 2023

Hi @mikaylagawarecki , were you able to take a look at this pr? Apologies for the ping, would just love to get this merged.

Copy link
Contributor

@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

Hey @tfsingh , so sorry for the delay! Could you add a test in test_nn that ensures the inconsistency is fixed please!

@tfsingh
Copy link
Contributor Author

tfsingh commented May 5, 2023

Of course, I just added one. I used self.assertRaises to make the CI tests pass, but I'm not sure why assertRaisesrRegex wasn't working in commit e6ae619:

AssertionError: "max_pool1d() Invalid computed output size: 0" does not match "max_pool1d() Invalid computed output size: 0"

These seem to be the same to me?

@mikaylagawarecki
Copy link
Contributor

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased onedpooling onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout onedpooling && git pull --rebase)

Copy link
Contributor

@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

Thanks!

@mikaylagawarecki
Copy link
Contributor

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 5, 2023
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@tfsingh
Copy link
Contributor Author

tfsingh commented May 8, 2023

@pytorchbot label "error checking"

@pytorch-bot
Copy link

pytorch-bot bot commented May 8, 2023

Didn't find following labels among repository labels: error checking

@tfsingh
Copy link
Contributor Author

tfsingh commented May 8, 2023

@pytorchbot label "module: error checking"

@pytorch-bot pytorch-bot bot added module: error checking Bugs related to incorrect/lacking error checking release notes: nn release notes category labels May 8, 2023
@tfsingh
Copy link
Contributor Author

tfsingh commented May 15, 2023

Hi @mikaylagawarecki , could you restart the merge when you get a chance? Thanks!

@mikaylagawarecki mikaylagawarecki added the topic: bug fixes topic category label May 15, 2023
@mikaylagawarecki
Copy link
Contributor

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased onedpooling onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout onedpooling && git pull --rebase)

@mikaylagawarecki
Copy link
Contributor

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

jcaip pushed a commit that referenced this pull request May 23, 2023
Fixes #99412 , correctly raising an error when an output of invalid size is calculated.

Would be happy to iterate on this if there are any issues.

Pull Request resolved: #99843
Approved by: https://github.com/mikaylagawarecki
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: error checking Bugs related to incorrect/lacking error checking open source release notes: nn release notes category topic: bug fixes topic 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.

the output of torch.nn.MaxPool1d is inconsistent on cpu and gpu
4 participants