Skip to content

TST: fix GPU failures #21294

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
Merged
merged 8 commits into from
Aug 3, 2024
Merged

TST: fix GPU failures #21294

merged 8 commits into from
Aug 3, 2024

Conversation

lucascolley
Copy link
Member

Reference issue

Closes gh-21227, closes gh-21292, closes gh-20957.

What does this implement/fix?

This PR makes SCIPY_DEVICE=cuda python dev.py test -b all pass for me locally, in an environment with CuPy, PyTorch and JAX.

Additional information

Please take a look at any parts you wrote and let me know if the modifications look okay!

This diff is longer than it would be if we figured out how to "stack" skip_xp_backends decorators. But I haven't given that any thought recently, it seems difficult.

@lucascolley lucascolley requested review from rgommers, ev-br, mdhaber and tylerjereddy and removed request for andyfaff, steppi, peterbell10 and person142 July 31, 2024 18:36
@lucascolley lucascolley added this to the 1.15.0 milestone Jul 31, 2024
@lucascolley lucascolley added array types Items related to array API support and input array validation (see gh-18286) and removed scipy.integrate scipy.fft scipy.differentiate labels Jul 31, 2024
@lucascolley
Copy link
Member Author

@mdhaber you wrote in #20957 (comment) that you think some modifications to the dev docs would be useful here. Could you clarify what you would like?

@mdhaber
Copy link
Contributor

mdhaber commented Jul 31, 2024 •
edited
Loading

The guide says:
image

and to add the @array_api_compatible decorator to tests. In some cases, this leads to GPU test failures not caught in CI or even in local runs with CuPy.

It would be nice to update this with either:

  • how to use from_dlpack rather than asarray to do the conversion (if we can agree that returning a correct result is preferable to raising an error) or
  • suggestions about skipping all the non-CPU backends that might fail, even if they are not tested in CI or commonly tested locally.

@lucascolley
Copy link
Member Author

lucascolley commented Jul 31, 2024 •
edited
Loading

Okay, I'll add an example using cpu_only=True (and exceptions now that that is new) in this PR, and a few sentences to explain that it should be used whenever the xp->np->xp pattern is used.

Of course, this can change if/when we start using from_dlpack.

even if they are not tested in CI or commonly tested locally.

We discussed this in the community meeting and we should be able to look into GPU CI soon.

lucascolley
lucascolley commented Jul 31, 2024
View reviewed changes
@lucascolley lucascolley mentioned this pull request Aug 1, 2024
Closed
rgommers
rgommers reviewed Aug 2, 2024
View reviewed changes
scipy/ndimage/tests/test_filters.py
@@ -178,7 +179,8 @@ def test_correlate01(self, xp):
output = ndimage.convolve1d(array, weights)
assert_array_almost_equal(output, expected)

@skip_xp_backends("jax.numpy", reasons=["output array is read-only."])
@skip_xp_backends("jax.numpy", reasons=["output array is read-only."],
cpu_only=True, exceptions=['cupy', 'jax.numpy'])
Copy link
Member

Choose a reason for hiding this comment

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

How can jax.numpy be both the first argument (the backend to skip) and in the exceptions list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Being the first argument means "always skip due to a reason in reasons". Being in exceptions means "don't get skipped by cpu_only when not on CPU as delegation is implemented".

In this case, I just went with the blanket "delegation is implemented for CuPy and JAX in this module", even though the support for JAX is patchy. In its current state, JAX could be removed from exceptions here. But if (somehow) JAX was no longer skipped via the first argument, we would want to add it back to exceptions to avoid the cpu_only skip.

Evidently this API is not perfectly intuitive, but I haven't thought of anything better yet. Please let me know if you do!

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that explanation seems reasonable. I can't think of something much cleaner. Explicitly explaining this case in the skip_xp_backends docstring would be useful, to avoid future confusion to the extent possible.

rgommers
rgommers reviewed Aug 2, 2024
View reviewed changes
rgommers
rgommers reviewed Aug 2, 2024
View reviewed changes
rgommers
rgommers reviewed Aug 2, 2024
View reviewed changes
rgommers
rgommers approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

I added a couple of comments/questions, but none are blocking. Overall this looks great, so +1 to merge soon from me.

@lucascolley
Copy link
Member Author

Thanks Ralf, I'll give the docs an update then this is ready.

Unless the test failures are annoying anyone locally, in which case feel free to merge immediately.

@lucascolley lucascolley force-pushed the xp-skips branch 2 times, most recently from b3e5329 to 45fb169 Compare August 3, 2024 14:53
lucascolley
lucascolley commented Aug 3, 2024
View reviewed changes
@lucascolley lucascolley merged commit a5808a3 into scipy:main Aug 3, 2024
@lucascolley lucascolley deleted the xp-skips branch August 3, 2024 17:19
@lucascolley
Copy link
Member Author

merged - clean-up and docs update inbound

This was referenced Aug 3, 2024
Merged
Closed
Open
@rgommers
Copy link
Member

rgommers commented Aug 7, 2024

We discussed this in the community meeting and we should be able to look into GPU CI soon.

FYI I did put in that credit card request after the meeting and just received a reply on it - should arrive soon 🤞🏼

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

@rgommers rgommers rgommers approved these changes

@ev-br ev-br Awaiting requested review from ev-br

@mdhaber mdhaber Awaiting requested review from mdhaber

@tylerjereddy tylerjereddy Awaiting requested review from tylerjereddy

Assignees
No one assigned
Labels
array types Items related to array API support and input array validation (see gh-18286) maintenance Items related to regular maintenance tasks
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.

TST: ndimage: GPU test failures BUG: stats/fft/differentiate/optimize: test suite failures with CuPy 13.2.0 TST, MAINT: array API GPU test failures
3 participants
@lucascolley @mdhaber @rgommers