Port linalg_eigh and linalg_eigvalsh to structured by lezcano · Pull Request #79072 · pytorch/pytorch

lezcano · 2022-06-07T22:19:12Z

Stack from ghstack:

-> Port linalg_eigh and linalg_eigvalsh to structured #79072

This follows the structure of linalg.svd.

This follows the structure of linalg.svd. [ghstack-poisoned]

facebook-github-bot · 2022-06-07T22:19:17Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/79072
📄 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 our office hours

✅ No Failures (0 Pending)

As of commit 735a532 (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

This follows the structure of linalg.svd. ghstack-source-id: 78f42dc Pull Request resolved: #79072

pytorch-bot · 2022-06-08T21:23:40Z

The ciflow/all label was recently removed. It ran very expensive periodic CI jobs when most contributors did not need them. If you just want to check that you won't be reverted, use ciflow/trunk. If you really want the old ciflow/all behavior, add ciflow/trunk and ciflow/periodic.You can use any of the following

ciflow/trunk (.github/workflows/trunk.yml): all jobs we run per-commit on master
ciflow/periodic (.github/workflows/periodic.yml): all jobs we run periodically on master
ciflow/android (.github/workflows/run_android_tests.yml): android build and test
ciflow/nightly (.github/workflows/nightly.yml): all jobs we run nightly
ciflow/binaries: all binary build and upload jobs
ciflow/binaries_conda: binary build and upload job for conda
ciflow/binaries_libtorch: binary build and upload job for libtorch
ciflow/binaries_wheel: binary build and upload job for wheel

This follows the structure of linalg.svd. [ghstack-poisoned]

This follows the structure of linalg.svd. ghstack-source-id: a7d5033 Pull Request resolved: #79072

This follows the structure of linalg.svd. [ghstack-poisoned]

This follows the structure of linalg.svd. ghstack-source-id: 5bcc072 Pull Request resolved: #79072

lezcano · 2022-06-13T14:11:34Z

+               # Could not run 'aten::linalg_eigh' with arguments from the 'AutogradMeta' backend.
+               DecorateInfo(unittest.expectedFailure, 'TestFakeTensorNonErroring', 'test_fake'),


No clue why I'm seeing this error in this test and in the test_meta.py. Any idea @albanD ?

There were moves to make Meta a backend bit
cc @bdhirsh do you know what this error is about?

it probably has to do with Meta being moved to the backend bit, but that error is pretty strange. Let me see if I can debug locally.

Ah, found it....

Diagnosis: we have a meta function for linalg_eigh written directly in python, that gets registered to the Meta dispatch key:

pytorch/torch/_meta_registrations.py

Line 116 in bc82a5f

@torch.library.impl(meta_lib, "linalg_eigh")

.

I think we should leave the python function around in case its useful, but you can remove the @torch.library.impl(meta_lib, "linalg_eigh") line that registers it to the dispatcher (since this PR will give us meta tensor support through structured kernels, instead).

Why did that break stuff? linalg_eigh is now a CompositeImplicitAutograd kernel, which basically means that we should be getting support for it for all backends (including meta) through its decomposition. By registering a meta kernel to it directly, that basically tells the dispatcher "hey, now that I have a direct kernel for the meta backend, then don't run the decomposition anymore when you're using the meta backend

I'm also curious what you both think about the naming - we now have a private _linalg_eigh op that implements the behavior of the 4 public ops.

Consolidating the implementation into one op seems great, although backends will now need to support a private operator in order to support better linalg coverage (this is the case for a couple other linalg ops too).

I know there have been some conversations around "decomposing into private / unstable API's" being kinda annoying for backends. What do you think about that w.r.t. to the private linalg ops? Are they just more likely to change over time, so making them private is safer?

All this are very good points. I had a discussion about this with @mruberry the other day. What we thought is that, for now, the prim operations can simply be those from the public API. Going forward, we may want to have a lower level operation to be the prim with something similar to a LAPACK API, and then have svd, eigh and so on dispatch to these in Python.

In any case, following the current structure:

Makes implementing operations for backends fairly easily. If they have the LAPACK-like operation, they can always write two helper functions that call into it less than 10 LOC

Having these operations written as structured kernels makes prims easy to write, and will make moving to a LAPACK-style API later on rather easy.

IvanYashchuk

Looks good!

I don't know about TestFakeTensorNonErroring and how to fix it.

This follows the structure of linalg.svd. [ghstack-poisoned]

albanD

You most likely don't want the ideep submodule change in this PR?

albanD · 2022-06-14T14:16:12Z

+               # Could not run 'aten::linalg_eigh' with arguments from the 'AutogradMeta' backend.
+               DecorateInfo(unittest.expectedFailure, 'TestFakeTensorNonErroring', 'test_fake'),


There were moves to make Meta a backend bit
cc @bdhirsh do you know what this error is about?

This follows the structure of linalg.svd. [ghstack-poisoned]

This follows the structure of linalg.svd. ghstack-source-id: 66a97bd Pull Request resolved: #79072

lezcano · 2022-06-14T14:35:50Z

Sorry for the submodule point. Fixed. Should we just merge this one and figure out the MetaAutograd point later or should we wait to fix it?

albanD

I think that's fine to merge this for now while we wait on resolution for the meta part.
Could you move the discussion to an issue though and pin Brian there?

lezcano · 2022-06-14T15:49:36Z

@pytorchbot merge -g

pytorchmergebot · 2022-06-14T15:50:52Z

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

Summary: This follows the structure of linalg.svd. Pull Request resolved: #79072 Approved by: https://github.com/IvanYashchuk, https://github.com/albanD Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/549a597c004f7e0254cd4dd8f1777c57e0f00c00 Reviewed By: malfet Differential Revision: D37156838 fbshipit-source-id: 84607565591fb7ab97238a09dbcc40a1d39cb102

Following #79072 (comment) [ghstack-poisoned]

Following #79072 (comment) ghstack-source-id: 809acf7 Pull Request resolved: #79786

Following #79072 (comment) Pull Request resolved: #79786 Approved by: https://github.com/ngimel, https://github.com/bdhirsh

Summary: Following #79072 (comment) Pull Request resolved: #79786 Approved by: https://github.com/ngimel, https://github.com/bdhirsh Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/648a6658ec7dbf17f815a7d7f4464f9e6f7cb08f Reviewed By: malfet Differential Revision: D37278842 fbshipit-source-id: 29197e92bdfd5f53e901367e4ca96645c6d6f9db

This follows the structure of linalg.svd. Pull Request resolved: pytorch#79072 Approved by: https://github.com/IvanYashchuk, https://github.com/albanD

Following pytorch#79072 (comment) Pull Request resolved: pytorch#79786 Approved by: https://github.com/ngimel, https://github.com/bdhirsh

Port linalg_eigh and linalg_eigvalsh to structured

8ab6efc

This follows the structure of linalg.svd. [ghstack-poisoned]

lezcano requested review from IvanYashchuk, albanD, bdhirsh, mruberry, ngimel, nikitaved and soulitzer as code owners June 7, 2022 22:19

facebook-github-bot added the cla signed label Jun 7, 2022

lezcano added a commit that referenced this pull request Jun 7, 2022

Port linalg_eigh and linalg_eigvalsh to structured

1368d2e

This follows the structure of linalg.svd. ghstack-source-id: 78f42dc Pull Request resolved: #79072

lezcano removed request for albanD, bdhirsh, mruberry, ngimel, nikitaved and soulitzer June 7, 2022 22:19

lezcano added module: linear algebra Issues related to specialized linear algebra operations in PyTorch; includes matrix multiply matmul release notes: composability release notes category topic: not user facing topic category labels Jun 7, 2022

pytorchbot added the open source label Jun 7, 2022

lezcano added ciflow/all ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR labels Jun 8, 2022

Update on "Port linalg_eigh and linalg_eigvalsh to structured"

9e00ca3

This follows the structure of linalg.svd. [ghstack-poisoned]

lezcano added a commit that referenced this pull request Jun 11, 2022

Port linalg_eigh and linalg_eigvalsh to structured

54dd43d

This follows the structure of linalg.svd. ghstack-source-id: a7d5033 Pull Request resolved: #79072

Update on "Port linalg_eigh and linalg_eigvalsh to structured"

d0f5031

This follows the structure of linalg.svd. [ghstack-poisoned]

lezcano added a commit that referenced this pull request Jun 13, 2022

Port linalg_eigh and linalg_eigvalsh to structured

e8d7265

This follows the structure of linalg.svd. ghstack-source-id: 5bcc072 Pull Request resolved: #79072

lezcano commented Jun 13, 2022

View reviewed changes

IvanYashchuk approved these changes Jun 14, 2022

View reviewed changes

Update on "Port linalg_eigh and linalg_eigvalsh to structured"

297a54b

This follows the structure of linalg.svd. [ghstack-poisoned]

albanD requested changes Jun 14, 2022

View reviewed changes

Update on "Port linalg_eigh and linalg_eigvalsh to structured"

735a532

This follows the structure of linalg.svd. [ghstack-poisoned]

lezcano added a commit that referenced this pull request Jun 14, 2022

Port linalg_eigh and linalg_eigvalsh to structured

63de9e4

This follows the structure of linalg.svd. ghstack-source-id: 66a97bd Pull Request resolved: #79072

albanD approved these changes Jun 14, 2022

View reviewed changes

pytorchmergebot added the Merged label Jun 14, 2022

pytorchmergebot closed this in 549a597 Jun 14, 2022

lezcano mentioned this pull request Jun 17, 2022

Remove python implementation for eigh meta #79786

Closed

lezcano added a commit that referenced this pull request Jun 17, 2022

Remove python implementation for eigh meta

6c0e04c

Following #79072 (comment) [ghstack-poisoned]

lezcano added a commit that referenced this pull request Jun 17, 2022

Remove python implementation for eigh meta

704e6f0

Following #79072 (comment) ghstack-source-id: 809acf7 Pull Request resolved: #79786

pytorchmergebot pushed a commit that referenced this pull request Jun 17, 2022

Remove python implementation for eigh meta

648a665

Following #79072 (comment) Pull Request resolved: #79786 Approved by: https://github.com/ngimel, https://github.com/bdhirsh

facebook-github-bot deleted the gh/Lezcano/88/head branch June 18, 2022 14:17

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 25, 2026

Remove python implementation for eigh meta

8ec374d

Following pytorch#79072 (comment) Pull Request resolved: pytorch#79786 Approved by: https://github.com/ngimel, https://github.com/bdhirsh

		# Could not run 'aten::linalg_eigh' with arguments from the 'AutogradMeta' backend.
		DecorateInfo(unittest.expectedFailure, 'TestFakeTensorNonErroring', 'test_fake'),

Conversation

lezcano commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

pytorch-bot Bot commented Jun 8, 2022

Uh oh!

lezcano Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

albanD Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

lezcano Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

IvanYashchuk left a comment

Choose a reason for hiding this comment

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

albanD Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

lezcano commented Jun 14, 2022

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

lezcano commented Jun 14, 2022

Uh oh!

pytorchmergebot commented Jun 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lezcano commented Jun 7, 2022 •

edited

Loading

facebook-github-bot commented Jun 7, 2022 •

edited

Loading