Skip to content

feat[notask]: build @qvac/fabric with the ROCm/HIP backend - #4057

Open
jpgaribotti wants to merge 6 commits into
tetherto:mainfrom
jpgaribotti:feature/fabric-hip-backend
Open

feat[notask]: build @qvac/fabric with the ROCm/HIP backend#4057
jpgaribotti wants to merge 6 commits into
tetherto:mainfrom
jpgaribotti:feature/fabric-hip-backend

Conversation

@jpgaribotti

@jpgaribotti jpgaribotti commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

qvac-fabric[hip-backend] was requested per-consumer by @qvac/vla-ggml, which paired it with include-rocm: true on prebuilds-vla.yml and include-rocm-sdk: true on its cpp-lint call. That was correct while vla built its own ggml.

#3998 migrates vla-ggml onto the shared @qvac/fabric runtime and removes all three, but nothing on the fabric side picks the feature up:

  • packages/fabric/vcpkg.json declares only vk-profiling; it never requests hip-backend.
  • prebuilds-fabric.yml passes only artifact-name-prefix and include-vulkan-sdk: true; include-rocm defaults to false in reusable-prebuilds.yml.
  • on-pr-fabric.yml's cpp-lint call passes no include-rocm-sdk.

Net effect once #3998 lands: include-rocm, include-rocm-sdk and .github/actions/setup-rocm have zero callers repo-wide, libqvac-ggml-hip.so is built by no pipeline and ships in no prebuild, and the HIP-preference branch at packages/vla-ggml/addon/src/utils/BackendSelection.cpp:132-148 becomes unreachable — hipDev is always null and pickBestGpuDevice() falls through to Vulkan. This gives back the ~18% gfx1151 warm-path win from #2781 / #2971 (479 → 384 ms).

Nothing turns red, because with no manifest requesting hip-backend the hip port is never resolved at all — the regression is a silent omission from the prebuild, not a build error. The vla-side CI that would have surfaced it is removed in the same change.

📝 How does it solve it?

Moves the request down to the shared runtime, so the backend ships once for every fabric consumer instead of per-addon.

  • packages/fabric/vcpkg.json — request qvac-fabric[hip-backend]. version>= is already 10297.0.0, the same revision vla pinned for HIP. The port's hip dependency is platform: "linux & x64"-gated, and vla requested the feature unqualified across all 9 prebuild targets on main, so an unconditional request is proven safe on darwin/windows/android.
  • packages/fabric/vcpkg-configuration.jsonqvac-registry-vcpkg baseline c57eec31f04e2447, matching vla. Required, not cosmetic: qvac-fabric[hip-backend] depends on hip with no version constraint, so hip's version comes from the pinned baseline, and the hip port does not exist at c57eec31. Nothing else this package selects moves — qvac-fabric and qvac-lint-cpp are pinned above their baseline entries by version>=, opencl / vcpkg-cmake / vcpkg-cmake-config are identical in both baselines, and spirv-headers resolves from the separately pinned microsoft/vcpkg registry. CI confirms the resolution: qvac-fabric[core,gpu-backends,hip-backend,llama]:x64-linux@10297.0.0 and hip:x64-linux@7.13.0.
  • prebuilds-fabric.ymlinclude-rocm: true, cross-compiling libqvac-ggml-hip.so (gfx1151) into the linux-x64 prebuild. No AMD GPU on the runner. Covers both callers of this workflow (on-pr-fabric.yml and on-merge-fabric.yml).
  • on-pr-fabric.ymlinclude-rocm-sdk: true on the cpp-lint call. Easy to miss but mandatory: ggml-config.cmake resolves find_dependency(hip/hipblas/rocblas) at configure time, which is exactly why fec63dd21 added this for vla.
  • packages/fabric/NOTICEhip attribution, so the notice-drift check stays green.
  • Version 0.8.00.9.0, CHANGELOG entry, and README backend/platform/build notes.

