fix(rocm): prefer system LLVM runtime on native Linux - #7448
Conversation
|
@codex review |
![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: 46ce872a85
ℹ️ 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".
| llvm_lib = os.path.join(d, "llvm", "lib") | ||
| if llvm_lib not in seen and os.path.exists(llvm_lib): |
There was a problem hiding this comment.
On the supported lib64 layout (covered by test_lib64_layout_is_found), d is <root>/lib64, so this constructs <root>/lib64/llvm/lib. ROCm's nested runtime path described by this change is <root>/lib/llvm/lib; therefore hosts with HSA under lib64 but LLVM in the standard nested path omit the system LLVM directory and can still bind libamd_comgr to the incompatible bundled libLLVM. Build the LLVM candidate from base, or probe both layouts, rather than deriving it only from the HSA directory.
Useful? React with 👍 / 👎.
…nd non-directories Two gaps found while simulating the fix against real ROCm layouts. 1. lib64 hosts got no LLVM dir. The candidate was built from the HSA dir, so a host with libhsa-runtime64 under lib64 probed <root>/lib64/llvm/lib. ROCm installs LLVM under <root>/lib/llvm regardless, so that host kept binding system libamd_comgr to the bundle's libLLVM: exactly the bug unslothai#7446 reports. Probe both spellings, the HSA dir's own first so a genuine lib64 layout still wins. When lib_sub is already "lib" the seen set collapses them. 2. os.path.exists accepted a non-directory. The serve-time caller joins these straight into LD_LIBRARY_PATH with no is-dir filter, so a file named llvm/lib reached the loader. os.path.isdir instead. Verified on a 27-case matrix built from real directory trees (not mocks), run on both Windows and Linux against three revisions: main, this PR as-is, and this commit. Zero regressions and zero reorderings of the pre-existing entries in every case, and the installer and launcher copies never disagree. The lib64 case goes [lib64] -> [lib64, lib/llvm/lib]; the file case drops the bogus entry; a symlinked llvm/lib resolves correctly on Linux. End-to-end loader check: built real ELF objects mirroring the shipped bundle (RUNPATH=$ORIGIN, an incomplete libLLVM.so.23.0git next to llama-server, system comgr from /opt/rocm/lib) and reproduced the reported failure verbatim, then confirmed the prepend clears it: before undefined symbol: LLVMInitializeSPIRVTarget -> after exit 0 Test helper now patches os.path.isdir alongside os.path.exists, else every fake host reports its nested llvm dir as missing. New cases: lib64 finding llvm under lib, lib64 preferring its own when both exist, and a real-filesystem check that a non-directory is not prepended. Removing the lib fallback from one copy reddens three tests including the two-copy parity guard. tests/studio/install: 1361 passed on Linux, 4 pre-existing environmental failures unchanged (3 managed-node-runtime under root, 1 the real /opt/rocm case already covered by unslothai#7397). 30/30 on the helper suite on Windows and Linux.
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
2 participants
Fixes #7446
Summary
lib/llvm/libdirectory in the native-Linux system runtime paths.Root Cause
The native-Linux ROCm path already puts the system
libdirectory ahead of the llama.cpp prebuilt bundle. However, ROCm installs its versioned LLVM runtime underlib/llvm/lib. Whenlibamd_comgr.so.3came from the system ROCm installation, the loader could still fall through to an incompatiblelibLLVM.so.*inbuild/bin, causing the missingLLVMInitializeSPIRVTargetsymbol.The nested system LLVM directory is now placed after the corresponding system ROCm
libdirectory and before the prebuilt bundle.Verification
uv run --no-project --python 3.11 --with pytest pytest tests/studio/install/test_rocm_native_linux_lib_dirs.py -q --tb=short-> 27 passeduv run --no-project --python 3.11 --with pytest pytest tests/studio/install/test_install_llama_prebuilt_logic.py -q --tb=short-> 60 passed, 1 skippeduvx --from pre-commit pre-commit run --files studio/install_llama_prebuilt.py studio/backend/core/inference/llama_cpp.py tests/studio/install/test_rocm_native_linux_lib_dirs.py-> passeduv run --no-project --python 3.11 --with pytest pytest tests/studio/install/test_rocm_support.py -q --tb=short-> 358 passed, 16 failed, 3 skipped; the same 16TestEnsureRocmTorchfailures reproduce on an unmodifiedmainworktree in this macOS environmentNotes for Reviewers