Skip to content

docs/hrx: hrx-backend-test builds again since #422 closed, and nothing runs it — both facts stated - #1392

Merged
bong-water-water-bong merged 2 commits into
mainfrom
1bit/i669-hrx-backend-test-doc
Nov 8, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
1bit/i669-hrx-backend-test-doc

Conversation

@bong-water-water-bong

@bong-water-water-bong bong-water-water-bong commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator

Docs only, one paragraph. Two claims in docs/hrx.md were half true and the stale half was hiding the useful half.

Stale: "hrx-backend-test still fails to build on either pin (#422), so that check is not covered yet." #422 is closed (2026-10-18) and its named cause — the stale dispatch-symmetric-i4.h include that f2099e9b deleted with the route switch it guarded — is gone from the tree (grep -rn dispatch-symmetric-i4 finds one comment and no include). Verified by actually building the target in a tree configured with HRX_SOURCE_DIR set: hrx-backend-test links and runs.

The part that was still true, and is the reason to fix the sentence: the binary carries a bare add_test with no ctest label, while the fork's build-*.yml workflows run ctest -L main; and scripts/hw-gate.sh builds targets=(llama-server test-backend-ops llama-perplexity). So its cases run nowhere automatically — they are correct and dark unless someone builds and runs the binary by hand. A sentence that says "does not build" invites the fix "make it build", after which a new case there still never executes.