No build or packaging change is needed. packages/fabric/CMakeLists.txt:70-87 already walks GGML_AVAILABLE_BACKENDS and stages MODULE_LIBRARY/SHARED_LIBRARY targets via add_bare_module(qvac-fabric EXPORTS ...), and package.json files already publishes prebuilds/*/qvac__fabric/**. Enabling the feature is sufficient for the .so to be staged and published.

There is no cpp-tests-fabric, and test-fabric-stack-actions.yml only exercises the composite actions, so those are the only two CI touchpoints.

The hip port is deterministic, not build-time fail-safe

Worth stating explicitly because it is the opposite of what you might assume, and an earlier revision of this description got it wrong. The port hard-errors when no ROCm SDK is present rather than installing empty, quoting its portfile.cmake:

a host-dependent "skip" would let the vcpkg binary cache conflate a no-HIP build with a real HIP build (identical ABI).

Only the runtime path is fail-safe: the DL loader skips libqvac-ggml-hip.so on non-AMD hosts and falls back to Vulkan/CPU. Consequences:

  • Building packages/fabric for linux-x64 now requires a ROCm/TheRock install, found via ROCM_PATH or /opt/rocm. Previously only vla-ggml developers needed one; this moves the requirement onto everyone who builds the shared runtime. Documented in the README build section. Other platforms are unaffected (linux & x64 gate).
  • Any pipeline that builds fabric on linux-x64 without include-rocm / include-rocm-sdk will now fail rather than silently degrade. Both such call sites are updated here.

🧪 How was it tested?

Full green CI on this exact commit via workflow_dispatch on the branch — run 32878043355, success in 23m 7s, all 9 prebuild targets plus cpp-lint, sanity-checks, fabric-npm-consumer-smoke and merge-guard.

Downloaded the resulting fabric-linux-x64 artifact and inspected it:

  • linux-x64/qvac__fabric/libqvac-ggml-hip.so is present — 67.0 MiB uncompressed, 11.6 MiB gzipped — beside libqvac-ggml-vulkan.so and the 14 CPU variants.
  • Its only embedded code object is hipv4-amdgcn-amd-amdhsa--gfx1151, confirming the intended target and that it is not fattened with extra architectures.
  • cpp-lint logs show ggml::ggml-hip among the linkable targets.

So the packaging cost is concrete: +11.6 MiB to the published tarball for every @qvac/fabric consumer, on linux-x64 only.

Still needs hardware: runtime confirmation on a Strix Halo (gfx1151) host that BackendSelection logs preferring HIP/ROCm GPU. Not reproducible in CI — the runners compile HIP but have no AMD GPU.

Not verified byte-for-byte: that the other 8 targets are unchanged. Satisfied by construction instead — the hip dependency is platform: "linux & x64"-gated, so the feature contributes nothing off linux-x64, and all 8 built green.

actionlint was not run locally: it is not installed on this machine and the repo forbids curl | bash installers. The policy-tests job in security-baseline.yml lints both edited workflows.

⚠️ Why the automatic PR checks are red

on-pr-fabric.yml is triggered by pull_request_target, which loads the workflow YAML from the base branch. Until this PR merges, the auto checks run main's copies of prebuilds-fabric.yml and on-pr-fabric.yml — which do not pass include-rocm / include-rocm-sdk — against this branch's source tree, which does request hip-backend. Mismatched by construction.

The two runs are a clean controlled experiment, same head SHA aaaeca1:

run trigger result
32875616325 pull_request_target failure
32878043355 workflow_dispatch success

The failing run's logs print the resolved inputs as include-rocm: false and include-rocm-sdk: false while checking out aaaeca1, and the only jobs that fail are exactly the two this PR edits (cpp-lint, prebuild / linux-x64) plus the merge-guard aggregate.

They fail in two different-looking ways, same root cause:

  • prebuild / linux-x64 — vcpkg binary cache miss, so the portfile ran and hard-errored: hip port: no ROCm SDK found.
  • cpp-lint — cache hit (hip:x64-linux@7.13.0 restored in 197 ms, portfile never executed), so it failed later at consumer configure: CMake Error at build/_vcpkg/x64-linux/share/hip/hip-config.cmake:1 (include): /lib/cmake/hip/hip-config.cmake. The leading / is the port's relocatable include("$ENV{ROCM_PATH}/…") shim expanding an unset ROCM_PATH to empty. Noting it because a warm binary cache can disguise a missing SDK as a successful hip install.

Both resolve on merge, and for any PR branched off a main that contains this change.

🔌 API Changes

None. @qvac/fabric has no JS API; this only changes what ships inside the linux-x64 prebuild.

Notes for reviewers

This is a cost/policy decision, not just a checkbox. hip-backend was opt-in per-consumer precisely so that only vla paid for it. With the runtime shared, that granularity is gone: every fabric consumer's linux-x64 prebuild carries the HIP module (+11.6 MiB compressed), every fabric linux-x64 build and lint needs ROCm on the runner, and — because the port is deterministic — so does every local linux-x64 build.

Two alternatives were considered and rejected:

  1. Gate it behind a manifest feature + CMake option, mirroring vk-profiling, with prebuilds-fabric.yml passing -D HIP_BACKEND=ON. Keeps local builds working without ROCm. Rejected to stay consistent with how vla-ggml already declares it, and to avoid a build configuration that CI exercises but developers do not.
  2. Drop HIP explicitly — remove the dead branch in BackendSelection.cpp, fix the packages/vla-ggml/CHANGELOG.md claim that HIP "is provided by @qvac/fabric when present in its prebuilds", and delete the now-callerless include-rocm / include-rocm-sdk inputs and the setup-rocm action. This is the right change if the size or build cost is judged unacceptable.

Merge ordering: independent of #3998 and safe to merge in either order. Merging this first means HIP is never absent from a published prebuild.

Raised from review of #3998.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Review Status

Current Status: ✅ APPROVED
Approvals so far: Team Lead: 1, Member: 1

@jpgaribotti jpgaribotti self-assigned this Aug 25, 2026
@jpgaribotti jpgaribotti added prebuilds CI: run only prebuild/cache stage (requires verified) run-cpp-addon-tests CI: run C++ addon tests (requires verified) run-desktop-addon-tests CI: run desktop integration tests (requires verified) labels Aug 25, 2026
The qvac-fabric[hip-backend] feature was requested per-consumer by
@qvac/vla-ggml, which paired it with include-rocm on its own prebuild
workflow and include-rocm-sdk on its cpp-lint. Now that vla-ggml consumes
the shared runtime instead of building its own ggml, nothing in the repo
requests the feature and no pipeline installs ROCm, so libqvac-ggml-hip.so
ships in no prebuild and the HIP preference branch in vla's
BackendSelection is unreachable.

Move the request to the shared runtime so the backend ships once for every
consumer. packages/fabric/CMakeLists.txt already stages GGML_AVAILABLE_BACKENDS
MODULE targets into the prebuild and package.json already publishes
prebuilds/*/qvac__fabric/**, so no build or packaging change is needed.

- packages/fabric/vcpkg.json: request qvac-fabric[hip-backend]
- prebuilds-fabric.yml: include-rocm: true for the linux-x64 cross-compile
- on-pr-fabric.yml: include-rocm-sdk: true on cpp-lint, since
  ggml-config.cmake resolves find_dependency(hip/hipblas/rocblas) at
  configure time

Refs tetherto#3998, QVAC-19291, QVAC-23995
gianni-cor
gianni-cor previously approved these changes Aug 25, 2026
GustavoA1604
GustavoA1604 previously approved these changes Aug 25, 2026
Comment thread packages/fabric/vcpkg.json
Comment thread .github/workflows/on-pr-fabric.yml
The changelog and the prebuilds-fabric.yml comment claimed the build was
fail-safe when no ROCm SDK is present. The hip port is the opposite by
design: it hard-fails rather than installing empty, so the vcpkg binary
cache cannot conflate a no-HIP build with a real HIP build under an
identical ABI hash. Only the runtime fallback (DL loader skipping the
module on non-AMD hosts) is fail-safe.

Also document the resulting linux-x64 build prerequisite in the README,
since the hip dependency is unconditional.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

prebuilds CI: run only prebuild/cache stage (requires verified) run-cpp-addon-tests CI: run C++ addon tests (requires verified) run-desktop-addon-tests CI: run desktop integration tests (requires verified)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants