Skip to content

CI Adds manylinux1 to building wheels #19235

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

Merged
merged 21 commits into from
Jan 25, 2021

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Fixes #19233

What does this implement/fix? Explain your changes.

We have been getting errors from users that are downloading the source from pypi. This PR adds manylinux1 to wheel builder.

@thomasjpfan thomasjpfan changed the title CI Adds manylinux1 [cd build] CI Adds manylinux1 to building wheels Jan 21, 2021
@thomasjpfan thomasjpfan marked this pull request as draft January 21, 2021 19:20
@alfaro96
Copy link
Member

Thank you @thomasjpfan for your PR!

I would prefer to set these environment variables:

CIBW_MANYLINUX_X86_64_IMAGE: manylinux1
CIBW_MANYLINUX_I686_IMAGE: manylinux1

To build the wheels for manylinux1. I do not think that we need to build for both, manylinux1 and manylinux2010, since manylinux1 wheels works for manylinux2010.

@thomasjpfan
Copy link
Member Author

I think the advantage of building both is that we can be sure that we are able to build the "newer" version of manylinux***. Since manylinux2010 and manylinux2014 both require pip >=19.*, we can most get away with building manylinux1 and manylinux2014. This is what scipy uploads to pypi.

@thomasjpfan thomasjpfan marked this pull request as ready for review January 21, 2021 20:24
@alfaro96
Copy link
Member

alfaro96 commented Jan 21, 2021

I think the advantage of building both is that we can be sure that we are able to build the "newer" version of manylinux***. Since manylinux2010 and manylinux2014 both require pip >=19.*, we can most get away with building manylinux1 and manylinux2014. This is what scipy uploads to pypi.

If I am not wrong, scipy only builds manylinux2014 for the aarch64 architecture and manylinux1 otherwise.

Edit: numpy builds wheels for manylinux1 and manylinux2010, so I am okay with building both. But would not be easier to set:

manylinux_image: [manylinux1, manylinux2010]

in the already defined build matrix and use:

exclude:
   - os: windows-latest
     manylinux_image: manylinux_1
   - os: windows-latest
     manylinux_image: manylinux_2010
   - os: macos-latest
     manylinux_image: manylinux_1
   - os: macos-latest
     manylinux_image: manylinux_2010

By this way, we would not need to duplicate code.

Off topic: I am missing the aarch64 wheels for scikit-learn==0.24.1. Are we out of travis credits?

@thomasjpfan
Copy link
Member Author

If I am not wrong, scipy only builds manylinux2014 for the aarch64 architecture and manylinux1 otherwise.

Ah you are correct, numpy builds the full matrix

Off topic: I am missing the aarch64 wheels for scikit-learn==0.24.1. Are we out of travis credits?

Yea we are out of credits :(

@alfaro96
Copy link
Member

If I am not wrong, scipy only builds manylinux2014 for the aarch64 architecture and manylinux1 otherwise.

Ah you are correct, numpy builds the full matrix

I edit my original answer with a possible idea to avoid code duplication.

Off topic: I am missing the aarch64 wheels for scikit-learn==0.24.1. Are we out of travis credits?

Yea we are out of credits :(

😢

@thomasjpfan
Copy link
Member Author

Before we merge this, I would want to make sure this actually resolves the original issue. In the original issue, the readthedocs instance is using pip 20.3, which, in principle, should have fetched manylinux2010.

@glemaitre
Copy link
Member

We can upload the file in test.pypi.org and have an environment to try to fetch it.
However, we need a system similar to the one in readthedoc (because for me it already fetches the wheel without issue)

@glemaitre
Copy link
Member

FYI in the original issue, I just think that the build was starting at the moment of the upload and only the tar.gz file was available. The current builds are fetching the manylinux2010 wheels. However, we need to have pip>=19.3 that might not be always the case.

Base automatically changed from master to main January 22, 2021 10:54
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.

LGTM.

@ogrisel ogrisel added this to the 0.24.1 milestone Jan 25, 2021
@ogrisel ogrisel added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Jan 25, 2021
Copy link
Member

@alfaro96 alfaro96 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 @thomasjpfan!

@ogrisel
Copy link
Member

ogrisel commented Jan 25, 2021

I tagged this PR for 0.24.1 as we can backport this to the 0.24.X branch and trigger a build there to generate manylinux1 artifacts and upload them manually with twine.

@glemaitre glemaitre merged commit 1f26735 into scikit-learn:main Jan 25, 2021
@glemaitre
Copy link
Member

I will manage the backport.

glemaitre pushed a commit that referenced this pull request Jan 25, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
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.

Missing wheels for readthedocs environment
4 participants