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

CI Add wheel builds for Python 3.11 #24446

Merged
merged 51 commits into from Oct 20, 2022
Merged

Conversation

cmarmo
Copy link
Member

@cmarmo cmarmo commented Sep 15, 2022

Reference Issues/PRs

Fixes #24427

What does this implement/fix? Explain your changes.

This pull request adds to github workflows the build and test of wheels for python 3.11

Any other comments?

Python 3.11 is still release candidate: a number of manual hack are necessary to download the right dependencies.

To summarize the failures:

 =================================== ERRORS ====================================
  ________________________ ERROR collecting test session ________________________
  Python\Lib\importlib\__init__.py:126: in import_module
      return _bootstrap._gcd_import(name[level:], package, level)
  <frozen importlib._bootstrap>:1206: in _gcd_import
      ???
  <frozen importlib._bootstrap>:1178: in _find_and_load
      ???
  <frozen importlib._bootstrap>:1149: in _find_and_load_unlocked
      ???
  <frozen importlib._bootstrap>:690: in _load_unlocked
      ???
  Python\Lib\site-packages\_pytest\assertion\rewrite.py:168: in exec_module
      exec(co, module.__dict__)
  Python\Lib\site-packages\sklearn\conftest.py:15: in <module>
      from sklearn.datasets import fetch_20newsgroups
  Python\Lib\site-packages\sklearn\datasets\__init__.py:22: in <module>
      from ._twenty_newsgroups import fetch_20newsgroups
  Python\Lib\site-packages\sklearn\datasets\_twenty_newsgroups.py:46: in <module>
      from ..feature_extraction.text import CountVectorizer
  Python\Lib\site-packages\sklearn\feature_extraction\__init__.py:8: in <module>
      from ._hash import FeatureHasher
  Python\Lib\site-packages\sklearn\feature_extraction\_hash.py:10: in <module>
      from ._hashing_fast import transform as _hashing_transform
  E   ImportError: DLL load failed while importing _hashing_fast: The specified module could not be found.
  =========================== short test summary info ===========================
  ERROR  - ImportError: DLL load failed while importing _hashing_fast: The spec...
  !!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
  ============================== 1 error in 2.46s ===============================
  • MacOS : wheel build is failing with this traceback fixed setting SETUPTOOLS_USE_DISTUTILS=stdlib. The test test_grid_search_failing_classifier fails.

.github/workflows/wheels.yml Outdated Show resolved Hide resolved
@cmarmo
Copy link
Member Author

cmarmo commented Sep 17, 2022

Tests failing on Windows:

  • test_gradient_boosting_early_stopping from sklearn\ensemble\tests\test_gradient_boosting.py,
  • test_grid_search_failing_classifier

@cmarmo cmarmo added this to the 1.2 milestone Sep 20, 2022
@cmarmo
Copy link
Member Author

cmarmo commented Sep 27, 2022

Hello, I'm unable to the debug on Windows ... perhaps someone might have a look to the last failing Windows test?
Thanks!

@jjerphan
Copy link
Member

Thanks for this work, @cmarmo!

Hello, I'm unable to the debug on Windows ... perhaps someone might have a look to the last failing Windows test? Thanks!

It's weird that this test fail on Windows only. 🤔

@jeremiedbb: was the IndexError you once observed on Windows by any chanced related to scikit-learn/sklearn/ensemble/_gradient_boosting.pyx? Or was it related to HGBT?

@jeremiedbb
Copy link
Member

was the IndexError you once observed on Windows by any chanced related to scikit-learn/sklearn/ensemble/_gradient_boosting.pyx? Or was it related to HGBT?

No they are all related to the HGBT

@jjerphan
Copy link
Member

jjerphan commented Sep 29, 2022

A merge and voilà: all green ! ✔️ 🙃

More seriously, I think it's weird that this test failed before but now passes.

@cmarmo
Copy link
Member Author

cmarmo commented Sep 29, 2022

@jjerphan , that would be awesome ... but ... the test does not pass during the wheel building with Windows python 3.11 and scipy 1.10 ...
All is green otherwise. :)

@cmarmo
Copy link
Member Author

cmarmo commented Sep 29, 2022

