Skip to content

move shared code into scripts, re-arrange layers, other small maintenance#356

Merged
rapids-bot[bot] merged 16 commits intomainfrom
use-scripts
Jan 27, 2026
Merged

move shared code into scripts, re-arrange layers, other small maintenance#356
rapids-bot[bot] merged 16 commits intomainfrom
use-scripts

Conversation

@jameslamb
Copy link
Copy Markdown
Member

@jameslamb jameslamb commented Jan 23, 2026

Similar to rapidsai/docker#840, proposes moving more shared code into scripts, to reduce duplication.

Notes for reviewers

Moves shared code into bind-mounted scripts

Using scripts reduces duplication and therefore the risk of unintended inconsistencies.

Using bind mounts instead of ADD / COPY keeps the docker context small and ensures those files only needed at build time don't end up in the built images.

Combines RUN steps into fewer layers

This can result in smaller images via better compression and avoids some per-layer overhead in building, pushing, and pulling.

I tried to do these only in cases where it was very unlikely to change the caching behavior (like combining 2 steps that create static files / directories) or where I felt it notably improved code clarity.

This images are pulled many many many more times than they're built and they're almost all built uncached on CI runnes, so I did not worry too much about the impact on build caching.

Updates examples in docs to CUDA 13.1.0

ref: rapidsai/build-planning#236

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jan 23, 2026
Comment thread README.md
```text
rapidsai/ci-conda:25.10-cuda13.0.2-ubuntu24.04-py3.13
rapidsai/ci-conda:cuda13.0.2-ubuntu24.04-py3.13
rapidsai/ci-conda:25.10-cuda13.1.0-ubuntu24.04-py3.13
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if [[ "${#PACKAGES_TO_INSTALL[@]}" != "0" ]]; then
echo "could not find some necessary packages, temporarily installing them"
if type -f apt-get >/dev/null 2>&1; then
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this form instead of LINUX_VER here because all ci-conda images, regardless of LINUX_VER, start from condaforge/miniforge3. That happens to be Ubuntu 24.04 right now.

Checking for apt-get is more future-proof than parsing out /etc/lsb-release or something, I think.

@jameslamb jameslamb changed the title WIP: [NOT READY FOR REVIEW] move shared code into scripts, re-arrange layers, other small maintenance move shared code into scripts, re-arrange layers, other small maintenance Jan 27, 2026
@jameslamb jameslamb requested a review from gforsyth January 27, 2026 13:19
@jameslamb jameslamb marked this pull request as ready for review January 27, 2026 13:19
@jameslamb jameslamb requested a review from a team as a code owner January 27, 2026 13:19
Copy link
Copy Markdown
Contributor

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, James! I'm a big fan of consolidating all of the configuration and install paths into shared files.

I flagged a few small things -- nothing that will have any impact on execution, so feel free to fix or ignore as you see fit.

Comment thread context/scripts/install-tools Outdated
Comment thread .pre-commit-config.yaml
@@ -1,4 +1,4 @@
# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If pre-commit touches its own config, does it need to bump the copyright date?

I feel like we're on the edge of a horrible paradox.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff looks weird now.

#58 had the same change (updating a hook version) that I had here. And that passed pre-commit.ci without the copyright bump. I am confused 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this passed without the copyright updated: https://results.pre-commit.ci/run/github/504062804/1769455570.c0AHX-kCQ5CvUHagdoIBvg

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But whatever either way, I think we want this. #58 changed this in 2026, the copyright date should expand to include 2026 🤷🏻

Comment thread citestwheel.Dockerfile Outdated
Comment thread citestwheel.Dockerfile
Co-authored-by: Gil Forsyth <gforsyth@users.noreply.github.com>
@jameslamb jameslamb requested a review from gforsyth January 27, 2026 15:13
Copy link
Copy Markdown
Contributor

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jameslamb
Copy link
Copy Markdown
Member Author

Thanks Gil!

@jameslamb
Copy link
Copy Markdown
Member Author

/merge

@rapids-bot rapids-bot Bot merged commit b5439af into main Jan 27, 2026
290 checks passed
@jameslamb jameslamb deleted the use-scripts branch January 27, 2026 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants