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

MAINT: 1.12.0rc2 prep #19797

Merged

Conversation

tylerjereddy
Copy link
Contributor

@tylerjereddy tylerjereddy commented Jan 2, 2024

Backports included (so far):

  1. DOC: update release notes with all deprecations for 1.12 release #19735
  2. TST: skip RGI(..., method="pchip" for complex values) #19748
  3. BUG: Make FMM classes py::module_local (fix for 1.12RC) #19751
  4. MAINT: Avoid use of aligned_alloc in pocketfft on windows #19761
  5. MAINT: Cumulative simpson follow-up comments #19709
  6. BUG: Fix nbinom.logcdf for invalid input #19779
  7. BUG: support sparse Hessian in Newton-CG #19785
  8. TST: loosen tolerances for tests that fail otherwise on windows+MKL #19800
  9. TST: fix compatibility with pytest 8 #19806 (merge conflicts resolved manually on the second commit)
  10. REL: bump copyright to 2024 #19830
  11. TST: move reference data for test_real_transforms to a fixture #19842
  12. BLD: improve scipy-openblas dependency check #19859

TODO:

  • depend on a stable release of Pythran that pulls in setuptools per MAINT: need stable Pythran release for SciPy 1.12.0 RC2 #19795
  • confirm that conda-forge release manager is happy; a few test tol bumps may still be needed I think cc @h-vetinari
  • gradually turn the CI back on, and eventually test the wheel builds here again, but for now cirrus and circle are both skipped (full doc build still passing locally and full suite passing on x86_64 Linux locally; and so far backports are small patches)

j-bowhay and others added 5 commits January 2, 2024 10:47
PchipInterpolator is nonlinear in data, hence does not guarantee that
pchip(x, y) == pchip(x, y.real) + 1j*pchip(x, y.imag)

The test was passing by chance.
* MSVC is too non-conformant and broken...
* Also harden detection of cpp standard on MSVC platforms

Co-Authored-By: H. Vetinari <h.vetinari@gmx.com>
* update the SciPy `1.12.0` release notes following
more backports for the RC2 release

[skip cirrus] [skip circle]
@tylerjereddy tylerjereddy added the maintenance Items related to regular maintenance tasks label Jan 2, 2024
@tylerjereddy tylerjereddy added this to the 1.12.0 milestone Jan 2, 2024
@github-actions github-actions bot added C/C++ Items related to the internal C/C++ code base scipy.fft scipy.interpolate scipy.io labels Jan 2, 2024
@lucascolley lucascolley removed scipy.io scipy.interpolate scipy.fft C/C++ Items related to the internal C/C++ code base labels Jan 2, 2024
@h-vetinari
Copy link
Member

  • a few test tol bumps may still be needed I think cc @h-vetinari

Opened a PR: #19800
(we can manage even if it doesn't get backported, but would be nice to have)

@h-vetinari
Copy link
Member

If we want compatibility with pytest 8.0 (the just released rc1 shows up in the prerelease job), we also need the first two commits from #19806 (I want to avoid squash-merging it for reasons explained in the PR).

@dschmitz89
Copy link
Contributor

One more backport candidate as it fixes a bug in my opinion: #19779

@tylerjereddy
Copy link
Contributor Author

Ok, I'll be sure to carefully make the requested additional backports. If you think it should be backported and doesn't have the label yet, it is also useful to add it. I can always remove it if there's a gruesome merge conflict or something like that.

@lucascolley
Copy link
Member

gh-19830 updated the copyright to 2024

@tylerjereddy
Copy link
Contributor Author

with the new stable release of Pythran out, I'll try to do RC2 tomorrow (on PTO today) if there are no hiccups

nprav and others added 9 commits January 9, 2024 11:28
* MAINT: integrate.cumulative_simpson: various improvements
* BUG: stats.nbinom.logcdf: fix IndexError
For example, in the deprecation of positional use of `tol=` in sparse solvers.
By and large, these should all be filtered out in the implementation,
not in the tests, see scipy#19805.
* now that Pythran stable release is out, require `0.15.0`
in `pyproject.toml`/correct the version bounds (we need this
newer version to pull in `setuptools` with Python `3.12`)
* update the SciPy `1.12.0` release notes following additional
backports for RC2

[skip cirrus] [skip circle]
@lucascolley
Copy link
Member

lucascolley commented Jan 9, 2024

damn, the bot is playing up again. I don't have time to look into it in the next few days unfortunately. It may be because this PR is to the maintenance branch.

EDIT: yeah, I think it's just because the changes haven't been backported to this branch. Not sure whether it's worth doing that.

@tylerjereddy
Copy link
Contributor Author

I'm not sure what to make of the Python 3.11 Windows sdist job failure in the absence of Pythran. I see a few things in the log like ..\scipy\meson.build:482: WARNING: The variable(s) 'PYTHRAN_INCDIR', 'PYTHRAN_VERSION' in the input file 'scipy\__config__.py.in' are not present in the given configuration data. and PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: '.\\.mesonpy-feqwgwl_\\meson-private\\cmake_scipy-openblas\\CMakeFiles\\CMakeScratch.

That failure persisted through a restart of the job, and scipy-openblas isn't actually being used according to the config stage (assuming that path refers to the package on PyPI..).

@h-vetinari
Copy link
Member

I'm not sure what to make of the Python 3.11 Windows sdist job failure in the absence of Pythran.

This looks pretty unrelated to pythran, or perhaps it's triggering a bug somewhere else (e.g. somehow, a race or duplication for creating scratch space for scipy-openblas).

+ meson dist --allow-dirty --no-tests --formats gztar
WARNING: Repository has uncommitted changes that will not be included in the dist tarball
Created D:\a\scipy\scipy\.mesonpy-sgr84eaw\meson-dist\SciPy-1.12.0rc2.tar.gz
Traceback (most recent call last):
  File "C:\hostedtoolcache\windows\Python\3.11.7\x64\Lib\shutil.py", line 624, in _rmtree_unsafe
    os.rmdir(path)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: '.\\.mesonpy-sgr84eaw\\meson-private\\cmake_scipy-openblas\\CMakeFiles\\CMakeScratch'

@tylerjereddy
Copy link
Contributor Author

It is showing up in PRs against main now too, so I think it is "ok" for me to proceed with testing wheel builds and considering this non-specific to the release branch: gh-19852

* an empty commit to test wheel builds prior
to `1.12.0` RC2 release process

[wheel build]
@tylerjereddy
Copy link
Contributor Author

@mdhaber do you recognize the MacOS ARM wheel test failures for TestRankData related to np.nan handling? I tried locally with NumPy 1.22.4 during build phase, and latest stable NumPy during test phase, with the same version of Python on Mac ARM, and didn't immediately reproduce.

@tylerjereddy
Copy link
Contributor Author

Studying the MacOS ARM64 Python 3.9 failure, in particular for test_nan_policy_omit_3d, rankdata appears to be returning an integer type array instead of float64, where there are zeros instead of np.nan at various locations. We do deviate from main on rankdata, but only via an enhancement in gh-19776, not via a bugfix. It is also a bit hard to imagine why this would start failing in RC2 vs. RC1.

I did do some side-by-side log diffing against a previous success in Cirrus, and nothing immediately stood out to me re: dependency versions that were suspiciously different. Matching the versions of the deps locally on an ARM Mac still isn't reproducing for me either.

@tylerjereddy
Copy link
Contributor Author

Maybe I'll give https://github.com/cirruslabs/cirrus-cli a go in the morning, to see if I can get some traction locally.

@rgommers
Copy link
Member

rgommers commented Jan 12, 2024

in particular for test_nan_policy_omit_3d, rankdata appears to be returning an integer type array instead of float64, where there are zeros instead of np.nan at various locations

I don't see the cause of the problem here yet (and can't reproduce with cython 3.0.8 and numpy 1.22.4 on macOS arm64), but the rankdata behavior doesn't look good. The dtype of the returned array should not be depending on both the value of the method keyword and the values in the array (it's always float64 if nans are present, and it may be integer if no nans).

