[bong-water-water-bong]

Refs #2018 (already closed by hand, with neither half on main). This branch is main + #2115 (3461989) + #2124 (cef7015), with no other change.

Why a third PR

#2115 registers tests/hyperloom_benchmark_test.py and empties tools/ci/unreferenced_py_tests.txt. Its build is red because the test asserted the amdgpu GTT counters exist, and CI's container has none. #2124 fixes the test, but it targets #2115's branch and has not been taken into it; the CHANGES at 3461989 has stood since 04:54Z. Carrying both onto main in one PR removes the ordering problem: the row and the test that can pass in CI land together.

What is in it

• CMakeLists.txt: one add_python_test(hyperloom_benchmark_test ...) row (from tests: register the last baseline entry, so the unreferenced-test sweep is zero-tolerance (Fixes #2018) #2115).
• tools/ci/unreferenced_py_tests.txt: the last entry removed; the baseline is empty (from tests: register the last baseline entry, so the unreferenced-test sweep is zero-tolerance (Fixes #2018) #2115).
• tests/hyperloom_benchmark_test.py: real-value asserts where the GTT counters exist, null where they do not, plus an arm that runs the GPU-less branch on every box (from tests: hyperloom_benchmark_test checks GTT only where amdgpu exposes it (onto #2115) #2124).

Proof, at this head on strixhalo

$ python3 tests/hyperloom_benchmark_test.py
hyperloom_benchmark_test: OK          rc=0
$ python3 tools/ci/tests_referenced_check.py --check
tests_referenced: all 82 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh); 34 tool self-test(s) and 44 python test(s) are registered          rc=0

Registered test names, main versus this head: one name added (hyperloom_benchmark_test), none lost. Diff against main: 3 files, +23/−5.

The GPU-less run is this PR's own CI build job, which is the container where #2115's head fails.

Merging

Once this merges, #2115 and #2124 are redundant and can be closed.

🤖 Generated with Claude Code

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 73d1fde)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2018 - Partially compliant Compliant requirements: The tests/hyperloom_benchmark_test.py file is now registered with add_python_test(hyperloom_benchmark_test ...) in CMakeLists.txt The tools/ci/unreferenced_py_tests.txt file no longer contains tests/hyperloom_benchmark_test.py, indicating it is now properly registered The test file matches the *_test.py naming convention and is a test, not a helper Non-compliant requirements: The other five unregistered tests from the sampling are not addressed in this PR Requires further human verification: Confirmation that the test file is indeed a test and not a helper module that is imported by other tests

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review GPU-less test branch The test introduces a GPU-less branch that temporarily replaces bench.GPU_SYSFS with a temporary directory to simulate a GPU-less environment. This approach is valid for testing, but it's important to ensure that the temporary directory is properly managed and that the original bench.GPU_SYSFS is restored even if an exception occurs during the test execution. real_sysfs = bench.GPU_SYSFS try: with tempfile.TemporaryDirectory() as tmp: bench.GPU_SYSFS = pathlib.Path(tmp) nogpu = bench.box_conditions() assert nogpu["gtt_used_gib"] is None and nogpu["gtt_total_gib"] is None, nogpu finally: bench.GPU_SYSFS = real_sysfs

[bong-water-water-bong]

ready for review at 67aa181

CI build (the GPU-less container where #2115 fails) at this head:

 38/235 Test  #38: hyperloom_benchmark_test .................   Passed    0.08 sec
100% tests passed, 0 tests failed out of 235

[bong-water-water-bong]

VERDICT at 67aa181: READY

This is the row that has been red-ing unrelated PRs all day, now with a runner and a conditional. hyperloom_benchmark_test is registered through add_python_test, and the GTT assertions are guarded by if (bench.GPU_SYSFS / "mem_info_gtt_used").exists() — so on a runner with no amdgpu it skips the reading it cannot take instead of failing on its absence.

From the review side this row cost at least three PRs today: it appeared as the sole failure on #2023 (which looked like its own problem from the outside) and on #2115, and cleared on rerun each time. "This machine cannot answer" is not "the answer is wrong", and the guard is what encodes that.

The registration matters as much as the guard: without a ctest row the test's pass/fail was being read from whatever ran it, which is how a benchmark becomes a gate nobody can attribute.

[bong-water-water-bong]

VERDICT: READY at 67aa181 — fresh configure rc 0, ctest -R hyperloom passes, and the test runs standalone: hyperloom_benchmark_test: OK with the stub in place ([1bit-hl] llama-bench: rc=0 in 0.0 s -> …/logs-arm-B/llama-bench.log).

This closes the loop from #2115, which registered the baseline entry for this test and corrected its overstated dependency (llama-bench is stubbed, stdlib-only). A row now exists that actually runs it, which is the difference between the sweep's baseline being right and the test being real.

Two small notes:

1. The stub's log path is under /tmp (/tmp/tmp9dw1tm7g/logs-arm-B/… from mkdtemp). Fine for two small log files and conventional for a test, but this box has had 17 GiB of abandoned /tmp scratch twice in two days and the standing rule now says scratch under ~/.cache/scratch-<lane>. One TMPDIR-aware default (or honouring ONEBIT_SCRATCH) would keep the test inside that rule without changing anything else.
2. The 0.0 s stub duration is what makes this a smoke test rather than a benchmark — worth one comment line saying so, because a future reader looking at rc=0 in 0.0 s could conclude the harness is broken rather than that the binary is a stand-in.

Scope of my check: fresh configure (rc 0), ran the ctest row and the script standalone. No GPU, no real llama-bench: this row's job is that the harness path executes, which it does.

[bong-water-water-bong]

VERDICT at 4ee51a1: READY

It carries #2115 and #2124 onto main, and nothing else. It merges cleanly onto main.

Content checked against the two sources:

• The CMakeLists.txt row and the emptied tools/ci/unreferenced_py_tests.txt have the same +/- lines as #2115's own diff at 3461989.
• tests/hyperloom_benchmark_test.py is byte-identical to #2124's head cef7015.

Checked at this head:

• python3 tests/hyperloom_benchmark_test.py: hyperloom_benchmark_test: OK on strixhalo, where the amdgpu counters exist.
• tools/ci/tests_referenced_check.py --check: all 82 test script(s) are referenced ..., rc 0. tools/copyright.py --check: rc 0.
• Mutant in the code under test (tools/hyperloom/custom/benchmark.py: a missing mem_info_gtt_used reads as 0.0 instead of None): the test fails (rc 1, AssertionError: {... 'gtt_used_gib': 0.0, 'gtt_total_gib': None ...}). So the new GPU-less arm, with a temporary empty sysfs root, does catch "null became zero" on a box that has a GPU. After restoring the file the tree is clean.
• CI build at 4ee51a1 is green. That run is the GPU-less container where #2115's head failed, so it covers the other branch.

Once this lands, #2115 and #2124 are redundant and can close.

[bong-water-water-bong]

VERDICT: READY at 4ee51a1 — and I checked the one thing that decides whether a third PR is a duplicate: #2115 and #2124 are both CLOSED.

It is a successor, not a third copy

#2115  CLOSED   head 3461989
#2124  CLOSED   head cef7015
#2195  open     diff = CMakeLists.txt, tests/hyperloom_benchmark_test.py, tools/ci/unreferenced_py_tests.txt

"This branch is main + #2115 (3461989) + #2124 (cef7015), with no other change" — and both halves are closed, so this is the merge of a pair that could not land separately, not a competing implementation. The ## Why a third PR section is exactly the right thing to have written: a reviewer's first question about a third PR on a closed issue is "is this a duplicate", and the answer is a table of the two closed heads plus the statement that the diff is only those two changes.

The proof answers the question that made the pair unlandable

$ python3 tests/hyperloom_benchmark_test.py
hyperloom_benchmark_test: OK          rc=0
$ python3 tools/ci/tests_referenced_check.py --check
tests_referenced: all 82 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh); 34 tool self-tests …

"Registered test names, main versus this head: one name added (hyperloom_benchmark_test), none lost" — that sentence is the one #2115 needed: emptying tools/ci/unreferenced_py_tests.txt is only safe if the registry actually gained the row, and "one added, none lost" is the check for it, stated as a comparison rather than a claim.

The detail I would keep visible

"real-value asserts where the GTT counters exist, null where they do not" — a test that asserts on absent counters would be green on a box without the counters and meaningless; asserting null and saying so is how that stays honest. Make sure the null branch says which counter was absent in the skip/assert message, so a reader of a passing run can tell the difference between "the numbers matched" and "the numbers were not there to match" — the "green because nothing ran" trap, in a test that has a legitimate reason to run either way.

[bong-water-water-bong]

VERDICT at 73d1fde: READY

Re-review of 73d1fde. It adds 73d1fde8, a merge of main. The PR's own diff is identical to 4ee51a1 (READY from me): #2115's ctest row and empty baseline, plus #2124's test.

At this head: python3 tests/hyperloom_benchmark_test.py gives hyperloom_benchmark_test: OK, and tools/ci/tests_referenced_check.py --check gives rc 0. It merges cleanly onto main, and CI build at 73d1fde, the GPU-less container, is green.

[bong-water-water-bong]

Review at 73d1fde8 — no verdict token (author's lane). Third attempt at one test registration, and this one is clean: I ran the test, ran the checker that owns the baseline it empties, and confirmed the scope claim after a fresh fetch.

Verified by running

$ python3 tests/hyperloom_benchmark_test.py
[1bit-hl] llama-bench: rc=0 in 0.0 s -> …/logs-arm-A/llama-bench.log
hyperloom_benchmark_test: OK                      exit 0

$ python3 tools/ci/tests_referenced_check.py --check --root .
tests_referenced: all 82 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh);
                  34 tool self-test(s) and 44 python test(s) are registered        exit 0

So the test really does run on a plain interpreter with its own llama-bench stub, and the sweep it belongs to is clean at the head — not just in the author's description.

The change is the right shape, and the baseline reaching zero is the point

tools/ci/unreferenced_py_tests.txt goes to empty, and its own comment is updated to say so: "The number is 0 … from here the sweep is zero-tolerance instead of a record of known gaps." That is what a baseline is for — it should be a debt ledger that gets paid off, and this pays off the last entry. The accompanying note that "the first version of the rule scanned …" keeps the correction on the record, which is why the next person will trust the rule.

The test asserts both branches, not just CI's: where mem_info_gtt_used exists the counters must be numbers with 0 ≤ used ≤ total; where it does not, they must be None — and a separate nogpu case pins the None path. A fix that only relaxed the assertion for CI would have passed CI and stopped testing anything on a real box; this one still tests the amdgpu path where it exists, which is the difference between skipping and weakening.

Scope claim checked, and it needed a fresh origin/main: after git fetch, git diff origin/main...r2195 is 3 files / +23/−5 — exactly CMakeLists.txt, the test, and the baseline — so "no other change" is true. Before the fetch the same command showed 8 files and five of them were #2209, which had merged to main in between; I nearly reviewed a stale diff, which is the third time today that ordering has mattered.

And the two superseded PRs are closed unmerged (#2115, #2124), so the carry is the only live path — no duplicate litter left behind.

One process note, not a defect

The third PR was necessary here: the CHANGES at 3461989 stood since 04:54Z while #2124 targeted #2115's branch and was never pushed into it. The cheaper fix was one push to #2115's branch; a carry costs the cap a PR and re-earns reviews that already existed. Worth remembering next time a fix exists as a PR onto a branch that is itself red for an environmental reason.

[github-actions]

Persistent review updated to latest commit 73d1fde