docker: pin ordering-edge HIP/HSA in native and nightly release images

[ZhangLirong-amd]

ROCm 7.2.4 base images ship HIP/HSA without ordering-edge signals. Build the paired runtime from ROCm/rocm-systems commit b539bf7eebfd99ad0a69668caa1f4037034d501f, following the backport used by vllm-project/vllm#55099, and replace the libraries in native ATOM images.

Both docker/Dockerfile and docker/atom_release.dockerfile include the same inline builder and final replacement. The latter is the path used by the nightly Docker Release workflow. Build ROCr first, then HIP/CLR against its headers; install the pair after all dependencies. Preserve old versioned library paths with hard links, copying up the source files before linking so Docker layers retain alias identity. Record the source commit in /opt/rocm/.info/atom-rocm-runtime-commit. Non-7.2.4 bases keep their stock runtimes. Future updates change the commit pin; rollback uses the previous image.

Validation:

• Confirmed nightly run 35487175387 checked out 3a6ccd8b51d404e5c41635c1ad9f0a676a1c4e4b and built docker/atom_release.dockerfile. That image retained stock runtimes because this Dockerfile lacked the replacement steps.
• Parsed the actual release Dockerfile with BuildKit and checked that it includes the runtime stage and default atom_image target. Verified both Dockerfiles have identical runtime build/replacement blocks, valid shell syntax, and a clean git diff --check.
• Built the exact final COPY/replacement block on the reported nightly image using previously compiled libraries from the pinned commit, with CPU quota limited to 0.25 core. Verified the commit marker, both artifact hashes, hsa_amd_signal_create_v2, paired library loading, and inode plus RTLD_NOLOAD identity through all original versioned filenames.
• Executed the extracted build/install guards for ROCm 10.0.0, 7.2.40, 7.2.3, and a missing version file; all skipped without changing runtime hashes or writing a marker.

The inline source builder and complete native/OOT/SGLang images were not rebuilt in this validation. No GPU APIs or GPU workloads were run for these checks.

[github-actions]

🏷️ CI Guide

Runs automatically on every eligible PR before approval:

• ✅ Pre Checkin: Black, Ruff, catalog schema validation, non-GPU unit tests

Heavy model tests:

• ✅ Run after the PR is approved and Pre Checkin passes
• ✅ Run immediately when an approval review is submitted
• ✅ Can be requested before approval with labels

Label	Tests
ci:full	Run all heavy PR model tests: native ATOM, vLLM, and SGLang
ci:atom	Run native ATOM model accuracy tests
ci:vllm	Run ATOM vLLM OOT model accuracy tests
ci:sglang	Run ATOM SGLang model accuracy tests

Heavy jobs are skipped when the PR is not approved and no matching ci:* label is present.
Add labels via the sidebar or gh pr edit 2300 --add-label <label>

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical release-path and runtime-build blockers remain, plus a status-reporting issue.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Release workflow bypasses the ROCm runtime integration · New
• Missing OpenGL development packages break rocclr configuration · New

What changed in this PR

Adds a ROCm 7.2.4 ordering-edge HIP/HSA runtime with switching, rollback, Docker integration, documentation, and tests.

Changes:

• Builds the pinned runtime pair conditionally.
• Adds checksum-backed stock/patched switching and rollback.
• Documents usage and validates switching behavior.

File	Reviewed changes
tests/​test_rocm_runtime_switch.py	Tests runtime switching, rollback, aliases, and failure cases.
docker/​rocm-runtime/​switch_runtime.py	Implements paired runtime selection and backups; status handling lacks a marker for disabled stock images.
docker/​rocm-runtime/​README.md	Documents builds, switching, rollback, and validation.
docker/​rocm-runtime/​build.sh	Builds the backport; lacks required OpenGL development packages for rocclr.
docker/​Dockerfile	Adds runtime stages and reselection; release Docker paths remain unintegrated.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Release integration, build-argument propagation, and runtime switching have unresolved issues blocking approval.