Still failing... I have looked for Windows virtual machines on the cloud and I only found Azure: but they ask me for my credit card number to open a free account... and I don't like this. Does anyone know other options that I can use for debugging the failure? Thanks!

@jjerphan
Copy link
Member

jjerphan commented Sep 29, 2022

@jjerphan , that would be awesome ... but ... the test does not pass during the wheel building with Windows python 3.11 and scipy 1.10 ...

Ah yes, my bad.

In GradientBoosting{Regressor,Clasiffier} the number of estimators which must be smaller when tolerance is larger, but I think the failing test's definition is too strict.

On the setup Windows where this test fails, the number of estimators is slightly smaller for a combination, but I do not think this should be fatal for the CI. Hence, I think this test can be rewritten a bit to empirically check that the number of estimators is increasing when the tolerance decreases.

PS: I have created #24541 to propose a resolution.

@glemaitre
Copy link
Member

On the setup Windows where this test fails, the number of estimators is slightly smaller for a combination, but I do not think this should be fatal for the CI

Actually, this could be problematic because it means that on a Windows machine, depending on the NumPy and SciPy versions (I assume) you will get a different model. I think that we should investigate this issue.

It could be cool to investigate whether using the NumPy and SciPy versions of the Python 3.10 builds would also trigger the failure. It means that we should build from source to make this test.

@ogrisel
Copy link
Member

ogrisel commented Sep 30, 2022

If the difference in loss values is on the order of machine precision, it could just be caused by different versions of the compiler / libc runtime libraries. But if it's more than that, maybe there is a real bug.

@EwoutH
Copy link

EwoutH commented Oct 7, 2022

#24541 is now merged! @cmarmo could you rebase your branch and see if it solves the problem?

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

All 🟢! Thank you, @cmarmo!

Here are a few comments and a question regarding the index to publish new wheels on.

.github/workflows/wheels.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated
Comment on lines 14 to 15
# nightly scipy 1.10.0.dev0 are the first wheels available for python 3.11
"scipy==1.10.0.dev0; python_version>='3.11'",
Copy link
Member

Choose a reason for hiding this comment

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

