refactor: build wheels and conda packages using Python limited API - #21329
Conversation
|
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. |
gforsyth
added
improvement
|
/ok to test |
|
/ok to test |
|
/ok to test |
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
A few small suggestions, mostly cosmetic, but overall looks great! I spot-checked CI jobs and their logs and saw all the things I'd expect and nothing I didn't 😁
|
|
||
| # If `RAPIDS_PY_VERSION` is set, use that as the lower-bound for the stable ABI CPython version | ||
| # This is only applied to pylibcudf, cudf, and cudf_kafka, not other Python packages | ||
| PYTHON_ARGS_WITH_PY_API=("${PYTHON_ARGS_FOR_INSTALL[@]}") |
There was a problem hiding this comment.
Very minor comment, but instead of switching which list is used, I think it'd be a little easier to follow this if you did something like this:
PY_API_ARGS=()
if [ -n "${RAPIDS_PY_VERSION}" ]; then
RAPIDS_PY_API="cp${RAPIDS_PY_VERSION//./}"
PY_API_ARGS+=("--config-settings" "skbuild.wheel.py-api=${RAPIDS_PY_API}")
fiAnd then every install call gets PYTHON_ARGS_FOR_INSTALL, and only the ones that link against Python also get PY_API_ARGS.
Like this (pseudocode with cmake stuff and if statements removed):
# pylibcudf
python -m pip install \
"${PYTHON_ARGS_FOR_INSTALL[@]}" \
"${PY_API_ARGS[@]}" \
.
# cudf-polars
python -m pip install \
"${PYTHON_ARGS_FOR_INSTALL[@]}" \
.Would you consider that?
There was a problem hiding this comment.
yeah, can definitely do that
| "-m" | ||
| "pip" | ||
| "install" | ||
| "--no-build-isolation" | ||
| "--no-deps" | ||
| "--config-settings" "rapidsai.disable-cuda=true" |
There was a problem hiding this comment.
| "-m" | |
| "pip" | |
| "install" | |
| "--no-build-isolation" | |
| "--no-deps" | |
| "--config-settings" "rapidsai.disable-cuda=true" | |
| --no-build-isolation | |
| --no-deps | |
| --config-settings="rapidsai.disable-cuda=true" |
Could we put the -m pip install back on each of the install lines? Similar to my suggestion from NVIDIA/raft#2943 (comment)
pip constraints don't support extras, and this dependency is showing up downstream in `cugraph`, preventing us from testing `cugraph` against the stable ABI `pylibcudf` wheels. We're already installing `pyarrow`, so I think we can remove this (temporarily) for testing purposes
This reverts commit aaf3818.
This reverts commit b3edb94.
…testing" This reverts commit d0341a8.
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
|
/merge |
…VIDIA#21329) General testing done in NVIDIA#20974 Opening a new PR to not disrupt any ongoing downstream testing using artifacts from the original draft PRs. Resolves NVIDIA#20974 xref rapidsai/build-planning#42 Ops-Bot-Merge-Barrier: true Authors: - Gil Forsyth (https://github.com/gforsyth) Approvers: - James Lamb (https://github.com/jameslamb) URL: NVIDIA#21329
2 participants
General testing done in #20974
Opening a new PR to not disrupt any ongoing downstream testing using artifacts from the original draft PRs.
Resolves #20974
xref rapidsai/build-planning#42
Ops-Bot-Merge-Barrier: true