Skip to content

Conversation

crusaderky
Copy link
Contributor

@crusaderky crusaderky commented Dec 19, 2024

Fix bug where

skip_xp_backends(cpu_only=True, reason=...)
skip_xp_backends(np_only=True, reason=...)
xfail_xp_backends(cpu_only=True, reason=...)
xfail_xp_backends(np_only=True, reason=...)

would disregard the user-provided reason and instead always used the default.

Also, when a test is decorated by

@skip_xp_backends(cpu_only=True)
@skip_xp_backends("torch")

the reason for the specific backend will now trump the one given for cpu_only (see evidence in comment below).

@github-actions github-actions bot added the maintenance Items related to regular maintenance tasks label Dec 19, 2024
@crusaderky crusaderky changed the title TST: skip|xfail_xp_backends suppresses reason= TST: skip|xfail_xp_backends disregards reason= Dec 19, 2024
Comment on lines -335 to -336
raise ValueError("please provide a singleton `reason` "
"when using `np_only`")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redundant with pytest's own error

@j-bowhay j-bowhay requested a review from lucascolley December 19, 2024 13:09
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

LGTM

@lucascolley
Copy link
Member

lucascolley commented Dec 19, 2024

what happens if we perform a cpu_only skip and a skip with torch in backends - which reason takes precedence when using torch CUDA? If so, is that what we want?

@crusaderky
Copy link
Contributor Author

what happens if we perform a cpu_only skip and a skip with torch in backends - which reason takes precedence when using torch CUDA? If so, is that what we want?

The reason from cpu_only took precedence, and it's most likely not what we want. I've updated the PR:

@skip_xp_backends(cpu_only=True, reason="cpu reason")
@skip_xp_backends("torch", reason="torch reason")
def test1(xp):
    pass
$ python dev.py test -v
scipy/signal/tests/test_signaltools.py::test1[numpy] PASSED                                                                                          [ 20%]
scipy/signal/tests/test_signaltools.py::test1[array_api_strict] PASSED                                                                               [ 40%]
scipy/signal/tests/test_signaltools.py::test1[torch] SKIPPED (torch reason)                                                                          [ 60%]
scipy/signal/tests/test_signaltools.py::test1[cupy] SKIPPED (cpu reason)                                                                             [ 80%]
scipy/signal/tests/test_signaltools.py::test1[jax.numpy] PASSED           

$ SCIPY_DEVICE=cuda python dev.py test -v
scipy/signal/tests/test_signaltools.py::test1[numpy] PASSED                                                                                          [ 20%]
scipy/signal/tests/test_signaltools.py::test1[array_api_strict] PASSED                                                                               [ 40%]
scipy/signal/tests/test_signaltools.py::test1[torch] SKIPPED (torch reason)                                                                          [ 60%]
scipy/signal/tests/test_signaltools.py::test1[cupy] SKIPPED (cpu reason)                                                                             [ 80%]
scipy/signal/tests/test_signaltools.py::test1[jax.numpy] SKIPPED (cpu reason) 

Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

can you update the docstring now that a custom reason is allowed with cpu_only

@lucascolley lucascolley added the array types Items related to array API support and input array validation (see gh-18286) label Dec 19, 2024
@crusaderky
Copy link
Contributor Author

can you update the docstring now that a custom reason is allowed with cpu_only

done

Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

thanks @crusaderky !

@lucascolley lucascolley changed the title TST: skip|xfail_xp_backends disregards reason= TST: skip|xfail_xp_backends disregards reason= Dec 19, 2024
@lucascolley lucascolley added this to the 1.16.0 milestone Dec 19, 2024
@lucascolley lucascolley merged commit 03cdb80 into scipy:main Dec 19, 2024
31 of 33 checks passed
@crusaderky crusaderky deleted the skip_xp_reason branch December 19, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants