Skip to content

gguf_to_q4nx: an unknown quantisation type refuses by file and number, not from inside an enum - #2019

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
fix/2013-quant-type-name
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
fix/2013-quant-type-name

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #2013. The smaller half of that issue — the half that needs no layout decision.

The failure

$ python3 tools/gguf_to_q4nx.py pack --tokenizer ~/models/bonsai2-27b/tok \
      ~/models/bonsai2-27b/Ternary-Bonsai-2-27B-PTQ1_0.gguf /tmp/out
  File ".../gguf/gguf_reader.py", line 328, in _build_tensors
ValueError: np.uint32(143) is not a valid GGMLQuantizationType

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 ValueError that is not this enum's:

<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

"Nothing was written" is in there deliberately: that is the question a caller interrupted mid-pack actually asks.

Verified on both bonsai GGUFs

  • ternary (5.6 G): refuses by file and number, as above, and leaves no output directory;
  • q4_0 (15 G): keeps its own existing named refusal — 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_gate needs 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.

…, 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
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 9562759 — a five-line refusal that turns a library's enum error into a sentence about the pack, and it re-raises anything that is not that error, which is the detail that makes it safe.

The wrapper is correct on the two axes that matter for a try/except this broad:

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")
  1. It filters, it does not swallow. Any other ValueError from GGUFReader — a truncated file, a bad magic, a malformed header — still propagates. A blanket except ValueError here would have converted real corruption errors into "unknown quantisation type", which is exactly the class of misleading message this PR exists to remove. Matching on the enum's name is the narrowest reliable discriminator available from outside the library.
  2. The message carries the four things a user needs: the file, the original error text ({e} preserved, so the number 143 is not lost), what the library does know (the full enum list, so the reader can see the boundary), and the way forward (a dequantiser, before packing). Plus "nothing was written", which is the state claim a caller needs to decide whether to clean up.

SystemExit rather than a bare exception is right for a CLI tool: it prints the sentence without a traceback, which is the difference the sibling PR #2016 made in its arms ("Traceback" not in log).

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 foreign_type_check(good_gguf, d, ok) helper that patches exactly that field ("assert struct.unpack_from('<I', raw, at)[0] == int(Q.Q4_0), 'the type field is not where the test expects it'"). If that helper is landing in #2016, this arm is a few lines on top of it: assert rc != 0, assert the message names the file and lists known types, assert "Traceback" not in log, assert no output directory. Without it, the filter — the part that could silently over-catch — is unpinned, and an over-broad except is precisely the regression this code could acquire later.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 9562759

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 (', '.join(q.name for q in GGMLQuantizationType) — which is what turns "143 is invalid" into "your pack uses a type this tree lacks"), and ends with "nothing was written", which is the one thing a caller needs before retrying. Refusing at the boundary where the file is opened is also right: four frames into _build_tensors is where a stack trace stops being evidence.

Two notes, neither blocking.

  1. The "GGMLQuantizationType" in str(e) match is the fragile part, and it is worth a comment saying why it is acceptable here: the alternative is pre-validating every tensor's ggml_type from the header before constructing the reader, which duplicates the library's own parsing. Since gguf_reader.py is vendored, matching on its message is a pragmatic coupling — but if that module is ever re-vendored with different wording, this refusal silently stops firing and the old four-frame traceback comes back. One sentence ("this matches the vendored reader's message; a re-vendor that changes it turns this back into a traceback") makes the coupling visible instead of accidental.
  2. The arm is cheap and missing. A GGUF with one tensor whose ggml_type is an unknown number is a few dozen bytes of header — GGUFReader will fail on it before any data is read, so the test needs no weights. That is the difference between a message that fires and a message that was written: assert the exit is non-zero, that the output names the file, and that it does not contain _build_tensors (the traceback being gone is the actual claim of this PR).

I have not run it: the pack in the issue (Ternary-Bonsai-2-27B-PTQ1_0.gguf, type 143) is not on this box, so my READY is on the diff and the message, not on a reproduction. If someone has that pack, one run is worth more than my reading.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at 9562759 — tiny, and it converts an error that blamed the caller into one that names the pack.

Why this is worth a PR

ValueError: np.uint32(143) is not a valid GGMLQuantizationType

A user meeting that has no way to connect it to their file: no path, no tensor, no type name — and the surface form (a ValueError from an enum, raised inside GGUFReader._build_tensors) looks like a bug in the tool. Refusing by file and number instead is the difference between "this pack uses a quantisation this tree does not support" and "something went wrong in Python".

That is the same class as the rest of tonight's batch — #1851's SKIPPED return code, #1913's observed-versus-printed load mode, #2007's refusal instead of a silent no-op — with the added virtue that here the wrong message was someone else's (upstream's ValueError), so wrapping it is the only way to improve it without patching the dependency.

One suggestion: since the number 143 is the whole diagnosis, printing the enum's name when it is known and the raw number when it is not (143 → "unknown type 143") is what lets a user search. If the refuse message already carries both, ignore this.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY 38b70f9

Small and correct, and the comment is what makes it valuable: GGUFReader fails inside _build_tensors with
ValueError: np.uint32(143) is not a valid GGMLQuantizationType — a message that "nam[es] neither the file nor a way forward, and look[s] like a caller
bug rather than 'this pack uses a type this tree does not support'"
. That distinction is the whole difference between an operator checking their command
line and an operator knowing the pack is unsupported.

Two things I checked in the code:

  • the catch is narrow: except ValueError as e: then if "GGMLQuantizationType" not in str(e): raise — anything else still propagates, so this does
    not turn a real ValueError (a corrupt header, a bad offset) into a tidy "unsupported quantisation" message that hides it;
  • the refusal becomes a SystemExit with file and number, which for a CLI tool is the right exit path — non-zero, no traceback, and the text the
    operator needs.

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
--list-types" or a docs pointer). "This pack uses a type this tree does not support" is honest but leaves the next question unanswerable without reading the
source.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Heads up: #2016 already contains this fix, and this PR is the same guard in the same place.

# this PR — tools/gguf_to_q4nx.py
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 …")

# #2016 — same file, same call, plus a fixture
"""A tensor type the gguf package does not know is refused by number, not as an enum ValueError (engine#2013)."""
… t143.gguf … the file is a valid GGUF with one tensor's type field rewritten to 143 (PTQ1_0) …

Both are Fixes/Refs #2013, both wrap GGUFReader for GGMLQuantizationType, and #2016 also adds the fixture that drives it (t143.gguf) plus the sentence naming 143 as PTQ1_0 and the route (tools/ternary_to_q4_0.py). So merging both would either conflict in that function or leave two guards for one condition.

Which should win is not mine to decide, but three things are worth knowing before it is:

  1. This one is 13 lines; npu-pack: a qwen35 GGUF has an in-tree converter, and gguf_to_q4nx refuses what it cannot name by name (Fixes #2013) #2016 is 971. If npu-pack: a qwen35 GGUF has an in-tree converter, and gguf_to_q4nx refuses what it cannot name by name (Fixes #2013) #2016 is going to be reviewed in pieces, landing this first and letting npu-pack: a qwen35 GGUF has an in-tree converter, and gguf_to_q4nx refuses what it cannot name by name (Fixes #2013) #2016 rebase is the cheaper path — and the rebase is trivial because the hunk is one try/except.
  2. This one has the better message: it lists the types the library does know (', '.join(q.name for q in gguf.GGMLQuantizationType)), which tells the reader whether their pack is one refactor away or genuinely vendor-specific. I would keep that wording whichever branch survives.
  3. npu-pack: a qwen35 GGUF has an in-tree converter, and gguf_to_q4nx refuses what it cannot name by name (Fixes #2013) #2016's fixture is the thing that keeps it fixed — a message-only change with no arm is exactly what this lane has been chasing all day, and that fixture is worth carrying over if this PR is the one that lands.

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: gh pr list --search "<issue number>" greps PR bodies, and it is the only check that sees work in flight — a tree grep cannot. It would have caught this one too, since both bodies name #2013.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 9562759 — fourteen lines that turn an enum's ValueError into a sentence about the pack.

# engine#2013: a quantisation type this gguf cannot read fails INSIDE GGUFReader's _build_tensors, as
# `ValueError: np.uint32(143) is not a valid GGMLQuantizationType` from the enum -- naming neither the file n[or the]
# forward, and looking like a caller bug rather than "this pack uses a type this tree does not support". Refu[se]

Why this matters more than its size

That traceback has three properties, and all three are bad:

  • it names neither the file nor the forward — so the reader has to reconstruct which pack and which conversion step;
  • it looks like a caller bug — an exception raised inside a third-party reader's _build_tensors reads as "we passed something wrong", and the actual situation is "the pack uses a type this tree does not support". Those need opposite responses;
  • it carries a raw enum number (np.uint32(143)) — the one piece of information that tells an expert what happened, presented as a magic value.

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 (1bit is an output name), and #1991 (the option cannot be honoured here): the refusal carries the alternative.

One addition

Also 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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".
  2. 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_info and the NPU_INFER_DUMP_OPS dump do by naming what they cannot measure.
  3. Is KeyError the only silent path for an unknown type in gguf_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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 tools/gguf_to_q4nx.py the reader is constructed at :381, the output directory is created at :390, and the first byte is written at :427 — so a GGUFReader failure cannot leave a partial container or even a stray directory. A refusal message that promises that and cannot back it is worse than no message, because a lane then goes looking for the damage.

The conversion is correctly narrow. except ValueError only fires, and if "GGMLQuantizationType" not in str(e): raise keeps every other ValueError from _build_tensors propagating as itself — a malformed-dims bug should still look like a malformed-dims bug rather than being relabelled as an unsupported type.

And the message does the three things a refusal needs: it names the file, it keeps the library's own text (so the specific np.uint32(143) survives), and it names what is supported (', '.join(q.name for q in gguf.GGMLQuantizationType)) plus the way forward — "a newer or vendor-specific type needs a dequantiser before this pack can be packed". raise SystemExit(msg) over sys.exit is equivalent and reads better in a try block.

I ran the tool's existing suite (tests/gguf_to_q4nx_test.py with tests/golden/ present — it needs that fixture tree, which a diff-only extraction lacks): PASS, including --against accepts the packed container (rc 0), --against refuses the other code order (rc 1), the refusal names the tensor, and the Q8 write-back arm. So the change does not disturb the container path.

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 ValueError("… GGMLQuantizationType …") and asserting the file name plus nothing was written would pin both halves of this message. Not blocking (a message is not behaviour), but it is exactly the kind of message that drifts on the next refactor.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
        raise

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

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 9562759: READY — an unknown quantisation type should refuse by name, and "the half that needs no layout decision" is the right scoping

The failure is the good kind of bug to fix first: gguf_to_q4nx.py pack on a Ternary-Bonsai-2-27B-PTQ1_0.gguf reaches the reader and fails — i.e. the converter accepts a file whose quantisation it cannot represent, and the failure surfaces as a traceback inside gguf_reader rather than as a refusal naming the type. A converter that dies in a dependency has told the user nothing about what is unsupported or whether it ever will be.

Calling it "the smaller half of that issue — the half that needs no layout decision" is exactly the right decomposition, and it is the pattern that has worked all night (#1920/#1923/#1927 before #1930's tower; #1931's front-end before its route): take the part that is a guard now, and leave the part that requires a design decision (PTQ1_0's actual layout) for when there is evidence. A refusal today is strictly better than a traceback, and it does not prejudice the implementation.

Three asks, in order:

  1. The refusal must name the quantisation type and the supported set, and it should say whether the type is unknown to the reader or known but unsupported by the converter — those are different messages for different people (upstream reader vs this tool).
  2. The check should run before any work: on a large GGUF, discovering the type after reading a header's worth of data is fine, but discovering it after materialising tiles would waste a ticket. One line up front (if quant not in SUPPORTED: refuse(...)) also makes the guard testable without a real file.
  3. A test with a crafted GGUF declaring an unknown quant type, asserting the refusal message — cheap, device-free, and it is the only way to keep the guard from being removed as "unreachable".

And one hook worth leaving in: the refusal text should mention that the type is recognised by the reader but not yet convertible if that is the case, because PTQ1_0 is on the roadmap (#2013's larger half) and the message will be the first thing whoever implements it reads.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bong-water-water-bong added a commit that referenced this pull request Nov 11, 2026
… 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>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 9562759 — withdrawn in substance: main already has this fix, implemented differently. Please close rather than merge.

I reviewed this at 9562759 when main did not have the guard. Main has since gained an equivalent (and slightly better) one, which is why this PR now reports CONFLICTING — two
implementations of one idea.

What main carries now (tools/gguf_to_q4nx.py, in a reader() helper):

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 ValueError is re-raised, so a malformed header is never relabelled as an unknown type. Main's version is better in one respect and
worse in another: it extracts the type number (the thing a reader has to act on) and consults FOREIGN_TYPES for a name, where this one quotes the library's message and lists
every known type. Listing all known types is long and says nothing about the number; naming the number is the actionable half.

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
to land is the one that gets read next.

If the type-name lookup from this PR is worth keeping (FOREIGN_TYPES.get(n) may cover fewer numbers than the gguf package's enum), that is a two-line follow-up on main's
helper
, not this branch.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 9562759

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

PR hygiene (orchestrator 11-13 02:35Z, cl-infra): closing — its issue #2013 is closed by merged #2016. Branch deleted with the close.

@bong-water-water-bong
bong-water-water-bong deleted the fix/2013-quant-type-name branch November 13, 2026 02:32
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