Skip to content

Fix tests for numpy 2 and array api compat #29436

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

Merged

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Jul 8, 2024

Fixes #29396.

Update the array API tests and the _average private helper for compatibility with NumPy 2.0.0.

Copy link

github-actions bot commented Jul 8, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 0ee1a21. Link to the linter CI: here

@ogrisel
Copy link
Member Author

ogrisel commented Jul 8, 2024

I triggered a CUDA CI run here:

EDIT: green!

The tests pass locally with numpy 2.0.0 and array-api-strict 2.0.1.

@ogrisel ogrisel marked this pull request as ready for review July 8, 2024 16:22
@ogrisel
Copy link
Member Author

ogrisel commented Jul 8, 2024

I marged "No changelog needed" but this is debatable as our _average implementation now mimics NumPy 2's np.average when raising exception on invalid inputs rather than what was done for NumPy 1.

@ogrisel ogrisel changed the title Fix test for numpy 2 and array api compat Fix tests for numpy 2 and array api compat Jul 8, 2024
@lesteve
Copy link
Member

lesteve commented Jul 9, 2024

The tests pass locally with numpy 2.0.0 and array-api-strict 2.0.1.

Yep, I confirm this as well.

I marged "No changelog needed" but this is debatable as our _average implementation now mimics NumPy 2's np.average when raising exception on invalid inputs rather than what was done for NumPy 1.

I guess I would add a changelog because the exception type does change when weights is 1d. So this may affect a (very) small fraction of downstream users. On a similar note I have been wondering on when is a changelog needed and asked in our Discord to get people feelings about this: https://discord.com/channels/731163543038197871/1260125820379463720/1260134931783225347.

@ogrisel
Copy link
Member Author

ogrisel commented Jul 9, 2024

I guess I would add a changelog because the exception type does change when weights is 1d. So this may affect a (very) small fraction of downstream users.

I tried to come up with a changelog entry but found it too awkward to explain: this change is in a private helper that will impact most scoring metrics (but not all) only when array API dispatch is enabled (and only for very weird input shapes). Furthermore the change is similar from a change from numpy 1 to numpy 2, so I think very few people will be impacted (upgrading existing code bases with scikit-learn using array API on code that only works with numpy 1 and has not been updated for numpy 2 yet).

So I would rather not add a verbose and hard to understand changelog entry for this PR. But if you disagree I can still do it.

@lesteve
Copy link
Member

lesteve commented Jul 9, 2024

So I would rather not add verbose and hard to understand changelog entry for this PR. But if you disagree I can still do it.

Seems reasonable not to add a changelog indeed, given the complexities involved.

@betatim
Copy link
Member

betatim commented Jul 9, 2024

Only saw the test failure after approving it. Looks like finfo() now does not accept None as dtype anymore. However, for now it still works and returns information about the default dtype. I think we could use np.float64 instead of None as the type doesn't seem to depend on which namespace is being used.

@ogrisel
Copy link
Member Author

ogrisel commented Jul 9, 2024

I will fix the warnings asap.

…umPy's default floating point precision level
@ogrisel
Copy link
Member Author

ogrisel commented Jul 9, 2024

I triggered a new CUDA CI run here:

EDIT: they are green.

@ogrisel
Copy link
Member Author

ogrisel commented Jul 9, 2024

@lesteve shall we enable auto-merge on this? This would make it saner to help review the many pending array API PRs.

@lesteve
Copy link
Member

lesteve commented Jul 9, 2024

At a quick glance the diff looks fine, two issues:

  • pylatest_conda_forge_mkl is red, looks like an issue with device in the numpy<2 device handling, device is as string 'cpu' rather than a function:
        if np_version < parse_version("2.0.0") or np_version >= parse_version("2.1.0"):
            # NumPy 2.0 has a problem with the device attribute of scalar arrays:
            # https://github.com/numpy/numpy/issues/26850
>           assert device(array_xp) == device(result)
E           TypeError: 'str' object is not callable

array      = array([[ 0,  3,  0],
       [ 2, -1,  0],
       [ 0,  0,  0],
       [ 9,  8,  7],
       [ 4,  0,  5]])
array_namespace = 'torch'
array_xp   = tensor([[ 0,  3,  0],
        [ 2, -1,  0],
        [ 0,  0,  0],
        [ 9,  8,  7],
        [ 4,  0,  5]])
axis       = -2
csr_container = <class 'scipy.sparse._csr.csr_array'>
device     = 'cpu'
  • not a blocker, but something I have noticed is that the pylatest_pip_openblas_pandas build is a lot longer than previously. More than 50 minutes in this PR (see build log) vs 27 minutes in the last commmit on main (see build log). Do you expect array API tests to have such an impact? This seem to have started happening in the first commit introducing array-api tests 57970d6

@ogrisel
Copy link
Member Author

ogrisel commented Jul 9, 2024

I will revert 021e07b to avoid the device problem. EDIT: actually this is a fixable problem, no need to revert.

I suspect that the test time is related to the Python version. Let me re-pin to 3.9 for now with a comment and see of the new CI run confirms this. If this is the case we should investigate separately if we can reproduce. Maybe it's related to coverage tracing performance in recent Python versions?

@ogrisel
Copy link
Member Author

ogrisel commented Jul 9, 2024

We are back to 27 min with Python 3.9...

I think this is good to merge as is but we should definitely investigate the cause of the slowness with Python 3.12.

@lesteve lesteve merged commit a922568 into scikit-learn:main Jul 9, 2024
30 checks passed
@lesteve
Copy link
Member

lesteve commented Jul 9, 2024

Merging, thanks!

The slow CI is weird but it does remind me something (not sure at all this was related but I can try to find it again ...)

@ogrisel ogrisel deleted the fix-test-for-numpy-2-and-array-api-compat branch July 9, 2024 16:12
@ogrisel ogrisel restored the fix-test-for-numpy-2-and-array-api-compat branch July 9, 2024 16:12
@ogrisel ogrisel deleted the fix-test-for-numpy-2-and-array-api-compat branch July 9, 2024 16:12
@ogrisel
Copy link
Member Author

ogrisel commented Jul 9, 2024

Thanks for the reviews.

@lesteve
Copy link
Member

lesteve commented Jul 9, 2024

Found the slow CI with Python 3.12 I had in mind. At the time it was seen when moving the scipy-dev build to Python 3.12: #28383.

The work-around was to move dataset download to another build so this may not be related.

I guess it is easy to test your coverage idea by disabling coverage in the CI build ...
There seems to be some reports going this direction e.g. nedbat/coveragepy#1665. Setting COVERAGE_CORE=sysmon may be a way to speed that up on Python 3.12 which seems to do the trick: nedbat/coveragepy#1665 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array API tests fail on main
3 participants