Naive question: If we rely on nightly builds of SciPy which uses this alternative index, must we publish wheels for Python 3.11 on this index uniquely? (I am not familiar with the CD enough to know if it's the case.)

Copy link
Member

Choose a reason for hiding this comment

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

I think our Python 3.11 wheel should only be on the alternative index until SciPy and NumPy releases a Python 3.11 wheel on PyPI. (If NumPy and SciPy does not have 3.11 wheels on PyPI, then users with 3.11 would need to build NumPy and SciPy when installing the scikit-learn wheel.)

Once SciPy and NumPy release Python 3.11 wheels on PyPI, we can upload wheels there too.

Implementation-wise: When this PR is merged to build 3.11 wheels, there are two releasing situations. It depends on if scikit-learn 1.2 is released before or after SciPy and NumPy wheels are on PyPI for Python 3.11:

  • before: Update build_tools/github/check_wheels.py to ignore a specific version of Python when counting the number of wheels, likely through a cli argument. This can be done in a follow up PR (We may need to do this anyway so we can build and test with Python-dev but also make a release before the Python-dev version is released)
  • after: Nothing needs to change.

Copy link

@EwoutH EwoutH Oct 7, 2022

Choose a reason for hiding this comment

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

NumPy has their Python 3.11 files on PyPI, even for some time already: https://pypi.org/project/numpy/1.23.2/#files

SciPy unfortunately not yet, but their are very close with the 1.9.2 release with Python 3.11 wheels expected within a week: scipy/scipy#16851 (comment)

build_tools/github/build_wheels.sh Show resolved Hide resolved
build_tools/github/build_minimal_windows_image.sh Outdated Show resolved Hide resolved
@ogrisel
Copy link
Member

ogrisel commented Oct 17, 2022

Waiting for scipy/scipy#17224 to be addressed.

In the mean time, could you please update the travis build to also build the linux arm64 wheels as previously mentioned in #24446 (comment)?

run: |
# Test with nightly dependencies for nightly builds
echo "PIP_EXTRA_INDEX=https://pypi.anaconda.org/scipy-wheels-nightly/simple" >> $GITHUB_ENV
echo "PIP_PRE=1" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we do that only for the 3.11 build? The other builds should work with the released version of numpy / scipy (once scipy/scipy#17224 backported as part of scipy 1.9.3).

Copy link
Member Author

@cmarmo cmarmo Oct 17, 2022

Choose a reason for hiding this comment

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

I am not completely aware of the scipy wheels publication procedure.
In my opinion if it had been possible to test scikit-learn nightly wheels against scipy nightly wheels the failure in #24612 and the one scipy/scipy#17224 is willing to fix (both related to 3.10 too), would have been detected before the release of scipy 1.9.2 , and the backporting would not have been necessary.
The question is how far do you want to go with testing.

Copy link
Member

Choose a reason for hiding this comment

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

Now that SciPy 1.9.3 has been released, I think we need to remove this step from this PR if we want to publish stable wheels once Python 3.11 is officially release.

We in the meantime create another PR for adapting scikit-learn nightly builds' dependences (incorporating this step in if needed). What do you think?

Copy link
Member

@ogrisel ogrisel Oct 20, 2022

Choose a reason for hiding this comment

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

Sorry @cmarmo I had missed your reply. Indeed the testing against the nightly builds of our dependencies is a different goal than publishing our own nightly built wheels for project that depend upon us. I think that for the latter we should build always build the wheels we publish against stable dependencies not have our wheel publishing infrastructure blocked by a bug/regression in the dev branch of one of our dependencies.

But I agree with your suggestions and @jjerphan's that we might want to upgrade our nightly [scipy-dev] tests (currently configured on Azure pipelines and only for linux) to also run the tests on all the platforms, probably by reusing the config done on github actions for both goals (testing against dev dependencies and publishing our own dev wheels).

But let's do that in a dedicated follow-up PR.

@cmarmo
Copy link
Member Author

cmarmo commented Oct 18, 2022

arm64 on travis with python 3.11 and scipy 1.92 fails with the same error : should be fixed by scipy/scipy#17224 (reassuring... :) )

@cmarmo
Copy link
Member Author

cmarmo commented Oct 18, 2022

scipy/scipy#17239 : scipy 1.9.3 on its way.

@ogrisel
Copy link
Member

ogrisel commented Oct 18, 2022

Actually I just realized that numpy and scipy have already uploaded cp311 wheels for their latest stable release on pypi.org:

I find this a bit surprising since CPython 3.11 final is not yet published but maybe this is pragmatic because they anticipate that CPython developers will not introduce an ABI breaking change in between 3.11-rc2 and 3.11 final.

So it means that:

  • we can probably stop using the scipy nightly extra index (once scipy 1.9.3 is published)
  • we can merge this PR before CPython 3.11 final is released and even release cp311 wheels for scikit-learn 1.1.2 (or 1.1.3 if released in the mean time).

@cmarmo
Copy link
Member Author

cmarmo commented Oct 18, 2022

Actually I just realized that numpy and scipy have already uploaded cp311 wheels for their latest stable release on pypi.org:

* https://pypi.org/project/numpy/#files

* https://pypi.org/project/scipy/#files

@ogrisel, yes, this happened here and here

So it means that:

* we can probably stop using the scipy nightly extra index (once scipy 1.9.3 is published)

Sorry , I feel a bit unheard 😇... do you mind checking #24446 (comment) and its answers and #24446 (comment)? Thanks!

@cmarmo
Copy link
Member Author

cmarmo commented Oct 20, 2022

All green!

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @cmarmo.

Just a few last comments since SciPy 1.9.3 is released whilst Python 3.11 is still using released candidates: I would be conservative and would prefer waiting until at least Monday 24th October and making sure that scikit-learn will not to depend on release candidates for Python 3.11 before releasing those new wheels. What do you think? What do you prefer?

build_tools/github/vendor.py Show resolved Hide resolved
build_tools/github/build_minimal_windows_image.sh Outdated Show resolved Hide resolved

# Copy and install the Windows wheel
COPY $WHEEL_NAME $WHEEL_NAME
COPY $CONFTEST_NAME $CONFTEST_NAME
RUN echo PIP_EXTRA_INDEX_URL = $env:PIP_EXTRA_INDEX_URL
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @cmarmo: using ARG should be enough to set variables.

run: |
# Test with nightly dependencies for nightly builds
echo "PIP_EXTRA_INDEX=https://pypi.anaconda.org/scipy-wheels-nightly/simple" >> $GITHUB_ENV
echo "PIP_PRE=1" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

Now that SciPy 1.9.3 has been released, I think we need to remove this step from this PR if we want to publish stable wheels once Python 3.11 is officially release.

We in the meantime create another PR for adapting scikit-learn nightly builds' dependences (incorporating this step in if needed). What do you think?

.github/workflows/wheels.yml Outdated Show resolved Hide resolved
.github/workflows/wheels.yml Outdated Show resolved Hide resolved
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.

Assuming this PR can now build without the dev dependencies, +1 for merge as explained in the following:

Comment on lines 130 to 135
- name: Set pip extra options
if: github.event_name == 'schedule'
run: |
# Test with nightly dependencies for nightly builds
echo "PIP_EXTRA_INDEX=https://pypi.anaconda.org/scipy-wheels-nightly/simple" >> $GITHUB_ENV
echo "PIP_PRE=1" >> $GITHUB_ENV
Copy link
Member

@ogrisel ogrisel Oct 20, 2022

Choose a reason for hiding this comment

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

As discussed in https://github.com/scikit-learn/scikit-learn/pull/24446/files#r1000353644, let's focus this PR on building and publishing 3.11 wheels against stable dependencies and remove the use of dev dependencies from this PR.

We can tackle nightly testing against dev dependencies (without uploading) in a dedicated PR.

To do that, we could introduce a second github actions scheduled event with a specific env variable/config flag to also disable the final upload to anaconda or pypi.

We will also have to re-implement the [scipy-dev] commit message trigger currently implemented in our Azure Pipelines config and delete that config from Azure Pipelines.

Suggested change
- name: Set pip extra options
if: github.event_name == 'schedule'
run: |
# Test with nightly dependencies for nightly builds
echo "PIP_EXTRA_INDEX=https://pypi.anaconda.org/scipy-wheels-nightly/simple" >> $GITHUB_ENV
echo "PIP_PRE=1" >> $GITHUB_ENV

Copy link
Member

Choose a reason for hiding this comment

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

@cmarmo I did not have time to write that comment and you addressed it concurrently :)

@cmarmo
Copy link
Member Author

cmarmo commented Oct 20, 2022

@ogrisel, @jjerphan , @thomasjpfan I have removed the nightly test against the nightly builds.
Please note that the cleaning in #24657 was made here already too (I didn't check the PR I just thought the cleaning was necessary once removed win32).

@ogrisel ogrisel merged commit 38c34af into scikit-learn:main Oct 20, 2022
@ogrisel
Copy link
Member

ogrisel commented Oct 20, 2022

Merged! Thanks @cmarmo!

@EwoutH
Copy link

EwoutH commented Oct 20, 2022

Thanks for this major effort @cmarmo and congratulations on getting this merged!

@ogrisel Would a backport to the 1.1 maintenance be possible, and a new (1.1.3) patch release? That would allow to get Python 3.11 wheels on PyPI before the Python 3.11.0 stable release next week.

@glemaitre
Copy link
Member

glemaitre commented Oct 20, 2022 via email

@cmarmo cmarmo deleted the wheel-python-311 branch October 20, 2022 19:43
@thomasjpfan thomasjpfan modified the milestones: 1.2, 1.1.3 Oct 24, 2022
@thomasjpfan thomasjpfan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Oct 24, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 26, 2022
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
glemaitre pushed a commit that referenced this pull request Oct 26, 2022
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@EwoutH
Copy link

EwoutH commented Oct 26, 2022

@glemaitre Thanks a lot for the new release! Python 3.11 wheels are up on PyPI, which is fantastic!

Could you maybe add the release notes that this is the first release with Python 3.11 wheels?

392781 added a commit to 392781/scikit-ntk that referenced this pull request Oct 27, 2022
Takes into account scikit-learn 1.1.3 release which creates wheels for Python 3.11: scikit-learn/scikit-learn#24446
392781 added a commit to 392781/scikit-ntk that referenced this pull request Oct 27, 2022
Takes into account scikit-learn 1.1.3 release which creates wheels for Python 3.11: scikit-learn/scikit-learn#24446
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2022
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build / CI To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.11 wheels
7 participants