Skip to content

hrx: hrx-env names the HSA runtime, and a parity row compares CPU and device output (engine#674) - #883

Merged
bong-water-water-bong merged 5 commits into
mainfrom
hrx-env-device-parity
Nov 3, 2026
Merged

bong-water-water-bong merged 5 commits into
mainfrom
hrx-env-device-parity

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Root cause

scripts/hrx-env.sh is the one place that sets up the environment for HRX processes the engine does not launch
itself ("test-backend-ops, llama-bench, llama-cli, a harness that starts llama-server on its own"). It set the
engine#715 save-area trio and nothing else. On this box the trio is necessary and not sufficient: the HAL names the
HSA runtime with IREE_HAL_AMDGPU_LIBHSA_PATH and otherwise resolves the system ROCr, which with HSA_USE_SVM=0
registers no HRX0. The failure is silent at the device-list level, so a harness measures nothing and looks like a
null result:

$ source scripts/hrx-env.sh && llama-cli --list-devices
Available devices:
  (none)

scripts/hw-gate.sh has always had to work around this itself (LIBHSA=$(find "$TOOLCHAIN" -path '*/libhsa-runtime64.so.1' ...)
then export it) before sourcing the script. This moves that into the recipe, with serve's own fallback (the TheRock
library), keeping a caller's value if it set one.

Found while running engine#674's reproducer: ten -dev HRX0 arms died in 1 s on use --list-devices to see a list of available devices, i.e. the issue's own 1-second reproducer does not run from a shell on this box without the export.

Measured before/after (strixhalo, gfx1151, 2026-11-03)

command result
before source scripts/hrx-env.sh && llama-cli --list-devices Available devices: (none) — every -dev HRX0 arm dies in 1 s
after the same HRX0: AMD Radeon 8060S Graphics (Node 1) (gfx1151) (122880 MiB, 73281 MiB free)
after, caller override IREE_HAL_AMDGPU_LIBHSA_PATH=/nonexistent.so source scripts/hrx-env.sh --check value kept, --check refuses (rc 3) with the reason
after, no TheRock installed unset + nothing found warning only, rc 0 (a box whose system ROCr works is not broken by this)

