-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Adds CPython 3.10 testing [python-dev] #20942
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
Thanks. The pip build error is not very helpful:
shall we run pip with |
This build would probably benefit from using ccache with a reusable ccache folder. Edit: apparently ccache integration can be achieved with https://github.com/hendrikmuhs/ccache-action |
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.
It seems to be fast enough. @thomasjpfan why is this PR still in draft mode? Do you have other things in mind?
For scikit-learn, we could use cibuildwheel instead of a traditional build + test steps and publish the resulting wheel to https://anaconda.org/scipy-wheels-nightly/scikit-learn/files in addition to the other nightly builds.
But +1 to merge even without this, as long at it stays green :)
pip install --no-build-isolation -e . | ||
- name: Test scikit-learn | ||
run: | | ||
cd /tmp && python -m pytest -n 2 --pyargs sklearn |
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.
Even if our OPENMP-based Cython code should now avoid oversubscription problems in docker containers, numpy/scipy might still use too many threads and slow down the execution.
To be safe I think we should set OMP_NUM_THREADS=2
/ OPENBLAS_NUM_THREADS=2
globally for this workflow.
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.
@thomasjpfan Shall we set the environment variable in this file as well?
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.
The environment variables are set globally in lines 42-45:
env:
INSTALL_NUMPY: ${{ matrix.install-numpy }}
INSTALL_SCIPY: ${{ matrix.install-scipy }}
OMP_NUM_THREADS: '2'
OPENBLAS_NUM_THREADS: '2'
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.
Would this work and be useful?
cd /tmp && python -m pytest -n 2 --pyargs sklearn | |
cd /tmp && python -m pytest -n $OMP_NUM_THREADS --pyargs sklearn |
run: | | ||
echo "::set-output name=commit_msg::$(git log --no-merges -1 --oneline)" | ||
|
||
nightly: |
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.
Maybe rename to nightly-cpython
or upstream-nighly
.
- name: Install scikit-learn | ||
run: | | ||
export PATH="/usr/lib/ccache:/usr/local/opt/ccache/libexec:$PATH" | ||
pip install --no-build-isolation -e . |
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.
To get more informative outputs in case of problems:
pip install --no-build-isolation -e . | |
pip install -v --no-build-isolation -e . |
Also, it might be interesting to set SKLEARN_BUILD_PARALLEL=3
to speed up this step a bit.
I forgot to change it while testing ccache. It's ready.
I was going to wait for MacPython/scipy-wheels#124 to get merged before having our own wheels. Should we upload scikit-learn wheels for 3.10 without SciPy wheels? |
It's true that scikit-learn's Python 3.10 nightly wheels won't be that useful without scipy's but we don't strictly need to wait for them to do it ourselves :) |
But's let's merge this PR as it is as it's already useful the way it is and we can do the wheel uploading later. |
Wheels update
|
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 for setting this workflow, @thomasjpfan.
I just have one naive suggestion.
pip install --no-build-isolation -e . | ||
- name: Test scikit-learn | ||
run: | | ||
cd /tmp && python -m pytest -n 2 --pyargs sklearn |
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.
Would this work and be useful?
cd /tmp && python -m pytest -n 2 --pyargs sklearn | |
cd /tmp && python -m pytest -n $OMP_NUM_THREADS --pyargs sklearn |
We may not need this anymore if we have #21232 Edit: I think it would be good to keep this around to always test on the latest dev build of Python. I am thinking of keeping this CI on Python 3.10 and then move to 3.11 when the Python RC comes up. The tests would be slow in the beginning because wheels are not built yet for NumPy, SciPy, or pandas during the early in the RC phase. |
We could make it a CRON job at night like the build that check all nightly builds? |
I think it's good to test against the nightly versions of CPython / numpy / scipy irrespective of their specific version numbers. |
Closing as we are testing on python 3.10 on azure now. |
This PR adds python 3.10 testing to GitHub actions. Currently numpy has a 3.10 wheel to use and scipy needs to be build from source.
With yearly Python releases, I think it is good to have this type of testing set up.