Skip to content

CI Add CPython 3.13 free-threaded build and remove historic nogil Python 3.9 build #29191

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Jun 10, 2024
Merged

CI Add CPython 3.13 free-threaded build and remove historic nogil Python 3.9 build #29191

merged 13 commits into from
Jun 10, 2024

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Jun 5, 2024 •
edited
Loading

@lesteve lesteve mentioned this pull request Jun 5, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 5, 2024 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: a8a84e0. Link to the linter CI: here

@adrinjalali adrinjalali added Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels Jun 5, 2024
@lesteve
Copy link
Member Author

lesteve commented Jun 5, 2024

Actually now that scipy free-threaded wheels are available (I thought it was going to take a bit more time than this) I think this will be easier if I change this PR to be about replacing CPython 3.9 nogil with CPython 3.13 free-threaded.

So I am going to make this PR as draft and remove the labels.

@lesteve lesteve marked this pull request as draft June 5, 2024 09:07
@lesteve lesteve removed Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels Jun 5, 2024
@adrinjalali
Copy link
Member

Feel free to ping when it's ready then :)

@lesteve lesteve force-pushed the remove-nogil-3.9 branch from e53ad17 to c979671 Compare June 5, 2024 09:32
@lesteve lesteve added the free-threading PRs and issues related to support for free-threaded CPython (a.k.a. nogil or no-GIL, PEP 703) label Jun 5, 2024
@lesteve lesteve changed the title CI Remove historic nogil Python 3.9 build CI Add CPython 3.13 free-threaded build and remove historic nogil Python 3.9 one Jun 5, 2024
@lesteve lesteve changed the title CI Add CPython 3.13 free-threaded build and remove historic nogil Python 3.9 one CI Add CPython 3.13 free-threaded build and remove historic nogil Python 3.9 build Jun 5, 2024
ogrisel
ogrisel reviewed Jun 5, 2024
View reviewed changes
@lesteve
Copy link
Member Author

lesteve commented Jun 5, 2024

Good news the CPython 3.13 free-threaded tests pass in the CI 🎉, see build log

Now time for clean-up!

@lesteve
Copy link
Member Author

lesteve commented Jun 5, 2024 •
edited
Loading

OK I am marking this as ready for review!

Things to note:

  • for the lock-file I have used the same strategy as for the Cpython 3.9 nogil: there is a long docker command that you need to run by hand, i.e. it is not part of the lock-file auto-update. When CPython 3.13 is released (scheduled October 2024), this can be cleaned up, and made similar to other CI builds. I have tried to explain the reason in build_tools/azure/cpython_free_threaded_requirements.txt which also has the long-winded docker command to update the lock-file.
  • for some reason some our tests require setuptools, there was an error in one of the earlier CPython 3.13 CI run but the build log is not available anymore. Probably this can be cleaned up in a separate PR. Probably the reason it was not seen before is that pip does not depend on setuptools in CPython 3.13?
  • I have used free-threaded everywhere rather than nogil because this is the preferred naming for CPython developers, see for example Add support for Python 3.13 free-threaded build #28978 (comment).

@lesteve lesteve marked this pull request as ready for review June 5, 2024 15:48
@lesteve lesteve mentioned this pull request Jun 5, 2024
Closed
@lesteve
Copy link
Member Author

lesteve commented Jun 7, 2024

ping @adrinjalali @ogrisel for a review 🙏, see #29191 (comment) for a high-level summary.

ogrisel
ogrisel approved these changes Jun 7, 2024
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Thanks very much @lesteve!

@lesteve @ogrisel
Update build_tools/azure/cpython_free_threaded_requirements.txt
a8a84e0 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
betatim
betatim reviewed Jun 10, 2024
View reviewed changes
build_tools/azure/install.sh
# build_tools/azure/cpython_free_threaded_requirements.txt when there
# is a CPython 3.13 free-threaded wheel
# For now, we need the development version of Cython which has CPython
# 3.13 free-threaded fixes so we install it from source
Copy link
Member

Choose a reason for hiding this comment

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

About 30hours ago a cython wheel appeared on https://anaconda.org/scientific-python-nightly-wheels/Cython/files (I think someone worked on this during the scientific python summit?)

Is it worth using that instead of building from source?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, I am a bit confused though that there is only a pure Python wheel and not python version specific wheels like there is on PyPI ...

Copy link
Member

Choose a reason for hiding this comment

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

is it because cython itself doesn't contain any compiled code/c extensions?

betatim
betatim approved these changes Jun 10, 2024
View reviewed changes
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

LGTM. Merging to see it in action. The comment about using the cython nightly wheel can be done in a new PR I think.

@betatim betatim enabled auto-merge (squash) June 10, 2024 07:36
@betatim betatim merged commit 85015e1 into scikit-learn:main Jun 10, 2024
28 checks passed
@lesteve lesteve deleted the remove-nogil-3.9 branch June 10, 2024 08:09
This was referenced Jun 10, 2024
Merged
Merged
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
Merged
11 tasks
@vfdev-5 vfdev-5 mentioned this pull request Aug 15, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@betatim betatim betatim approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

Assignees
No one assigned
Labels
Build / CI free-threading PRs and issues related to support for free-threaded CPython (a.k.a. nogil or no-GIL, PEP 703)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

⚠️ CI failed on Linux_nogil.pylatest_pip_nogil (last failure: Jun 10, 2024) ⚠️
4 participants
@lesteve @adrinjalali @ogrisel @betatim