ci: ship test-backend-ops in the Vulkan and ROCm prebuilts - #146
ci: ship test-backend-ops in the Vulkan and ROCm prebuilts#146danielhanchen wants to merge 2 commits into
Conversation
When someone reports garbled output on hardware we do not have, we currently have no way to ask them which kernel is at fault. Our bundles carry 23 executables and none of them can answer that question. test-backend-ops checks every backend op against the CPU reference and names the op, the quant type and the shape that disagrees. That is the difference between "Qwen breaks on my 8060S" and "MUL_MAT with IQ4_NL fails on gfx1151", which is the level a bisect or an upstream issue actually needs. Upstream's own releases include it; ours configure with -DLLAMA_BUILD_TESTS=OFF, so it is never built. Best-effort on all four legs: reconfigure the existing build directory with tests on, build the single target, and never fail the job over a diagnostic. The bundle steps tar all of build/bin, so it ships with no packaging change. Same shape as the existing DiffusionGemma steps. The Windows ROCm leg is shell: cmd, which has no errexit, so the job's status is whatever the last command returned. Appending anything after the main build would have masked a build failure. Gated with an explicit exit /b 1 on the real build before the diagnostic runs.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3af831bf77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # is currently no way to ask them which kernel is at fault. Best-effort: | ||
| # the main bundle must never fail over a diagnostic. | ||
| if cmake .. -DLLAMA_BUILD_TESTS=ON > /dev/null 2>&1; then | ||
| if cmake --build . -j "$(nproc)" --target test-backend-ops; then |
There was a problem hiding this comment.
When a user extracts a Linux ROCm bundle and invokes test-backend-ops normally, the executable cannot locate its bundled shared libraries: this target links against shared llama-common/llama libraries and CMake gives build-tree binaries an absolute runner-build RPATH, while the later Set RPATH for portable distribution step only patches *.so* and llama-* (unsloth-prebuilt-rocm.yml:850-856). Because test-backend-ops matches neither pattern, every Linux ROCm artifact ships the new diagnostic with a stale build-path RPATH unless the user manually sets LD_LIBRARY_PATH; include this executable in that patching loop or configure it with an $ORIGIN RPATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it defeats the point of the PR: the loop globs *.so* and llama-*, test-backend-ops matches neither, so it would have shipped with the runner's build-tree RPATH and not started on a user's machine. A diagnostic nobody can launch is worse than no diagnostic, because it looks like it is there.
Fixed by adding it to the patchelf loop rather than configuring a separate $ORIGIN RPATH, which keeps it consistent with how everything else in that bundle is treated.
The Vulkan legs turn out to be unaffected: they configure with CMAKE_BUILD_WITH_INSTALL_RPATH=ON and CMAKE_INSTALL_RPATH='$ORIGIN', so every binary from that tree already carries it, and Windows has no RPATH at all. So this was Linux ROCm only.
The portable-RPATH loop globs *.so* and llama-*, and test-backend-ops matches neither. It links the same shared llama/ggml libraries, so it would have shipped with the runner's build-tree RPATH and failed to start on a user's machine unless they set LD_LIBRARY_PATH by hand. That is the one thing the binary exists to do, so this was a shipping bug rather than a cosmetic one. Caught by Codex review. The Vulkan legs are unaffected: they configure with CMAKE_BUILD_WITH_INSTALL_RPATH=ON and CMAKE_INSTALL_RPATH='$ORIGIN', so every binary from that tree already carries it. Windows has no RPATH; DLLs load from the bundle directory.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Overview
Builds
test-backend-opsin the Vulkan and ROCm prebuilts so it ships in the bundles. Four legs: Linux and Windows on each workflow. Best-effort everywhere.Why
When someone reports garbled output on hardware we do not have, we currently cannot ask them which kernel is at fault. Our Windows Vulkan zip carries 23 executables and not one of them can answer that question, because both workflows configure with
-DLLAMA_BUILD_TESTS=OFF.test-backend-opschecks every backend op against the CPU reference and names the op, the quant type and the shape that disagrees. That is the difference between "Qwen breaks on my 8060S" and "MUL_MATwithIQ4_NLfails on gfx1151", which is the level a bisect or an upstream issue actually needs.This is not hypothetical. Right now we have open reports where the only distinguishing feature we can find between a file that works and a file that crashes is the presence of a rare quant type, and no way to test that claim on the reporter's device. Upstream's own release tarballs include the binary; we are the ones dropping it.
What changed
Each leg reconfigures the existing build directory with
-DLLAMA_BUILD_TESTS=ONand builds the single target, so it is incremental rather than a second build. The bundle steps already tar all ofbuild/bin, so no packaging change is needed. Same shape as the existing DiffusionGemma steps, including the rule that the main bundle is never failed over a diagnostic.One thing worth flagging in review. The Windows ROCm leg is
shell: cmd, which has no errexit, so the step's status is whatever the last command returned. Appending anything after the main build would have silently masked a build failure. That build is now gated with an explicitif errorlevel 1 exit /b 1before the diagnostic runs, which is a small correctness improvement to that leg independent of this change.Size cost is a few MB against a 32 MB bundle.
Verification
Not run here. The four build legs in this repo are the check; what I am looking for is all four green and
test-backend-opspresent in every uploaded artifact. If a leg cannot build it, the intended behaviour is a warning in the log and a bundle without it, not a failed release.