Review effort: Lite
Findings: 2

Open (2)

• Missing OpenGL development packages break rocclr configuration
• Release workflow bypasses the ROCm runtime integration

[Copilot]

Copilot review overview

🟡 Changes recommended

Apply the runtime changes to the release Dockerfile or update the workflow to use this file.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Apply native-image backport to the Dockerfile used by the release workflow · New
• Release workflow bypasses the ROCm runtime integration

Resolved since last review (1)

• Missing OpenGL development packages break rocclr configuration

[Copilot]

Copilot review overview

🟡 Changes recommended

A real BuildKit build of each runtime stage and default final target is still required.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 2

Open (3)

• Release workflow bypasses the ROCm runtime integration
• Run a complete ROCm 7.2.4 BuildKit validation · New
• Run a complete ROCm 7.2.4 BuildKit validation · New

Resolved since last review (1)

• Apply native-image backport to the Dockerfile used by the release workflow

[valarLip]

Review — pin ordering-edge HIP/HSA in native and nightly release images

Reviewed at 5a3f14b13. The problem is real and the replacement method is the right one — hard links over /opt/rocm/lib is what upstream vLLM does, and I checked that torch does not bundle its own HIP/HSA (RPATH $ORIGIN, NEEDED libamdhip64.so.7, no copies under /opt/venv), so replacing the ROCm copy genuinely takes effect on the rocm7 path.

What concerns me is the shape around it, and it has one clean statement:

This PR exists because a nightly shipped stock runtimes and nobody noticed. Every failure path in it ends in exit 0, and nothing anywhere checks that the swap landed.

Provenance: [verified] means I read the deciding code or ran the command myself. [reported] means the shape matches the code but I did not trace it end to end.

---

1. The safety net was in the first commit and was deleted before submission [verified]

52fc33f13  "docker: enable ROCm 7.2.4 ordering-edge runtime with rollback"
  docker/rocm-runtime/README.md           +93
  docker/rocm-runtime/build.sh            +83
  docker/rocm-runtime/switch_runtime.py  +107
  tests/test_rocm_runtime_switch.py      +101

3a6ccd8b5 removes all four. With them went the ROCM_ORDERING_EDGE knob, whose deleted README described exactly the mode that is now missing:

"Set it to 0 to build with stock libraries; 1 requires a 7.2.4 base and fails on an unsupported version."

Only the silent auto branch survives, so there is no way to say "this build must carry the backport". The nightly release workflow has no mode in which a non-7.2.4 base is an error rather than a silent downgrade.

It also means the submitted version has no test at all, where the first draft had 101 lines of one.

2. Nothing verifies the swap, and the marker that could is never read [verified]

Both guards fail open:

if ! grep -Eq '^7\.2\.4([+-]|$)' /opt/rocm/.info/version; then exit 0; fi

(The regex itself is correct — it matches 7.2.4, 7.2.4-42, 7.2.4+123 and rejects 7.2.40; I checked.)

An empty /staging, a soname mismatch (§3), a clobbered /opt/rocm (§4), or a later apt revert (§6) all end at exit 0 with a successfully published image carrying the stock runtime.

The stage already writes the thing that would catch it:

git rev-parse HEAD > /staging/.info/atom-rocm-runtime-commit

grep -rn atom-rocm-runtime-commit over the repo returns two write sites and zero readers. And every in-image smoke test (LMCache import, atomesh --version, aiperf --help) runs before the COPY at line 826, so they exercise the old libraries only.

Smallest fix that closes the whole class — after ldconfig, under the same 7.2.4 condition:

test -s /opt/rocm/.info/atom-rocm-runtime-commit
nm -D /opt/rocm/lib/libhsa-runtime64.so.1 | grep -q hsa_amd_signal_create_v2

3. The staging half and the fixup half disagree about the soname major [verified]

# staging (:331-332) — major-agnostic
cp -P /rocr-install/lib/libhsa-runtime64.so* /staging/lib/
cp -P /clr-install/lib/libamdhip64.so*       /staging/lib/

