-
Notifications
You must be signed in to change notification settings - Fork 226
use 'rapids-init-pip' in wheel CI, other CI changes #1926
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
This comment was marked as resolved.
This comment was marked as resolved.
/ok to test |
In the first commit I pushed, I tried 2 different forms of docs-build:
needs: conda-python-build
secrets: inherit
uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@run-script
with:
build_type: pull-request
node_type: "gpu-l4-latest-1"
arch: "amd64"
container_image: "rapidsai/ci-conda:latest"
run_script: "ci/build_docs.sh"
docs-build-w-script:
needs: conda-python-build
secrets: inherit
uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@run-script
with:
build_type: pull-request
node_type: "gpu-l4-latest-1"
arch: "amd64"
container_image: "rapidsai/ci-conda:latest"
script: "ci/build_docs.sh" Looks like they both worked! https://github.com/rapidsai/rmm/actions/runs/15147030536/job/42585696835?pr=1926 So I'm confident that merging rapidsai/shared-workflows#357 wouldn't break any repos currently using |
/ok to test |
/ok to test |
/ok to test |
# using env variable PIP_CONSTRAINT is necessary to ensure the constraints | ||
# are used when created the isolated build environment | ||
echo "librmm-${RAPIDS_PY_CUDA_SUFFIX} @ file://$(echo "${LIBRMM_WHEELHOUSE}"/librmm_"${RAPIDS_PY_CUDA_SUFFIX}"*.whl)" > ./build-constraints.txt | ||
# Using env variable PIP_CONSTRAINT (initialized by 'rapids-init-pip') is necessary to ensure the constraints |
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.
I think it would be nice if PIP_CONSTRAINT sounded more like a file path. It’s not literally the constraints, but a path to a file containing a list of constraints. That way we would know to append with >>
instead of extending a string, and cat
it instead of echo
it to get the contents. What do you think? That said, I am happy to approve this as-is. If this sparks ideas for you, feel free to adapt the implementation. Perhaps the pip implementation is clear enough about using a file rather than a literal string.
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.
I broadly agree with you, a name like PIP_CONSTRAINT_FILE
would make the usage a bit clearer.
But we're somewhat constrained by pip
... PIP_CONSTRAINT
is its name for this: https://pip.pypa.io/en/latest/cli/pip_install/#cmdoption-c
It's important that an environment variable with literally that name be exported, so that the constraints will also apply to pip
invocations where we can't or don't directly pass --constraint
, like those that happen when setting up the isolated build environment when you use pip wheel
/ python -m build
with build isolation.
feel free to adapt the implementation
I can imagine something like defining another variable PIP_CONSTRAINT_FILE
or RAPIDS_PIP_CONSTRAINT_FILE
like this:
export RAPIDS_PIP_CONSTRAINT_FILE="${PIP_CONSTRAINT}"
but I'm nervous that that'd be a net increase in complexity. For example, we'd have to be careful about keeping that variable and PIP_CONSTRAINT
in sync.
I think we should proceed with just using PIP_CONSTRAINT
for this.
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.
Because we're so close to code freeze and because this discussion is just about ergonomics for library developers (not anything user-facing or a question of correctness), I'm going to merge this.
But happy to continue this conversation and change this pattern in 25.08 if you have other ideas!
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.
Great! Thanks for explaining. No changes needed.
rapids-pip-retry install \ | ||
-v \ | ||
--constraint ./constraints.txt \ | ||
--constraint "${PIP_CONSTRAINT}" \ |
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.
TIL you can pass this twice!
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.
Yep! You can pass --requirement / -r
multiple times too. Really helpful for merging together multiple sources of configuration like this.
/merge |
Description
Proposes a batch of small, mostly-unrelated changes to building and packaging.
These are all part of RAPIDS-wide efforts to make it easier to reproduce CI locally and to use artifacts from one project's CI in another project's CI.
Contributes to rapidsai/build-planning#178
ci/release/update-version.sh
Contributes to rapidsai/shared-workflows#356
script
to workflows using it, instead of relying on a default value coming from the workflow fileContributes to rapidsai/shared-workflows#337
run_script
toscript
in uses of thecustom-job
workflowContributes to rapidsai/build-planning#179
rapids-init-pip
near the beginning of all CI scripts that usepip
Contributes to rapidsai/gha-tools#145
rapids-configure-conda-channels
is not used in any builds scripts usingrattler-build
Notes for Reviewers
Checklist