Skip to content

ci(rocm): build, link and generate on a self-hosted gfx1151 runner - #1989

Merged
inureyes merged 1 commit into
mainfrom
feature/issue-1811-rocm-ci
Sep 27, 2026
Merged

inureyes merged 1 commit into
mainfrom
feature/issue-1811-rocm-ci

Conversation

@inureyes

@inureyes inureyes commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

Refs #1811, #1801.

Nothing in CI compiles the rocm feature at all: the cargo check jobs do not carry it and the CUDA jobs point at NVIDIA runners. Since the ROCm half of every MLX pin bump lives in src/lib/mlx-cpp/patches-rocm/, a bump that breaks the overlay reaches main unseen.

Why build, link and generate rather than just check

Each step catches a class the one before it cannot.

cargo check never invokes the linker. That is how the CUDA link regression in #1274 passed on a tree that could not link a single integration test. A kernel can compile and link while launching nothing, which is the shape the missing primitives in #1825 had, so the job runs a real forward pass. LD_LIBRARY_PATH is unset for that step, because the binary must find the ROCm libraries through the rpath its build script emits, and #1802 shipped without that rpath on two crates whose test binaries then failed to start with libamdhip64.so.7: cannot open shared object file.

The output assertion was hollow at first, and that is worth reading

The first draft counted word-like tokens over the whole log and required eight. It passed on a run that generated nothing, because the diagnostics alone carry 143 of them:

my grep counts: 143 word-like tokens
...but the content channel is EMPTY. Those words are diagnostics:
mlxcel custom kernel backend rocm no custom kernel

Two causes. Merging stderr into stdout interleaves the [mlx-rocm] device lines into the generated text, on the same physical line as the echoed prompt, so no line filter removes them. And a reasoning-capable checkpoint sends every token to the hidden channel on a prompt this short, leaving the content channel legitimately empty on a healthy run.

The step now keeps the streams in separate files, passes --show-reasoning, and counts only the text between the Generating... banner and the summary line. Verified against three logs before it went in: a good run passes, a run whose decode loop finished with no text is rejected, and a log with no ROCm backend line is rejected. It also asserts that no CUDA compute capability is printed, so the #1805 defect cannot come back unnoticed.

The job is parked, not left to queue

The first version of this PR targeted the self-hosted runner unconditionally, and opening it showed why that is wrong. GitHub does not skip a job whose runner is absent, it queues it, for up to 24 hours. While it sits there the entire workflow run stays in progress, so no job in that run has readable logs, and the check reads as "still running" to a reviewer. Every PR touching the HIP overlay would have paid that until a runner appeared.

So rocm-build is gated on vars.ROCM_CI_ENABLED == 'true' as well as the path filter. It is genuinely inert until someone registers the runner and sets the repository variable, at which point one flip activates it with no further change here.

Parking it silently would be its own trap: a reviewer would see no ROCm check at all on an overlay change and reasonably conclude one ran. ROCm CI status always reports which state the gate is in, and carries administration: read so it can say whether the runner is already registered rather than only "cannot tell". It never fails a PR, because whether a runner is attached is an infrastructure fact and not a defect in the change under review.

Documentation

Host requirements for the runner are in this workflow's own comments, next to the job, which is where the GB10 runner's are. docs/installation.md is for building mlxcel, not for operating CI.

