MNT Use cibuildwheel to build the wheels #17921
Conversation
There was a problem hiding this comment.
Thanks @alfaro96 ! Are we going to also upload them somewhere from this same script?
Thank you @adrinjalali for the quickness! The idea is to add a step to upload the wheels and source distribution to the Anaconda repositories ( WDYT? |
|
Some tests are not passing while testing the wheels. I will investigate this issue and let you know why this is happening. Nevertheless, it is pretty straightforward to build the wheels with |
|
It seems we are running into memory issues. For instance:
|
|
Hey @ogrisel, I have finally found the right way to go (and, IMHO, the best and cleanest solution). The It is important to note that the command is run on each built wheel before testing it (between the build and install + test steps). Let us wait for green in the CD, but I think that this solution is gonna work pretty well. Thank you @ogrisel for your valuable comments! |
|
Moreover, I have added the wheels for Python 3.9. |
|
The generated wheels vendor |
| CIBW_TEST_REQUIRES: pytest pandas threadpoolctl | ||
| # Test that there are no links to system libraries | ||
| CIBW_TEST_COMMAND: pytest --pyargs sklearn && | ||
| python -m threadpoolctl -i sklearn |
There was a problem hiding this comment.
Do we need code to check the output of python -m threadpoolctl -i sklearn?
There was a problem hiding this comment.
We could write another helper python script that does this using the Python API of threadpoolctl instead of the command line.
I still think it's good to print it on stdout for manual checks and debugging.
There was a problem hiding this comment.
By the way I manually checked the output of one of the last green build for windows below and it looks good:
[
{
"filepath": "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpnijjd_bt\\lib\\site-packages\\sklearn\\.libs\\vcomp140.dll",
"prefix": "vcomp",
"user_api": "openmp",
"internal_api": "openmp",
"version": null,
"num_threads": 2
},
{
"filepath": "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpnijjd_bt\\lib\\site-packages\\numpy\\.libs\\libopenblas.NOIJJG62EMASZI6NYURL6JBKM4EVBGM7.gfortran-win_amd64.dll",
"prefix": "libopenblas",
"user_api": "blas",
"internal_api": "openblas",
"version": "0.3.9.dev",
"num_threads": 2,
"threading_layer": "pthreads"
},
{
"filepath": "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpnijjd_bt\\lib\\site-packages\\scipy\\.libs\\libopenblas.3HBPCJB5BPQGKWVZAVEBXNNJ2Q2G3TUP.gfortran-win_amd64.dll",
"prefix": "libopenblas",
"user_api": "blas",
"internal_api": "openblas",
"version": "0.3.9",
"num_threads": 2,
"threading_layer": "pthreads"
}
]|
Is there anything else to do? |
Addressing https://github.com/scikit-learn/scikit-learn/pull/17921/files#r518173353 would be a nice to have but I am fine with merging this PR as it is and further improve it other PRs. |
Indeed, I still want to provide the wheels for |
|
@thomasjpfan @rth shall we merge this to test it a bit "for real" and then open new PRs with improvements (e.g. ARM builds, additional sanity checks...) |
|
I haven't reviewed, but sound good to me. Great work @alfaro96 ! |
|
@rth I pinged you because you marked this PR as 1 change requested a while ago but I believe your concerns have been addressed. Let's wait for @thomasjpfan then. |
|
Oh I now see that it's an artifact of the lack of cancel review button on github... Sorry for the noise. |
That would be nice 😉. Should we specify the |
|
Let me merge it with the |
|
Okay, I forgot to add a trigger for push on the A quick fix is coming. |
|
yes I just noticed... cd60a35. |
Reference Issues/PRs
Closes MacPython/scikit-learn-wheels#27.
Closes MacPython/scikit-learn-wheels#61.
What does this implement/fix? Explain your changes.
This PR moves the
scikit-learnwheel builder to thescikit-learnorganization using thecibuildwheelpackage.