-
Notifications
You must be signed in to change notification settings - Fork 56
remove reliance on rapidsai/ci-conda #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
base: main
Are you sure you want to change the base?
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.
| --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.
| 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.
| 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.
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 fineNotes for Reviewers
This was the easy part
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