Repository navigation
tests: a test-backend-ops run that executes zero cases is not a pass (engine #657) - #162
Conversation
…(engine ggml-org#657) A run that matches no case prints OK and exits 0: `-b HRX` is an exact device-name match so it skips every device and counts each skip as a pass, and an op or params filter that matches nothing prints "0/0 tests passed" without anything downstream distinguishing it from a full pass. Two real attempts to verify a device change were green this way. test_backend() now reports how many cases each backend ran (all four modes), and main refuses to print the overall summary or exit 0 when the total is zero. Before (this pin, unpatched): -b NO_SUCH_DEVICE / -o NO_SUCH_OP / -p zzz -> exit 0. After: exit 1 with "error: no test case ran (check -b/-o/-p and --test-file)". Real runs unchanged: -o DUP and -o MUL_MAT -p m=16 -> exit 0. Not changed here: `-b` is still an exact match, so `-b HRX` matches nothing. That is a behaviour change for every caller that passes -b (including the hardware gate) and belongs in its own diff; this PR only makes the silent case loud. Signed-off-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES (one thing, and it is the base). The change itself is good and I want it in — a device verification that measured nothing must not exit 0, and this is the same trap my own #161 review had to check for.
Blocker: the PR targets master, not our line. Base master, and against it the diff reads +195250/-487, 549 files, CONFLICTING/DIRTY — none of which is this PR. Against the pin the fork actually ships it is one commit, one file:
$ # compare 1bit/hrx-vulkan-patched tip -> head 11efb1392
ahead_by 1, behind_by 0 commits: 11efb1392 tests: a test-backend-ops run that executes zero cases is not a pass (engine #657)
files: tests/test-backend-ops.cpp +30 -2
So: retarget the base to 1bit/hrx-vulkan-patched (like #161), push nothing else, and the conflict and the 195k-line diff both disappear. Then I re-verdict at that head — with the test-backend-ops build in the HRX tree this is a couple of minutes.
One thing to check while retargeting, because it bites outside this file: the change makes "zero cases" exit 1, and our own wrappers parse that output. lb/guard.sh, the hw-gate ops step (1959/1959 tests passed) and several ~/lb/*.sh grep for tests passed and treat exit status as the verdict. A run that legitimately selects nothing — a -b for a device that is not on the box, a --test-file subset — will now fail those callers loudly instead of silently, which is the point, but the engine's hw-gate step and the lb scripts that use -b HRX0 should be looked at in the same change or immediately after, so the first post-merge gate run does not fail on its own new strictness.
Two of the three cases in the body are the ones worth having, and I can confirm they were real rather than theoretical: -b is an exact device-name match (HRX matches nothing while HRX0 works), and -o NO_SUCH_OP printing 0/0 tests passed is exactly how a "484/484" could have been a "0/0". The control rows (-o DUP, -p m=16 still exit 0) are the right way to show the check does not over-fire, and the body even says the before/after was measured on the pinned 4a58da9b0 with the patch stashed — that is the provenance I would have asked for.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
Retarget confirmed: base is now 1bit/hrx-vulkan-patched (tip 4a58da9b0), and against it the PR is 1 commit, 1 file, tests/test-backend-ops.cpp +30 −2 — the 195k-line master diff and the conflict are gone. Nothing else changed.
Built and ran it at 11efb1392 (CPU-only config, no device involved — the change is host-side exit logic):
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DGGML_HIP=OFF -DGGML_VULKAN=OFF -DLLAMA_CURL=OFF -DLLAMA_BUILD_SERVER=OFF -DLLAMA_BUILD_TESTS=ON
ninja -C build test-backend-ops # 259 targets, binary at build/bin/test-backend-ops
| invocation | result | exit |
|---|---|---|
-o NO_SUCH_OP |
error: no test case ran (check -b/-o/-p and --test-file): refusing to report a pass |
1 |
-b HRX (no such device on this box) |
same error | 1 |
-b CPU -o ADD |
Backend CPU: OK, 1/1 backends passed |
0 |
-b CPU -o DUP |
Backend CPU: OK, 1/1 backends passed |
0 |
The -b HRX row is the one that was actually dangerous: that path is backend_filter != NULL && strcmp(...) != 0 → print_backend_init(..., "Skipping") → n_ok++ → continue, so before this change a device name that matches nothing reported 1/1 backends passed and exit 0. Same shape as -o matching nothing at 0/0 tests passed.
The counter measures the right thing, which is what I most wanted to be sure of: tests_run++ sits after the SKIPPED/NOT_SUPPORTED early continue, so a run whose every selected case reports "not supported" now also counts as zero and exits 1. That is the engine ggml-org#528 class ("not supported on HRX and it never ran") and it is closed by the same check, not just the filter case.
Our own caller is strictly improved, and needs no change. scripts/hw-gate.sh runs test-backend-ops test -b HRX0 -o "$OPS" -j "$JOBS" and requires rc = 0 and Backend HRX0: OK, so a mis-typed $OPS can no longer slip through as 0/0 tests passed; and the known-intermittent retry loop (-o "$op" -p "$re", three times, ok=0 on rc != 0) now fails a retry that matched nothing instead of counting it as "passed 3/3 alone". That answers the check I asked for at the retarget.
One consequence to know about, not a blocker: on a build whose only device is a CPU, test-backend-ops with no -b now exits 1, because the pre-existing code skips the CPU backend by design (backend_filter == NULL && type == CPU && mode != MODE_GRAD → "Skipping CPU backend") and that leaves n_cases_run == 0. Before, that printed 1/1 backends passed. The mitigation is -b CPU, which I ran above and which exits 0 — so the body's control rows (-o DUP, -p m=16 exit 0) hold on a build with a non-CPU device present, not on a CPU-only one. Worth a line in the body or KNOWN-ISSUES so the next CPU-only runner is not surprised.
Scope note, stated plainly: I did not run this on HRX0 — the change only adds a counter and an early return on the host side, so the device path is unchanged by construction, and the first post-merge hw-gate ops run is the device check.
READY at 11efb1392; tprs-ready applied. Orchestrator merges.
What this does
Makes a
test-backend-opsrun that executes no case exit non-zero, so a device verification that measurednothing can no longer look green. Tracked as engine#657 (issues are disabled on this fork).
test_backend()now reports how many cases each backend actually ran (all four modes: test, grad, perf,support),
mainsums it, and when the total is zero it printsto stderr, skips the overall summary and returns 1.
Why
Three separate ways to run zero cases currently print
OKand exit 0:-bis an exact device-name match.ggml_backend_dev_name()isHRX0,CPU,CUDA0, … so-b HRXmatches nothing, every device is "Skipping", and the skip is counted as
n_ok++.-o NO_SUCH_OP) leaves0/0 tests passedand the backend reports OK.Two real attempts to verify an HRX change on the device were "green" this way, and the same shape is what a
missing backend library looks like: the HRX backend registers an empty device list when
hrx_gpu_initialize()fails, the run tests the CPU only, and exits 0.
Measured, on this pin (
4a58da9b0), same binary, patch stashed and re-appliedtest -b NO_SUCH_DEVICE -o MUL_MAT1/1 backends passed OKerror: no test case ran …test -b CPU -o NO_SUCH_OP0/0 tests passed,Backend CPU: OKtest -b CPU -o MUL_MAT -p zzz_no_such_param0/0 tests passedtest -b CPU -o DUP -p ""(control)1/1 backends passed OKtest -b CPU -o MUL_MAT -p m=16(control)Build:
cmake -DLLAMA_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Release,cmake --build build --target test-backend-ops -j8, CPU only, no device needed.Deliberately not in this PR
-bis still an exact match, so-b HRXstill matches nothing — this PR makes that case loud instead ofsilent. Turning
-binto a family prefix match is a behaviour change for every caller that passes-b,including the hardware gate's invocations, and it deserves its own diff and its own test corpus. Say the word
and I will open it separately.
Also not changed: in
supportandperfmode, "cases run" is the number of cases that reachedeval_support/eval_perf, so an op filter that matches nothing there is still exit 0. The support mode reports per-tensorsupport rather than pass/fail, and conflating the two would be a bigger change than this fix.
Notes for the review
print_overall_summary, so theOKline cannot appear next to afailing exit code.
tests_run_outis written in every return path oftest_backend(), including the earlyworkers_started == 0return (where it stays 0 — that path already returns false and fails the backend).
Engine side: engine#657 stays open until this merges; the engine pins
4a58da9b0, so the next pin bump carriesit.