-
Notifications
You must be signed in to change notification settings - Fork 16
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
Merge miniforge-cuda into ci-imgs #167
Conversation
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.
Thanks Hyunsu! 🙏
Had a few minor suggestions below
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.
Thanks very much! Overall, this looks good to me I like the approach of sharing a single Dockerfile for ci-conda
and miniforge-cuda
.
Some things I checked, that are working well:
- the matrix in
matrix.yaml
here as of this PR exactly matches https://github.com/rapidsai/miniforge-cuda/blob/main/matrix.yaml - the
Dockerfile
changes here accurately capture the content of https://github.com/rapidsai/miniforge-cuda/blob/main/Dockerfile - the patterns for tags match what's been published at https://hub.docker.com/r/rapidsai/miniforge-cuda/tags (thanks especially for catching the
-base-
forminiforge-cuda
, I might have missed that)
However, I see 2 things that should be changed:
Ensure intermediate images are getting cleaned up in rapidsai/staging:
on DockerHub
I think maybe something is broken with the jobs that delete temporary images?
I still see lots of *-167-*
images at https://hub.docker.com/r/rapidsai/staging/tags?page=&page_size=&ordering=&name=-167-. That 167
corresponds to the PR number here on GitHub.
In theory, I think those should be getting removed by this:
delete-temp-images: |
Could you please look into that?
This PR does not finish the work
This PR shouldn't say it "closes" the work to move miniforge-cuda
into this repo. Once this is merged and a few days have gone by with no issues, there are a few other things I think need to be done to consider this work complete. I just put those up in a task list at rapidsai/build-planning#48.
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 looks fine to me, aside from @jameslamb's comments on intermediate image cleanup and follow-up tasks.
@@ -1,9 +1,17 @@ | |||
# Define the values used for the "latest" tag | |||
conda: | |||
miniforge-cuda: | |||
CUDA_VER: "12.2.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.
We need to update all these to 12.5. But that can be a follow-up PR.
#172 is handling this in the same way as rapidsai/docker#709. |
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.
Changes in this PR look good and we should merge this. But please do see the other follow-up items I mentioned before we consider rapidsai/build-planning#48 complete.
Addresses rapidsai/build-planning#48