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

REL scikit-learn 1.4.1 #28414

Merged
merged 122 commits into from
Feb 13, 2024
Merged

REL scikit-learn 1.4.1 #28414

merged 122 commits into from
Feb 13, 2024

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Feb 13, 2024

This is the branch preparing the 1.4.1 release.

For this release, we need to

TODO list:

@glemaitre glemaitre marked this pull request as draft February 13, 2024 14:51
Copy link

github-actions bot commented Feb 13, 2024

✔️ Linting Passed

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

Generated for commit: 24fefa5. Link to the linter CI: here

StefanieSenger and others added 27 commits February 13, 2024 16:18
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: ArturoAmorQ <arturo.amor-quiroz@polytechnique.edu>
…core (scikit-learn#28156)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
…ikit-learn#28062)

Co-authored-by: Alejandro Martin <alejandro.martingil@tno.nl>
…-learn#28196)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
ogrisel and others added 5 commits February 13, 2024 16:24
Co-authored-by: Tim Head <betatim@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
scikit-learn#28390)

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
…mpurity` (scikit-learn#28327)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
…cikit-learn#28167)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
@glemaitre glemaitre marked this pull request as ready for review February 13, 2024 17:18
@glemaitre glemaitre merged commit 0d00f7b into scikit-learn:1.4.X Feb 13, 2024
46 of 50 checks passed
@thomasjpfan
Copy link
Member

remove the NumPy < 2 pinning that is inside the setup.py

Is it safe to remove the pin now? I thought we had to wait for NumPy 2.0RC before we can remove it.

@glemaitre
Copy link
Member Author

Is it safe to remove the pin now? I thought we had to wait for NumPy 2.0RC before we can remove it.

@lesteve and @ogrisel realized the current pin is also problematic. I don't exactly recall but it was an issue with having an old scikit-learn while updating.

@adrinjalali
Copy link
Member

the issue is that if we pin, the user will end up with an older version of scikit-learn when a numpy=2 is installed, since we haven't been pinning in the old versions.

@glemaitre
Copy link
Member Author

Right now, one would get:

A module that was compiled using NumPy 1.x cannot be run in
NumPy 2.0.0.dev0 as it may crash. To support both 1.x and 2.x
versions of NumPy, modules must be compiled against NumPy 2.0.

If you are a user of the module, the easiest solution will be to
either downgrade NumPy or update the failing module (if available).

NOTE: When testing against pre-release versions of NumPy 2.0
or building nightly wheels for it, it is necessary to ensure
the NumPy pre-release is used at build time.
The main way to ensure this is using no build isolation
and installing dependencies manually with NumPy.
For cibuildwheel for example, this may be achieved by using
the flag to pip:
    CIBW_BUILD_FRONTEND: pip; args: --no-build-isolation
installing NumPy with:
    pip install --pre --extra-index-url https://pypi.anaconda.org/scientific-python-nightly-wheels/simple
in the `CIBW_BEFORE_BUILD` step.  Please compare with the
solutions e.g. in astropy or matplotlib for how to make this
conditional for nightly wheel builds using expressions.
If you do not worry about using pre-releases of all
dependencies, you can also use `--pre --extra-index-url` in the
build frontend (instead of build isolation).
This will become unnecessary as soon as NumPy 2.0 is released.

If your dependencies have the issue, check whether they
have nightly wheels build against NumPy 2.0.

Ideally, we should release 1.4.2 that build against the NumPy 2.0.0.rc1 that allow to be compatible with the ABI. With the current status, when the NumPy RC is released, we are going to break all CIs that rely on the RC (pip install --pre) and install scikit-learn.

So we have 2 imperfect situation but I think this is more unlikely to have someone trying pip install numpy==2 scikit-learn than someone doing pip install --pre numpy scikit-learn.

If this is fine, I'll do a new tag 1.4.1-1 (I did not push to PyPI and conda-forge) and pin numpy.

@adrinjalali
Copy link
Member

Is there a way to have two sets of binaries on pypi, one against numpy=1 and one against numpy=2, and let pip decide which one to pull?

Also,

Right now, one would get:

Where do you get this? We shouldn't have any numpy=2 in our wheel / conda builds, do we?

@glemaitre
Copy link
Member Author

Where do you get this? We shouldn't have any numpy=2 in our wheel / conda builds, do we?

This is when installing NumPy dev so this would be the situation when numpy get released.

@thomasjpfan
Copy link
Member

Can we pin on the release branch 1.4.X but not pin on main?

@glemaitre
Copy link
Member Author

Can we pin on the release branch 1.4.X but not pin on main?

It is already what we had indeed.

@thomasjpfan
Copy link
Member

thomasjpfan commented Feb 14, 2024

Looking at numpy/numpy#24300 (comment) , it recommends always setting an upper bound for the release version.

we have 2 imperfect situation but I think this is more unlikely to have someone trying pip install numpy==2 scikit-learn

I see people doing:

# assume NumPy 2.0 is available at this time
pip install numpy

# Installs older version
pip install scikit-learn 

I do not see a great solution here.

@ogrisel
Copy link
Member

ogrisel commented Feb 14, 2024

Actually, based on experiments, it seems that if we configure scikit-learn 1.4.1 to depend on "numpy<2", then the second command (pip install scikit-learn) will downgrade numpy to the latest version that is compatible.

The only problem is:

pip install scikit-learn numpy==2.0.0

once numpy 2.0.0 is released, then pip will install the last scikit-learn version without an upper bound pin on numpy, which is scikit-learn 1.3.2 in our case. This is what is is being discussed here: numpy/numpy#24300 (comment).

However, arguably this is quite a rare case though.

Note that the command:

pip install scikit-learn numpy

issued when numpy 2.0.0 is released, and assuming scikit-learn 1.4.1 is the last stable released version with a dependency on "numpy<2", would install the last compatible numpy 1.x along with scikit-learn 1.4.1 as expected.

So upper bounding numpy<2 in scikit-1.4.x is not that bad in that respect.

@ogrisel
Copy link
Member

ogrisel commented Feb 14, 2024

I don't think we want to retrospectively upper bound all old scikit-learn releases and yank the non upper-bounded releases on pypi.org though. We can just accept that on rare occasions pip will do something weird with old releases.

@ogrisel
Copy link
Member

ogrisel commented Feb 14, 2024

However, now we have a problem because 1.4.1 has already been publicly tagged without the upper-bound dep on numpy. Neither the source tarball, wheels nor the conda-forge packages have been uploaded though.

So what should we do?

  • Deleting the 1.4.1 tag and retagging will annoy people with automated CI that monitor our github repo. EDIT: we did once in the past and some people complained and advised that we should never do this.
  • Tagging 1.4.1-1 and uploading tarball and wheels with 1.4.1 names without the -1 suffix is weird. Note that the -1 suffix is only possible in wheel filenames, not the tarball filename. EDIT: plus it's very tedious to manually rename the wheel filenames to add the -1 suffix before uploading manually to pypi.org.
  • Shall we the release 1.4.1 tarball with the numpy<2 upper bound, but that would not match the tag?
  • Shall we just not release 1.4.1 at all and instead release 1.4.1.post1 (with a matching new tag) straight away with the numpy<2 dependency?
  • Shall we release 1.4.1 (maybe just the tarball) then yank it, and then release 1.4.1.post1 straight away with the numpy<2 dependency.

EDIT: I am done editing that comment. I think I am in favor of one of the last 2 options.

@thomasjpfan
Copy link
Member

Shall we just not release 1.4.1 at all and instead release 1.4.1.post1 (with a matching new tag) straight away with the numpy<2 dependency?

If it works, then I would go with this option. (I do not know if we can issue a post release without a 1.4.1 release first.)

Otherwise, releasing 1.4.1 and yanking is okay with me.

@glemaitre
Copy link
Member Author

Yep I'm going with the post1 solution. This is the less questioning one and the most straightforward as a release process. Thanks everyone for the input. I'll open a new PR.

@lesteve
Copy link
Member

lesteve commented Feb 14, 2024

Agreed.

For completeness, if I understand #27658 correctly what is annoying for downstream packaging is deleting the tag and and reusing the same tag on a newer commit.

Not sure if deleting the 1.4.1 tag would be considered less a problem.

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