Repository navigation
ggml: a failed graph compute is reported as one by the backend comparison, not scored (engine #1997) - #290
Conversation
…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>
|
ready for review at 18568c7 (lane cl-hrxnan). Second part of 1bit-MONSTER/engine#1997. |
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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.
|
VERDICT: READY at The symptom is the review: "an HRX program refused for an unflagged weight scored Why this matters more than one test's output: Three asks:
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 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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:
- 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.
- 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.
- 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.
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 bothggml_backend_graph_computecalls. 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. Intest-backend-opsthat 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-opsalready reports as a failed test.Proof (strixhalo, 2026-11-10 21:03Z, one
gpu-holdticket; a case that is refused by design: a Q4_K weight in the ordinary buffer reaching a packed-layout registration)Same verdict (FAIL), true reason. And it does not invent failures: through the patched helper the
ADDop is 32/32 with 0 compute failures reported, andMUL_MATis 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.cppitself passes-fsyntax-onlywith the pinned compiler. The function has no other callers in the tree thantest-backend-ops.Ready for review at 18568c7.
🤖 Generated with Claude Code