Skip to content

Conversation

@Isalia20
Copy link
Collaborator

@Isalia20 Isalia20 commented Aug 17, 2025

Adds several unary functions and add. Enables tests for unary functions in test_sparse but not enabling other tests yet, needs more ops before we fully migrate to testing SparseMPS with test_sparse.py

cc @kulinseth @albanD @malfet @DenisVieriu97 @jhavukainen

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 17, 2025

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

⏳ 7 Pending, 1 Unrelated Failure

As of commit 52be981 with merge base d8a0bdb (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

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 Aug 17, 2025
@Isalia20 Isalia20 changed the title [MPS] sparse add unary funcs [MPS] sparse add unary funcs + add for sparse tensors Aug 17, 2025
@Isalia20 Isalia20 added the module: mps Related to Apple Metal Performance Shaders framework label Aug 17, 2025
@github-actions
Copy link
Contributor

Attention! native_functions.yaml was changed

If you are adding a new function or defaulted argument to native_functions.yaml, you cannot use it from pre-existing Python frontend code until our FC window passes (two weeks). Split your PR into two PRs, one which adds the new C++ functionality, and one that makes use of it from Python, and land them two weeks apart. See https://github.com/pytorch/pytorch/wiki/PyTorch's-Python-Frontend-Backward-and-Forward-Compatibility-Policy#forwards-compatibility-fc for more info.


Caused by:

@Isalia20 Isalia20 added the ciflow/mps Run MPS tests (subset of trunk) label Aug 17, 2025
@pytorch-bot pytorch-bot bot removed the ciflow/mps Run MPS tests (subset of trunk) label Aug 17, 2025
@Isalia20 Isalia20 added the ciflow/mps Run MPS tests (subset of trunk) label Aug 17, 2025
@pytorch-bot pytorch-bot bot removed the ciflow/mps Run MPS tests (subset of trunk) label Aug 17, 2025
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

I haven't look much into the ops implementation, but testing look problematic.
Generally speaking avoid skipping the test, instead prefer fail.
Also, prefer adding decorators to their respective OpInfo definitions rather than test itself

self.assertEqual(coalesced_mps._indices().cpu(), coalesced_cpu._indices())
self.assertEqual(coalesced_mps._values().cpu(), coalesced_cpu._values())

def test_sparse_add(self):
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 please elaborate why this long test is needed and isn't it covered by test_sparse?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I enabled unary function tests on test_sparse so far, before I enable other tests, which need more work, I added a test here so we can have some testing functionality before we completely move to testing with test_sparse.py

Copy link
Contributor

Choose a reason for hiding this comment

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

If there are testing on test_sparse, let's just used those...

Comment on lines 49 to 50
MPS_UNSUPPORTED_TYPES = [torch.double, torch.cdouble]
MPS_DTYPES = [t for t in get_all_dtypes() if t not in MPS_UNSUPPORTED_TYPES]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind moving this logic to common_mps and use from both test_mps and test_sparse_

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this completely

"linalg.eig": None,
"linalg.eigvals": None,
"put": None,
# "conj_physical": None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, leftover, removed

@Isalia20 Isalia20 added the ciflow/mps Run MPS tests (subset of trunk) label Aug 17, 2025
@pytorch-bot pytorch-bot bot removed the ciflow/mps Run MPS tests (subset of trunk) label Aug 17, 2025
@Isalia20 Isalia20 added the ciflow/mps Run MPS tests (subset of trunk) label Aug 17, 2025
@pytorch-bot pytorch-bot bot removed the ciflow/mps Run MPS tests (subset of trunk) label Aug 18, 2025
@pytorchmergebot pytorchmergebot added Reverted ci-no-td Do not run TD on this PR labels Aug 29, 2025
@pytorch-bot pytorch-bot bot dismissed malfet’s stale review August 29, 2025 19:55

This PR was reopened (likely due to being reverted), so your approval was removed. Please request another review.

malfet added a commit that referenced this pull request Aug 29, 2025
Always decorate for MPS device. This fixes regressions in sparse testing
that were introduced by #160839

ghstack-source-id: 12cb236
Pull Request resolved: #161814
@pytorch-bot pytorch-bot bot removed ciflow/trunk Trigger trunk jobs on your pull request ciflow/mps Run MPS tests (subset of trunk) labels Aug 29, 2025
@malfet malfet added ciflow/trunk Trigger trunk jobs on your pull request ciflow/mps Run MPS tests (subset of trunk) labels Aug 29, 2025
@malfet
Copy link
Contributor

malfet commented Aug 30, 2025

@pytorchbot merge -f "2nd time is the charm"

@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). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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: PR #160839 has not been reviewed yet

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@malfet
Copy link
Contributor

malfet commented Aug 30, 2025

@pytorchbot merge -f "Second time is the charm"

@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). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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

markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
Adds several unary functions and add. Enables tests for unary functions in test_sparse but not enabling other tests yet, needs more ops before we fully migrate to testing SparseMPS with `test_sparse.py`

Pull Request resolved: pytorch#160839
Approved by: https://github.com/malfet

Co-authored-by: Nikita Shulga <[email protected]>
markc-614 pushed a commit to markc-614/pytorch that referenced this pull request Sep 17, 2025
Adds several unary functions and add. Enables tests for unary functions in test_sparse but not enabling other tests yet, needs more ops before we fully migrate to testing SparseMPS with `test_sparse.py`

Pull Request resolved: pytorch#160839
Approved by: https://github.com/malfet

Co-authored-by: Nikita Shulga <[email protected]>
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
Adds several unary functions and add. Enables tests for unary functions in test_sparse but not enabling other tests yet, needs more ops before we fully migrate to testing SparseMPS with `test_sparse.py`

Pull Request resolved: pytorch#160839
Approved by: https://github.com/malfet

Co-authored-by: Nikita Shulga <[email protected]>
mansiag05 pushed a commit to mansiag05/pytorch that referenced this pull request Sep 22, 2025
Adds several unary functions and add. Enables tests for unary functions in test_sparse but not enabling other tests yet, needs more ops before we fully migrate to testing SparseMPS with `test_sparse.py`

Pull Request resolved: pytorch#160839
Approved by: https://github.com/malfet

Co-authored-by: Nikita Shulga <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-no-td Do not run TD on this PR ciflow/mps Run MPS tests (subset of trunk) ciflow/trunk Trigger trunk jobs on your pull request Merged module: mps Related to Apple Metal Performance Shaders framework open source release notes: sparse release notes category Reverted 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.

6 participants