Skip to content

[ATen][Sparse] Use Third-Party Eigen for sparse addmm #101814

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
Closed
wants to merge 14 commits into from
Closed

[ATen][Sparse] Use Third-Party Eigen for sparse addmm #101814

wants to merge 14 commits into from

Conversation

Aidyn-A
Copy link
Collaborator

@Aidyn-A Aidyn-A commented May 18, 2023 •
edited
Loading

This pull request adds a functionality for addmm with the following layouts:

    add(a_csr, b_csr)

    addmm(c_csr, a_csr, b_csr)
    addmm(c_csr, a_csr, b_csc)
    addmm(c_csr, a_csc, b_csc)
    addmm(c_csr, a_csc, b_csr)

using Eigen for non-MKL builds.

Edit: thanks to the comment #101814 (comment) the following ops were also added:

    add(a_csc, b_csc)

    addmm(c_csc, a_csr, b_csr)
    addmm(c_csc, a_csr, b_csc)
    addmm(c_csc, a_csc, b_csc)
    addmm(c_csc, a_csc, b_csr)

@pytorch-bot
Copy link

pytorch-bot bot commented May 18, 2023 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit e92166b with merge base e872bf8 (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 the release notes: sparse release notes category label May 18, 2023
@Aidyn-A Aidyn-A marked this pull request as ready for review May 19, 2023 01:23
@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented May 19, 2023

@pytorchbot label "ciflow/trunk"

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 19, 2023
@jbschlosser jbschlosser requested review from amjames, pearu and cpuhrsch and removed request for amjames May 19, 2023 20:52
@jbschlosser jbschlosser added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 19, 2023
@cpuhrsch
Copy link
Contributor

Thank you for the pull request @Aidyn-A - which issue does this address? Do you have timings on the performance?

@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented May 23, 2023 •
edited
Loading

Hi @cpuhrsch,
The reason why this PR exist is that sparse operations add, addmm and matmul are not available on CPUs without MKL. Therefore, the following architectures are left out:

  • All machines with ARM based CPUs
  • All Windows OS machines
  • All modern Mac machines

Among existing issues I found this one:

I do not have performance numbers at the moment, but I can provide some.

cpuhrsch
cpuhrsch reviewed May 23, 2023
View reviewed changes
ngimel
ngimel reviewed May 24, 2023
View reviewed changes
amjames
amjames reviewed May 24, 2023
View reviewed changes
Copy link
Collaborator

@amjames amjames left a comment

Choose a reason for hiding this comment

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

Couple of comments, however all of them assume that there is general agreement on introducing an eigen dependency.

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cpuhrsch
Copy link
Contributor

I'm going to import this to get a sense for the internal build setup and whether this needs to be co-landed.

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cpuhrsch
Copy link
Contributor

Various internal build system changes should make this GH1 landable, but I'm reimporting to verify. I'll be on PTO in a few days, but back July 10th. If it doesn't land before I go on PTO, I'll pick this back up then.

cpuhrsch
cpuhrsch reviewed Jun 21, 2023
View reviewed changes
aten/src/ATen/native/eigen/SparseBlasImpl.cpp Outdated
@@ -0,0 +1,381 @@
#include <ATen/native/eigen/SparseBlasImpl.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this file into
aten/src/ATen/native/*sparse*/eigen/SparseBlasImpl.cpp instead of aten/src/ATen/native/eigen/SparseBlasImpl.cpp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hope 6a91e00 is what you meant

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented Aug 9, 2023

Hi @cpuhrsch,
Hope you were able to apply the system changes you mentioned. Shall I rebase this PR to be up-to-date with the latest PyTorch?

@cpuhrsch
Copy link
Contributor

@Aidyn-A - Yes, could you please try to rebase and I'll confirm that it works internally? Please mention me once that's done.

@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented Sep 12, 2023

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

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

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@atalman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@github-actions github-actions bot closed this May 9, 2024
@Aidyn-A Aidyn-A reopened this Mar 11, 2025
@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented Mar 11, 2025

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot pytorchmergebot force-pushed the eigen_for_sparse_addmm branch from 5d21600 to cec2a79 Compare March 11, 2025 16:38
@github-actions github-actions bot closed this Apr 10, 2025
@Aidyn-A Aidyn-A reopened this Apr 30, 2025
@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented Apr 30, 2025

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot pytorchmergebot force-pushed the eigen_for_sparse_addmm branch from cec2a79 to e92166b Compare April 30, 2025 16:14
@Aidyn-A
Copy link
Collaborator Author

Aidyn-A commented May 5, 2025

@cpuhrsch re-opening this PR. Is there anything I can do to resolve the internal failures?

@github-actions github-actions bot closed this Jun 4, 2025
@Aidyn-A Aidyn-A mentioned this pull request Jun 6, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuhrsch cpuhrsch cpuhrsch left review comments

@amjames amjames amjames left review comments

@ngimel ngimel ngimel left review comments

@pearu pearu Awaiting requested review from pearu

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request open source release notes: sparse release notes category Stale 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.
9 participants
@Aidyn-A @cpuhrsch @facebook-github-bot @pytorchmergebot @atalman @amjames @ngimel @pytorchbot @jbschlosser