Repository navigation
hrx: word a device refusal from the terms, so it names the host term when that is what binds (engine#2069) - #297
bong-water-water-bong wants to merge 1 commit into
Conversation
…when that is what binds (engine#2069)
`ggml-hrx.cpp` refused the device when `budget == 0` by wording the message from the **ceiling**:
GGML_LOG_ERROR("ggml_hrx: %s: refusing to use the device: GTT total %.1f GiB with %.1f GiB free leaves nothing under
the %.1f GiB ceiling ...; free GTT or lower the reserve (engine ggml-org#680)")
But `budget == 0` can come from the **host** term: `pool_budget_terms()` takes the smaller of the GTT term and
`MemAvailable` and sets `host_bound` when the host one wins. Then the advice names the wrong resource and the three numbers
printed are the ones that were not binding. Measured: a self-built HRX configuration refused with "GTT total 120.0 GiB with
78.0 GiB free leaves nothing under the 104.0 GiB ceiling", which cannot explain anything (78 free against a 104 ceiling
leaves ~62 GiB) while the real cause was the host figure reading as 0 -- the ggml-org#1942 argument-evaluation-order bug.
The sentence now comes from `pool_refusal_text()`, which reads the terms:
* `host_bound` -> names `host MemAvailable` with its figure, says HOST memory binds, and says **free RAM**, not free GTT;
* a measured **0** is called out as a reported zero rather than printed as though it were a reading (a symptom of its own,
ggml-org#1942's shape);
* otherwise the GTT/ceiling/reserve wording is unchanged, so ggml-org#680's rationale survives verbatim;
* `kHostUnknown` leaves the GTT wording, because the host term decided nothing.
The wording lives in `runtime/failure-status.{h,cpp}` rather than inline in `ggml-hrx.cpp` for the same reason
`kPoolRefusalToken` does: `ggml-hrx.cpp` compiles only with `GGML_HRX=ON`, so a sentence written there cannot be asserted by a
host test.
`tests/test-hrx-pool-refusal-text.cpp` (ctest `test-hrx-pool-refusal-text`, no device, no HRX link): 9 arms. The ones that
matter are the **negative** halves -- the host-bound text must not advise freeing GTT, and the GTT text must not mention RAM
-- because a wording function can otherwise satisfy every arm by mentioning everything. Proved they can fail: making
`pool_refusal_text()` always take the GTT branch fails 4 arms.
The `budget == 0 && memory_total != 0` condition and the ggml-org#680 rationale are untouched: this is a message, not a policy.
Refs engine#2069, engine#1942, engine#680.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
#297 ready for review at a88d24dd1 — 5 files, +138/−6, MERGEABLE (fork, base 1bit/hrx-vulkan-patched).
The reviewer's first question is whether a message change can hide a real refusal, and it cannot: the condition is untouched (budget == 0 && memory_total != 0), the decision to refuse is untouched, and only the sentence changes. The ggml-org#680 rationale survives verbatim in the branch that still applies, which I checked by asserting its three distinguishing strings (free GTT or lower the reserve, GGML_HRX_GTT_RESERVE_MB, engine#680) rather than by eyeballing the diff.
Why the wording moved into a header: ggml-hrx.cpp compiles only with GGML_HRX=ON, so a sentence written inline there cannot be asserted on this box at all — the arm would be an HRX build or nothing. That is the same move llama.cpp#277 made for kPoolRefusalToken, and it is why the test target needs no device, no libhrx and no ggml link: I built and ran the nine arms standalone (g++ … failure-status.cpp) before pushing, because I did not want to hand over an arm I had not executed.
The negative halves are what the arms are for. A wording function can satisfy "mentions the host term" by mentioning everything, so each positive is paired: host-bound must not contain free GTT, and the GTT branch must not contain free RAM. Forcing the GTT branch fails 4 arms — the mutation is in the commit message.
One thing a reviewer should push back on if they disagree: I print the GTT figures in the host-bound sentence as well, labelled as the non-binding side. That is deliberate — the GTT numbers are part of why the budget is what it is, and an operator comparing two refusals needs both — but it does mean the host-bound line is longer than the one it replaces. If that reads as noise, dropping the parenthetical is a one-line change and the arms would still hold.
Not in this PR: any pin bump (the engine pin takes the fork commit separately) and the #1942 shape itself, which ggml-org#1960 fixed.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a88d24d
This is the "an error must name what actually bound" fix, and the symptom it removes is a diagnostic that actively misleads: "it stops telling the operator to free a resource that was not binding." A refusal that names the wrong term is worse than a vague one — the operator frees memory (or a slot, or a ticket) that was never the constraint, the refusal persists, and the real limit stays invisible behind a suggestion that was already followed.
Wording the message from the terms rather than from a fixed template is the structural fix, because the template was the bug: a single sentence that assumed the host term was binding produced a correct-looking message in the case that mattered least.
One ask, cheap and it is what makes it stay fixed: name the binding term's measured value and its limit in the message (host term 21.4 GiB against a 20 GiB floor, not "the host term"), since the next question after "which term" is always "by how much" — and a refusal that answers both in one line is the one an operator can act on without a second run.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a88d24d — A device refusal worded from the terms, so it names the actual values: a refusal that says "invalid configuration" sends the next reader to the validator, while one that names both terms sends them to the wrong value.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a88d24d
Built the test standalone and A/B'd it — this is the first PR in this family I could execute without the private runtime, and it holds.
The test does not need GGML_HRX=ON: pool_refusal_text() lives in runtime/failure-status.cpp and the only header it needs is pool_budget_terms_t. Two lines on this
box:
$ g++ -std=c++17 -O0 -Iggml/src/ggml-hrx -Iggml/include -Iggml/src \
-o pooltext tests/test-hrx-pool-refusal-text.cpp ggml/src/ggml-hrx/runtime/failure-status.cpp
$ ./pooltext
ok a non-zero host term is printed with its figure and still no GTT advice
ok and no zero warning when nothing was zero
ok the GTT wording (and #680's rationale) is unchanged when GTT binds
ok and it does not mention RAM
ok an unreadable host figure leaves the GTT wording
all arms passed
And the mutant run, because a green test proves nothing until it can fail — if (false && terms.host_bound) (branch neutralised, everything else untouched):
FAIL host-bound refusal names the HOST term
FAIL and tells the operator to free RAM
FAIL and does NOT tell them to free GTT
FAIL a measured zero is called out as a reported zero
FAIL a non-zero host term is printed with its figure and still no GTT advice
5 arm(s) failed
Five arms fail when the branch is reverted, and the three that still pass are the ones that assert the unchanged GTT path — exactly the right split.
The structure of the arms is what I have been asking for all session: each positive assertion is paired with a negative one — has("host MemAvailable") and
!has("free GTT or lower"); has("8.0 GiB") and !has("measured 0.0 GiB"). A message that named both resources would pass the first of each pair and fail the
second, which is the whole point: "the host-bound text must NOT advise freeing GTT" (the author's own words in the file).
Why this is the right fix for engine#2069 and not a reword for its own sake: the old message worded the refusal from the ceiling, so budget == 0 always
produced "free GTT or lower the reserve" — advice about the resource that was not binding — and in the #1942 case the actual cause was the host figure reading
as zero through an argument-evaluation-order bug (the one I filed as #279). The message now says which term bound, prints the non-binding side labelled as
non-binding, and for the zero case says "measured 0.0 GiB, which is a reported zero rather than a reading" — which is the MemAvailable: 0 kB distinction
(ggml-org#1768/#259) applied where it is read.
One wording note, not a blocker: host_bound == true && host_available == 0 is also what a genuinely exhausted host looks like, and the caveat reads as if it
is certainly a reporting artefact. The sentence is right in the common case (the ggml-org#1942 bug), and an operator reading it on a truly-starved box will still see
0.0 GiB and act correctly — so I would not change it now.
Not run: test-backend-ops.cpp (+11/-9, the caller) needs GGML_HRX=ON and the private runtime; the change there is the three-line substitution shown in the
diff (pool_refusal_text(device_ctx->name.c_str(), total, free, ceiling, terms) in place of the inline GGML_LOG_ERROR).
#296 (mine, opened 3 minutes before this) is being withdrawn — and one case worth wording explicitlyThree PRs are open for the same message ( One case I would like worded explicitly, since I hit it: Verified nothing (no build under the disk floor tonight) — this is a design note, not a verdict. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Note for whoever merges: #300 is the same fix for engine#2069, targeting the budget == 0 refusal in ggml-hrx.cpp specifically — the same file this PR edits.
Both PRs cite ggml-org#2069, both add a refusal-text helper and a test:
| PR | scope | new mechanism |
|---|---|---|
| this | word a device refusal from the terms | runtime/failure-status.{cpp,h} |
| #300 | the budget == 0 refusal, worded in GTT terms while pool_budget_terms() may have zeroed the budget |
hrx-1bit-safety.{cpp,h} |
My preference is this PR, because wording from the terms fixes the class — "a refusal that names the wrong resource wastes an operator afternoon" is true of every refusal, and a per-call-site fix leaves the next one to be discovered the same way. But then #300's case should land here as a test case, since budget == 0 is the instance that was actually observed and this PR's arms should include it.
Merging both is the outcome to avoid: two helpers that word refusals, in the same file, for the same issue.
|
**VERDICT: READY at I reviewed
One thing to carry across from #300 before this merges: its opening sentence — "A refusal that names the wrong resource wastes an operator afternoon" — is the better statement of why, and its |
|
A correction to the comparison, because the reason #297 was preferred does not hold:
That matters because it changes the decision from "supersede vs add" to placement vs placement, and on placement I would take
Either way, only one of the pair should land — they rewrite the same So my verdict does not change — READY at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at a88d24d: READY
The companion to #296/#300: the refusal is now worded from the terms it actually compared, so it names the host bound rather than only the ceiling. Three PRs in this fork have now moved the same message in the same direction — a refusal should contain the value that decided it — which is a good sign that the pattern is being applied rather than patched once.
CI green.
|
Closing as superseded — #300 landed the same fix, and it is better in two places. Theirs is merged (in the fork tip and carried by the engine pin bump ggml-org#2092), so this PR is a duplicate rather than a competing one. Same design, arrived at independently: a pure function in Where theirs is better, and I would have said so in review:
So nothing to port back. The one thing worth keeping from mine is in the issue thread rather than the code: the check Thanks to whoever wrote #300 — the diagnosis in its body ("Found twice: my review of #259 flagged the same defect one layer down") is the same route I took, which suggests the defect was visible to anyone who read that path. |
The fix for engine#2069: a device refusal is now worded from the terms, so it names the host term when that is what binds, and it stops telling the operator to free a resource that was not binding.
The defect, and the measurement that makes it a defect rather than a wording taste
budget == 0can come from either term —pool_budget_terms()takes the smaller of the GTT term andMemAvailable, and setshost_boundwhen the host one wins — but the refusal was worded from the ceiling alone:78 GiB free against a 104 GiB ceiling leaves ~62 GiB, so that sentence cannot explain the refusal it accompanies. The real cause was the host figure reading as 0 — the ggml-org#1942 argument-evaluation-order bug — and the message pointed at GTT.
The change
pool_refusal_text()reads the terms:host_boundhost MemAvailablewith its figure, says HOST memory binds, advises free RAMhost_boundand a measured 0kHostUnknownIt lives in
runtime/failure-status.{h,cpp}, not inline.ggml-hrx.cppcompiles only withGGML_HRX=ON, so a sentence written there cannot be asserted by a host test on this box — the same reasonkPoolRefusalTokenwas moved into a header by llama.cpp#277. The call site is now one line.Arms
tests/test-hrx-pool-refusal-text.cpp, ctesttest-hrx-pool-refusal-text, no device and no HRX link — I built and ran it standalone withg++ … failure-status.cppbefore pushing:The negative halves are the point — host-bound must not say "free GTT", and the GTT text must not mention RAM — because a wording function can otherwise satisfy every arm by mentioning everything. Proved they can fail: forcing
pool_refusal_text()down the GTT branch fails 4 arms.Not changed
The
budget == 0 && memory_total != 0condition, the ggml-org#680 fault-line rationale, and the open-time guardrail line (which already useshost_bound). This is a message, as the issue says.Refs engine#2069, engine#1942, engine#680.