fix(hip-kernel-provider): resolve rocm_kpack hermetically, never fetch by default - #11150
Draft
BrianHarrisonAMD wants to merge 1 commit into
Draft
fix(hip-kernel-provider): resolve rocm_kpack hermetically, never fetch by default#11150BrianHarrisonAMD wants to merge 1 commit into
BrianHarrisonAMD wants to merge 1 commit into
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
BrianHarrisonAMD
force-pushed
the
users/bharriso/kpack-hermetic-resolution
branch
2 times, most recently
from
August 21, 2026 23:52
6f13683 to
c563ccb
Compare
BrianHarrisonAMD
force-pushed
the
users/bharriso/kpack-hermetic-resolution
branch
2 times, most recently
from
August 22, 2026 00:05
b80e901 to
c79efbe
Compare
BrianHarrisonAMD
force-pushed
the
users/bharriso/kpack-hermetic-resolution
branch
2 times, most recently
from
August 22, 2026 02:27
ba0ed6d to
2717857
Compare
…h by default The build fetched rocm_kpack from the network at configure time, from unpinned sources, with no opt-in. Flagged as a P0 defect in review of #10489. TheRock passes -DROCKE_KPACK_PYTHON_DIR, which nothing here read, so the value was dropped and the ASM SDPA engine cloned rocm-systems' floating develop instead. CI run 32452718850 shows both halves: "Cloning into '...rocm_systems_kpack'" alongside "Manually-specified variables were not used by the project: ROCKE_KPACK_PYTHON_DIR". Add cmake/KpackPython.cmake, one resolution point for every kpack consumer: HIPKERNELPROVIDER_KPACK_PYTHON_DIR, then the deprecated ROCKE_KPACK_PYTHON_DIR, then a pinned fetch if HIPKERNELPROVIDER_KPACK_ALLOW_FETCH is ON (default OFF), else empty. The alias means no coordinated TheRock change is needed. A configured-but-invalid path is fatal rather than a fallback to fetching. Drop the configure-time `pip install zstandard msgpack`, which mutated the host environment from an unpinned index; dependencies are now checked and named. TheRock's requirements.txt already provides both. Callers set their own policy: descriptor-packaging hard-fails, since it cannot pack without rocm_kpack; the ASM SDPA step skips, since its .kpack output is unused at runtime today (the runtime loads loose .co via SdpaModuleCache). Make that skip fatal when Phase 2 lands. Verified across the resolution matrix: unset (no fetch), canonical, legacy-only, both set, invalid path (fatal), standalone layout, and ALLOW_FETCH=ON (pinned SHA). End-to-end, pack.py packed 164 .co files for gfx942 with round-trip SHA256 verification. Issue: #11149
BrianHarrisonAMD
force-pushed
the
users/bharriso/kpack-hermetic-resolution
branch
from
August 22, 2026 03:14
2717857 to
7e47307
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The hip-kernel-provider build fetched
rocm_kpackat CMake configure time from unpinned sources, with no way to opt out. Replaces the three fetch sites with one resolution module that reaches the network only when explicitly told to.Closes #11149. Raised in review of #10489 (r3834047349, r3834111054).
Risk Assessment
Risk 2. Build-system only; no runtime code changes and no change to shipped artifact contents.
Where a build previously fetched to produce ASM SDPA
.kpack, the default now produces none. Those archives are unused at runtime — the runtime loads loose.coviaSdpaModuleCache/loadKernelModule— so no behavior changes. Builds that supply a kpack path, including TheRock, are unaffected. Descriptor packaging is behindHIPDNN_ENABLE_KERNEL_INGESTOR(default OFF) and keeps its existing hard failure.ASIC Coverage
No ASIC impact. Build-time dependency resolution only; no kernel selection, dispatch, or codegen is touched. Standard PR CI is sufficient.
Testing Summary
Resolution matrix against
cmake/KpackPython.cmake: unset (no fetch), canonical flag, deprecated alias, both set (canonical wins), invalid path (fatal), standalone layout,ALLOW_FETCH=ON(lands on the pinned SHA), alias changed on a warm build directory, and the dependency probe with kpack importable, absent, and present-but-broken.Both CI jobs pack rather than skip: 164
.cofiles for gfx942 and 126 for gfx950, round-trip SHA256 verified.Testing Checklist
cmakeconfigure againstcmake/KpackPython.cmake- Status: Passedpack.py --arch gfx942- Status: Passed (164/164)--force-reinstallplus commit assertion - Status: PassedTechnical Changes
New
cmake/KpackPython.cmake— one resolution point for both kpack consumers:HIPKERNELPROVIDER_KPACK_PYTHON_DIRROCKE_KPACK_PYTHON_DIRHIPKERNELPROVIDER_KPACK_ALLOW_FETCHOFFHIPKERNELPROVIDER_KPACK_GIT_REFwhen no local source is configured.Unresolved returns empty and the caller sets policy. A configured path that does not contain
rocm_kpack/kpack.pyis fatal rather than a fetch fallback. The dependency probe imports therocm_kpackmodulespack.pyuses, under the interpreter andPYTHONPATHthe pack command runs with; it never installs.asm_sdpa_engine/CMakeLists.txt— drops thegit cloneof rocm-systemsdevelopand the configure-timepip install zstandard msgpack, which mutated the host environment from an unpinned index. Skips packing when kpack is unresolved, since the output is unused at runtime.descriptor-packaging/cmake/HkpPackaging.cmake— delegates to the shared module; still fails hard, as it cannot pack without kpack.hipdnn-superbuild-ci.yml— both jobs relied on the implicit fetch. They now install rocm-kpack at the pinnedKPACK_REFinto the build venv and pass its site-packages explicitly.--force-reinstallis required because rocm-kpack is version0.1.0at every commit, so pip would treat the pinned requirement as already satisfied in a warm venv; the installed commit is then asserted againstKPACK_REF. A post-build step fails the job if no.kpackarchives were produced.Follow-ups
Tracked on #11149: make the ASM SDPA skip fatal once the runtime loads
.kpack; migrate TheRock to the canonical flag and drop the alias; reviewrocke/CMakeLists.txt's venv pip installs.Sourcing kpack from TheRock's pinned
rocm-systemssubmodule was considered and rejected: this workflow builds against nightly wheels up to 14 days old that are not built from TheRock@main, so that commit is not the right correspondent. Both in-repo precedents for consuming rocm-systems in CI use explicit pins.