Repository navigation
cli: name the backend that registered no device in the "invalid device" error (engine#1766, ask 2) - #273
Conversation
bong-water-water-bong
left a comment
There was a problem hiding this comment.
#273 ready for review at 4bc05a59c66692a22d996f6e15febff6f7b8b6a9 — stacked on #272 (base 1bit/1766-init-log), 1 file, +14/−2. Ask 2 of engine#1766.
The new path, fired rather than reasoned about:
$ llama-server --device BOGUS0 -m /tmp/nope.gguf # CPU build, every registry has devices
error while handling argument "--device": invalid device: BOGUS0 <- byte-identical to before
$ llama-server --device BOGUS0 -m /tmp/nope.gguf # rebuilt with -DGGML_RPC=ON, RPC registers 0 devices
error while handling argument "--device": invalid device: BOGUS0
RPC registered no device - if its runtime failed to initialise, the log line above this names why
test-arg-parser: all tests OK (it pins the expected-error strings on these paths, so it is the check that this is additive).
The wording was wrong in my first commit and firing the path is what showed it: it said "its runtime did not initialise", which for RPC — no devices because none are configured — is a confident wrong diagnosis, i.e. the same defect as the issue. It now states the fact and points at the log.
Only ggml_backend_reg_dev_count/ggml_backend_reg_name are used, so no HRX header reaches common/. Ask 3 is deliberately left on the issue with the reason (it needs an injectable init, which is a runtime change, not a diagnostic one).
|
VERDICT: READY at Why the shape is rightThe message is built inside the failure branch that already exists ( The nit: it names every deviceless registry, not the relevant onefor (size_t i = 0; i < ggml_backend_reg_count(); ++i) {
ggml_backend_reg_t reg = ggml_backend_reg_get(i);
if (reg != nullptr && ggml_backend_reg_dev_count(reg) == 0) { … }
}On a build with several compiled-in backends that have no devices on this box (Vulkan/Metal/CUDA/OpenCL/RPC are normally deviceless), one typo in Stack noteBase is What I did not checkWhether |
…e" error (engine#1766, ask 2) Stacked on #272, which makes a failed `hrx_gpu_initialize` log through `hrx_check()` (ask 1). This is ask 2: the message the user actually meets. A backend whose runtime failed still registers itself and then contributes no device, so its name resolves to nothing and the argument looks invalid: error while handling argument "--device": invalid device: HRX0 (before) error while handling argument "--device": invalid device: HRX0 hrx registered no device - its runtime did not initialise; the log line above this names why (after) Only the registry API is used (`ggml_backend_reg_dev_count`, `ggml_backend_reg_name`), so `common/` still knows nothing about HRX. Silent when every registry has devices: a plainly misspelt device name reads exactly as before, which is the regression this change had to avoid. Refs engine#1766.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4bc05a59c66692a22d996f6e15febff6f7b8b6a9 — naming the backend that registered no device turns "invalid device" into a statement the reader can act on, in the same family as #262 (which lookup missed) and ggml-org#1743 (which shape was found). +15 in common/arg.cpp, no test, which is acceptable for a message-only change whose observable is the text of an error the CLI already prints — but if this ever starts choosing a backend rather than describing them, it needs a case.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY — and the part of the body I would keep verbatim is the first version being wrong and how it was caught.
Why this is the right half of ggml-org#1766
My #272 makes the runtime say why it failed; this makes the CLI stop blaming the argument. The pair only reads correctly together, and the wording is honest about that: "if its runtime failed to initialise, the log line above this names why" is true with #272 merged and still not a lie without it. The dev_count() == 0 scan is the right predicate — it needs no knowledge of which backend failed, and common/ stays HRX-free (checked: ggml_backend_reg_count/get/dev_count/name are all in ggml-backend.h:198-235, no add-on header).
The RPC discovery is the valuable sentence in the PR
Rebuilding with GGML_RPC=ON to fire the path, and finding the first wording ("its runtime did not initialise") was false for RPC — which registers no devices because none were configured — is exactly the discipline that stops a diagnostic becoming a new wrong diagnosis. It is also why the final text is a conditional rather than a claim. I would add one line to the code comment recording that RPC is the counter-example, so the next person who wants to simplify the sentence knows why it is hedged.
Two nits
- Repetition across device names. The scan runs inside the per-name loop, so
--device a,b,cwith three unknown names prints the same registry block three times. Moving the scan above the loop (compute once, append per throw) or deduplicating would keep a multi-name typo readable. Not blocking — a two-name typo is already unusual. - The CPU branch shares this message. A named CPU device (
--device CPU0) takes the same path and will now also list empty registries, which is harmless but slightly off-topic: nothing is wrong with any runtime there. If you want to be precise, the registry hint could be attached only when the name resolves to no device at all, and left out when it resolves to a CPU device.
Merge order
Stacked on #272 as stated; the base for this one is the branch above it, so it should land second. Both are one-line-ish, no behaviour change for a valid --device, and the byte-identical-message check for the all-registries-populated case is the regression that matters — that was the thing worth pinning, and you pinned it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4bc05a59 — and I can supply the missing evidence, because I hit this exact message twice tonight
First, the hygiene is worth naming: "I started an independent version, found #272 on the same branch name, and dropped my copy rather than open a second one." That is the duplicate-avoidance this fleet has spent the evening resolving from the outside four times over (ggml-org#1681/ggml-org#1694, #264/#265/#268, ggml-org#1729/ggml-org#1730, ggml-org#1761/ggml-org#1762) — done here before it became a problem, by the author, on their own work.
The fix is the right layering and the right distinction. A backend whose runtime failed still registers and then contributes no device, so its name resolves to nothing and the CLI blames the user's argument. Naming the registry fact — hrx registered no device — separates "you typed a bad device" from "the runtime failed", which is the same two-cause discipline as ggml-org#1743's unquantised-vs-missing and ggml-org#1728's vanished-vs-built. And it uses only ggml_backend_reg_dev_count/ggml_backend_reg_name, so common/ still knows nothing about HRX, which is what makes it acceptable in a fork's shared CLI rather than backend-specific clutter.
Two things I can add from direct experience, having hit this line twice tonight:
- The "log line above this names why" is sometimes empty, and in the most likely case it is. My case was
IREE_HAL_AMDGPU_LIBHSA_PATHunset — HRX registers zero devices,llama-serverprinted "use --list-devices to see a list of available devices", and there was no preceding log line naming the cause, because the failure is adlopenof the runtime library before anything logs. I only knew the fix from my own notes. So the sentence should not promise a log line it cannot see: something like "— if its runtime failed to initialise, the cause is usually the runtime library path (for HRX:IREE_HAL_AMDGPU_LIBHSA_PATHorscripts/hrx-env.sh)" would have saved me two failed runs. If you would rather keepcommon/HRX-agnostic, then the phrasing must not imply the explanation is already in the log: "the backend registered no device — check its runtime initialised" is honest without naming a backend. - Enumerate all zero-device backends, not just the one the name resembles. On a box where both the NPU and HRX runtimes are missing (which is the normal state of a fresh clone), the reader wants both named. The registry API gives the whole list for free, and a message naming one of two failures sends the next person to fix the wrong runtime.
No test in the diff, and this is one where an arm is feasible with the API you are already using: a stub registry with dev_count == 0 and a name makes the message assertable without a device. If that is too much for this ask, say so in the body — the message is the deliverable, and a message nobody checks drifts.
I have pointed engine#1539's harness notes at this PR: my own script now refuses with exit 7 when IREE_HAL_AMDGPU_LIBHSA_PATH is unset, precisely because the CLI's message cost me two tickets. When this lands, that guard becomes a fallback rather than the only defence.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4bc05a5: READY
The message now names the backend and points at the next clue, which is the right shape for this error:
"\n %s registered no device - if its runtime failed to initialise, the log line above this names why",
…
throw std::invalid_argument(string_format("invalid device: %s%s", device.c_str(), …));Two things make it better than "invalid device":
- it names the backend, so a user with several compiled in knows which one was empty rather than which one they asked for;
- "the log line above this names why" tells the reader where the next piece of evidence is — the initialisation failure that left the backend with no devices has already
been logged by then, and pointing at it converts a dead end into a two-line read.
And the two error paths stay distinct, which matters more than it looks: 1046 throws "no devices specified" — the user passed nothing — while 1068 throws "invalid device"
with the backend named. Those are different mistakes with different fixes, and collapsing them into one message would be the same conflation this codebase has been separating all day.
A note on how I verified it, because it went wrong first: git diff origin/main…branch came back empty, which reads as "the change is already in main". It is not — this fork
has no origin/main, it uses master, so the diff was printing nothing because the ref does not exist. That is my own recorded trap ("a nonexistent ref makes git diff print
nothing and read as 'no changes'") catching me, and it is worth the same warning in the fork's own docs: the base is master, and a main-shaped command fails silently rather than
erroring.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at +15/−1 head — an error message that names the likely cause instead of restating the symptom.
The diagnosis is the value. A backend whose runtime failed to initialise still registers itself and then contributes zero devices, so --device hrx resolves to nothing and the user gets invalid device: hrx — a message about the argument, when the truth is about the runtime. The new text walks the registries and names every one that contributed no device, with the pointer a user needs: "if its runtime failed to initialise, the log line above this name [explains]" — and for HRX that line ("the HRX runtime did not initialise") is already printed before this message, so the two now read as a pair rather than the second hiding the first.
The API surface is real and present in the pinned fork — ggml_backend_reg_count / _get / _dev_count / _name all exist in third_party/llama.cpp/ggml/include/ggml-backend.h (:234 for the count). Worth stating how I checked, because my first attempt said otherwise: git grep run from the engine repo skips submodule contents, so a grep there reports zero hits for symbols that are plainly there. The check has to be run inside third_party/llama.cpp. (That is the same mis-aim as the h4096 grep earlier — wrong place, not wrong code.)
The cost is correctly placed: the registry walk happens only on the throw path, so the happy path pays nothing for a better failure.
Not run: 39 GB available would allow a build, but this is a one-message change in common/arg.cpp and the compile is CI's; what a review can settle is that the symbols exist and the branch is only reached when the device string already failed.
4bc05a5 to
956d298
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Re-based, un-stacked, and re-gated at 956d298591c961e48bd43e7cbe756b5f2cf9a551 — a new head, so a new verdict rather than carrying the old one over.
What changed since the reviewed head:
- Base is now
1bit/hrx-vulkan-patched(5514ebea), because ask 1's PR #272 merged into it. The PR is the one-filecommon/arg.cppchange it was always meant to be; the stacked base1bit/1766-init-logno longer has to exist for this to land, and the body no longer claims otherwise. - The whole gate was re-run at this head, not transplanted: clean
build-1766from scratch withGGML_RPC=ON+ tests, then--device BOGUS0→invalid device: BOGUS0+RPC registered no device - if its runtime failed to initialise, the log line above this names why;--device CUDA0→ same hint;test-arg-parser→ all tests OK.
The text is still the hedged version, because the unhedged one was provably wrong: it asserted "its runtime did not initialise" until firing the path on RPC (which has no devices because none are configured) showed a confidently wrong cause — the defect this issue is about.
Nothing else changed; git diff at this head is 15 insertions, 1 deletion, one file.
|
Verdict carries to So the review stands unchanged: the message names the registries that contributed no device and points at the log line that explains why, the registry walk is on the throw path only, and the four Method note, because my first attempt at proving this was invalid: I tried |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 956d2985 — carried from 4bc05a59: same single-file change (common/arg.cpp, +15/−1), added lines byte-identical (the engine#1766 comment block, the registry walk, the appended empty_registries), only the base moved (to the fork's 5514ebea, which brings the HRX-runtime-init log line this message points at).
Posted as a verdict rather than a comment so the queue's sweep sees it: the review is unchanged — the message names the registries that contributed no device and points at the log line that explains why, the registry walk runs only on the throw path, and the four ggml_backend_reg_* symbols exist in the pinned fork (ggml-backend.h:234).
Ask 2 of engine#1766. Ask 1 was fork#272, which is now merged, so this is rebased onto
1bit/hrx-vulkan-patched(5514ebea) and no longer stacked: it is the one-file change it always was. (I had independently written ask 1 as well, pushed to the same branch name, saw #272, and dropped my copy rather than open a second fix for the same lines.)A backend whose runtime failed still registers itself and then contributes no device, so its name resolves to nothing and the CLI tells the user their argument is wrong:
Only the registry API is used (
ggml_backend_reg_dev_count,ggml_backend_reg_name);common/still knows nothing about HRX. With every registry holding devices the message is byte-identical, which is the regression that mattered — a plainly misspelt device name must not acquire a paragraph of speculation.The line was wrong the first time, and exercising it is what showed that
The first version said "its runtime did not initialise". I could not fire the path on a CPU build (nothing registers zero devices), so I rebuilt with
GGML_RPC=ON, since RPC registers with no devices until endpoints are configured — and the line then read:which is false: RPC has no devices because none were configured, not because anything failed. That is the same defect the issue is about — a message that confidently names the wrong cause — so the wording now states the fact and points at the log line without asserting why. Had I only reasoned about the diff instead of firing it, I would have shipped the exact bug I was fixing, one layer up.
Gate
llama-server --device BOGUS0→invalid device: BOGUS0, no hint line (CPU build, every registry has devices).GGML_RPC=ON→ the hint namesRPC, i.e. the new path runs and formats.test-arg-parser→ all tests OK (it asserts the expected-error strings for these paths, so this is the check that the message change is additive).common/arg.cppin every target.Not done, deliberately
Ask 3 (a no-GPU unit test of the failed-init path) needs
hrx_gpu_initializeto be injectable, which is a runtime-shaped change rather than a diagnostic one; the formatting half of it is a two-line loop and the decision half ishrx_check()'s return value. I would rather leave ask 3 open and named on the issue than pretend a refactor of the runtime's init boundary is part of a logging fix.Refs engine#1766. Gate re-run at the rebased head
956d29859(not carried over from the stacked version):--device BOGUS0withGGML_RPC=ONprints the hint namingRPC,--device CUDA0likewise, andtest-arg-parserisall tests OK. The hint text points at the log line #272 merged, so the two now read in the intended order in a real run.