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.

Sign up for GitHub

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

Jump to bottom

[vmap] Fix searchsorted batch rule for self_logical_rank == 0 #99526

Closed
wants to merge 3 commits into from
Closed

[vmap] Fix searchsorted batch rule for self_logical_rank == 0 #99526

wants to merge 3 commits into from

Conversation

qqaatw
Copy link
Collaborator

@qqaatw qqaatw commented Apr 19, 2023 •
edited

@qqaatw qqaatw mentioned this pull request Apr 19, 2023
Closed
@pytorch-bot
Copy link

pytorch-bot bot commented Apr 19, 2023 •
edited

🔗 Helpful Links

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

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

✅ No Failures

As of commit 0d10a31:
💚 Looks good so far! There are no failures yet. 💚

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

@qqaatw qqaatw mentioned this pull request Apr 19, 2023
Closed
qqaatw added a commit that referenced this pull request Apr 19, 2023
@qqaatw
[vmap] Fix searchsorted batch rule for self_logical_rank == 0
570afde 
ghstack-source-id: 65b71a4c34a5b498ebaa84002c3b0ecc4c5765a1
Pull Request resolved: #99526
qqaatw added a commit that referenced this pull request Apr 19, 2023
@qqaatw
[vmap] Fix searchsorted batch rule for self_logical_rank == 0
3060509 
ghstack-source-id: 7593bbd283a755f995ca489a0ae02af7fcf9a86b
Pull Request resolved: #99526
@qqaatw qqaatw added the release notes: functorch release notes category; Pertaining to torch.func or pytorch/functorch label Apr 19, 2023
zou3519
zou3519 approved these changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

good catch, thank you!

qqaatw added a commit that referenced this pull request Apr 20, 2023
@qqaatw
[vmap] Fix searchsorted batch rule for self_logical_rank == 0
d498d6d 
ghstack-source-id: 80cbe6694e7904ed4d3981291239bc54ba073402
Pull Request resolved: #99526
@qqaatw
Copy link
Collaborator Author

qqaatw commented Apr 20, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 20, 2023
@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / macos-12-py3-arm64 / test (default, 1, 3, macos-m1-12)

Details for Dev Infra team Raised by workflow job

@qqaatw
Copy link
Collaborator Author

qqaatw commented Apr 20, 2023

@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

@zou3519 zou3519 mentioned this pull request Apr 20, 2023
Closed
@zou3519
Copy link
Contributor

zou3519 commented Apr 20, 2023

@pytorchbot revert -r "Regressed behavior"

@pytorch-bot
Copy link

pytorch-bot bot commented Apr 20, 2023

❌ 🤖 pytorchbot command failed:

@pytorchbot revert: error: the following arguments are required: -m/--message, -c/--classification

usage: @pytorchbot revert -m MESSAGE -c
                          {nosignal,ignoredsignal,landrace,weird,ghfirst}

Try @pytorchbot --help for more info.

@zou3519
Copy link
Contributor

zou3519 commented Apr 20, 2023

@pytorchbot revert -m "Regressed behavior" -c nosignal

@zou3519
Copy link
Contributor

zou3519 commented Apr 20, 2023

@qqaatw sorry, we had to revert this PR because it regressed behavior in searchsorted that wasn't tested in our test suite (see #99603). Please feel free to submit an updated PR if you're still interested!

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@qqaatw your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Apr 20, 2023
@pytorchmergebot
Revert "[vmap] Fix searchsorted batch rule for self_logical_rank == 0 (…
35ad512 
…#99526)"

This reverts commit 6580b16.

Reverted #99526 on behalf of https://github.com/zou3519 due to Regressed behavior
@qqaatw qqaatw reopened this Apr 20, 2023
@qqaatw
Copy link
Collaborator Author

qqaatw commented Apr 20, 2023 •
edited

@zou3519 I think those are separate issues. Even reverting this PR #99603 still fails. Can you please check again, thanks!

Specifically, this PR only handles the case where self_logical_rank==0 and doesn't touch other cases. The self_logical_rank in the new issue is 2 I think.

cc @kshitij12345

@zou3519
Copy link
Contributor

zou3519 commented Apr 20, 2023

@qqaatw My apologies, I should have checked before I did the revert. Yes, you are correct that this PR doesn't change the behavior, even after the revert the bug is still there. Let's get this PR merged.

zou3519
zou3519 approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

Sorry, the revert was a mistake. Let's get this PR merged.

@qqaatw
Copy link
Collaborator Author

qqaatw commented Apr 20, 2023

@zou3519 No worries, I will check if I can resolve the new issue later.

@qqaatw
Copy link
Collaborator Author

qqaatw commented Apr 20, 2023

@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

@facebook-github-bot facebook-github-bot deleted the gh/qqaatw/20/head branch June 8, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zou3519 zou3519 zou3519 approved these changes

@Chillee Chillee Awaiting requested review from Chillee

@kshitij12345 kshitij12345 Awaiting requested review from kshitij12345

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged merging open source release notes: functorch release notes category; Pertaining to torch.func or pytorch/functorch Reverted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@qqaatw @pytorchmergebot @zou3519 @pytorchbot