Skip to content
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

BUG/TST: fix test_array_api for cupy #19194

Merged
merged 1 commit into from Sep 8, 2023

Conversation

lucascolley
Copy link
Member

@lucascolley lucascolley commented Sep 6, 2023

Reference issue

N/A

What does this implement/fix?

On main, python dev.py test -t scipy/_lib/tests/test_array_api.py -b cupy gives the following two errors:

================================================================================================= FAILURES =================================================================================================
____________________________________________________________________________________________ test_asarray[cupy] ____________________________________________________________________________________________
scipy/_lib/tests/test_array_api.py:45: in test_asarray
    x, y = as_xparray([0, 1, 2], xp=xp), as_xparray(np.arange(3), xp=xp)
        xp         = <module 'cupy' from '/home/lucas/mambaforge/envs/scipy-dev/lib/python3.11/site-packages/cupy/__init__.py'>
scipy/_lib/_array_api.py:124: in as_xparray
    array = xp.asarray(array, dtype=dtype, copy=copy)
E   TypeError: asarray() got an unexpected keyword argument 'copy'
        array      = [0, 1, 2]
        check_finite = False
        copy       = None
        dtype      = None
        order      = None
        xp         = <module 'cupy' from '/home/lucas/mambaforge/envs/scipy-dev/lib/python3.11/site-packages/cupy/__init__.py'>
_____________________________________________________________________________________________ test_copy[cupy] ______________________________________________________________________________________________
scipy/_lib/tests/test_array_api.py:80: in test_copy
    y = copy(x, xp=_xp)
        _xp        = <module 'cupy' from '/home/lucas/mambaforge/envs/scipy-dev/lib/python3.11/site-packages/cupy/__init__.py'>
        x          = array([1, 2, 3])
        xp         = <module 'cupy' from '/home/lucas/mambaforge/envs/scipy-dev/lib/python3.11/site-packages/cupy/__init__.py'>
scipy/_lib/_array_api.py:165: in copy
    return as_xparray(x, copy=True, xp=xp)
        x          = array([1, 2, 3])
        xp         = <module 'cupy' from '/home/lucas/mambaforge/envs/scipy-dev/lib/python3.11/site-packages/cupy/__init__.py'>
scipy/_lib/_array_api.py:124: in as_xparray
    array = xp.asarray(array, dtype=dtype, copy=copy)
E   TypeError: asarray() got an unexpected keyword argument 'copy'
        array      = array([1, 2, 3])
        check_finite = False
        copy       = True
        dtype      = None
        order      = None
        xp         = <module 'cupy' from '/home/lucas/mambaforge/envs/scipy-dev/lib/python3.11/site-packages/cupy/__init__.py'>
========================================================================================= short test summary info ==========================================================================================
FAILED scipy/_lib/tests/test_array_api.py::test_asarray[cupy] - TypeError: asarray() got an unexpected keyword argument 'copy'
FAILED scipy/_lib/tests/test_array_api.py::test_copy[cupy] - TypeError: asarray() got an unexpected keyword argument 'copy'
======================================================================================= 2 failed, 3 passed in 1.14s ========================================================================================

This is because as_xparray is trying to use the copy parameter of xp.asarray where xp is the 'raw' cupy, which doesn't exist. We should be using the array_api_compat version of cupy instead.

The pattern here, which matches #19051, is that we should only pass namespaces to the xp keyword that are compatible with the standard i.e. after using array_namespace.

For copy, this is relatively obvious: the xp keyword exists for performance reasons, to avoid calling array_namespace twice. The problem with the current test is that it calls array_namespace zero times.

For as_xparray, this is less obvious: it may be that we want it to be able to accept 'raw' namespaces, and fetch the compat version itself. @tupui feel free to change that if wanted, but this PR fixes the issue for now.


On this branch, python dev.py test -t scipy/_lib/tests/test_array_api.py -b cupy now passes all tests.


Following #19157, this PR also removes the now unnecessary to_numpy (and the test for it), in favour of testing with our new assert_equal rather than np.testing.assert_equal.

Additional information

cc @andyfaff

@mdhaber mdhaber added scipy._lib maintenance Items related to regular maintenance tasks array types Items related to array API support and input array validation (see gh-18286) labels Sep 6, 2023
Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thanks @lucascolley. Did you consider moving this logic to as_xparray? I.e, i. Case we provide the namespace, map it to the one that we want instead.

@lucascolley
Copy link
Member Author

Did you consider moving this logic to as_xparray? I.e, i. Case we provide the namespace, map it to the one that we want instead.

We could add that, but is it necessary? I think that we should always be calling array_namespace first, but if you have a concrete example then sounds okay.

@tupui
Copy link
Member