It does correct what merged work had already invalidated there. Three status rows: affine MoE no longer needs MLXCEL_FUSED_MOE=0 (#1803), memory estimation on UMA hosts is correct rather than broken (measured in #1805), and the diagnostics report the AMD device rather than a CUDA compute capability (#1883). And three places that still told a reader to set that flag: the documented mlxcel-server command, the troubleshooting entry for the CUDA-backend abort, and the throughput paragraph.

Both flag removals were verified rather than inferred from the merge. mlxcel generate on Qwen3-30B-A3B-4bit runs without it, and the documented scripts/server_chat_smoke.sh passes against a server started without it:

health          200 {"status":"ok"}
models          200 Qwen3-30B-A3B-4bit
chat            200 finish_reason=stop channel=content text='Paris.'
chat stream     200 frames=221 done=True finish_reason=stop channel=content text='...'
OK

The 55 tok/s figure in the throughput paragraph is left as it stands, with its provenance stated, rather than replaced by my own 24-token ad-hoc run at 49 tok/s, which is not a comparable measurement. The troubleshooting entry now names the one case that still aborts this way, the sampler entry points in #1885.

Not done here

Registering the runner needs org admin, which this account does not have: the repository has no self-hosted runners and the GB10 runner is org-level. Two steps activate the gate once a runner exists: register it with the labels self-hosted and rocm-gfx1151, then set the repository variable ROCM_CI_ENABLED to true.

Until then three of the issue's acceptance criteria stay unchecked, namely that a PR touching the overlay builds and generates, that the log shows a GPU run, and that a deliberately broken overlay file fails the job. The fourth, that unrelated PRs do not trigger it, follows from the path filter and is visible on this PR. The issue stays open for those.

@inureyes inureyes added type:chore Maintenance tasks (build, CI, etc.) status:review Under review labels Sep 26, 2026
@inureyes
inureyes force-pushed the feature/issue-1811-rocm-ci branch from fa96ddb to 76724f4 Compare September 26, 2026 14:50
Nothing in CI compiles the `rocm` feature at all: the `cargo check` jobs do not carry it and the CUDA jobs point at NVIDIA runners. Since the ROCm half of every MLX pin bump now lives in `src/lib/mlx-cpp/patches-rocm/`, a bump that breaks the overlay reaches `main` unseen.

The job builds, links and generates, because each step catches a class the one before it cannot. `cargo check` never invokes the linker, which is how the CUDA link regression in #1274 passed on a tree that could not link a single test. A kernel can compile and link while launching nothing, which is the shape the missing primitives in #1825 had. `LD_LIBRARY_PATH` is unset for the generate step so the binary has to find the ROCm libraries through the rpath its build script emits; #1802 shipped without that rpath on two crates and their test binaries failed to start.

The output assertion is written against a measured log. The first draft counted word-like tokens over the whole log and passed on a run that generated nothing, because the diagnostics alone carry 143 of them. Merging stderr into stdout interleaves the device lines into the generated text, and a reasoning-capable checkpoint sends every token to the hidden channel on a prompt this short. The step now keeps the streams apart, passes `--show-reasoning`, and counts only the text between the banner and the summary; verified against a good log, a decode-finished-with-no-text log and a log with no ROCm backend line.

The job is parked behind `vars.ROCM_CI_ENABLED` rather than left to queue. GitHub does not skip a job whose self-hosted runner is absent, it queues it for up to 24 hours, and while it sits there the whole workflow run stays in progress so no job in that run has readable logs. Opening this PR demonstrated exactly that. Parking it means every PR touching the overlay is unaffected until someone registers the runner and flips the variable.

`ROCm CI status` then reports which state the gate is in, because a silently skipped job would let a reviewer see no ROCm check on an overlay change and conclude one ran. It reports the variable and nothing else, and never fails a PR.

Host requirements for the runner stay in this file's own comments, next to the job, which is where the GB10 runner's are. `docs/installation.md` is for building mlxcel, not for operating CI.

It does correct what merged work had already invalidated there. Three status rows: affine MoE no longer needs `MLXCEL_FUSED_MOE=0` (#1803), memory estimation on UMA hosts is correct rather than broken (measured in #1805), and the diagnostics report the AMD device rather than a CUDA compute capability (#1883). Three places that still told a reader to set that flag: the documented `mlxcel-server` command, the troubleshooting entry for the CUDA-backend abort, and the throughput paragraph.

Both flag removals were verified rather than inferred from the merge. `mlxcel generate` on Qwen3-30B-A3B-4bit runs without it, and `scripts/server_chat_smoke.sh` passes against a server started without it, streaming and non-streaming. The 55 tok/s figure is left as it stands with its provenance stated, because the run behind it is not comparable to a short ad-hoc generation. The troubleshooting entry now names the one case that still aborts this way, the sampler entry points in #1885.

Registering the runner needs org admin, so the job has nothing to run on yet and three of the issue's acceptance criteria stay unchecked.

Refs #1811, #1801.
@inureyes
inureyes force-pushed the feature/issue-1811-rocm-ci branch from 76724f4 to 76e4936 Compare September 26, 2026 21:03
@inureyes
inureyes merged commit d1c3df0 into main Sep 27, 2026
16 of 22 checks passed
inureyes added a commit that referenced this pull request Sep 27, 2026
Six path filters listed `.github/workflows/ci.yml`, so editing one job's comment re-ran all six jobs on GB10, the single shared Linux GPU runner that also serves release builds. PR #1989 demonstrated the cost: its diff was one workflow file and one document, GB10 was down, and the run sat queued about six hours without completing, which also made every job's log in that run unreadable and forced a merge past pending checks. The concurrency block already records the contention this runner is under, and `TECHNICAL_REPORTS/1774-ci-concurrency-group-20260911.en.md:74` recorded this same filter coverage during #1774 and worked around it rather than fixing it.

Separately, `cuda_arch` covered `src/lib/mlx-cpp/**`, which contains every backend's overlay, so a change confined to `patches-rocm/` woke both CUDA jobs. Most of epic #1801's pull requests are that shape. The `rocm` filter added in #1989 had the mirror defect.

The reason `ci.yml` sat in every filter was real, but aimed at the wrong failure. A workflow change breaks either by schema, where GitHub rejects the file and runs nothing so re-running heavy jobs catches nothing, or inside a job's own script, where only that job helps. #1989 broke the first way, from `permissions: administration: read`. So the file is dropped from the six filters and replaced by a hosted `actionlint` job plus a `ci:full` label that every self-hosted job now honours for the second case, and an advisory job that names the jobs a workflow change did not re-run, warning rather than failing so the cost does not come back.

Verified rather than assumed, and two checks changed the implementation. `dorny/paths-filter` ignores `!` patterns under its default quantifier, as its own README states, so `predicate-quantifier: 'some-with-excludes'` is set; it is backward compatible because no filter used negation before. And `patches/` is not the Metal overlay despite the name: `patches/mlx/backend/cuda/` holds 24 CUDA sources that `CMakeLists.txt:59-69` applies to CUDA builds, against 4 Metal files, so excluding `patches/**` wholesale would have dropped real CUDA coverage. Only the Metal subtree is excluded.

`actionlint` 1.7.7 reported 7 `runner-label` findings on this repository, enough to make a lint job red on arrival and then ignored. `.github/actionlint.yaml` lists the self-hosted labels instead of suppressing the rule.

That was not the whole baseline. The local run reported zero because this host has no shell linter installed, so the `run:` block checks were silently absent; the hosted runner ships one and the first job run found 21 info and 4 warning findings that predate it, plus 2 errors that were the job's own, from a comment line whose first word was read as a linter directive. The comment is reworded and the job runs at an error severity floor, with the info and warning backlog left as separate work. A negative control confirms the floor is not a hole: the invalid `permissions` key that broke #1989 is still rejected by name.

Filter behavior was simulated per changed-path shape before pushing: a workflow-only change wakes no self-hosted job, `patches-rocm/` wakes only the ROCm gate, `patches/mlx/backend/cuda/` still wakes both CUDA jobs, and `turbo/`, `CMakeLists.txt` and an MLX pin bump wake both backends. Note that `ci.yml` has no macOS job, so a Metal-only overlay change now triggers nothing here; it triggered the two CUDA jobs before, which never compiled it. Per-PR Metal coverage lives in `nightly-verify.yml` and was not affected either way.

Closes #1992.
inureyes added a commit that referenced this pull request Sep 28, 2026
Six path filters listed `.github/workflows/ci.yml`, so editing one job's comment re-ran all six jobs on GB10, the single shared Linux GPU runner that also serves release builds. PR #1989 demonstrated the cost: its diff was one workflow file and one document, GB10 was down, and the run sat queued about six hours without completing, which also made every job's log in that run unreadable and forced a merge past pending checks. The concurrency block already records the contention this runner is under, and `TECHNICAL_REPORTS/1774-ci-concurrency-group-20260911.en.md:74` recorded this same filter coverage during #1774 and worked around it rather than fixing it.

Separately, `cuda_arch` covered `src/lib/mlx-cpp/**`, which contains every backend's overlay, so a change confined to `patches-rocm/` woke both CUDA jobs. Most of epic #1801's pull requests are that shape. The `rocm` filter added in #1989 had the mirror defect.

The reason `ci.yml` sat in every filter was real, but aimed at the wrong failure. A workflow change breaks either by schema, where GitHub rejects the file and runs nothing so re-running heavy jobs catches nothing, or inside a job's own script, where only that job helps. #1989 broke the first way, from `permissions: administration: read`. So the file is dropped from the six filters and replaced by a hosted `actionlint` job plus a `ci:full` label that every self-hosted job now honours for the second case, and an advisory job that names the jobs a workflow change did not re-run, warning rather than failing so the cost does not come back.

Verified rather than assumed, and two checks changed the implementation. `dorny/paths-filter` ignores `!` patterns under its default quantifier, as its own README states, so `predicate-quantifier: 'some-with-excludes'` is set; it is backward compatible because no filter used negation before. And `patches/` is not the Metal overlay despite the name: `patches/mlx/backend/cuda/` holds 24 CUDA sources that `CMakeLists.txt:59-69` applies to CUDA builds, against 4 Metal files, so excluding `patches/**` wholesale would have dropped real CUDA coverage. Only the Metal subtree is excluded.

`actionlint` 1.7.7 reported 7 `runner-label` findings on this repository, enough to make a lint job red on arrival and then ignored. `.github/actionlint.yaml` lists the self-hosted labels instead of suppressing the rule.

That was not the whole baseline. The local run reported zero because this host has no shell linter installed, so the `run:` block checks were silently absent; the hosted runner ships one and the first job run found 21 info and 4 warning findings that predate it, plus 2 errors that were the job's own, from a comment line whose first word was read as a linter directive. The comment is reworded and the job runs at an error severity floor, with the info and warning backlog left as separate work. A negative control confirms the floor is not a hole: the invalid `permissions` key that broke #1989 is still rejected by name.

Filter behavior was simulated per changed-path shape before pushing: a workflow-only change wakes no self-hosted job, `patches-rocm/` wakes only the ROCm gate, `patches/mlx/backend/cuda/` still wakes both CUDA jobs, and `turbo/`, `CMakeLists.txt` and an MLX pin bump wake both backends. Note that `ci.yml` has no macOS job, so a Metal-only overlay change now triggers nothing here; it triggered the two CUDA jobs before, which never compiled it. Per-PR Metal coverage lives in `nightly-verify.yml` and was not affected either way.

Closes #1992.
inureyes added a commit that referenced this pull request Sep 28, 2026
…1993)

Six path filters listed `.github/workflows/ci.yml`, so editing one job's comment re-ran all six jobs on GB10, the single shared Linux GPU runner that also serves release builds. PR #1989 demonstrated the cost: its diff was one workflow file and one document, GB10 was down, and the run sat queued about six hours without completing, which also made every job's log in that run unreadable and forced a merge past pending checks. The concurrency block already records the contention this runner is under, and `TECHNICAL_REPORTS/1774-ci-concurrency-group-20260911.en.md:74` recorded this same filter coverage during #1774 and worked around it rather than fixing it.

Separately, `cuda_arch` covered `src/lib/mlx-cpp/**`, which contains every backend's overlay, so a change confined to `patches-rocm/` woke both CUDA jobs. Most of epic #1801's pull requests are that shape. The `rocm` filter added in #1989 had the mirror defect.

The reason `ci.yml` sat in every filter was real, but aimed at the wrong failure. A workflow change breaks either by schema, where GitHub rejects the file and runs nothing so re-running heavy jobs catches nothing, or inside a job's own script, where only that job helps. #1989 broke the first way, from `permissions: administration: read`. So the file is dropped from the six filters and replaced by a hosted `actionlint` job plus a `ci:full` label that every self-hosted job now honours for the second case, and an advisory job that names the jobs a workflow change did not re-run, warning rather than failing so the cost does not come back.

Verified rather than assumed, and two checks changed the implementation. `dorny/paths-filter` ignores `!` patterns under its default quantifier, as its own README states, so `predicate-quantifier: 'some-with-excludes'` is set; it is backward compatible because no filter used negation before. And `patches/` is not the Metal overlay despite the name: `patches/mlx/backend/cuda/` holds 24 CUDA sources that `CMakeLists.txt:59-69` applies to CUDA builds, against 4 Metal files, so excluding `patches/**` wholesale would have dropped real CUDA coverage. Only the Metal subtree is excluded.

`actionlint` 1.7.7 reported 7 `runner-label` findings on this repository, enough to make a lint job red on arrival and then ignored. `.github/actionlint.yaml` lists the self-hosted labels instead of suppressing the rule.

That was not the whole baseline. The local run reported zero because this host has no shell linter installed, so the `run:` block checks were silently absent; the hosted runner ships one and the first job run found 21 info and 4 warning findings that predate it, plus 2 errors that were the job's own, from a comment line whose first word was read as a linter directive. The comment is reworded and the job runs at an error severity floor, with the info and warning backlog left as separate work. A negative control confirms the floor is not a hole: the invalid `permissions` key that broke #1989 is still rejected by name.

Filter behavior was simulated per changed-path shape before pushing: a workflow-only change wakes no self-hosted job, `patches-rocm/` wakes only the ROCm gate, `patches/mlx/backend/cuda/` still wakes both CUDA jobs, and `turbo/`, `CMakeLists.txt` and an MLX pin bump wake both backends. Note that `ci.yml` has no macOS job, so a Metal-only overlay change now triggers nothing here; it triggered the two CUDA jobs before, which never compiled it. Per-PR Metal coverage lives in `nightly-verify.yml` and was not affected either way.

Closes #1992.
@inureyes
inureyes deleted the feature/issue-1811-rocm-ci branch October 6, 2026 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:review Under review type:chore Maintenance tasks (build, CI, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant