CI Add a nightly job that uses scikit-learn's nightlies#7621
CI Add a nightly job that uses scikit-learn's nightlies#7621rapids-bot[bot] merged 9 commits intorapidsai:mainfrom
Conversation
This adds a job that runs some of our unit tests with the nightly version of scikit-learn. This gives us a heads up on changes that are coming, as well as a chance to give feedback on regressions.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
| mkdir -p "${RAPIDS_TESTS_DIR}" | ||
|
|
||
| # Explicitly install those packages that we want to have from the nightly index. | ||
| rapids-pip-retry install \ |
There was a problem hiding this comment.
We could include the --extra-index-url options in dependencies.yaml but we want to control what is installed from the scientific-python-nightly index. If we add the --extra-index-url to the install command that installs all other dependencies would mean that we get all dependencies that exist on the nightly index from there. As of today we don't want to install all the dependencies that are available from that index.
There was a problem hiding this comment.
I don't think this is quite right. pip ignores pre-releases by default.
Using --isolated ignores any external configuration (to avoid "it works differently on my machine" issues). First, installing just scikit-learn (no constraint):
$ NIGHTLY_INDEX="https://pypi.anaconda.org/scientific-python-nightly-wheels/simple"
$ pip install --isolated --dry-run --no-cache-dir --extra-index-url=${NIGHTLY_INDEX} 'scikit-learn'
Looking in indexes: https://pypi.org/simple, https://pypi.anaconda.org/scientific-python-nightly-wheels/simple
...
Would install joblib-1.5.3 numpy-2.3.5 scikit-learn-1.8.0 scipy-1.16.3 threadpoolctl-3.6.0If you use a pre-release specifier in the requirement, then pip will consider pre-releases only for that package. For example, the following pulls in a nightly of scikit-learn but ignores nightlies for everything else:
$ pip install --isolated --dry-run --no-cache-dir --extra-index-url=${NIGHTLY_INDEX} 'scikit-learn>=1.8.0dev0'
Looking in indexes: https://pypi.org/simple, https://pypi.anaconda.org/scientific-python-nightly-wheels/simple
...
Would install joblib-1.5.3 numpy-2.3.5 scikit-learn-1.9.dev0 scipy-1.16.3 threadpoolctl-3.6.0side note: that "pre-release specifiers tell pip nightlies are OK" behavior led to some recent work to remove those specifiers where they'd been put in places where we did NOT want nightlies:
- Remove alpha specs on non-RAPIDS dependencies build-planning#144
- Remove alpha specs from non-RAPIDS dependencies #7568
The only way you'd get pre-releases of everything that has them on any of the considered indices is if you pass --pre or set the equivalent environment variable.
$ pip install --pre --isolated --dry-run --no-cache-dir --extra-index-url=${NIGHTLY_INDEX} 'scikit-learn'
Looking in indexes: https://pypi.org/simple, https://pypi.anaconda.org/scientific-python-nightly-wheels/simple
..
Would install joblib-1.5.3 numpy-2.5.0.dev0 scikit-learn-1.9.dev0 scipy-1.18.0.dev0 threadpoolctl-3.6.0I really think you'll want something like:
- put pre-release specifiers like
>=1.8.0dev0on all the dependencies where you're willing to accept nightlies - consolidate to a single
pip installso all requirements are considered together (separate calls increase the risk of a broken environment)
Given those constraints, totally up to you if you want the --extra-index-url in dependencies.yaml or not. I think it could be helpful for it to always get selected alongside constraints spelled like sciki-learn>=1.8.0dev0, but can also see how keeping it inline in this script prevents mistakes.
There was a problem hiding this comment.
I should rephrase the comment, because having the dependencies of scikit-learn from the anaconda nightly channel is what I was expecting to happen (what --pre does).
What I want to avoid is that one day we suddenly get nightlies for other direct cuml dependencies. This could happen because the package decides to start uploading wheels to the nightlies from one day to the next.
I also want to avoid having to update the 1.n.0dev0 to 1.n+1.0dev0 every time a new version is released. In particular because the old command will keep running without an error but not do the right thing. Once the release of 1.8.0 has happened scikit-learn>=1.8.0dev0 will no longer install the latest nightly and there won't be an error either.
There was a problem hiding this comment.
Sure, makes sense!
Once the release of 1.8.0 has happened scikit-learn>=1.8.0dev0 will no longer install the latest nightly and there won't be an error either.
As soon as there was a 1.8.1dev0 or something (the first nightly after the 1.8.0 release), pip should pick that up because it prefers the latest version.
And if there isn't a newer version on the nightly index then picking up 1.8.0 isn't a problem, because that is the latest version (just happens to also have been a released one).
That said, if you want to always guarantee this job installs the latest version on the nightly index, then the most reliable way I know to do that with pip is to download from the nightly index and then install locally-downloaded wheels.
Like this:
mkdir -p /tmp/nightly-wheels
pip download \
--no-deps \
--index-url=${NIGHTLY_INDEX} \
--prefer-binary \
-d /tmp/nightly-wheels \
'numpy>=2.5.0.dev0' \
'scikit-learn>=1.9.0dev0' \
'scipy>=1.18.0.dev0'
rapids-pip-retry install \
"${LIBCUML_WHEELHOUSE}"/libcuml*.whl \
"$(echo "${CUML_WHEELHOUSE}"/cuml*.whl)[test]" \
--requirement requirements.txt \
--constraint "${PIP_CONSTRAINT}" \
/tmp/nightly-wheels/*.whlPassing /tmp/nightly-wheels/*.whl to pip install as a requirement tells pip "you MUST install these specific files", which will prevent it from preferring other packages for those libraries.
There was a problem hiding this comment.
Would you be ok with merging it as it is?
I'd like to see this running so we can fix hiccups in time for 26.02. This means we'd need to merge within the next days so that I can observe what happens and have time left to fix it before Friday. We can fine tune pip when we run into problems with it or see potential for optimisation.
There was a problem hiding this comment.
I left 1 small comment about the CI config that should be resolved, but otherwise I won't block merging this.
If you're comfortable with the risks introduced by separating this into multiple pip install calls and happy generally that this is testing what you want to test, it's fine to merge.
Looks like you already have a ci-codeowners approval, so this would just need @dantegd to resolve his blocking review.
There was a problem hiding this comment.
I think we should merge this as-is and then explore whether to adopt some of the proposed improvements to make this more robust in a follow-up.
|
/ok to test 187fd31 |
Co-authored-by: James Lamb <[email protected]>
|
/merge |
## Summary Removes the specialized `ci/test_wheel_nightly_versions.sh` script in favor of re-using the standard `ci/test_wheel.sh` script in combination with "nightly" dependencies. This is a follow-up to #7621 . Authors: - Simon Adorf (https://github.com/csadorf) Approvers: - James Lamb (https://github.com/jameslamb) URL: #7684
Removes the specialized `ci/test_wheel_nightly_versions.sh` script in favor of re-using the standard `ci/test_wheel.sh` script in combination with "nightly" dependencies. This is a follow-up to rapidsai#7621 . Authors: - Simon Adorf (https://github.com/csadorf) Approvers: - James Lamb (https://github.com/jameslamb) URL: rapidsai#7684
This adds a job that runs some of our unit tests with the nightly version of scikit-learn. This gives us a heads up on changes that are coming, as well as a chance to give feedback on regressions.
The goal is to have a job that runs on cuml's
mainand uses the nightly version of scikit-learn.Failures in this jobs should not count towards the "N nights nightlies haven't passed" as it can happen that upstream projects produce broken nightlies or other incompatibilities that are beyond our control to resolve.
How can we test this? Locally using the default docker image for the custom job the script executes. But it would be good to be able to test this before merging it to
main.Closes #7564
cc @jameslamb for your interest. This is similar but different from what you suggested in the issue. In particular regarding using
dependencies.yaml(or not).