make conda installs in CI stricter #17013
Conversation
jameslamb
added
2 - In Progress
jameslamb
changed the title
jameslamb
changed the title
jameslamb
added
3 - Ready for Review
| @@ -5,6 +5,8 @@ set -euo pipefail | |||
|
|
|||
| . /opt/conda/etc/profile.d/conda.sh | |||
|
|
|||
| RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" | |||
There was a problem hiding this comment.
In build_docs.sh we export this, but here we're just setting it locally. I doubt it matters here but it might be worth establishing consistent patterns. Also applies to other files.
There was a problem hiding this comment.
My thinking here was "export it if it's already exported (as in the docs example), otherwise make it local", since I thought that had the least risk of causing other surprising problems.
I clicked around a little, and I think:
RAPIDS_VERSION= does not even need to be set in docs-build scriptsRAPIDS_VERSION_MAJOR_MINOR= does not need to be exportedRAPIDS_VERSION_NUMBER= needs to be exported, forrapids-upload-docs(code link in gha-tools)
For all the other scripts that are not docs builds, this PR only introduces NEW variables, so I think not exporting them is slightly safer.
Just pushed 16367a5 doing that. If that works, I'll make similar changes in the other PRs.
There was a problem hiding this comment.
This went fine in CI... docs built successfully and I see the expected versions of things in the environment. Docs look ok at the preview too: https://downloads.rapids.ai/ci/cudf/pull-request/17013/16367a5/docs/libcudf/html/index.html
There was a problem hiding this comment.
Some of these claims I made about the variables not needing to be exported were incorrect. Addressed in #17042 (and updated across all the other PRs for this issue).
|
/merge |
Follow-up to #17013 Changes relative to that PR: * switches to pinning CI conda installs to the output of `rapids-version` (`{major}.{minor}.{patch}`) instead of `rapids-version-major-minor` (`{major}.{minor}`), to get a bit more protection in the presence of hotfix releases * restores some exporting of variables needed for docs builds I made some mistakes in #17013 (comment). Missed that this project's Doxygen setup is expecting to find `RAPIDS_VERSION` and `RAPIDS_VERSION_MAJOR_MINOR` defined in the environment. https://github.com/rapidsai/cudf/blob/7173b52fce25937bb69e22a083a5de4655078fa1/cpp/doxygen/Doxyfile#L41 https://github.com/rapidsai/cudf/blob/7173b52fce25937bb69e22a083a5de4655078fa1/cpp/doxygen/Doxyfile#L2229 Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Kyle Edwards (https://github.com/KyleFromNVIDIA) URL: #17042
Contributes to rapidsai/build-planning#106 This project doesn't need any of the changes that are being made as part of that issue across most other repos... it's already doing all of its `pip` and `conda` installs in CI in single installs (which is strict and safe) 🎉 This small PR just makes its handling of environment variables in `ci/build_docs.sh` consistent with other RAPIDS repos, in the interest of keeping those scripts looking similar so it's easy to apply changes across all RAPIDS projects. ref: rapidsai/cudf#17013 (comment) Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Mark Harris (https://github.com/harrism) - Mike Sarahan (https://github.com/msarahan) URL: #1469
Description
Contributes to rapidsai/build-planning#106
Proposes specifying the RAPIDS version in
conda installcalls in CI that install CI artifacts, to reduce the risk of CI jobs picking up artifacts from other releases.Checklist