tupui commented Sep 6, 2023

To be clear, that would not necessarily change the way we use the API.

To me it just seems that the current parameter xp is not doing what we really want to if we have to massage it a bit. It's going to be error prone as people might forget to do get the namespace before. The alternative would be to make this parameter non optional. Then I think we have a better story.

@lucascolley
Copy link
Member Author

Okay, letting as_xparray take the 'raw' namespace sounds fine. How would you implement this?

Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

In case Pamphile wants an example shim on as_xparray to criticize, this one seems to have an equivalent effect to the patch in this PR:

--- a/scipy/_lib/_array_api.py
+++ b/scipy/_lib/_array_api.py
@@ -122,7 +122,11 @@ def as_xparray(
         # container that is consistent with the input's namespace.
         array = xp.asarray(array)
     else:
-        array = xp.asarray(array, dtype=dtype, copy=copy)
+        try:
+            array = xp.asarray(array, dtype=dtype, copy=copy)
+        except TypeError:
+            coerced_xp = array_namespace(xp.asarray(3))
+            array = coerced_xp.asarray(array, dtype=dtype, copy=copy)
 
     if check_finite:
         _check_finite(array, xp)
diff --git a/scipy/_lib/tests/test_array_api.py b/scipy/_lib/tests/test_array_api.py
index 1bc393d82..6b2ac1fd4 100644
--- a/scipy/_lib/tests/test_array_api.py
+++ b/scipy/_lib/tests/test_array_api.py
@@ -1,10 +1,9 @@
 import numpy as np
-from numpy.testing import assert_equal
 import pytest
 
 from scipy.conftest import array_api_compatible
 from scipy._lib._array_api import (
-    _GLOBAL_CONFIG, array_namespace, as_xparray, copy
+    _GLOBAL_CONFIG, array_namespace, as_xparray, copy, assert_equal
 )
 
 
@@ -43,7 +42,7 @@ def test_array_namespace():
 @array_api_compatible
 def test_asarray(xp):
     x, y = as_xparray([0, 1, 2], xp=xp), as_xparray(np.arange(3), xp=xp)
-    ref = np.array([0, 1, 2])
+    ref = xp.asarray([0, 1, 2])
     assert_equal(x, ref)
     assert_equal(y, ref)

tylerjereddy added a commit to tylerjereddy/scipy that referenced this pull request Sep 7, 2023
* Fixes scipy#19190

* the module-level skipping described in the above issue
was causing spurious skips in some cases as described there,
so move the tests in that module to a class guarded with
a `skipif` mark, which seems to be the preferred approach
anyway according to:
https://docs.pytest.org/en/7.1.x/reference/reference.html?highlight=pytest%20skip#pytest.skip

* on this branch I seem to get the expected behavior with
i.e.,
- `python dev.py test -v -- -k "test_concatenate_int32_overflow" -rsx`
  - `59382 deselected`
- `python dev.py test -m full -v -- -k "test_concatenate_int32_overflow" -rsx`
  - `1 passed, 59381 deselected`
- `python dev.py test -v -t scipy/sparse/tests/test_construct.py::TestConstructUtils::test_concatenate_int32_overflow -- -rsx`
  - `1 deselected`
- `SCIPY_ARRAY_API=1 python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -b cupy -- -rsx`
  - just the two failures expected from scipygh-19194
- `python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -b cupy -- -rsx`
  - same (the backend setting implies the env var)
- `python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -- -rsx`
  - `6 skipped`
- `python dev.py test -j 32 -b all`
  - `3 failed, 44291 passed, 2546 skipped, 149 xfailed, 12 xpassed`
    (failures noted above + PyTorch issue from scipygh-18930)
- `SCIPY_DEVICE=cuda python dev.py test -j 32 -b all`
  - `3 failed, 44198 passed, 2639 skipped, 149 xfailed, 12 xpassed`
    (failures noted above + PyTorch issue from scipygh-18930)

* could there be a `pytest` bug here somewhere? Yeah, probably. But
  given that following their docs suggestion to avoid the global skip
  scope seems to solve the problem, I'm inclined not to spend much time
  providing a small reproducer upstream and debating with them. It is
  probably related to the `-k` option collecting every single test node
  vs. the hand-tuned selection of the single node on the command line.
tylerjereddy added a commit to tylerjereddy/scipy that referenced this pull request Sep 7, 2023
* Fixes scipy#19190

* the module-level skipping described in the above issue
was causing spurious skips in some cases as described there,
so move the tests in that module to a class guarded with
a `skipif` mark, which seems to be the preferred approach
anyway according to:
https://docs.pytest.org/en/7.1.x/reference/reference.html?highlight=pytest%20skip#pytest.skip

* on this branch I seem to get the expected behavior with
i.e.,
- `python dev.py test -v -- -k "test_concatenate_int32_overflow" -rsx`
  - `59382 deselected`
- `python dev.py test -m full -v -- -k "test_concatenate_int32_overflow" -rsx`
  - `1 passed, 59381 deselected`
- `python dev.py test -v -t scipy/sparse/tests/test_construct.py::TestConstructUtils::test_concatenate_int32_overflow -- -rsx`
  - `1 deselected`
- `SCIPY_ARRAY_API=1 python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -b cupy -- -rsx`
  - just the two failures expected from scipygh-19194
- `python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -b cupy -- -rsx`
  - same (the backend setting implies the env var)
- `python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -- -rsx`
  - `6 skipped`
- `python dev.py test -j 32 -b all`
  - `3 failed, 44291 passed, 2546 skipped, 149 xfailed, 12 xpassed`
    (failures noted above + PyTorch issue from scipygh-18930)
- `SCIPY_DEVICE=cuda python dev.py test -j 32 -b all`
  - `3 failed, 44198 passed, 2639 skipped, 149 xfailed, 12 xpassed`
    (failures noted above + PyTorch issue from scipygh-18930)

* could there be a `pytest` bug here somewhere? Yeah, probably. But
  given that following their docs suggestion to avoid the global skip
  scope seems to solve the problem, I'm inclined not to spend much time
  providing a small reproducer upstream and debating with them. It is
  probably related to the `-k` option collecting every single test node
  vs. the hand-tuned selection of the single node on the command line.

[skip cirrus]
@lucascolley
Copy link
Member Author

I've resolved the conflicts following gh-19206.

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

I kind of like what you propose @tylerjereddy. What do you think @lucascolley?

Otherwise this LGTM and we could leave this as a follow up. So approving.

@lucascolley
Copy link
Member Author

Let's go with Tyler's method 👍 I'll modify today

@rgommers rgommers added this to the 1.12.0 milestone Sep 8, 2023
[skip cirrus] [skip circle]

Co-authored-by: Tyler Reddy <tyler.je.reddy@gmail.com>
@lucascolley
Copy link
Member Author

@tupui @tylerjereddy all good?

lucascolley pushed a commit to lucascolley/scipy that referenced this pull request Sep 8, 2023
* Fixes scipy#19190

* the module-level skipping described in the above issue
was causing spurious skips in some cases as described there,
so move the tests in that module to a class guarded with
a `skipif` mark, which seems to be the preferred approach
anyway according to:
https://docs.pytest.org/en/7.1.x/reference/reference.html?highlight=pytest%20skip#pytest.skip

* on this branch I seem to get the expected behavior with
i.e.,
- `python dev.py test -v -- -k "test_concatenate_int32_overflow" -rsx`
  - `59382 deselected`
- `python dev.py test -m full -v -- -k "test_concatenate_int32_overflow" -rsx`
  - `1 passed, 59381 deselected`
- `python dev.py test -v -t scipy/sparse/tests/test_construct.py::TestConstructUtils::test_concatenate_int32_overflow -- -rsx`
  - `1 deselected`
- `SCIPY_ARRAY_API=1 python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -b cupy -- -rsx`
  - just the two failures expected from scipygh-19194
- `python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -b cupy -- -rsx`
  - same (the backend setting implies the env var)
- `python dev.py test -v -t "scipy/_lib/tests/test_array_api.py" -- -rsx`
  - `6 skipped`
- `python dev.py test -j 32 -b all`
  - `3 failed, 44291 passed, 2546 skipped, 149 xfailed, 12 xpassed`
    (failures noted above + PyTorch issue from scipygh-18930)
- `SCIPY_DEVICE=cuda python dev.py test -j 32 -b all`
  - `3 failed, 44198 passed, 2639 skipped, 149 xfailed, 12 xpassed`
    (failures noted above + PyTorch issue from scipygh-18930)

* could there be a `pytest` bug here somewhere? Yeah, probably. But
  given that following their docs suggestion to avoid the global skip
  scope seems to solve the problem, I'm inclined not to spend much time
  providing a small reproducer upstream and debating with them. It is
  probably related to the `-k` option collecting every single test node
  vs. the hand-tuned selection of the single node on the command line.

[skip cirrus]
@tylerjereddy tylerjereddy merged commit 1fce6d5 into scipy:main Sep 8, 2023
22 checks passed
@tylerjereddy
Copy link
Contributor

I did a final confirmation of the testing status improvement locally, then merged, thanks.

@lucascolley lucascolley deleted the xp-test-fix branch September 8, 2023 16:22
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 scipy._lib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants