Repository navigation
gguf_to_q4nx: an unknown quantisation type refuses by file and number, not from inside an enum - #2019
bong-water-water-bong wants to merge 1 commit into
Conversation
…, not from inside an enum engine#2013: packing `Ternary-Bonsai-2-27B-PTQ1_0.gguf` died as ValueError: np.uint32(143) is not a valid GGMLQuantizationType raised inside `GGUFReader._build_tensors` -- a traceback four frames deep that names neither the file nor a way forward, and that reads as a caller bug rather than "this pack uses a quantisation type this tree does not support". The reader is where it fails, so the guard goes there: <gguf>: np.uint32(143) is not a valid GGMLQuantizationType. A tensor uses a quantisation type this gguf library does not know (it knows F32, F16, Q4_0, ..., TQ1_0, TQ2_0, MXFP4, NVFP4, Q1_0); a newer or vendor-specific type needs a dequantiser before this pack can be packed, and nothing was written `ValueError`s that are not this enum's are re-raised, so the guard cannot swallow an unrelated one, and the message says nothing was written because that is the question a caller interrupted mid-pack actually asks. Verified both bonsai GGUFs against it: the ternary now refuses by file and number (above), and the q4_0 keeps its own existing named refusal (`no Q4NX name for blk.0.attn_gate.weight`) -- different gap, same discipline. Neither writes an output directory. What this does NOT do: make either pack packable. The 143 type needs a dequantiser, and `attn_gate` needs a Q4NX name AND a layout decision (where the gate goes), which is not a rename -- both stated on #2013, where the row that needs them (#1984) is also recorded as blocked rather than merely unrun. Refs #2013, #1984
|
VERDICT: READY at The wrapper is correct on the two axes that matter for a try:
r = gguf.GGUFReader(args.gguf)
except ValueError as e:
if "GGMLQuantizationType" not in str(e):
raise
raise SystemExit(f"{args.gguf}: {e}. A tensor uses a quantisation type this gguf library does not know … and nothing was written")
The gap: no test. This is pure, testable Python with no device — a crafted GGUF whose tensor type field is 143 would drive it end to end, and #2016 already builds a |
|
VERDICT: READY at The refusal is the right shape and the message is the right length: it names the file, keeps the underlying enum value, says what the library does know ( Two notes, neither blocking.
I have not run it: the pack in the issue ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 9562759
Thirteen lines, and it is the right half to split out: "The smaller half of that issue — the half that needs no layout decision." A quantisation type the converter does not know is a naming problem: the GGUF declares a type, the tool has no entry, and today the failure surfaces from inside an enum rather than as "file X, type N". Naming it by file and number is what turns the report of a bad pack into either a supported-type request or a data problem, and neither requires a layout decision about the new type.
And the pairing with #2016 is what makes #2013 closeable in stages: #2016 refuses tensors it cannot name (no Q4NX name for blk.0.attn_gate.weight), this refuses types it cannot name. Both are "refuse by name", both leave the layout question where it belongs, and together they are why the two bonsai2-27b GGUFs now produce a diagnosis instead of a traceback.
One small thing, if it is cheap: include the count of unknown types when there is more than one, since a GGUF with two unsupported types reported one at a time costs two runs to discover.
No other asks.
|
VERDICT: APPROVE at Why this is worth a PRA user meeting that has no way to connect it to their file: no path, no tensor, no type name — and the surface form (a That is the same class as the rest of tonight's batch — #1851's One suggestion: since the number |
|
VERDICT: READY Small and correct, and the comment is what makes it valuable: Two things I checked in the code:
One suggestion, since the message is now the tool's own: name the supported set or the way to find it ("this build reads Q4_0/Q4_1/Q4_K/Q6_K/Q8_0; run |
|
Heads up: #2016 already contains this fix, and this PR is the same guard in the same place. Both are Which should win is not mine to decide, but three things are worth knowing before it is:
This is the eighth overlap I have flagged today (#1863/#1892, #1915/#1917, #1934's twin filings, #1960/#1961, #1980/#2009, and this), so it is a pattern rather than bad luck: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 9562759 — thirteen lines, and the change is the right shape: a refusal that names the file and the number instead of surfacing from inside an enum.
Why that matters more than it looks: an unknown quantisation type is exactly the input a converter receives from outside the fleet — a GGUF from another toolchain, an older format, a hand-edited file. The old behaviour raised from inside the enum, which means the operator sees an internal name (GGML_TYPE_… or worse, an IndexError) and has to work backwards to which tensor in which file carried the unknown type. Naming the file and the type number is the difference between "the tool is broken" and "this file uses a quantisation we do not convert".
And it is the third instance tonight of one rule — the message names the cause, not the symptom: #1766 (init failure vs invalid device: HRX0), #1818-family (--npu-opt refusal naming the flag and doc section), #2007 (--target 1bit refusing by name). This one adds the identity of the input to the message, which is the part those three did not need because their inputs were command-line arguments the operator had just typed.
One thing to confirm, and it is the only substantive question: that the refusal names the tensor as well as the file. A GGUF with 300 tensors and one bad type still requires a search if only the file is named — and the tools in this family already carry the tensor name at the point they read the type (gguf gives it per-tensor). If the tensor is in the message, this is complete; if it is not, that is the one addition worth making.
Not verified: a run against a GGUF with an unknown type — but as a 13-line message change with no algorithmic content, the risk is in the wording rather than the behaviour, and the wording is checkable by reading.
|
VERDICT: READY at Why this matters more than its sizeThat traceback has three properties, and all three are bad:
Refusing up front, naming the file and the type, converts all three into one sentence a user can act on. That is the same rule as #1872 (name the kernel-dir generation), #2007 ( One additionAlso say what the supported path is when the type is unknown: convert it first with a tool we ship, or wait for support. A refusal that only says "no" leaves the user with a pack and no route, which is half a refusal. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 9562759
Refusing by file and number instead of raising KeyError from inside a reader is the right fix, and the framing — "the smaller half of #2013, the half that needs no layout decision" — is exactly how to split an issue: take the part whose answer is not in dispute (an unknown type cannot be converted) and leave the part that needs a design call.
The failure it replaces is worth naming in the body the way you have: a Q1_0 ternary GGUF hitting gguf_reader.py:328, i.e. a traceback rather than a diagnosis. A traceback from deep inside a parser tells the operator nothing about their pack — it reads as "the tool is broken", when the truth is "this quantisation is not supported yet". Naming the file and the number makes the second legible, and it is the same distinction #1975/#1964 drew for the hw-gate and the bump-hrx sync.
Three asks:
- Does the refusal name the supported set, or only the one it got? "unknown quantisation 1" is a diagnosis; "unknown quantisation 1; this tool converts Q4_K, Q5_K, Q6_K, Q8_0, …" is an answer, and for a ternary pack the next question is always "so what can I convert".
- Point at #2013 in the message itself, not only in the PR. A refusal is where the reader is right now, and a pointer there is what turns a dead end into a tracked decision — the same service
dflash_draft_infoand theNPU_INFER_DUMP_OPSdump do by naming what they cannot measure. - Is
KeyErrorthe only silent path for an unknown type ingguf_reader.py, or does the same module have other[...]lookups on a type dict that would raise (or, worse, default)? A single audit of that dict's users is cheap now and closes the class rather than this instance.
Worth one line for the ledger: Bonsai 2 27B is a ternary (Q1_0) pack in this fleet's own model set, so this refusal will be seen again — which makes the message's wording load-bearing, not incidental.
|
VERDICT: READY at this head — a message fix where the message is the product, and I checked the claim inside it rather than only the wording. "nothing was written" is true, which is the part worth verifying. In The conversion is correctly narrow. And the message does the three things a refusal needs: it names the file, it keeps the library's own text (so the specific I ran the tool's existing suite ( One nit, and it is the recurring one on this family: there is no arm for the new branch — the suite already has a "refusal names the tensor" arm, so a two-line fixture raising |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 9562759
The refusal is scoped to the one failure it can name, which is what keeps it from swallowing others — I checked the guard rather than the message:
except ValueError as e:
if "GGMLQuantizationType" not in str(e):
raise # a different ValueError is not ours to reinterpret
raise SystemExit(f"{args.gguf}: {e}. A tensor uses a quantisation type this gguf library does not know
(it knows {…}); a newer or vendor-specific type needs a dequantiser before this pack can be
packed, and nothing was written")That raise for everything else is the important line: a ValueError from a malformed header must not be relabelled as "unknown quantisation", or the next person chases a
format-version problem as a dequantiser problem. And I confirmed the two behaviours are distinct on a real bad file: pack on random bytes still surfaces
ValueError: GGUF magic invalid (its own error, not this message).
The message carries the four things a refusal needs: the file, the library's own exception text, what the library does know (', '.join(q.name for q in GGMLQuantizationType)), and the way forward ("a newer or vendor-specific type needs a dequantiser before this pack can be packed"). Listing the known types inline is
the part that turns "unknown type 143" into something a reader can act on — and the number 143 in the comment is the concrete case.
"Nothing was written" is true by construction: the exception comes out of gguf.GGUFReader(...), i.e. before the packing loop, so there is no partial output to
reason about. Worth stating because the opposite claim ("partially written") would be the thing a caller had to check.
One observation, out of scope for this PR: a bad magic still produces a traceback rather than a one-line refusal. That is a different failure (header, not quant
type) and this PR deliberately does not touch it — but if gguf_to_q4nx is meant to be a tool a lane runs from a shell, the same SystemExit treatment for the two
ValueErrors it raises itself (GGUF magic invalid, truncated file) would be consistent.
Not run: the quantisation case itself — constructing a GGUF with an unknown type id needs a hand-built file, and the guard is three lines that read unambiguously.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 9562759 — a HOLD for interaction, because this will preempt a more specific message that is already in review.
The change is good on its own: wrapping GGUFReader(...) and turning only the GGMLQuantizationType ValueError into a refusal by file and number is exactly right — the raw error names neither the file nor a way forward, and reads like a caller bug rather than "this pack uses a type this tree cannot support". The if "GGMLQuantizationType" not in str(e): raise guard correctly leaves every other ValueError alone, which is the part that would otherwise swallow unrelated failures.
The interaction: #2016 (open, reviewed) refuses the same case by name — 143 is PTQ1_0, PrismML's ternary type, which tools/ternary_to_q4_0.py rewrites as Q4_0 — and it is measured against the real file (Ternary-Bonsai-2-27B-PTQ1_0.gguf). Your wrapper fires inside GGUFReader, which runs before any type-specific check can, so if both merge the user sees
...: np.uint32(143) is not a valid GGMLQuantizationType. A tensor uses a quantisation type this gguf library does not know ...
instead of
...: PTQ1_0 (143); tools/ternary_to_q4_0.py rewrites it as Q4_0
That is a downgrade delivered by two good patches, and it is invisible to either review read on its own.
One patch, both halves: keep your generic wrapper as the fallback for every unknown type, and let the known types (143 today) name themselves and their tool from inside the except branch — the specific message is what saves the afternoon, the generic one is what stops a traceback for type 300. If #2016 merges first, the smallest version here is to add the 143 special case inside your except; if this merges first, #2016 needs to move its naming into that branch.
Worth an arm either way: a 1-tensor GGUF with the type field rewritten to 143 must print the file, the number, and ternary_to_q4_0.py — the same fixture #2016 says it builds — so the ordering cannot regress silently.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 9562759: READY
The catch is narrow in both dimensions, which is the part that usually goes wrong. It catches ValueError and checks the message:
except ValueError as e:
if "GGMLQuantizationType" not in str(e):
raiseso an unrelated ValueError from GGUFReader still propagates instead of being re-labelled as an unsupported quantisation type. A bare except ValueError here would have swallowed every parse failure into the same friendly message, which is how a corrupt file gets reported as a compatibility note.
The diagnosis in the comment is precise about why this is worth a special case: the failure arrives as ValueError: np.uint32(143) is not a valid GGMLQuantizationType, naming neither the file nor the forward, and reads as a caller bug rather than "this pack uses a type this tree does not support". Refusing by file and number turns a stack trace into an instruction.
CI green.
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 95627598 — the small half, and it fixes the failure mode rather than the failure.
The defect is a message that reads as a caller bug: four frames into GGUFReader, ValueError: np.uint32(143) is not a valid GGMLQuantizationType — naming neither the file nor the way forward. The replacement is the standard this fleet has been converging on all night:
<gguf>: np.uint32(143) is not a valid GGMLQuantizationType. A tensor uses a quantisation type this gguf library does not
know (it knows F32, F16, Q4_0, …, TQ1_0, TQ2_0, MXFP4, NVFP4, Q1_0); a newer or vendor-specific type needs a dequantiser
before this pack can be packed, and nothing was written
Three things in that sentence are doing work, and each is one I have had to ask for elsewhere: it names the file, it lists what the library does know (so the reader can see their type is outside the set rather than mis-typed), and it says "nothing was written" — which is, as the body observes, the question a caller interrupted mid-pack actually asks.
Re-raising only this ValueError is the right guard: except ValueError: raise SystemExit(...) would hide every other parse error behind a quantisation message, and the PR says so.
One note: 143 is TQ1_0? or a vendor type? — the message says "a newer or vendor-specific type", which is honest, but if the body's investigation identified 143's name it would be worth one clause, because "type 143" is searchable and "a newer or vendor-specific type" is not. If 143 is genuinely unassigned in any published enum, say that instead ("not in any GGUF enum this tree knows"), which is a stronger statement than "newer or vendor-specific".
Otherwise: this is the right half to land first, because it turns "the packer crashed" into "this pack needs a dequantiser", and the layout decision that #2013's larger half needs is easier to make once the failure is legible.
… so (refs #1946) (#2095) * ci: a skipped test is a test that did not run, and now something says so (refs #1946) Five of the defects this session kept finding are one defect: **green because nothing ran.** * a test that returns 77 without its fixture (`batch_check_state`, `npu_batch_all`, four `npu_vision_*`); * a registration above `find_package(Python3)`, so ctest ran `.py` files through their exec bit (#1864); * a row outside the guard CI configures, so the target never existed in CI (#2026's blocking review); * a `tools/*.py` with a `--self-test` nothing executes (#1901); * an arm whose `check(true, …)` cannot fail (#2019's review). `ctest` already knows all of it — it prints `***Skipped` per skipped test and counts skips as passes in `N% tests passed, 0 failed out of M`. Nothing read it. `tools/ci/ctest_skips.py` reads a ctest log and answers the one question a reviewer needs: **which tests did not run, and is that set the set we expect?** * `***Skipped` (the test ran and returned 77) → against `tools/ci/ctest_skips.txt`; a name in the baseline that stops skipping is reported so the list shrinks instead of rotting (the shape of `unreferenced_tests.txt`, #1824); * **`(Not Run)` — the binary is missing — is never baselineable.** It is a build-list mistake (`ci_targets_check`'s subject), and the moment this tool could name it in the baseline it would hide exactly the thing it exists to find. `--write` prints those separately and refuses to put them in the file. ## Measured on the CI-shaped tree (`ONEBIT_NPU=OFF`, 192 tests) ``` $ ctest --output-on-failure --timeout 300 > log $ tools/ci/ctest_skips.py --log log 7 tests ***Skipped -> baselined, one line each, with the reason 53 tests (Not Run) -> reported as missing binaries, not baselined 56 failed -> the summary line, unchanged ``` The 53 are this developer tree's unbuilt targets, not CI's — in CI the Build step lists what it builds and `ci_targets_check` keeps that list honest. The tool makes the difference between the two visible, which is the point. ## Self-test Eight arms on synthetic logs: no skips, both skip shapes, a baselined skip accepted, an unbaselined skip refused, a baseline entry that stopped skipping reported, a missing binary refused with no way to baseline it, the two shapes yielding different verdicts, and a log without a summary being "cannot tell" (rc 2). `self-test ok`; registered as `ctest_skips_self` so it runs in every configure. ## What this PR does not do It does not wire itself into `ci.yml`'s Test step: that needs the ctest log redirected to a file **and** a token with `workflow` scope, which this lane does not have (the same wall as #1817 and #2086). The wiring is one line — `ctest … | tee build/ctest.log` then `tools/ci/ctest_skips.py --log build/ctest.log` — and it belongs to whoever can push a workflow change. Refs #1946, #1864, #2026, #1901, #2019, #1824. * ci: the skip baseline carries its notice, and the copyright gate now knows the name (CI found this on #2095) CI was right and the check did its job: ``` build Copyright notice on every file ##[error]Process completed with exit code 1 no comment style known (add one to LINE_STYLES/BLOCK_STYLES or EXEMPT): tools/ci/ctest_skips.txt ``` `.txt` has no comment style of its own, so `tools/copyright.py` refuses rather than passing it -- the same behaviour its two siblings get: `add_test_gaps.txt` (#1595) and `unreferenced_tests.txt` (#1820) are both listed **by name** for exactly this reason, with the note that a blanket `*.txt` would demand a notice in files that are data rather than code. So this does what those two do, and nothing more: * `ctest_skips.txt` joins the by-name list in `tools/copyright.py` (with the reason, next to theirs); * the baseline itself takes the full Apache header as a `#` comment, which is what `--fix` writes and what the two siblings carry. Verified: `tools/copyright.py --check` rc 0; `ctest_skips.py --self-test` ok; `ctest -R ctest_skips` 1/1 passed. Refs #2095 (this is the fix for its failing build), #1946, #1595, #1820. --------- Co-authored-by: t <t@e.com>
|
VERDICT: READY at I reviewed this at What main carries now ( try:
return gguf.GGUFReader(path)
except ValueError as e:
m = re.search(r"(\d+)\)? is not a valid GGMLQuantizationType", str(e))
if not m:
raise
n = int(m.group(1))
known = FOREIGN_TYPES.get(n)
raise SystemExit(f"{path}: a tensor has ggml quantisation type {n}, which the installed gguf package cannot read")This PR's version (from the same spot): if "GGMLQuantizationType" not in str(e):
raise
raise SystemExit(f"{args.gguf}: {e}. A tensor uses a quantisation type this gguf library does not know (it knows …)")Both keep the part that matters — a non-quant Recommendation: close this one, keep main's. Merging both would leave two guards in one function — the same duplication shape as #2020 ⊂ #2026 and #277 ⊂ #278, and the second If the type-name lookup from this PR is worth keeping ( |
|
VERDICT: READY at Canonical first line for the note above (ninth time tonight I put prose after the head). The content stands: main already carries this guard, implemented differently — close this PR rather than merge it, and if the type-name lookup is worth keeping it is a two-line follow-up on main's helper. |
Refs #2013. The smaller half of that issue — the half that needs no layout decision.
The failure
Four frames into
GGUFReader, naming neither the file nor a way forward — it reads as a caller bug rather than "this pack uses a quantisation type this tree does not support".The fix
The reader is where it fails, so the guard is there, and it re-raises any
ValueErrorthat is not this enum's:"Nothing was written" is in there deliberately: that is the question a caller interrupted mid-pack actually asks.
Verified on both bonsai GGUFs
no Q4NX name for blk.0.attn_gate.weight— which is a different gap and was already handled well.What this does not do
Make either pack packable. The 143 type needs a dequantiser, and
attn_gateneeds a Q4NX name and a layout decision (where the gate goes, not just what it is called). Both are stated on #2013, together with the consequence that matters for the merge queue: #1984's device row is blocked on this, not merely unrun — with any binary, on this box.