In main the way the function has been written has been improved, but in 1.12.x there's additionally a not so great recursive implementation using np.apply_along_axis that interplays with the lack of type stability. The bug comes down to this:

>>> import numpy as np
>>> def func(x):
...     if np.isnan(x).any():
...         return x.astype(np.float64)
...     else:
...         return x.astype(np.int64)
...
>>> np.apply_along_axis(func, 0, np.array([[0, np.nan], [1, 2]])).dtype
dtype('int64')
>>> np.apply_along_axis(func, 0, np.array([[np.nan, 0], [1, 2]])).dtype
dtype('float64')

Final problem is probably that the random numbers used in the test aren't stable (hash('falafel') is not a fixed seed!), so the above issue occurring is going to depend on details of how hash is implemented for the given CPython version and platform.

Since the implementation already has been improved in main, I recommend to change the seed to a fixed value that makes the test pass in 1.12.x in this PR - if it has a nan in position [0, 0] things should work.

@rgommers
Copy link
Member

That seeding problem is more widespread. I'll turn this into a new issue for rankdata and will fix up the problems in main.

mgorny and others added 4 commits January 12, 2024 11:23
Move the code for getting the reference data in test_real_transforms
to a fixture.  This easily ensures that the data is loaded only once
(the previous caching logic did not work correctly), and it makes it
possible to easily close the data files afterwards.

This also fixes test flakiness on PyPy.  Unlike CPython, PyPy does not
aggressive GC open files.  Therefore, when every test iteration opened
the files again, the number of open files quickly grew to the system
limit and caused further `open()` calls to fail.

Fixes scipy#19553
This avoids looking for scipy-openblas with CMake (which we never want),
and avoids looking for it twice (that was an oversight). As a result,
we should be robust to whatever is the underlying problem of
the CI failures reported in scipygh-19852 are.

Closes scipygh-19852

[skip cirrus] [skip circle]
* pin the random seed in `test_nan_policy_omit_3d`; see
detailed discussion at:
scipy#19797 (comment)
* update the SciPy `1.12.0` release notes following
more backports/fixes

[wheel build]
@tylerjereddy
Copy link
Contributor Author

Two more backports were added in (updated the original list above), a random seed fix for test_nan_policy_omit_3d was added thanks to Ralf's analysis above, and the release notes were updated again.

After checking that full test suite and doc build pass locally, I've proceeded straight to another test of the full wheel builds, to see where we stand now.

@tylerjereddy
Copy link
Contributor Author

CI is fully green, including regular + wheel builds, in it goes.

@tylerjereddy tylerjereddy merged commit baa283b into scipy:maintenance/1.12.x Jan 12, 2024
44 checks passed
@tylerjereddy tylerjereddy deleted the treddy_1_12_0rc2_prep branch January 12, 2024 20:09
@tylerjereddy tylerjereddy changed the title WIP, MAINT: 1.12.0rc2 prep MAINT: 1.12.0rc2 prep Jan 12, 2024
@h-vetinari
Copy link
Member

FWIW, testing of rc2 in conda-forge is looking good!

rgommers added a commit to rgommers/scipy that referenced this pull request Jan 16, 2024
The use of `hash('a_descriptive_string')` is cute, but unfortunately
hashes are not reproducible at all. This resulted in a hard to debug
failure in scipygh-19797. So remove all usages of `hash()`, and also
use `np.random.RandomState` for good measure since that has more
stability guarantees than `np.random.default_rng`.
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.

None yet