CI: Create Dockerfiles for running NIXL ontop of ROCm#1936
Conversation
|
👋 Hi riley-dixon! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
📝 WalkthroughWalkthroughAdds ROCm base and development Docker images, parallel dependency build stages, non-root runtime assembly, ROCm CI compilation, and AMD GPU detection. ChangesROCm build stack
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant ROCmBaseImage
participant DockerfileRocm
participant BuildRocm
participant NIXL
ROCmBaseImage->>DockerfileRocm: Supply ROCm runtime and development packages
DockerfileRocm->>BuildRocm: Provide dependency and toolchain environment
BuildRocm->>NIXL: Build and install NIXL and nixlbench with ROCm enabled
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ci/dockerfiles/Dockerfile.rocm:
- Around line 555-564: Restrict the sudoers entry created in the user setup RUN
block to only the specific elevation command(s) required by the image, following
the base image’s $SUDO pattern, rather than granting "${LOGIN}" unrestricted
passwordless access with ALL=(ALL) NOPASSWD: ALL. Keep the sudoers file
ownership and permissions unchanged.
- Line 186: Correct the spelling in the ROCm pre-release comment from
“targetting” to “targeting” so codespell and pre-commit checks pass.
- Line 558: Update the useradd invocation in the Dockerfile to include the -l
option alongside the existing high UID configuration, preventing creation of the
oversized lastlog file while preserving the current user setup.
- Line 501: Update the ARCH build-argument handling in both Dockerfile.rocm
stages so direct arm64 builds use the host architecture, preferably by deriving
it from uname -m as in .gitlab/build-rocm.sh; otherwise document the required
--build-arg ARCH value in the build instructions. Ensure the selected ARCH
controls both the Rust installer and lib/${ARCH}-linux-gnu paths.
- Around line 467-489: Update the UCX configure invocation in the ucx-build
stage to source the existing ROCm build environment and pass --with-rocm using
ROCM_INSTALL_PATH. Preserve the remaining configure options and build flow so
UCX is compiled with ROCm GPU-memory support.
In @.ci/dockerfiles/Dockerfile.rocm.base:
- Around line 16-17: Remove the stale build-context comment referencing the
`AMDRootCA.crt` `COPY`; retain the `ROCM_VERSION` image-tagging note because it
still documents the `Dockerfile.rocm` base-image pinning behavior.
- Around line 63-65: Update the ROCm apt source configuration in the
Dockerfile’s repository setup to remove trusted=yes and configure the
AMD-published ROCm GPG key via a keyring and signed-by= option. Preserve the
existing repository URL and architecture while ensuring apt validates package
signatures.
In @.ci/scripts/common.sh:
- Around line 82-83: Update the GPU detection logic in the amd-smi branch of the
common detection flow to use amd-smi’s structured device query rather than
grepping plain static output. Ensure AMD detection is attempted when nvidia-smi
is present but does not detect a usable NVIDIA GPU, instead of being skipped
solely because the command exists, while preserving HAS_GPU as a boolean result.
In @.gitlab/build-rocm.sh:
- Around line 379-381: Remove the unused UCCL_COMMIT_SHA assignment from the
ROCm build script, leaving the existing comments that document UCCL being
skipped for ROCm unchanged.
- Line 175: Remove the trailing spaces from the blank line in the build-rocm.sh
script, leaving the line empty so the trailing-whitespace check passes.
- Around line 2-3: Update the SPDX-FileCopyrightText header in build-rocm.sh to
use the repository-accepted NVIDIA-formatted copyright pattern instead of the
current Advanced Micro Devices, Inc. text, while preserving the existing
SPDX-License-Identifier line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 959f5a0c-3998-4638-a801-560616017e1e
📒 Files selected for processing (4)
.ci/dockerfiles/Dockerfile.rocm.ci/dockerfiles/Dockerfile.rocm.base.ci/scripts/common.sh.gitlab/build-rocm.sh
| RUN if ! getent group "${_GID}" > /dev/null 2>&1; then \ | ||
| groupadd -g "${_GID}" "${_GROUP}"; \ | ||
| fi && \ | ||
| useradd -u "${_UID}" -g "${_GID}" -m -s /bin/bash "${_LOGIN}" && \ | ||
| mkdir -p "${_HOME}" && \ | ||
| chown -R "${_UID}":"${_GID}" "${_HOME}" && \ | ||
| mkdir -p /etc/sudoers.d && \ | ||
| echo "${_LOGIN} ALL=(ALL) NOPASSWD: ALL" > /etc/sudoers.d/${_LOGIN} && \ | ||
| chmod 440 /etc/sudoers.d/${_LOGIN} && \ | ||
| chown root:root /etc/sudoers.d/${_LOGIN} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚖️ Poor tradeoff
Passwordless, unrestricted sudo for the "non-root" runtime user.
svc-nixl ALL=(ALL) NOPASSWD: ALL grants the runtime user root-equivalent privileges without a password, which largely negates the security benefit of running as a non-root user. If this is only meant for CI/dev convenience, consider scoping the sudoers rule to the specific commands that actually need elevation (e.g., pip install --break-system-packages per the base image's $SUDO pattern) instead of ALL.
🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 555-555: useradd without flag -l and high UID will result in excessively large Image.
(DL3046)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ci/dockerfiles/Dockerfile.rocm around lines 555 - 564, Restrict the sudoers
entry created in the user setup RUN block to only the specific elevation
command(s) required by the image, following the base image’s $SUDO pattern,
rather than granting "${LOGIN}" unrestricted passwordless access with ALL=(ALL)
NOPASSWD: ALL. Keep the sudoers file ownership and permissions unchanged.
There was a problem hiding this comment.
This is based off of the existing Dockerfile.base which currently does these exact steps.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i "Dockerfile.base" .ci/dockerfiles
echo "---"
rg -n "sudoers|NOPASSWD|useradd" .ci/dockerfiles/Dockerfile.base 2>/dev/nullLength of output: 599
Confirmed — Dockerfile.base (Line 62) already uses the identical ${_LOGIN} ALL=(ALL) NOPASSWD: ALL pattern, so this is existing precedent rather than something new introduced in Dockerfile.rocm.
@riley-dixon Thanks for confirming — that checks out against .ci/dockerfiles/Dockerfile.base Line 62, which uses the exact same pattern. Since this PR is intentionally mirroring the existing base image's setup for consistency, I won't press further on this here. If you'd ever like to tighten the sudoers scope, it'd make sense to do it consistently across both Dockerfiles rather than diverging in this one.
✏️ Learnings added
Learnt from: riley-dixon
Repo: ai-dynamo/nixl PR: 1936
File: .ci/dockerfiles/Dockerfile.rocm:555-564
Timestamp: 2026-07-13T23:01:30.809Z
Learning: In the ai-dynamo/nixl repository, `.ci/dockerfiles/Dockerfile.rocm`'s `final` stage user/sudo setup (creating `svc-nixl` with `ALL=(ALL) NOPASSWD: ALL` sudoers access) intentionally mirrors the existing pattern in `.ci/dockerfiles/Dockerfile.base` (Line 61-64). This is established precedent in the codebase, not a new security regression introduced by the ROCm Dockerfile.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # SPDX-FileCopyrightText: Copyright (c) Advanced Micro Devices, Inc. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the copyright/SPDX check config and its expected pattern.
fd -aH -t f -i copyright .github .ci 2>/dev/null
rg -nH -i 'SPDX-FileCopyrightText|copyright' .github 2>/dev/null | head -n 40
# Show a passing header from an existing script for comparison
fd -aH 'build.sh' .gitlab 2>/dev/null -x sed -n '1,6p' {}Repository: ai-dynamo/nixl
Length of output: 2266
Update the SPDX copyright line .gitlab/build-rocm.sh uses Advanced Micro Devices, Inc. here, but the repo’s copyright hook only accepts the NVIDIA-formatted SPDX-FileCopyrightText pattern. Align this header with the repo-accepted format before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitlab/build-rocm.sh around lines 2 - 3, Update the SPDX-FileCopyrightText
header in build-rocm.sh to use the repository-accepted NVIDIA-formatted
copyright pattern instead of the current Advanced Micro Devices, Inc. text,
while preserving the existing SPDX-License-Identifier line.
Source: Pipeline failures
riley-dixon
force-pushed
the
feat/rocm-dockerfile
branch
from
d710496 to
bfa1aa1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitlab/build-rocm.sh:
- Around line 79-82: Define LIBFABRIC_INSTALL_DIR with the default INSTALL_DIR
value before the LD_LIBRARY_PATH, CPATH, PATH, and PKG_CONFIG_PATH export block
in the build script. Ensure these exports and any later libfabric_path usage
receive the resolved directory when PRE_INSTALLED_ENV skips other assignment
logic.
- Around line 387-398: Update the UCX configure invocation in the build script
to include the ROCm installation path via a --with-rocm option using
ROCM_INSTALL_PATH, alongside the existing verbs, dm, gdrcopy, and
UCX_CUDA_BUILD_ARGS options. Preserve the remaining configure flags unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 72f12789-4d03-4772-8509-edb645132af4
📒 Files selected for processing (4)
.ci/dockerfiles/Dockerfile.rocm.ci/dockerfiles/Dockerfile.rocm.base.ci/scripts/common.sh.gitlab/build-rocm.sh
The base Dockerfile creates a minimal install of ROCm, along with a few other pre-requisites before the build image takes over. This may be replaced once a sufficiently new official ROCm docker image is available. Dockerfile.rocm is based on the existing build.sh script used by NIXL for building the NIXL environment. Instead of running in a script, the source dependency build & install steps are extracted into a multi-stage Docker build process to streamline the overall build. A build-rocm.sh is a copy of build.sh that is slightly modified to build NIXL with ROCm support that may be used by existing tools. Signed-off-by: Riley Dixon <riley.dixon@amd.com>
riley-dixon
force-pushed
the
feat/rocm-dockerfile
branch
from
8d11616 to
a473bc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ci/dockerfiles/Dockerfile.rocm:
- Around line 583-602: Update the runtime PATH definition near ENV NVM_DIR to
include the nvm versioned bin directory under ${NVM_DIR}/versions/node, ensuring
nvm-installed node, npm, and azurite binaries are directly executable without
sourcing nvm.
In @.ci/dockerfiles/Dockerfile.rocm.base:
- Around line 62-66: Update the ROCm repository setup RUN command to create
/etc/apt/keyrings before the gpg --dearmor redirect writes amdrocm.gpg. Keep the
existing key download and repository configuration unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: aa1db8d2-7cac-4a85-84c2-fc031485c18b
📒 Files selected for processing (4)
.ci/dockerfiles/Dockerfile.rocm.ci/dockerfiles/Dockerfile.rocm.base.ci/scripts/common.sh.gitlab/build-rocm.sh
|
@ Reviewers The copyright check is failing on I noticed the check already exempts Could you please clarify how you'd like external-contributor copyright attribution handled here — e.g. extending the existing directory exemption to |
1 participant
The build process introduced in this PR is based on the change introduced by #1900 .
What?
Adds 2 Dockerfiles:
Adds build-rocm.sh, a copy of build.sh that is slightly modified to build NIXL with ROCm support that may be used by existing tools.
Edits common.sh, tests if nvidia-smi is present prior to use to avoid emitting an error message. Provides a fallback to try amd-smi if nvidia-smi is not present.
Why?
This provides initial support for building & running NIXL in a ROCm environment.
How?
Building the base image:
Building the NIXL ROCm image:
Running the NIXL ROCm image:
Building NIXL from within the container using build-rocm.sh:
Summary by CodeRabbit