-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Build and test Python 3.12 wheels #27027
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
Conversation
.github/workflows/wheels.yml
Outdated
CIBW_ENVIRONMENT: SKLEARN_SKIP_NETWORK_TESTS=1 | ||
SKLEARN_BUILD_PARALLEL=3 | ||
PIP_EXTRA_INDEX_URL=https://pypi.anaconda.org/scientific-python-nightly-wheels/simple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the CI issues, I think we should set PIP_EXTRA_INDEX_URL
only for Python 3.12. Otherwise, the other jobs will pick it up and try to build packages from source.
In general, I think it's better to isolate these environment variables to Python 3.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any suggestions on how to do this without too much pain? Would it be at the github actions (or Cirrus) level, finding a way to define a variable depending on which matrix entry we are in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expand the matirx configuration for 3.12 with a new prerelease
item:
- os: macos-latest
python: 311
platform_id: macosx_arm64
prerelease: True
and then add it to the env
:
CIBW_PRERELEASE_PYTHONS: ${{ matrix.prerelease }}
Then updated build_wheels.sh
to adjust CIBW_ENVIRONMENT
based on CIBW_PRERELEASE_PYTHONS
.
.github/workflows/wheels.yml
Outdated
CIBW_ENVIRONMENT: SKLEARN_SKIP_NETWORK_TESTS=1 | ||
SKLEARN_BUILD_PARALLEL=3 | ||
PIP_EXTRA_INDEX_URL=https://pypi.anaconda.org/scientific-python-nightly-wheels/simple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expand the matirx configuration for 3.12 with a new prerelease
item:
- os: macos-latest
python: 311
platform_id: macosx_arm64
prerelease: True
and then add it to the env
:
CIBW_PRERELEASE_PYTHONS: ${{ matrix.prerelease }}
Then updated build_wheels.sh
to adjust CIBW_ENVIRONMENT
based on CIBW_PRERELEASE_PYTHONS
.
@@ -14,6 +14,10 @@ cp $WHEEL_PATH $WHEEL_NAME | |||
# Dot the Python version for identyfing the base Docker image | |||
PYTHON_VERSION=$(echo ${PYTHON_VERSION:0:1}.${PYTHON_VERSION:1:2}) | |||
|
|||
# TODO: remove this when Python 3.12 is released | |||
if [[ "$PYTHON_VERSION" == "3.12" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we condition on CIBW_PRERELEASE_PYTHONS == True
here to and add the "-rc"
?
When 3.12 gets released, we just have to remove prerelease
from the build matrix, without needing to update this script anymore.
There are some failures in the Python 3.12 builds but they are also in I will wait for pandas to upload a Python 3.12 wheel to scientific-python-nightly-builds pandas-dev/pandas#54447 so this can be added back to CIBW_TESTING_REQUIRES. pandas is waiting for numpy 1.26 release which should happen soonish since Python 3.12 release candidate has been released. |
Pandas has started uploading a python 3.12 wheel to scientific-python-nightly-wheels so I gave it another go. There does not seem to be any issue with python 3.12 as far as I can see:
I am not too sure how we want to proceed forward on this:
|
…nto python-3.12-wheels
…nto python-3.12-wheels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at the most recent build logs, the linux build error looks real and the windows one is from building pandas from source.
I think it's important to fix the linux issue, but we can leave the windows build for when pandas releases a wheel for 3.12.
The setuptools dependency issue should be fixed (for real this time hopefully) in #27420 That should fix the Linux build. |
Now that pandas has Python 3.12 wheels in its latest release (2.1.1), this makes it easier for us. This is only testing on Python 3.12 for CirrusCI as suggested in #27027 (review) Do we want to upload Python 3.12 wheels for scikit-learn 1.3.1? FYI Python 3.12 release is scheduled for October 2. |
I think this would be really great, having stable Python 3.12 wheels on release! |
CIBW_BUILD: cp311-macosx_arm64 | ||
CIBW_BUILD: cp312-macosx_arm64 | ||
# TODO: remove when Python 3.12 is released | ||
CIBW_PRERELEASE_PYTHONS: True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can now be removed, since cibuildwheel v2.15.0 CPython 3.12 wheels are built by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good to know!
I'd rather wait for Python 3.12 to be released before removing all of them because we rely on CI_BW_PRERELEASE_PYTHONS
on Windows for building the custom Docker image.
As discussed in the monthly meeting, we are aiming at uploading the wheels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lesteve Could you update the setup.py
metadata and add that we support Python 3.12 meaning:
"Programming Language :: Python :: 3",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
"Programming Language :: Python :: 3.12",
It would allow to update the badge at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on my side. @ogrisel @thomasjpfan do you want to have a new look.
Once merge, I will figure out what is needed to upload the new wheel on PyPI and build the binary in conda-forge.
Yep, there are already back-ported indeed: https://github.com/scikit-learn/scikit-learn/commits/1.3.X |
Any chance wheels could be uploaded to PyPI (maybe even manually) before Python 3.12.0 releases Monday (2023-10-02)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the updated comments.
@@ -114,6 +125,9 @@ jobs: | |||
- os: macos-latest | |||
python: 310 | |||
platform_id: macosx_arm64 | |||
- os: macos-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update line 118 above to something like:
# MacOS arm64
# The wheel for the latest Python version is built and tested on
# Cirrus CI but due to limited build time for free accounts on Cirrus
# CI, we build the macOS arm64 wheels for the other Python versions on
# Github Actions via cross-compilation (without running the tests).
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Thanks @lesteve! |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
As suggested in #26886 (review)