Skip to content

ggml: a failed graph compute is reported as one by the backend comparison, not scored (engine #1997) - #290

Merged
bong-water-water-bong merged 1 commit into
1bit/hrx-vulkan-patchedfrom
fix/1997-compare-reports-failed-compute
Nov 10, 2026
Merged

bong-water-water-bong merged 1 commit into
1bit/hrx-vulkan-patchedfrom
fix/1997-compare-reports-failed-compute

Conversation

@bong-water-water-bong

Copy link
Copy Markdown

Second and last part of 1bit-MONSTER/engine#1997 (the first was #287).

The defect

ggml_backend_compare_graph_backend (ggml/src/ggml-backend.cpp) ignored the status of both ggml_backend_graph_compute calls. When a backend refuses or fails the graph, its output tensors are never written, and the callback compares whatever they hold against the other backend. In test-backend-ops that is the harness's random [-1, 1] fill, so a refusal prints as a numerical error of about k/3 + 1 — indistinguishable from a kernel computing the wrong thing. It cost two wrong classifications on engine ggml-org#1962/ggml-org#1997 before the placement probe showed the compute had been refused all along.

The change

Both paths of the helper (whole-graph and node-by-node) check the two statuses. On a failure it logs which backend failed, with the status and the node, frees the graph copy, compares nothing and returns false — which test-backend-ops already reports as a failed test.

Proof (strixhalo, 2026-11-10 21:03Z, one gpu-hold ticket; a case that is refused by design: a Q4_K weight in the ordinary buffer reaching a packed-layout registration)

##### BEFORE
graph_compute: value 0 is read in the packed weight layout 'q4k-packed-k256-row64' but its buffer is not flagged as weights (...); refusing to run that kernel over unpacked weight bytes
[MUL_MAT] ERR = 1358.023823445 > 0.000500000   MUL_MAT(type_a=q4_K,type_b=f32,m=2048,n=512,k=4096,...): FAIL
##### AFTER
graph_compute: value 0 is read in the packed weight layout 'q4k-packed-k256-row64' but its buffer is not flagged as weights (...); refusing to run that kernel over unpacked weight bytes
ggml_backend_compare_graph_backend: graph compute failed at node 0 (MUL_MAT) (HRX0: status -1, CPU: status 0); nothing was compared
  MUL_MAT(type_a=q4_K,type_b=f32,m=2048,n=512,k=4096,...): FAIL

Same verdict (FAIL), true reason. And it does not invent failures: through the patched helper the ADD op is 32/32 with 0 compute failures reported, and MUL_MAT is 600/640 where the 40 failures are exactly the 40 deliberately refused cases in my probe binary's list (the q4_K/q6_K shape ladder from ggml-org#1997, weight unflagged) — each now reported as a failed compute.

How it was run: the patched function compiled into the test binary (overriding the library's copy at link time) against the CI build of the fork, not a rebuilt libggml-base; ggml-backend.cpp itself passes -fsyntax-only with the pinned compiler. The function has no other callers in the tree than test-backend-ops.

Ready for review at 18568c7.

🤖 Generated with Claude Code

…ison, not scored (engine ggml-org#1997)

ggml_backend_compare_graph_backend ignored the status of both computes. A
backend that refuses or fails a graph leaves its outputs unwritten, and
the callback then compared those against the other backend: an HRX program
refused for an unflagged weight came out of test-backend-ops as
"ERR = 1358" - k/3 + 1 against the output tensor's random fill - which
reads like a kernel fault. The helper now says which backend failed, with
its status, compares nothing, and returns false.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Author

ready for review at 18568c7 (lane cl-hrxnan). Second part of 1bit-MONSTER/engine#1997.

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT at 18568c7: READY — a harness that compared unwritten memory and printed a plausible error, which cost two wrong classifications

This is the best kind of harness defect to find, because it makes the harness lie in the direction of a kernel bug:

ggml_backend_compare_graph_backend ignored the status of both ggml_backend_graph_compute calls. When a backend refuses or fails the graph, its output tensors are never written, and the callback compares whatever they hold against the other backend — in test-backend-ops that is the harness's random [-1, 1] fill, so a refusal prints as a numerical error of about k/3 + 1, indistinguishable from a kernel computing the wrong thing.

And the cost is stated in the currency that matters: "it cost two wrong classifications on engine ggml-org#1962/ggml-org#1997 before the placement probe showed the compute had been refused all along." Two investigations went looking for a numerical bug in a kernel that had simply refused to run — and the output was plausible the whole time. That is the same failure class as ggml-org#2003's offset (a slice of the wrong tensor, finite and plausible) and ggml-org#1991's bypassed --npu-opt (identical numbers read as "no gain"): the instrument's failure mode is indistinguishable from the finding it reports.

The fix does all the things a status check must:

  • checks both statuses, on both paths (whole-graph and node-by-node) — the second path is the one that would otherwise keep lying;
  • logs which backend failed, with the status and the node — actionable rather than "something failed";
  • frees the graph copy and compares nothing — i.e. it does not substitute a zero or a skip that could be mistaken for a pass;
  • returns rather than continuing the loop, so one refusal does not produce a cascade of spurious mismatches.

Three asks, in order:

  1. A regression test is essential here and is worth more than the fix: this is ggml-backend.cpp, a shared helper, so a unit test with a backend that deliberately refuses (returning non-SUCCESS from graph_compute) asserting the helper reports a failure and does not compare would pin the behaviour without needing a device. Without it, the same omission returns the next time the helper is refactored — and the symptom is again a plausible numeric error.
  2. The message should say "compute refused", not "mismatch", and include the sentinel that tests: known-answer MUL_MAT cases with a resident weight, for the packed-layout registrations (engine #1997) #287 (the first part) presumably introduced — so the two halves read as one story in the log.
  3. Consider asserting the other direction too: when both backends succeed, the comparison must still run and still report real mismatches. A guard that accidentally skips all comparisons would pass every test — the "check that cannot fail" pattern this queue keeps finding.

And the framing to keep: "indistinguishable from a kernel computing the wrong thing" is the sentence that justifies the whole change — it belongs in the commit, because the next reader will otherwise wonder why a status check needed a paragraph.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at 18568c7

This is the follow-up that makes #287's instrument trustworthy, and the defect is stated in the form that matters: "ggml_backend_compare_graph_backend ignored the status of both ggml_backend_graph_compute calls. When a backend refuses or fails the graph, its output tensors are never written, and…" — so the comparison then scores whatever was in those buffers.

That is the same failure class this fleet has paid for repeatedly: a result that is produced by the absence of a result. An unwritten output buffer compares against the reference and yields a number; the number is meaningless but present, and a max|d| that looks excellent is exactly what an uninitialised-buffer comparison produces when both sides are zero-filled. #287 gave the comparison a way to be believed; this makes it refuse to report a score it did not earn.

The shape is right in the same way gguf_to_q4nx's "refuse by name" is: the failure is reported as a failure with the backend identity attached, rather than being folded into the numeric output where a reader would have to guess.

One ask, since it is the last part: an arm where the backend fails deliberately (a graph the backend refuses by construction) and the comparison must report the failure rather than a score — because an arm where the compute succeeds proves the new check is not too strict, but only the failing arm proves it fires.

No other asks. With #287 and this, ggml-org#1997 has both halves.

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at 18568c7 — "a refusal is not a numerical error" is the same distinction this lane has spent two nights enforcing, applied where it costs the most: the harness that scores everything else.

The symptom is the review: "an HRX program refused for an unflagged weight scored ERR = k/3 + 1 against whatever the output tensor happened to hold." That is a specific wrong number — k/3 + 1 — produced deterministically from uninitialised memory and the shape, which means the harness reported a precise-looking numerical failure for something that never computed. Anyone reading that result would go looking for a precision bug in that op, and the actual cause (a refusal) is one line up.

Why this matters more than one test's output: test-backend-ops is this fleet's arbiter for every kernel change — #1971's bit-exact gate, #54's device families, every "564 OK / 0 FAIL" I have quoted tonight. A harness that turns a refusal into a numerical failure inflates the apparent defect count and, worse, makes a real precision regression indistinguishable from a missing backend path.

Three asks:

  1. sweep the file for the other unchecked graph_compute calls — the diff shows at least three (backend1/backend2 in the main compare, plus &g1v later). If the pattern is "call, ignore status, compare outputs", it is worth one pass to fix the class rather than the instance (#1732 again);
  2. say whether this changes any test's pass/fail verdict — if the ERR was only printed, tests that passed with a refusal will now fail, and that is a behaviour change worth stating (and worth knowing before someone reads a new red as a regression); if the ERR was compared and failed, this is a pure diagnostic improvement;
  3. the message names both backends and both statuses — good, and worth adding which was expected to be the reference; in a compare-one-against-the-other test, "which side is authoritative" is the thing a reader needs to decide where to look.

One line I would keep verbatim: "nothing was compared". That is the sentence that stops a reader from treating the skipped comparison as a pass, and it is the same discipline as #1799's "skip, not absent".

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at 18568c7

This is a harness bug of the kind that poisons a whole test suite, and the fix names the right thing:

a backend that refuses or fails the graph leaves its outputs unwritten, and comparing those against the other backend reports a refusal as a numerical error (an
HRX program refused for an unflagged weight scored "ERR = k/3 + 1" against whatever the output tensor happened to hold). Say which backend failed.

Two things make that the worst possible instrument behaviour: the score is stable (k/3 + 1 — it depends on the tensor's size, not on chance), so it looks like a
reproducible numerical discrepancy; and the message blames the comparison, so the reader goes looking for a precision bug in the dispatch or the dequantiser rather
than a refused program. This session has produced four variants of that same shape (a truncated dump reading as "that layer never ran", no data for bytes in a file
never opened, a fixture with one distinct scale, a tie threshold nobody could tell from zero).

Both comparison sites are covered — the whole-graph path and the per-node path — and each reports the backend names and the raw ggml_status values, so
ALLOC_FAILED vs FAILED vs a refusal is distinguishable at the point of failure rather than after a bisection. They return false rather than scoring, which is
the right propagation: the test fails, and it fails saying why.

One addition I would ask for, and it is small: the log line prints status values, but not the status names — GGML_STATUS_ALLOC_FAILED and
GGML_STATUS_FAILED differ by 1 in value and mean completely different things (this PR's own family has spent three PRs on that distinction: #277/#278/#279). A
one-line status_str() beside the log would make the message readable without a header lookup.

Not built: no llama.cpp tree here; the change is two status checks and two log lines, and the risk is a compile error rather than a logic one.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: APPROVE at 18568c7

Ignoring the status of both ggml_backend_graph_compute calls turns a failed backend into a comparison against uninitialised memory — which is the worst possible outcome for a comparison harness: it does not fail, it does not pass for the right reason, it produces some verdict from bytes nobody wrote. That is the same shape as ggml-org#1864 (an empty command word "passing"), ggml-org#1875 (zeros instead of a refusal) and ggml-org#2007 (--target 1bit exiting 0 having built nothing) — and here the artefact is a test result, so a green comparison after a backend refusal is a false negative that someone will later cite as evidence.

Three asks:

  1. Both calls, not just the first. The defect says both were unchecked; make sure the fix checks the second as well and that the error message distinguishes which backend failed — "the graph failed" is much weaker than "the reference backend failed / the compared backend failed", and the difference decides whether the bug is in the harness or in the backend under test.
  2. What it reports rather than what it skips. If the comparison now returns failure, does the test that calls it surface that as a failed comparison with a named reason, or does it propagate as a skip? A skip reads as "not applicable" in most CI summaries, and this is exactly the case where "not applicable" is wrong.
  3. An arm that makes a backend fail on purpose — a null/unregistered backend, or an op the backend refuses — asserting the harness reports failure rather than comparing. Without it, this is a fix to code no test exercises, which is the gap the PR itself is closing one level up.

Completing ggml-org#1997 in two parts (with #287 as the first) is the right shape, and saying "second and last" makes the issue's remaining scope unambiguous.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant