TST/BUG: fix array API test skip decorators

[lucascolley]

Reference PR

Fixes decorators which were introduced in #18668.

What does this implement/fix?

• functools.wraps is introduced to skip_if_array_api_backend so that it can be used as a decorator together with pytest.mark.parametrize.
• skip_if_array_api_gpu is rewritten in a similar style to skip_if_array_api_backend so that it functions properly. Previously, when used with python dev.py test -b all, decorated tests would be skipped on every backend, not just those which use GPU.

Additional information

The discussion leading up to this PR occurred in the comments of #19005.

If there is a CI fail due to # type: ignore[misc], perhaps this can be removed now? done.

[lucascolley]

@tylerjereddy CI is green, how does this look?

[tylerjereddy]

If I do this to test on this branch:

--- a/scipy/cluster/tests/test_vq.py
+++ b/scipy/cluster/tests/test_vq.py
@@ -282,7 +282,8 @@ class TestKMean:
 
     @array_api_compatible
     @skip_if_array_api_backend('numpy.array_api')
-    def test_kmeans2_rank1_2(self, xp):
+    @pytest.mark.parametrize("junk", [1, 2])
+    def test_kmeans2_rank1_2(self, xp, junk):
         data = xp.asarray(TESTDATA_2D)
         data1 = data[:, 0]
         kmeans2(data1, xp.asarray(2), iter=1)

python dev.py test -j 32 -b all -- -k "test_kmeans2_rank1_2"

I get: no tests ran in 18.76s, which doesn't seem quite right--shouldn't we still run for torch here? This is particularly complex, because it looks like class TestKMean also has a class-level GPU skip. It also seems to me like we should report the skip(s) with reasons(s) rather than "no tests ran."

If I delete the class-level skip for TestKMean in this branch and main, the behavior in this branch is indeed superior than a cherry-pick of the same diff onto main, where the original problem of breaking parametrize returns:

TypeError: TestKMean.test_kmeans2_rank1_2() missing 1 required positional argument: 'junk'

So, this is a bit long-winded, but I think it may be important to consider three things here:

1. This is probably a solid improvement (I'm slightly biased, obviously)
2. It isn't the full story yet, and we should probably open an issue for the behavior of class level + function level custom decorators we add, though I'd also be inclined to avoid the class-level ones as much as possible...
3. This is getting pretty complex to review and test locally already--I hate to say it, but I wonder if we're going to need tests for our new test infrastructure for sanity reasons (this could maybe be discussed in the issue). Would still be messy because devices not available on CI. Obviously, would be nice if this infra could be maintained externally/community level, but that's perhaps farther out...

[lucascolley]

I get: no tests ran in 18.76s, which doesn't seem quite right--shouldn't we still run for torch here?

I suspect that this is down to my use of if torch.cuda.is_available():. I hoped that this would not skip pytorch-cpu but it seems it is. I don't know enough about torch to say how to skip only on GPU then...

[lucascolley]

Hmm maybe not, I still get no tests ran with -b numpy rather than -b all. I'm still not confident in if torch.cuda.is_available(): though.

[tylerjereddy]

I think we can probably agree this is getting confusing though.

[lucascolley]

The decorator of this branch in combination with the class-level skip is completely broken: python dev.py test -b all -t scipy/cluster/tests/test_vq.py::TestKMean gives

========================================================================================== no tests ran in 0.04s ===========================================================================================
ERROR: not found: /home/lucas/dev/myscipy/build-install/lib/python3.10/site-packages/scipy/cluster/tests/test_vq.py::TestKMean
(no name '/home/lucas/dev/myscipy/build-install/lib/python3.10/site-packages/scipy/cluster/tests/test_vq.py::TestKMean' in any of [<Module test_vq.py>])

[tylerjereddy]

Yeah, I was a getting a bit worried that we'll end up going in circles because we'll fix N behaviors and cause just 1 other thing to break instead, so that's why the "testing the tests" idea is suggested, but ugh.

[lucascolley]

for what it's worth, class-level skips with skip_if_array_api_backend are currently completely broken on main too, giving the same error as above. It seems that the decorators (other than the faulty GPU skip on main) are written to only wrap functions, not classes. Can we modify these decorators to wrap classes too, or would a separate decorator be needed?

[tylerjereddy]

python dev.py test -j 32 -b all -s cluster does seem a bit more sensible here vs. main with 50 tests skipped on this branch vs. 389 on main.

[lucascolley]

Strangely, while I get 389 skips on main, I only get 32 skips here (with 110 passes). Could another dev run this so we can see which number is the odd one out?

Edit: the difference may be that I am not running PyTorch GPU.

[tylerjereddy]

I'll leave this open for a bit if that's "ok," just want perhaps one other core dev to read my comment about the complexities above to make sure I'm not misunderstanding, etc, and so folks are on the same page.

[lucascolley]

I think it would be sensible for now to remove the class-level skips from cluster and open an issue as you have suggested.

More testing is needed of PyTorch CPU/GPU to check whether this is doing what we want it to, but if we can get that working and remove class-level skips then I think that this will be looking like a decent improvement.

[lucascolley]

Just pushed a commit where the torch device check should be correct, i.e. tests are not skipped on PyTorch CPU when CUDA is available but not used.

[lucascolley]

this function is moved above the hypothesis stuff from #18927 so that the array API stuff is not split on either side.

[lucascolley]

Noting that this PR is related to #18668 (comment) from @rgommers, which is concerned with the fact that @skip_if_array_api skips for numpy. It seems desirable to reach a point where all tests pass for numpy when the flag is set.

Perhaps we want to have a decorator which skips every backend apart from numpy.

Some notes from @tupui about @skip_if_array_api:

I had 2 cases in mind which for now are mixed together:

1. Skip because we know it does not work
2. Skip because we are ok not running a test and save some CI time

I would be fine separating the 2 use cases or adding some comments to make it more clear that we want to do one or the other

It may be an improvement for @skip_if_array_api_backend to accept multiple backends, or be merged with @skip_if_array_api into one decorator which has all of the needed functionality, perhaps including the option to only run for numpy too.

These changes could be considered in this PR, but it might be better to leave them for a follow-up.

[rgommers]

Skip because we are ok not running a test and save some CI time

I've already found this to be a hindrance, it makes it harder to make changes and then run all tests with a single test command. Imho this isn't worth doing for the small-ish gains in test runtime, there are better ways of speeding up the test suite. In addition it's extra boilerplate; so I'd still prefer to simply remove this decorator.

These changes could be considered in this PR, but it might be better to leave them for a follow-up.

Agreed to do this in a separate PR.

[rgommers]

@tylerjereddy want to hit the green button if you are happy with this?

[tylerjereddy]

Yeah, I think it is a step in the right direction.

[tupui]

Looks like a good improvement to me 👍 thanks @lucascolley (will let Tyler merge)