Second half: the row that compares placements (engine#674)

serve_e2e.sh requires each device to answer; no row compares one placement's text with another's. #674 is exactly
that failure — a placement change produced plausible-looking garbage — so this adds
tests/hrx_cpu_weight_parity.sh, wired as ctest hrx_cpu_weight_parity next to serve_e2e_hrx/serve_e2e_cpu:

Greedy decoding at temperature 0 must be byte-identical across all three, because placement changes where a number is
computed, never what it is. Skips (77) without a gfx1151; fails, not skips, when the machine has a GPU and HRX0 did
not register, with the IREE_HAL_AMDGPU_LIBHSA_PATH diagnosis (which is how the trap above gets caught rather than
silently passing).

run (one gpu-hold, Qwen3-0.6B-Q4_K_M) result
pin d8bd0ff6052a, row as added PASS — cpu / weightscpu / offload all say The capital of France is Paris. But wait, there's also a city called
same row, same binaries, with the do-not-merge decline knob GGML_HRX_169_DECLINE=MUL_MAT,MUL as a test input FAIL — weightscpu says irse ##Hmm ####maybemaybe, cpu and offload unchanged, so the row reports FAIL placement changed the answer
the same knob on a llama-cli rerun, for provenance MUL_MAT,MUL → inaryиона this this is. (the issue's own garbage, byte-identical); none and MUL_MAT → correct

So the row would have caught the corruption #674 is about, and the knob is used only as a test input — it stays
unmerged and out of this PR.

What this does not do

It does not close #674. On merged code the issue's headline does not reproduce: ten arms of the 1-second reproducer at
origin/main 23d6972 / pin d8bd0ff6052a — -dev none, -dev HRX0 -ngl 0, -ngl 0 with -ot CPU pins on
attn_norm/attn_q/token_embd weights, --no-op-offload, -ngl 4, -ngl 99, -fa 0, and the decline knob itself
(absent from the binary) — all produce Okay, the user is asking, byte-identical. The corrupting placement was only ever
produced by the do-not-merge patch, and #173's refusal band makes the equivalent budget-driven split unreachable. This PR
adds the instrument that keeps the invariant checkable and fixes the environment trap that made its reproducer unrunnable
from a shell; the close/re-scope word on #674 is the owner's.

Refs engine#674, engine#715, engine#315.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

#883 ready for review at b1f4271

Two halves, both measured on strixhalo with one gpu-hold:

  1. scripts/hrx-env.sh now names the HSA runtime. Before: source scripts/hrx-env.sh && llama-cli --list-devices printed (none) and every -dev HRX0 arm died in 1 s; after: HRX0 (122880 MiB, 73281 free). scripts/hw-gate.sh already had to work around this itself, so this is the same fix in the recipe every other script sources.
  2. tests/hrx_cpu_weight_parity.sh (ctest hrx_cpu_weight_parity): cpu / device-with-weights-on-CPU / fully-offloaded greedy text must be identical. Passes at pin d8bd0ff6052a; and it FAILS when run with the do-not-merge GGML_HRX_169_DECLINE=MUL_MAT,MUL knob as a test input, which is the engine#674 corruption — i.e. the row catches that class.

Independent re-derivation, cheapest command: IREE_HAL_AMDGPU_LIBHSA_PATH=/nonexistent.so source scripts/hrx-env.sh --check must exit 3, and env -u IREE_HAL_AMDGPU_LIBHSA_PATH bash -c 'source scripts/hrx-env.sh; echo $IREE_HAL_AMDGPU_LIBHSA_PATH' must print the TheRock library on this box.

Not a close for #674 (its headline does not reproduce at the pin; ten arms of the 1-second reproducer are byte-identical). PR body has the table.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 4fe4790)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

715 - Partially compliant

Compliant requirements:

  • Engine sets the trio itself when it detects unfavourable capability lines
  • Log the libhsa path + sha256 in the HRX init line of every run

Non-compliant requirements:

  • Fail loudly at init with the recipe (still needs explicit init-time check)
  • Settle the release-runtime question (still needs explicit runtime selection)

Requires further human verification:

  • Verification that the init-time check is properly implemented
  • Confirmation that the release-runtime question is fully addressed

674 - Partially compliant

Compliant requirements:

  • Added tests/hrx_cpu_weight_parity.sh to compare CPU and device outputs
  • Added test to CMakeLists.txt to run the parity test

Non-compliant requirements:

  • None

Requires further human verification:

  • None

173 - Partially compliant

Compliant requirements:

  • Fix for modprobe PATH issue in mkimage.sh
  • Improved error logging in guard() function

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Runtime Path Resolution

The script now automatically fills IREE_HAL_AMDGPU_LIBHSA_PATH when not set, which is good for resolving the device access issue. However, the fallback logic only checks for specific paths and does not validate if the found library is compatible with the current system configuration. This could lead to a scenario where a non-functional runtime is selected, causing silent failures or unexpected behavior.

if [ -z "${IREE_HAL_AMDGPU_LIBHSA_PATH:-}" ]; then
    for _cand in /opt/rocm-therock/lib/python*/site-packages/_rocm_sdk_core/lib/libhsa-runtime64.so.1 \
                 /opt/rocm-therock/lib/libhsa-runtime64.so.1; do
        if [ -e "$_cand" ]; then
            export IREE_HAL_AMDGPU_LIBHSA_PATH=$_cand
            break
        fi
    done
    unset _cand
fi
Check Mode Implementation

The --check mode in hrx-env.sh provides validation for the trio variables but does not fully enforce the requirement that the runtime path must be valid. It warns if the path does not exist but does not prevent execution if the path is invalid. This could still allow execution with an incorrect runtime, leading to silent failures.

if [ "${1:-}" = --check ] && [ "$_hrx_env_opt_out" = 0 ]; then
    for _v in HSA_USE_SVM HSA_USERPTR_FOR_PAGED_MEM HRX_ALLOW_NO_SVM; do
        if [ -z "${!_v:-}" ]; then
            echo "hrx-env: $_v is not set, so an HRX process started from here would page-fault on its first" >&2
            echo "         graph replay (engine#715). Export the trio, or HRX_SVM=1 to opt out deliberately:" >&2
            echo "           HSA_USE_SVM=0 HSA_USERPTR_FOR_PAGED_MEM=0 HRX_ALLOW_NO_SVM=1" >&2
            return 3 2>/dev/null || exit 3
        fi
    done
    # A path that does not exist is worse than an unset one: the HAL fails to dlopen and reports no device, which reads
    # like a GPU problem. Unset-but-nothing-found stays a warning, because a box whose system ROCr works is fine.
    if [ -n "${IREE_HAL_AMDGPU_LIBHSA_PATH:-}" ] && [ ! -e "$IREE_HAL_AMDGPU_LIBHSA_PATH" ]; then
        echo "hrx-env: IREE_HAL_AMDGPU_LIBHSA_PATH=$IREE_HAL_AMDGPU_LIBHSA_PATH does not exist, so the HAL cannot load a" >&2
        echo "         runtime and registers no HRX0 (--list-devices prints '(none)'). Point it at libhsa-runtime64.so.1." >&2
        return 3 2>/dev/null || exit 3
    elif [ -z "${IREE_HAL_AMDGPU_LIBHSA_PATH:-}" ]; then
        echo "hrx-env: no TheRock libhsa found and IREE_HAL_AMDGPU_LIBHSA_PATH is unset; the HAL will resolve the system" >&2
        echo "         ROCr. That works on a box with a full ROCm install and registers no HRX0 on a box whose device only" >&2
        echo "         opens through TheRock's runtime -- check --list-devices before trusting a measurement." >&2
    fi
    # engine#715: three HSA runtimes live on this box and a run has to name the one it used, or a fault report is
    # ambiguous. Only --check prints, so sourcing this file stays silent for the callers that do not ask.
    if [ -n "${IREE_HAL_AMDGPU_LIBHSA_PATH:-}" ] && [ -e "$IREE_HAL_AMDGPU_LIBHSA_PATH" ]; then
        if command -v sha256sum >/dev/null 2>&1; then
            echo "hrx-env: runtime $IREE_HAL_AMDGPU_LIBHSA_PATH sha256 $(sha256sum "$IREE_HAL_AMDGPU_LIBHSA_PATH" | cut -c1-16)" >&2
        else
            echo "hrx-env: runtime $IREE_HAL_AMDGPU_LIBHSA_PATH (no sha256sum on this machine)" >&2
        fi
    fi
fi

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Head moved to 104173d4d after the pr_agent ticket note, and one thing it flagged is now closed: #715 requires that a run name which HSA runtime it used (three live on this box, with sha256). --check now prints it, and the prefix it prints is the one #715's own table records for the runtime the engine picks by default:

$ source scripts/hrx-env.sh --check
hrx-env: runtime /opt/rocm-therock/lib/python3.14/site-packages/_rocm_sdk_core/lib/libhsa-runtime64.so.1 sha256 21a6682452e159cf

Plain source stays silent, so the callers that do not ask are unaffected. scripts/hw-gate.sh already sources with --check, so gate logs now carry the runtime identity for free.

Scope of the rest of the ticket note, stated so the review is not misled:

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 104173d

… device output (engine#674)

scripts/hrx-env.sh set the engine#715 save-area trio but not IREE_HAL_AMDGPU_LIBHSA_PATH, so a tool started
from a shell (llama-cli, llama-bench, test-backend-ops, a harness that starts llama-server itself) resolved the
*system* ROCr, which with HSA_USE_SVM=0 registers no HRX0 at all: --list-devices prints "(none)" and every arm of a
harness silently measures nothing. scripts/hw-gate.sh has always had to set that variable itself before sourcing this
file; now the recipe does it once (the same TheRock fallback serve uses), a caller's own value still wins, --check
refuses a path that does not exist, and --check names the runtime it resolved with its sha256, because engine#715's
table has three HSA runtimes on this box and a fault report is ambiguous without the name.

tests/hrx_cpu_weight_parity.sh is the row that compares placements instead of only requiring that each device answers
(engine#674): one greedy prompt through the CPU, through the device with every layer assigned to the CPU, and fully
offloaded, compared byte for byte. It skips (77) without a gfx1151 and fails, rather than skipping, when the machine has
a GPU and HRX0 did not register.

Measured on strixhalo (gfx1151), 2026-11-03:
  before: source scripts/hrx-env.sh && llama-cli --list-devices -> Available devices: (none)
          (ten -dev HRX0 arms of a #674 harness died in 1 s on this)
  after:  the same command -> HRX0: AMD Radeon 8060S Graphics (Node 1) (gfx1151) (122880 MiB, 73281 MiB free)
          source scripts/hrx-env.sh --check ->
          hrx-env: runtime .../_rocm_sdk_core/lib/libhsa-runtime64.so.1 sha256 21a6682452e159cf
          (the sha256 prefix engine#715's table records for the runtime the engine picks by default)
  row at the pin d8bd0ff6052a: cpu / weightscpu / offload all answer
          "The capital of France is Paris. But wait, there's also a city called" -> PASS
  row with the do-not-merge decline knob (MUL_MAT,MUL) as a test input:
          weightscpu "irse ##Hmm ####maybemaybe", cpu and offload unchanged -> FAIL, as it must
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Head 49b38bf5f: last self-review pass — removed a trap/srv_pid pair from the test that could not do what it implied (arm runs inside a command substitution, so the parent shell never sees that pid; every path in arm already kills its own server). No behaviour change; re-verified the two non-device paths at this head: ONEBIT_HRX_E2E_TARGETS=gfx9999 → skip 77, and a dangling IREE_HAL_AMDGPU_LIBHSA_PATH → FAIL the GPU is present but … lists no HRX0 rc 1.

Also for whoever reads the checks: pr_agent is red on every push here for a workflow reason, not a finding — its run is cancelled by its own superseding run (Canceling since a higher priority waiting request for pr-agent-883 exists, all steps success, conclusion cancelled). Everything else at this head: build pass 4m4s, Analyze (c-cpp|actions|python|rust) and CodeQL pass, hw_gate skipping (no GPU runner).

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 49b38bf

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY

At 49b38bf5f. Both halves of the PR are gates that bite, and I ran both.

scripts/hrx-env.sh --check names the runtime on this box, which is the whole point of engine#715's ambiguity:

hrx-env: runtime /opt/rocm-therock/lib/python3.14/site-packages/_rocm_sdk_core/lib/libhsa-runtime64.so.1 sha256 21a6682452e159cf

That is the same library and the same sha256 prefix my own notes have for the TheRock nightly, so the resolution order
(baked ONEBIT_HRX_LIBHSA → /opt/rocm-therock pip layout → /opt/rocm) lands on the right one here. bash -n clean on
both scripts.

The parity row passes on the device, and it is the row #674 needed. From a full checkout of this head, under
gpu-hold -l "pi prs 883", three arms, one greedy prompt, 0.6B Q4_K_M:

cpu        said:  The capital of France is Paris. But wait, there's also a city called …
weightscpu said:  The capital of France is Paris. But wait, there's also a city called …
offload    said:  The capital of France is Paris. But wait, there's also a city called …
ok   the CPU answers at all
ok   device with every layer on the CPU answers the CPU's text
ok   device with every layer offloaded answers the CPU's text
PASS placement does not change the answer

And the trap it is built for is real — I hit it by accident and the test did the right thing. My first attempt passed a
llama-server path that did not exist; the GPU was present, so instead of skipping it failed with

FAIL the GPU is present but …/llama-server lists no HRX0
     HRX needs a runtime it can open the device through: set IREE_HAL_AMDGPU_LIBHSA_PATH

which is exactly the silent-no-device failure mode I documented in this lane (IREE_HAL_AMDGPU_LIBHSA_PATH missing → empty
device list → test-backend-ops tests the CPU and exits 0). A row that fails with the variable named is worth more than
three that skip.

Live check before my re-verdicts: all six other PRs I just verdicted either merged (#878) or are being re-headed by the
waiter, so this one is the only fresh gate in the batch.

@bong-water-water-bong bong-water-water-bong added the tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 978e01f

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 92c9317

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Preparing review...

@bong-water-water-bong
bong-water-water-bong merged commit 2fc5c4f into main Nov 3, 2026
8 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the hrx-env-device-parity branch November 3, 2026 11:45
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 4fe4790

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 3/5 tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hrx: any graph whose weight ops all run on the CPU computes garbage (1 s reproducer)

1 participant