# fixup (:831-832) — hardcoded
ours_hsa="$(readlink -f libhsa-runtime64.so.1)"
ours_hip="$(readlink -f libamdhip64.so.7)"

The comment at :824 says "changing ROCM_RUNTIME_COMMIT selects a different backport", so the pin is expected to move. If a future commit builds libamdhip64.so.8, the cp succeeds under the new name while readlink -f libamdhip64.so.7 resolves to the still-present stock symlink. The loop then hard-links stock onto stock, ldconfig keeps the stock soname resolvable, and the build reports success having done nothing — with a fresh mtime from the touch making it look patched under ls -l.

Derive the majors from the staged filenames or from the ELF SONAME. Same at docker/Dockerfile:622.

Related, and the reason the touch deserves a guard [reported]: readlink -f prints a would-be path and exits 0 for a dangling target, so if the COPY did not deliver the symlink, touch "$ours_hip" creates it as an empty regular file and the loop hard-links that 0-byte file over the real HIP runtime. set -eux cannot catch it. A [ -L libamdhip64.so.7 ] && [ -s "$ours_hip" ] || exit 1 before the touch would. (The touch itself is necessary — it forces the overlayfs copy-up that lets the layer tar record the hard links as aliases — which is worth a comment, since it is the one non-obvious line in the block and is explained only in the PR body.)

4. COPY is unconditional, and /opt/rocm is a symlink [verified premise, unverified consequence]

COPY --from=build_rocm_runtime /staging/ /opt/rocm/    # unconditional
RUN ... if ! grep -Eq '^7\.2\.4...' ...; then exit 0; fi   # gated

mkdir -p /staging/lib at :296 runs before the builder's exit 0, so on a skipped flavor /staging is {lib/} and the COPY still writes it.

On this box /opt/rocm is a two-hop alternatives-managed chain:

/opt/rocm -> /etc/alternatives/rocm -> /opt/rocm-7.1.1

and atom_release.dockerfile:159 creates it explicitly on the rocm10 path (ln -s ${ROCM_HOME} /opt/rocm). Whether BuildKit traverses that chain or materialises the destination as a real directory is not stated anywhere in the PR, and the body says no full image was rebuilt. If it does not traverse, /opt/rocm becomes a tree containing only lib/ and .info/ — and /opt/rocm/{bin,llvm,share,amdgcn} disappear, breaking the consumers this same file names at :156-158 (AITER's amdgpu-arch, RCCL install.sh, Mooncake cmake, /opt/rocm/share/amd_smi). The next RUN then greps a now-missing .info/version, ! grep is true, and it exit 0s.

I could not verify BuildKit's behaviour without a build. Given the blast radius and that the riskiest write in the PR runs on the flavors the gate says to skip, it is worth either confirming it on a real build or gating the COPY with the same condition.

5. docker/Dockerfile is a hand-maintained copy that CI never builds [verified]

grep -rn "docker/Dockerfile" .github/ returns nothing. Every CI path uses -f docker/atom_release.dockerfile or the vllm/sglang release files. That file also keys the 7.2.4 gate to a floating base:

ARG BASE_IMAGE="rocm/pytorch:latest"      # docker/Dockerfile:2

versus the sha256 digest pin at atom_release.dockerfile:3. When latest moves off 7.2.4 the feature vanishes with no marker and a skip message BuildKit hides on a cache hit — and nothing ever builds it to find out. Either pin its base or drop the block from it.

This is also the duplication problem in general: the ~63-line stage and the ~18-line epilogue are byte-identical in two Dockerfiles with no shared mechanism. The stage next door has already drifted this way — build_rccl in atom_release.dockerfile:338-350 gained ARG BUILD_RCCL=1 and the /rccl-pkgs copy-out while docker/Dockerfile:365-379 still carries the old unconditional form.

6. The downstream images revert it, and this repo already documents the mechanism [verified]

$ dpkg -S /opt/rocm-7.1.1/lib/libamdhip64.so.7
hip-runtime-amd: /opt/rocm-7.1.1/lib/libamdhip64.so.7

The files being hard-linked are dpkg-owned, and the hard links make their md5sums mismatch. vllm_release.dockerfile:38 runs apt --fix-broken install -y, which reinstalls them.

The same file spells the mechanism out for RCCL at :20-29:

"The custom dpkg -i --force-all leaves rocm-hip's rccl version dep 'broken' on purpose; the OOT apt --fix-broken install below satisfies it by reinstalling the STOCK ROCm rccl into /opt/rocm, which then shadows the custom build"

— and pays for a full backup (:30-35) and restore (:157-168) because of it. The new HIP/HSA hard links have no equivalent. So rocm/atom-dev:vllm-latest and the SGLang image ship without the backport while the native image has it, with no build-time or CI signal that the two diverge.

---

Smaller, and one suggestion about altitude

• cmake --build ... --parallel 16 is hardcoded in four places, while the same file threads ARG MAX_JOBS into the aiter build (:372, :387) and uses make -j$(nproc) for Mooncake (:552). MAX_JOBS exists because CLR-style C++ builds OOM on constrained runners; 16 also idles most of the 128-core release runner. --parallel ${MAX_JOBS:-$(nproc)}. [verified]
• Provenance is recorded only as a file inside the image. docker-release.yaml:356-368 has a dedicated step stamping com.amd.rocm.version and friends, which hard-fails rather than publish an unlabelled tag — so published images will now carry a hand-built runtime differing from the com.amd.rocm.version they advertise, discoverable only by docker running them. ROCM_RUNTIME_COMMIT is also not a build-arg, so re-rolling the runtime during an incident needs a merge to main. [reported]
• Only libamdhip64.so* is replaced. libhiprtc, libhiprtc-builtins and libamd_comgr are built and versioned by CLR as a matched set, and libtorch_hip.so lists NEEDED libhiprtc.so.7 directly. Upstream vLLM has the same gap, so this is inherited rather than introduced — but a newer libamdhip64 against a stock hiprtc/comgr surfaces as a hiprtcCompileProgram failure that looks like an unrelated JIT bug. Worth a line in the comment saying it was considered. [reported]
• ENV HSA_ENABLE_IPC_MODE_LEGACY=1 at :218 is a workaround for a bug in the very ROCr being replaced (the comment cites [Issue]: hsa_amd_ipc_memory_create permanently pins GPU memory after hipFree (ROCm 7.0+) rocm-libraries#6266). Swapping libhsa-runtime64 changes exactly the code that env var steers; worth confirming the legacy branch is still the right one against the pinned commit. [reported]
• The epilogue comment says "ldconfig cannot redirect a hard link", which inverts upstream's reason — Dockerfile.rock_base:348-349 reads "Hard links rather than symlinks because ldconfig rewrites SONAME symlinks but cannot re-point an inode". A reader cannot derive why hard links are required from the current wording. [verified]
• The new block greps .info/version while the file already sets ENV ATOM_BASE_IMAGE=${BASE_IMAGE} at :210 explicitly "so later stages can branch on 'is this the rocm10 flavor' without guessing", with six existing gates using it. Two gate mechanisms that can disagree on a future 7.3 base. [verified]

On altitude: building this runtime once as a separately published artifact keyed on ROCM_RUNTIME_COMMIT, with both Dockerfiles reduced to a single COPY --from=<pinned-image>, would collapse §2, §3, §5 and the duplication at once — and it is what the :824 comment ("Preserve old image tags for rollback") is already reaching for. That is also roughly what 52fc33f13 was building before it was deleted.

---

Things I checked and am not flagging, so they do not get re-litigated: the ^7\.2\.4([+-]|$) regex is correct; exit 0 in the &&-chained builder RUN does skip the remainder; unmatched globs and the [ -f ] && [ ! -L ] || continue precedence are both fine; -ef is supported by dash; the non-7.2.4 legs do not pay for a source build (the exit 0 lands before apt-get update); torch does not bundle its own HIP/HSA, so the /opt/rocm replacement is effective on rocm7; and libhsakmt needs no pairing (only libhsakmt.a exists, statically linked into ROCr).

Of the above, §2 is the one I would hold on — not because the swap is likely to fail, but because this PR was written in response to a failure that shipped undetected, and it adds no way to detect the same class again. §1 is worth a sentence in the PR body either way: reviewers should know the escape hatch and the test existed and were dropped on purpose.

[Copilot]

Copilot review overview

🟡 Changes recommended

Both Dockerfiles have unresolved moderate issues affecting build parallelism and versioned library replacement.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Redeclare MAX_JOBS in the runtime build stage · New
• Redeclare MAX_JOBS in the runtime build stage · New

Resolved since last review (3)

• Release workflow bypasses the ROCm runtime integration
• Run a complete ROCm 7.2.4 BuildKit validation
• Run a complete ROCm 7.2.4 BuildKit validation

[ZhangLirong-amd]

Replying to the review above.

Thanks — this is a careful read, and the framing is right: this stage exists because a nightly shipped the stock runtime silently, and as submitted it had no way to catch the same class again. I've pushed a628358 addressing §2 and §3. Below is what I changed, what I verified, and where I disagree.

§4 — settled, BuildKit does traverse the symlink chain

You flagged this as the one you couldn't verify without a build, so I ran it.

Your premise is right. On the 7.2.4 image it really is two hops, and the target really does hold the consumers you listed:

/opt/rocm -> /etc/alternatives/rocm -> /opt/rocm-7.2.4
$ ls /opt/rocm/
amdgcn  bin  include  lib  libexec  llvm  share

Minimal reproduction of exactly that shape (docker 29.7.2, buildx v0.36.1):

FROM busybox AS src
RUN mkdir -p /staging/lib && echo newlib > /staging/lib/newfile

FROM busybox
RUN mkdir -p /opt/rocm-7.2.4/lib /opt/rocm-7.2.4/bin /opt/rocm-7.2.4/llvm /etc/alternatives \
 && echo tool > /opt/rocm-7.2.4/bin/amdgpu-arch \
 && ln -s /opt/rocm-7.2.4 /etc/alternatives/rocm \
 && ln -s /etc/alternatives/rocm /opt/rocm
COPY --from=src /staging/ /opt/rocm/

Result:

/opt/rocm                      lrwxrwxrwx -> /etc/alternatives/rocm   (still a symlink)
/opt/rocm/bin/amdgpu-arch      present
payload landed at              /opt/rocm-7.2.4/lib/newfile

bin and llvm survive and the destination is not materialised as a real directory. So the blast radius doesn't happen and the COPY does not need the gate for that reason. Worth noting it's builder-version-dependent, so I'd rather it be covered by the §2 assertions — which now fail closed regardless of how the payload got there — than by a second gate.

§2 — fixed, and the marker now has a reader

You're right that grep -rn atom-rocm-runtime-commit returned two writers and zero readers. The install step now asserts both marker files and ends on the symbol:

hsa="$(grep -m1 '^libhsa-runtime64\.so\.' /opt/rocm/.info/atom-rocm-runtime-sonames)"
nm -D "/opt/rocm/lib/$hsa" | grep -q hsa_amd_signal_create_v2

I used your suggested symbol and confirmed it separates the two cases on real images:

rocm/atom-dev:agentic-dsv4-updated-rocm  ->  00000000000d2700 T hsa_amd_signal_create_v2@@ROCR_1   PASS
stock 7.2.4 image                        ->  (no match)                                            FAIL

nm is already in the final image, so this adds no dependency.

§3 — fixed; dangling-link failure reproduced

The soname major now comes from what the builder actually staged, recorded next to the commit marker:

( cd /staging/lib && ls -1 libhsa-runtime64.so.* libamdhip64.so.* \
    | grep -E '\.so\.[0-9]+$' ) > /staging/.info/atom-rocm-runtime-sonames

On the shipped image the versioned filenames differ: the backport builds libamdhip64.so.7.2.53211-b539bf7 against stock libamdhip64.so.7.2.70204. Both still use SONAME major 7, so this does not reproduce a major-version bump. Recording the staged sonames removes the hardcoded-major assumption; a future major bump would still need consumer ABI compatibility validation.

Your dangling-link point was the more dangerous half, and it reproduces:

$ ln -s does_not_exist.so.7 libfake.so.7
$ readlink -f libfake.so.7 ; echo "rc=$?"
/tmp/rltest/does_not_exist.so.7
rc=0
$ touch "$(readlink -f libfake.so.7)" && ls -la does_not_exist.so.7
-rw-r--r-- 1 root root 0 does_not_exist.so.7

readlink -f exits 0 and prints a would-be path, so the touch creates a 0-byte regular file that the loop then hard-links over the real runtime, and set -eux never sees it. Guarded now:

if [ -L "$soname" ]; then [ -e "$soname" ]; fi
ours="$(readlink -f "$soname")"
[ -s "$ours" ]

I also took your touch comment — it now says it forces the overlayfs copy-up so the layer tar records the hard links as aliases — and rewrote the epilogue comment, which did invert upstream's reason. It now reads: ldconfig rewrites SONAME symlinks but cannot re-point an inode.

For the record, the replacement itself does work end to end on the shipped image:

libamdhip64.so.7 -> libamdhip64.so.7.2.53211-b539bf7
libamdhip64.so.7.2.70204 -ef libamdhip64.so.7   # both links=2, same inode

Smaller items

--parallel 16 now threads ${MAX_JOBS:-$(nproc)} in all four places.

Two I'm noting rather than changing: only libamdhip64.so* is replaced while libhiprtc/libamd_comgr are a matched CLR set — inherited from upstream, and I agree it deserves a line so the next person doesn't debug it as a JIT bug. And HSA_ENABLE_IPC_MODE_LEGACY=1 steers code inside the ROCr being replaced; I haven't re-confirmed the legacy branch against the pinned commit.

§1, §5, §6 — acknowledged, not addressed here

All three are fair and none are fixed by this commit.

§1: the knob and the 101-line test were dropped on purpose to cut the surface area, and you're right that what survived has no way to say "this build must carry the backport". I'll put this in the PR body either way so it isn't rediscovered.

§5: docker/Dockerfile having no CI path and a floating rocm/pytorch:latest base is a real trap, and it's the same duplication the build_rccl drift already shows. Pinning it and dropping the block are both defensible; I'd rather decide that separately than bundle it here.

§6: the downstream package-install steps are a real place where the replacement could be overwritten, and the RCCL backup/restore documents an actual dependency-driven reinstall for RCCL. However, dpkg ownership or an md5 mismatch alone does not establish that apt --fix-broken install will reinstall HIP/HSA. I have not verified the apt transaction and final runtime hashes in both downstream images, so I cannot claim they currently ship stock HIP/HSA. The HIP/HSA links have no equivalent backup/restore or final downstream assertion; that remains a validation gap.

On altitude: publishing the runtime once as its own artifact keyed on ROCM_RUNTIME_COMMIT, with both Dockerfiles reduced to a single COPY --from=<pinned-image>, does collapse §2, §3, §5 and the duplication together, and it is what 52fc33f13 was heading toward. I'd like to do that as a follow-up rather than reopen this PR's scope — the new checks now reject missing markers, missing or empty runtime targets, and an HSA library without the backported symbol when the native 7.2.4 install branch runs. They do not cover a skipped version gate or a later downstream package transaction.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Both Dockerfiles need bounded build parallelism and complete legacy SONAME relinking.

Review effort: Lite
Findings: 2

Open (2)

• Redeclare MAX_JOBS in the runtime build stage
• Redeclare MAX_JOBS in the runtime build stage