-
Notifications
You must be signed in to change notification settings - Fork 55
remove reliance on rapidsai/ci-conda, use a versions.yaml #834
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
| compute-matrix: | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: rapidsai/ci-conda:latest |
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.
This is just some string manipulation in bash + jq + yq... I don't think we need to pay the cost of pulling a container image, and I think we can pretty safely just accept whatever jq / yq version GitHub is shipping in its images (ref: https://github.com/actions/runner-images/tree/main/images/ubuntu).
| *.tar.gz | ||
| *.tgz | ||
| *.whl | ||
| *.zip |
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.
Saw .ruff_cache not .gitignore'd and that led me to updating this file which led me to adding a few more things just for extra safety.
Dockerfile
Outdated
| --prefer-binary \ | ||
| --upgrade \ | ||
| 'conda-merge==0.3.*' \ | ||
| 'rapids-dependency-file-generator==1.20.*' |
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.
Pinning these to the version ranges we're currently getting just for a bit of extra safety (since I'm touching the line anyway).
|
I think the I think it's probably this git checkout main
git pull upstream main
git describe --tags --abbrev=0
# v25.12.00bDeleting it should fix this, but I asked RAPIDS Ops offline to confirm before I do that. |
|
good news: removing that bad news: the builds are failing because some libraries were missed in rapidsai/build-planning#233 I'll go update them and come back to this. |
|
The tagging issue and Next, CI's failing because
|
bdice
left a comment
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.
A couple cleanup suggestions.
Dockerfile
Outdated
| ARG RAPIDS_BRANCH="main" | ||
|
|
||
| SHELL ["/bin/bash", "-euo", "pipefail", "-c"] | ||
| ARG YQ_VER=4.49.2 |
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.
Could we make this use a versions.yaml like in ci-imgs? https://github.com/rapidsai/ci-imgs/blob/main/versions.yaml
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.
Sure, I can expand this PR to include that.
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've pushed commits doing this. Spot-checked a few and I think it's working.
ref: https://github.com/rapidsai/docker/actions/runs/20798063149/job/59736606018?pr=834
Dockerfile
Outdated
| RUN <<EOF | ||
| apt-get update | ||
| apt-get install -y --no-install-recommends rsync | ||
| apt-get install \ |
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.
Generally we keep the flags on the first line (it's not that long) and we also avoid indenting the \ characters. Example: https://github.com/rapidsai/ci-imgs/blob/55b24734b7d594e768599771dcae2e718af52f9a/ci-conda.Dockerfile#L208-L215
Let's follow that style here.
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.
Sure, no problem. This style of indenting the \ over is something I've grown used to looking at lots of RAPIDS devcontainers code recently (like this), I kind of like it.
But don't feel strongly at all, I'll change this to match what we do in other Dockerfiles, no problem.
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've updated this in recent commits, and also pulled in the put-packages-in-arrays stuff from rapidsai/ci-imgs#343 / rapidsai/ci-imgs#342 (comment)
|
I think this is ready for another review. |
|
/merge |
|
|
Contributes to #832
Removes reliance on https://hub.docker.com/r/rapidsai/ci-conda in this repo. It was only being used in 2 places, both of which could be run in simpler environments:
compute-matrixjob that just needsbash,jqandyq--> regular oldubuntu-latestGitHub Actions VM is finebash,git,pip, and a Python interpreter -->python:{version}container is fineOther changes based on PR reviewers:
versions.yaml(borrowing the approach from rapidsai/ci-imgs)notset(ref: add CUDA 12.5 images #689 (comment))notsettounsetbecauseunsetis a valid shell commandNotes for Reviewers
This is only the beginning
To be clear, this project still heavily relies on
rapidsai/miniforge-cuda(the base image forrapidsai/ci-conda. So there's more work to do for #832.But this at least means we no longer need to care about anything in these lines breaking the things in this repo 😁 :
https://github.com/rapidsai/ci-imgs/blob/55b24734b7d594e768599771dcae2e718af52f9a/ci-conda.Dockerfile#L183-L313