refactor(build): build pure Python wheel and noarch: python conda#738
refactor(build): build pure Python wheel and noarch: python conda#738rapids-bot[bot] merged 14 commits intorapidsai:mainfrom
noarch: python conda#738Conversation
|
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. |
jakirkham
left a comment
There was a problem hiding this comment.
Thanks Gil! 🙏
Had a couple minor comments and a question below
ci/build_docs.sh
Outdated
|
|
||
| rapids-logger "Downloading artifacts from previous jobs" | ||
| PYTHON_CHANNEL=$(rapids-download-conda-from-github python) | ||
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name "conda_python" cuxfilter --pure)") |
There was a problem hiding this comment.
I don't think shell lets us nest quotes like this. If we do need to nest quotes, the usual way is through concatenation of strings, which involves alternating between single and double quotes
That said, given other commands similar to this one don't use quotes, maybe we can just drop them
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name "conda_python" cuxfilter --pure)") | |
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name conda_python cuxfilter --pure)") |
There was a problem hiding this comment.
bash doesn't have quoting rules that I like, but they do allow for nesting quotes -- and alternating single and double quotes is dangerous in bash as they have different expansion semantics (specifically that single quotes prevent variable expansion)
ci/build_wheel.sh
Outdated
|
|
||
| ../ci/validate_wheel.sh "${RAPIDS_WHEEL_BLD_OUTPUT_DIR}" | ||
|
|
||
| RAPIDS_PACKAGE_NAME="$(rapids-package-name wheel_python cuxfilter --pure --cuda "$RAPIDS_CUDA_VERSION")" |
There was a problem hiding this comment.
| RAPIDS_PACKAGE_NAME="$(rapids-package-name wheel_python cuxfilter --pure --cuda "$RAPIDS_CUDA_VERSION")" | |
| RAPIDS_PACKAGE_NAME="$(rapids-package-name wheel_python cuxfilter --pure --cuda ${RAPIDS_CUDA_VERSION})" |
There was a problem hiding this comment.
I think that's a shellcheck smell -- you always quote around variable expansions to prevent unexpected word-splitting or globbing
ci/test_notebooks.sh
Outdated
|
|
||
| rapids-logger "Downloading artifacts from previous jobs" | ||
| PYTHON_CHANNEL=$(rapids-download-conda-from-github python) | ||
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name "conda_python" cuxfilter --pure)") |
There was a problem hiding this comment.
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name "conda_python" cuxfilter --pure)") | |
| PYTHON_CHANNEL=$(rapids-download-from-github $(rapids-package-name conda_python cuxfilter --pure)") |
ci/test_python.sh
Outdated
|
|
||
| rapids-logger "Downloading artifacts from previous jobs" | ||
| PYTHON_CHANNEL=$(rapids-download-conda-from-github python) | ||
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name "conda_python" cuxfilter --pure)") |
There was a problem hiding this comment.
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name "conda_python" cuxfilter --pure)") | |
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name conda_python cuxfilter --pure)") |
ci/test_wheel.sh
Outdated
|
|
||
| RAPIDS_PY_CUDA_SUFFIX="$(rapids-wheel-ctk-name-gen "${RAPIDS_CUDA_VERSION}")" | ||
| CUXFILTER_WHEELHOUSE=$(RAPIDS_PY_WHEEL_NAME="cuxfilter_${RAPIDS_PY_CUDA_SUFFIX}" RAPIDS_PY_WHEEL_PURE="1" rapids-download-wheels-from-github python) | ||
| CUXFILTER_WHEELHOUSE=$(rapids-download-from-github "$(rapids-package-name "wheel_python" cuxfilter --pure --cuda "$RAPIDS_CUDA_VERSION")") |
There was a problem hiding this comment.
| CUXFILTER_WHEELHOUSE=$(rapids-download-from-github "$(rapids-package-name "wheel_python" cuxfilter --pure --cuda "$RAPIDS_CUDA_VERSION")") | |
| CUXFILTER_WHEELHOUSE=$(rapids-download-from-github "$(rapids-package-name wheel_python cuxfilter --pure --cuda ${RAPIDS_CUDA_VERSION})") |
.github/workflows/build.yaml
Outdated
| date: ${{ inputs.date }} | ||
| script: ci/build_python.sh | ||
| sha: ${{ inputs.sha }} | ||
| pure-conda: true |
There was a problem hiding this comment.
Do we need something similar in the case of wheels?
There was a problem hiding this comment.
There is already a pure-wheel: true input for wheels -- that's been changed to handle matrix filtering upstream.
|
This is ready for another look @jakirkham |
jameslamb
left a comment
There was a problem hiding this comment.
It does look to me like this is achieving what we want (only 2 conda builds and 2 wheel builds, 1 per CUDA major) and the package metadata looks correct.
But I left a couple of suggestions on the implementation, enough that I think it's worth blocking the PR.
|
|
||
| rapids-logger "Downloading artifacts from previous jobs" | ||
| PYTHON_CHANNEL=$(rapids-download-conda-from-github python) | ||
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-package-name conda_python cuxfilter --pure --cuda "${RAPIDS_CUDA_VERSION}")") |
There was a problem hiding this comment.
I left a suggestion about this on the upstream issue... I think we'll need to update the RAPIDS developer docs to account for this new need to invoke rapids-package-name:
|
|
||
| RAPIDS_PACKAGE_NAME="$(rapids-package-name wheel_python cuxfilter --pure --cuda "${RAPIDS_CUDA_VERSION}")" | ||
| export RAPIDS_PACKAGE_NAME |
There was a problem hiding this comment.
| RAPIDS_PACKAGE_NAME="$(rapids-package-name wheel_python cuxfilter --pure --cuda "${RAPIDS_CUDA_VERSION}")" | |
| export RAPIDS_PACKAGE_NAME |
Was this left over from an earlier iteration?
It's at the end of this script that isn't called by anything else, so I don't think it'll actually affect anything.
There was a problem hiding this comment.
No, that's required -- the conda-python-build shared workflow looks for that environment variable and if it is set it uses that as the artifact name when uploading.
There was a problem hiding this comment.
This is build_wheel.sh, I don't think it's involved in the conda build, right?
There was a problem hiding this comment.
Oops, missed the filename. The same pattern also applies for wheels-build.yaml
There was a problem hiding this comment.
Oh I see, it's setting it for the benefit of this:
To be used as input in the upload step:
It wasn't obvious to me that this was modifying something in the environment for the next shared-workflows step to pick up. But I understand how that's working now.
Co-authored-by: James Lamb <jaylamb20@gmail.com>
jameslamb
left a comment
There was a problem hiding this comment.
No other comments from me, very excited we're improving the handling of libraries like this! Thanks for the explanations.
|
/merge |
xref rapidsai/build-planning#43
waiting on rapidsai/shared-workflows#441