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 support for torch.linalg.vander #91749

Closed
wants to merge 15 commits into from
Closed

vmap support for torch.linalg.vander #91749

wants to merge 15 commits into from

Conversation

jazzysoggy
Copy link
Contributor

@jazzysoggy jazzysoggy commented Jan 5, 2023 •
edited by pytorch-bot bot

Adds vmap support for torch.linalg.vander in a similar manner to how view_as_complex is implemented.

#91700

cc @zou3519 @Chillee @samdow @soumith @kshitij12345 @janeyx99

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 5, 2023 •
edited

🔗 Helpful Links

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

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

❌ 3 Failures

As of commit 9a5a7d4:

NEW FAILURES - The following jobs have failed:

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

@jazzysoggy jazzysoggy marked this pull request as ready for review January 5, 2023 18:17
@kshitij12345
Copy link
Collaborator

@jazzysoggy Thank you for the PR

Looking at the CI failure link, it looks like you should add entry in (do refer other entries in that file) https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/functorch/BatchRulesDecompositions.cpp

This is because linalg_vander is a CompositeImplicitAutograd op i.e. it is implemented using other operators. (So given that the operators which are used to implement linalg_vander have a batching rule, linalg_vander should work).

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

@kshitij12345 How do I fix the failure due to the test passing instead of failing?

@kshitij12345
Copy link
Collaborator

@jazzysoggy

From the CI failure, we can see that test_unimplemented_batched_registration is failing for linalg_vander. So we can assume that we are marking this op as expected failure for this test. However, since this PR fixes that (by adding a batching rule), we will have to remove that xfail.

I would suggest to grep for test_unimplemented_batched_registration and try to find the relevant xfails for it.

Let me know if you need more suggestions :)

CI Failure Snippet:

======================================================================
[2226](https://github.com/pytorch/pytorch/actions/runs/3850590625/jobs/6561243328#step:10:2228)
XPASS [0.001s]: test_unimplemented_batched_registrations_[aten::linalg_vander] (__main__.TestFunctorchDispatcher)
[2227](https://github.com/pytorch/pytorch/actions/runs/3850590625/jobs/6561243328#step:10:2229)
----------------------------------------------------------------------

lezcano
lezcano requested changes Jan 9, 2023
View reviewed changes
aten/src/ATen/functorch/BatchRulesDecompositions.cpp Outdated
@@ -144,7 +144,7 @@ TORCH_LIBRARY_IMPL(aten, FuncTorchBatchedDecomposition, m) {
OP_DECOMPOSE2(less, Tensor );
OP_DECOMPOSE(linalg_cond);
OP_DECOMPOSE(linalg_cholesky);
OP_DECOMPOSE(linalg_det);
OP_DECOMPOSE(c);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change looks unintended.

aten/src/ATen/functorch/BatchRulesDecompositions.cpp Outdated
@@ -159,6 +159,8 @@ TORCH_LIBRARY_IMPL(aten, FuncTorchBatchedDecomposition, m) {
OP_DECOMPOSE(linalg_svd);
OP_DECOMPOSE(linalg_svdvals);
OP_DECOMPOSE(linalg_tensorinv);
OP_DECOMPOSE(linalg_vander);
OP_DECOMPOSE(linalg_vander, int);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line is incorrect and unnecessary.

@IvanYashchuk IvanYashchuk removed their request for review January 9, 2023 07:00
lezcano
lezcano reviewed Jan 9, 2023
View reviewed changes
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

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

#91749 is still missing, but otherwise it should be good.

Consider running locally the tests in test/functorch for linalg_vander to avoid running through too many CI cycles.

@kshitij12345
Copy link
Collaborator

@jazzysoggy It seems that you will also have to remove the following xfail as this PR implements batching rule for linalg_vander.

pytorch/test/functorch/test_vmap_registrations.py

Line 182 in 630ef6c

"aten::linalg_vander",

Post the change, you can verify it locally by python test/functorch/test_vmap_registrations.py

@lezcano
Copy link
Collaborator

lezcano commented Jan 16, 2023

the failures are relevant. Please run locally the tests in test/functorch with -vk vander before updating the PR to make sure all the relevant tests are passing.

@kshitij12345 kshitij12345 mentioned this pull request Jan 18, 2023
Closed
@jazzysoggy
Copy link
Contributor Author

Can I ignore the current errors seeing as they do not to the actual testing, which passed, but rather the uploading of the test results and missing a "typing extension"

lezcano
lezcano approved these changes Jan 19, 2023
View reviewed changes
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

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

Right, thank you!

@lezcano lezcano added ciflow/trunk Trigger trunk jobs on your pull request module: functorch Pertaining to torch.func or pytorch/functorch release notes: functorch release notes category; Pertaining to torch.func or pytorch/functorch labels Jan 19, 2023
@lezcano
Copy link
Collaborator

lezcano commented Jan 19, 2023

@pytorchbot merge -f "errors are unrelated"

@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).

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chillee Chillee Chillee left review comments

@lezcano lezcano lezcano approved these changes

@zou3519 zou3519 Awaiting requested review from zou3519

@kshitij12345 kshitij12345 Awaiting requested review from kshitij12345

@srossross srossross Awaiting requested review from srossross

@nikitaved nikitaved Awaiting requested review from nikitaved

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: functorch Pertaining to torch.func or pytorch/functorch open source release notes: functorch release notes category; Pertaining to torch.func or pytorch/functorch triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jazzysoggy @kshitij12345 @lezcano @pytorchmergebot @Chillee @pytorchbot @mikaylagawarecki