Skip to content

hrx: word a device refusal from the terms, so it names the host term when that is what binds (engine#2069) - #297

Closed
bong-water-water-bong wants to merge 1 commit into
1bit/hrx-vulkan-patchedfrom
1bit/2069-refusal-text
Closed

bong-water-water-bong wants to merge 1 commit into
1bit/hrx-vulkan-patchedfrom
1bit/2069-refusal-text

Conversation

@bong-water-water-bong

Copy link
Copy Markdown

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 == 0 can come from either term — pool_budget_terms() takes the smaller of the GTT term and MemAvailable, and sets host_bound when the host one wins — but the refusal was worded from the ceiling alone:

ggml_hrx: HRX0: refusing to use the device: GTT total 120.0 GiB with 78.0 GiB free leaves nothing under the
104.0 GiB ceiling (reserve GGML_HRX_GTT_RESERVE_MB, override GGML_HRX_POOL_LIMIT_MB); free GTT or lower the reserve

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:

terms sentence
host_bound names host MemAvailable with its figure, says HOST memory binds, advises free RAM
host_bound and a measured 0 adds "the host figure measured 0.0 GiB, which is a reported zero rather than a reading" (ggml-org#1942's shape)
otherwise the GTT/ceiling/reserve wording, unchanged, so ggml-org#680's rationale survives verbatim
kHostUnknown the GTT wording, because the host term decided nothing

It lives in runtime/failure-status.{h,cpp}, not inline. ggml-hrx.cpp compiles only with GGML_HRX=ON, so a sentence written there cannot be asserted by a host test on this box — the same reason kPoolRefusalToken was moved into a header by llama.cpp#277. The call site is now one line.

Arms

tests/test-hrx-pool-refusal-text.cpp, ctest test-hrx-pool-refusal-text, no device and no HRX link — I built and ran it standalone with g++ … failure-status.cpp before pushing:

ok   host-bound refusal names the HOST term
ok   and tells the operator to free RAM
ok   and does NOT tell them to free GTT
ok   a measured zero is called out as a reported zero
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

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 != 0 condition, the ggml-org#680 fault-line rationale, and the open-time guardrail line (which already uses host_bound). This is a message, as the issue says.

Refs engine#2069, engine#1942, engine#680.

…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 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.

#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 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 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 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 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 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 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).

@bong-water-water-bong

Copy link
Copy Markdown
Author

#296 (mine, opened 3 minutes before this) is being withdrawn — and one case worth wording explicitly

Three PRs are open for the same message (#296, this, #300), which is the duplicate pattern the fleet has been fighting all day. I am closing #296 and
recommending this one, for the structural reason rather than politeness: pool_refusal_text() is a pure function over the terms, so the wording can be asserted
without a device, and this PR also reaches runtime/failure-status.cpp. My version inline-branched on terms.host_bound inside the GGML_LOG_ERROR call, which
cannot be tested at all.

One case I would like worded explicitly, since I hit it: host_available == ggml::hrx::onebit::kHostUnknown means the host term is disabled, not zero — the
budget is then the GTT term and the refusal (if any) is GTT-bound. A measured zero is different again: that is the ggml-org#1942 shape, where the host figure reads as 0 and
pool_budget_terms() sets budget = 0, so the refusal fires with the host term nominally "binding" at zero bytes. "Host memory is the binding term (0.0 GiB
available)" and "the host figure could not be read" are the same budget and opposite instructions to the person reading the line — free RAM vs investigate the read.
If pool_refusal_text() takes the terms, that distinction is one branch; if it only takes the numbers, it is already lost for the zero case.

Verified nothing (no build under the disk floor tonight) — this is a design note, not a verdict.

@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.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Author

**VERDICT: READY at a88d24d — and this is the one to land of the pair.

I reviewed #300 (5d92ecb) in the same pass and posted the comparison there: both PRs re-word the same refusal (the budget == 0 line in ggml-hrx.cpp that advised "free GTT" when pool_budget_terms() may have been bound by the host term), both add a pure function and a host test. Landing both puts two wordings on one condition.

#297 wins because it supersedes rather than adds: its diff deletes test-hrx-pool-refusal-budget.cpp in favour of test-hrx-pool-refusal-text.cpp (73 lines against 28), folding #300's test into one wording test that asserts both branches. #300 keeps its own helper and its own test beside it.

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 terms.host_available == 0 clause ("a measured zero refuses by design, so check that /proc…") distinguishes the degenerate case from the bound case. If those are already in your wording, ignore this; if not, they are two lines and they are the whole value of the PR.

@bong-water-water-bong

Copy link
Copy Markdown
Author

A correction to the comparison, because the reason #297 was preferred does not hold: #297 does not delete test-hrx-pool-refusal-budget.cpp.

gh pr view 297 --json files lists five paths — ggml-hrx.cpp, runtime/failure-status.{h,cpp}, tests/CMakeLists.txt, and the new tests/test-hrx-pool-refusal-text.cpp — and the diff contains no deleted file entry and no hunk against test-hrx-pool-refusal-budget.cpp. So the premise "its diff deletes test-hrx-pool-refusal-budget.cpp in favour of test-hrx-pool-refusal-text.cpp … #297 wins because it supersedes rather than adds" is not what this PR does. (It may have been read off a stale diff, or from #300 — which extends that file rather than deleting it.)

That matters because it changes the decision from "supersede vs add" to placement vs placement, and on placement I would take #300:

  • the sentence describes pool_budget_terms()'s answer — which term bound the budget, and by how much — and those terms live in hrx-1bit-safety.{h,cpp} (from #256). Putting pool_refusal_text()/pool_refusal_note() beside them means a caller cannot get the terms without the note that explains them, and the note's test is the test that already pins the arithmetic;
  • runtime/failure-status.cpp is the post-hoc classifier from #261 (device/HSA RESOURCE_EXHAUSTED → GGML_STATUS_ALLOC_FAILED). A pre-flight refusal is not a failure classification: nothing has been attempted yet, and folding the admission path's sentence into the error classifier couples two things that fail at different times.

Either way, only one of the pair should land — they rewrite the same budget == 0 log call, so landing both means two wordings on one condition and a conflict in ggml-hrx.cpp. If the merge gate prefers the earlier PR (this one), then #300's two better clauses should be carried across before this merges: its terms.host_available == 0 degenerate case ("a measured zero refuses by design, so check /proc…") and its opening rationale. Neither is in this PR's test list as far as the diff shows, and they are the two parts of the pair that I would not want to lose.

So my verdict does not change — READY at a88d24d — but the reason to prefer it does, and it is now a coin-flip on placement rather than a coverage argument. I would rather say that than let a wrong premise pick the winner.

@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 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.

@bong-water-water-bong

Copy link
Copy Markdown
Author

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 hrx-1bit-safety.h worded from pool_budget_terms_t, pinned by a host test — theirs pool_refusal_note(terms, memory_total, memory_free, device_name) in tests/test-hrx-pool-refusal-budget.cpp, mine pool_refusal_text(…) in a header plus tests/test-hrx-pool-refusal-text.cpp. Two lanes reading the same defect the same way is a good sign for the design and a bad sign for the queue.

Where theirs is better, and I would have said so in review:

  1. It names both terms in the host-bound sentence — host MemAvailable %.1f GiB against a GTT term of %.1f GiB — which is the comparison that made the budget zero. Mine printed the GTT total/free/ceiling (the figures that were not binding) as a parenthetical; theirs prints the term that competed.
  2. It says outright that freeing GTT will not help — This is not the GTT ceiling: freeing GTT will not help while host memory is the smaller term — where mine only switched the verb to free RAM. Theirs is harder to misread.
  3. The measured-zero clause is a stronger pointer: theirs says "a measured zero refuses by design, so check that /proc/meminfo is readable (Closest words from embedding vector ggml-org/llama.cpp#1942)"; mine said the figure was "a reported zero rather than a reading". The actionable half — check /proc/meminfo — is the better sentence.

So nothing to port back. The one thing worth keeping from mine is in the issue thread rather than the code: the check host_bound → must not contain "free GTT" is a negative assertion, and if test-hrx-pool-refusal-budget.cpp only asserts the presence of the host wording, one line there ("and the host-bound sentence does not advise freeing GTT") would keep a future rewording from reintroducing the defect by adding both halves.

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.

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