Why I hit it: I added a device case to that binary today (the lifetime-counter invariant from engine #841, on a fork branch whose PR lands once its two-run validation finishes) and went looking for where it would run. It does not run anywhere; the honest options are a label a device CI respects or a place in the gate's target list, and both are decisions for whoever owns those lists rather than for the author of one case.

No code, no behaviour change. If you would rather the paragraph not name the two lists, the minimum claim worth keeping is that the test builds and is not covered by anything automated.

…both

The line claimed the test "still fails to build on either pin (#422)", which stopped being true on
2026-10-18: #422 is closed and its named cause - the stale `dispatch-symmetric-i4.h` include - was
deleted by f2099e9b. Verified by building the target in a tree configured with `HRX_SOURCE_DIR` set
(`hrx-backend-test` links and runs).

The failure that matters is the next one down, and the old sentence hid it: even building, the binary
executes nowhere automatically. It is registered with a bare `add_test` and no ctest label, while the
fork's workflows run `ctest -L main`, and `scripts/hw-gate.sh` builds
`targets=(llama-server test-backend-ops llama-perplexity)`. So its cases - including any new one - are
correct and dark until one of those two lists carries it.

Written down so the next person adding a case there knows to check the runner, and so #422 does not get
re-filed against a tree that no longer has the include.

@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 13b942147.

All four claims in the paragraph check out, and I checked the "nothing runs it" half adversarially because that is the half a reader will
act on:

f2099e9b   "hrx: keep our ggml-hrx backend on AMD's core", 2026-10-06
           files: …/dispatch_registration/common/dispatch-symmetric-i4.h  -> status=removed, +0/−37
           (so the sentence names the right commit, and that commit really deleted the header)
#422       CLOSED 2026-10-18T16:41:39Z                          -> the text says "closed 2026-10-18"
ctest      tests/CMakeLists.txt:383  add_test(NAME hrx-backend-test COMMAND hrx-backend-test)  -> no LABELS
           every ctest in .github/workflows/* uses -L main (the -E variants keep the same filter)
           no workflow names hrx-backend-test at all
hw-gate.sh --bin DIR   test the llama.cpp binaries in DIR (llama-server, test-backend-ops, llama-perplexity)   (:44)
           hrx-backend-test appears 0 times in hw-gate.sh, and nothing in scripts/ or tools/ runs it either

So "builds, but nothing runs it — correct and dark" is accurate as written, including the specific list you quote.

On "builds": I could not rebuild the target here, and I would rather say that than imply I did. What I could check statically is the
failure mode #422 was: every local #include "…" in tests/hrx-backend-test.cpp resolves at the pinned tree (the ggml*.h ones resolve
elsewhere in the tree — ggml/include and friends — which is why a naive containment check reports them missing), and the only surviving
mention of dispatch-symmetric-i4.h is the comment at the test file's :11225 saying it was deleted. That is consistent with "the build
works" and inconsistent with "a stale include is still there", which is the best a reader of this doc needs.

One thing the paragraph could add for free, since it already names both gaps: tests/CMakeLists.txt:383 is the single line that
decides this, so the fix is either a LABELS main there or a name in hw-gate's target list at :44. Naming the line turns "nobody runs it"
into "here is the one-token change", which is what makes a follow-up issue actionable rather than a note. Not blocking — the sentence is
true and the trap it describes (a case added there is correct and dark) is the useful part.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction to the body, made within the hour: it named "fork llama.cpp#236" as the case I added, and #236 is another lane's PR (differential known-answer cases for the routed expert kernel, #572). Mine is a branch that opens its PR once the two-run validation finishes — it is not #236 and I should not have guessed a number. The paragraph now says what the case is without citing one; nothing else in the PR changes, and the two claims it is about (the test builds since #422 closed; nothing runs it) are unaffected.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

422 - Partially compliant

Compliant requirements:

  • The hrx-backend-test now builds successfully
  • The stale include dispatch-symmetric-i4.h is no longer referenced in the test
  • The test no longer fails to build due to the deleted header

Non-compliant requirements:

  • The test is still not automatically run as part of CI workflows

Requires further human verification:

  • Verification that the test actually runs in CI workflows (if desired) would require changes to CI configuration

841 - Partially compliant

Compliant requirements:

  • No changes to the cache behavior or accounting logic

Non-compliant requirements:

  • The underlying memory accounting issue in retained_bytes() is not addressed

Requires further human verification:

  • The memory accounting behavior described in the ticket remains unchanged
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Outdated Documentation

The documentation still states that hrx-backend-test does not run anywhere automatically, which is accurate but could be misleading. While the build issue (#422) is resolved, the test still lacks automatic execution in CI workflows, which may lead to confusion about its operational status.

`generate_stubs.py --check` passes against the built `libamdhip64.so`. `hrx-backend-test` builds at
the pinned pair again since f2099e9b deleted the stale `dispatch-symmetric-i4.h` include that broke it
(#422, closed 2026-10-18), but **nothing runs it yet**: it carries no ctest label (the fork's
`build-*.yml` workflows run `ctest -L main`) and `hw-gate.sh`'s target list is `llama-server test-backend-ops
llama-perplexity`, so its cases only execute when someone builds and runs the binary by hand. Until one
of those two lists carries it, a case added there is correct and dark.

@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 9edfebf463ca4c2e0a0b08156fc4468e49f23a44

This head adds only a merge of main on top of 13b94214, which was reviewed READY. The first parent is 13b9421, the PR's own diff has patch-id 2208de9a both before and after the merge, and no main commit in the merge touches docs/hrx.md.

I re-checked the paragraph's claims against the current pins rather than carrying them forward unread (fork at ed43da0b, main's pin):

  • It builds, and the cause is gone. dispatch-symmetric-i4 occurs once in the fork, in a comment (tests/hrx-backend-test.cpp:11225), with no include.
  • No label. tests/CMakeLists.txt:380-383 is add_executable(hrx-backend-test …) + add_test(NAME hrx-backend-test …), and no set_tests_properties or LABELS names it anywhere in a CMake file.
  • CI filters on -L main. build-cpu.yml:105, build-apple.yml:74/110 and build-3rd-party.yml:56 all run ctest -L main, so an unlabelled test never runs there.
  • hw-gate.sh:393 is targets=(llama-server test-backend-ops llama-perplexity).

So "builds, and nothing runs it" is exact. Naming the two lists is what turns this into an actionable sentence.

@bong-water-water-bong
bong-water-water-bong merged commit e2a23c6 into main Nov 8, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/i669-hrx-backend-test-doc branch November 8, 2026 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant