-
Notifications
You must be signed in to change notification settings - Fork 26.3k
[MPS] sparse add unary funcs + add for sparse tensors #160839
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.
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
Conversation
🔗 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 SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ⏳ 7 Pending, 1 Unrelated FailureAs of commit 52be981 with merge base d8a0bdb ( 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. |
Attention! native_functions.yaml was changedIf 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: |
malfet
left a comment
There was a problem hiding this 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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
test/test_sparse.py
Outdated
| MPS_UNSUPPORTED_TYPES = [torch.double, torch.cdouble] | ||
| MPS_DTYPES = [t for t in get_all_dtypes() if t not in MPS_UNSUPPORTED_TYPES] |
There was a problem hiding this comment.
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_
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, leftover, removed
This PR was reopened (likely due to being reverted), so your approval was removed. Please request another review.
|
@pytorchbot merge -f "2nd time is the charm" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: PR #160839 has not been reviewed yet |
|
@pytorchbot merge -f "Second time is the charm" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
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]>
…h#160839)" This reverts commit 93c5112. Reverted pytorch#160839 on behalf of https://github.com/atalman due to test_sparse_csr.py::TestSparseCompressedCPU::test_consistency_SparseCSR_asinh_cpu_complex64 [GH job link](https://github.com/pytorch/pytorch/actions/runs/17329155095/job/49201551217) [HUD commit link](https://hud.pytorch.org/pytorch/pytorch/commit/93c5112f46a978a029644ae599979416ead5c917) ([comment](pytorch#160839 (comment)))
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]>
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]>
…h#160839)" This reverts commit 93c5112. Reverted pytorch#160839 on behalf of https://github.com/atalman due to test_sparse_csr.py::TestSparseCompressedCPU::test_consistency_SparseCSR_asinh_cpu_complex64 [GH job link](https://github.com/pytorch/pytorch/actions/runs/17329155095/job/49201551217) [HUD commit link](https://hud.pytorch.org/pytorch/pytorch/commit/93c5112f46a978a029644ae599979416ead5c917) ([comment](pytorch#160839 (comment)))
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]>
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.pycc @kulinseth @albanD @malfet @DenisVieriu97 @jhavukainen