Skip to content

tools: q4nx_diff names the layout that matches, instead of calling a good container BAD (#1110) - #1116

Merged
bong-water-water-bong merged 2 commits into
mainfrom
1bit/q4nx-diff-both-layouts
Nov 5, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
1bit/q4nx-diff-both-layouts

Conversation

@bong-water-water-bong

@bong-water-water-bong bong-water-water-bong commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Towards #1110 (this is the check, not the converter fix — no closes).

Scope note (2026-11-05 11:4x): the padded-grid decode half I first put on this branch now lives in
#1119 (pi-npufeat, tools/q4nx_diff.py + tests/q4nx_diff_grid_test.py + one add_test()), so only one PR
carries that logic and it gets a CI test. This PR is now exactly the verdict change below: --layout both, the
tolerance, the verdict line and --self-test. q4nx_diff.py is touched by both PRs in the same function
(_decode_pass/decode_cmd): whichever lands second needs a small rebase, and I will rebase this one onto #1119
when it merges — the two changes are independent in behaviour.

q4nx_diff.py decode --layout <flm|ours> compares a container against its source GGUF, but it compares one layout chosen by
the caller, and its pass condition was "no element differs anywhere". Both halves mislead:

  • a container packed from a Q4_K_M GGUF into q4_1 is a requantization, so exact equality is impossible — the tool
    reported every honest container as BAD (measured: a correct Qwen3-0.6B pack shows worst max|err| 4.1e-2, our own ZR1
    pack 5.2e-3);
  • choosing the wrong layout turns that into a confident "BAD" for a good container while the other layout would have matched —
    which is exactly how the converter mismatch behind npu: a container whose q4_1 tile order differs from ours decodes as noise with no error, and no check catches it #1110 stayed invisible: the same container reads 4.1e-2 under
    --layout ours and 1.6 under --layout flm
    .

What changes

--layout both runs each layout and the verdict names the one that matched — or says none did and which was closest.
"Matches" is a tolerance, not equality: worst max|err| ≤ 0.1 (the Q4_K_M → q4_1 requantization error measured on this box)
and at least 3× better than the rival layout. --self-test pins the verdict on six synthetic result sets, no model and
no device needed.

Measured (this box, today)

container verdict rc
Qwen3-0.6B repacked with the converter that works (~/wt/npu-rt-753-pr/…) matches layout ours (worst max|err| 4.130e-02, next layout flm 1.638e+00) 0
the same model repacked with ~/amd-oss/FLM_Q4NX_Converter (d1d5232) matches NO layout: closest is flm with worst max|err| 1.149e+00 (> 1.000e-01, or not 3x better than ours) - the container's tiles are not these weights 1
our own ZR1-1.5B-npu-q4km-v2 pack matches layout ours (worst max|err| 5.239e-03, next layout flm 2.001e+00) 0

So it separates the two converter generations by ~50× on the same model, weights and prompt-free path, and it is what a
converter sanity gate needs: no engine rebuild, no device, no npu-hold.

Scope

The two tile decoders are unchanged and --layout flm|ours still behaves exactly as before; this only adds both, the verdict
line, the tolerance and the self-test. tools/copyright.py --check rc 0.

Which layout is which — corrected by pi-npufeat's measurement (see #1110), because my first wording ("flm looks like an older
generation") was too simple.
Repacking the same GGUF with both converters and trying every variant:

converter under --layout ours under --layout flm
third_party/npu-rt-753-pr (the one that works) requant scale (2.96e-2 / 2.31e-2) — this is the layout npu/forward reads not it
~/amd-oss/FLM_Q4NX_Converter (d1d5232) 5.48 signed 3.46; only its Q6_K-sourced tensor reaches requant scale under flm(unsigned) 2.31e-2, its Q4_K-sourced one matches none of the three

So d1d5232's tile order depends on the source quant type (or changed mid-way) — there is more than one difference, no single
"flm order" describes it, and a guard that tries to detect the flm order would be guessing. That is why the gate this PR adds
matters in the shape it has: it names the layout that matches or says none does, and the durable fix for #1110 is a declared
tile order in the container
, not another decoder guess. I did not touch decode_flm; #1119's second commit (5576165) makes the
tool's docstring say per layout what is validated against what.

…good container BAD

`q4nx_diff.py decode --layout <flm|ours>` compares a container against its source GGUF, but it compares
*one* layout chosen by the caller, and its pass condition was "no element differs anywhere". Both halves
mislead:

* a container packed from a Q4_K_M GGUF into q4_1 is a REQUANTIZATION, so exact equality is impossible --
  the tool reported every honest container as `BAD` (measured: a correct Qwen3-0.6B pack shows worst
  max|err| 4.1e-2, and our own ZR1 pack 5.2e-3);
* picking the wrong layout turns that into a confident "BAD" for a good container while another layout
  would have matched -- which is how the converter mismatch behind engine#1110 stayed invisible: the
  same container reads 4.1e-2 under `--layout ours` and 1.6 under `--layout flm`.

So `--layout both` tries each layout, and the verdict names the one that matched (or says none did, and
which was closest). "Matches" is a tolerance, not equality: worst max|err| <= 0.1 (the Q4_K_M -> q4_1
requantization error measured on this box) AND at least 3x better than the rival layout. `--self-test`
pins the verdict on six synthetic result sets without touching a model.

  good container  -> "matches layout ours (worst max|err| 4.130e-02, next layout flm 1.638e+00)"   rc 0
  bad converter   -> "matches NO layout: closest is flm ... the container's tiles are not these weights"  rc 1
  our own pack    -> "matches layout ours (worst max|err| 5.239e-03, next layout flm 2.001e+00)"   rc 0

The two decoders are unchanged, `--layout flm|ours` still works exactly as before, and this is the check
engine#1110 proposes to run against a converter: it needs no engine rebuild and no device.

@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 1158836c — the verdict line does what it claims, and I ran it against a real container rather than a synthetic pair.

$ q4nx_diff.py --self-test
self-test passed: the verdict names the matching layout, accepts a lossy-but-correct pack, and refuses tiles from other weights

$ q4nx_diff.py decode --gguf ~/models/zyphra-zr1/ZR1-1.5B-bf16.gguf \
                      --q4nx ~/models/npu/ZR1-1.5B-npu-q4km-v2 --layout both
layout=ours: exact=0 inexact=197 skipped=58 padded=0   worst max|err| 7.617e-02
matches layout ours (worst max|err| 7.617e-02, next layout flm worst max|err| 3.199e+00)     rc 0

Two things in that output are the argument for the PR. First, the honest container now gets a verdict with a margin — 7.6e-2 for the layout that matches against 3.2e+0 for the one that does not, a factor of ~42 — so "which layout is this" is answered by the data instead of by the flag the operator guessed. Second, printing the runner-up is what makes the wrong-guess failure mode impossible to hide: in the failing case the two numbers will be indistinguishable or inverted, and a reader sees that rather than a bare BAD.

The per-tensor BAD lines still print, and I think that is right: the tensor threshold is a lossy-repack reality (Q4_K_M → q4_1 cannot be element-exact), and the new summary is the thing that decides. The apparent contradiction — 197 "BAD" rows and rc 0 — is exactly the confusion this PR removes, and the verdict line resolves it in one sentence.

Ordering note: #1119 also touches tools/q4nx_diff.py and predates this one (its --layout has only flm|ours, no both). So land #1116 first, then #1119 rebased on it — the other order drops the both option, which is the piece that made the #1110 mismatch visible.

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1110 - Partially compliant

Compliant requirements:

  • The PR introduces logic to detect and name the correct layout when decoding containers, improving detection of mismatched tile orders
  • The PR adds a --layout both option that tries each layout and names the one that matches, preventing a good container from being incorrectly marked as BAD

Non-compliant requirements:

Requires further human verification:

  • The PR does not include the actual layout ID in the container format, which is a requirement from the ticket
  • The PR does not include the offline check in tools/check_q4nx_dir.py, which is a requirement from the ticket
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Layout Detection Logic

The new layout detection logic in verdict() function attempts to determine which layout matches best by comparing error thresholds. However, the thresholds (LAYOUT_MATCH_MAX_ERR and LAYOUT_MATCH_RATIO) are hardcoded and may not be universally applicable across different models or quantization schemes. A more adaptive or configurable approach might be needed to ensure robustness across various containers.

# The worst per-element error a CORRECT container can show: a q4_1 container packed from a Q4_K_M GGUF
# is a requantization, so exact equality is impossible and "no error anywhere" would call every honest
# container BAD. Measured on a Qwen3-0.6B pack (Q4_K_M -> q4_1): worst max|err| 4.1e-2. A container whose
# tiles belong to different weights lands an order of magnitude above it (measured 1.1-2.0).
LAYOUT_MATCH_MAX_ERR = 0.1
LAYOUT_MATCH_RATIO = 3.0     # ... and it must beat the other layout by this much


def verdict(results):
    """(rc, line) for {layout: (exact, inexact, worst_max_err)}.

    A container is laid out ONE way, and a caller that guesses wrong gets a confident "BAD" for a good
    container. So name the layout that matched, and when none did, name the closest and say so instead
    of implying the container is broken.
    """
    if not results:
        return 2, "nothing was compared"
    order = sorted(results, key=lambda k: results[k][2])
    best = order[0]
    worst = results[best][2]
    rival = results[order[1]][2] if len(order) > 1 else None
    exact = results[best][0]
    took = (exact > 0 and worst == 0.0) or (
        worst <= LAYOUT_MATCH_MAX_ERR and (rival is None or rival >= worst * LAYOUT_MATCH_RATIO))
    if took:
        other = "" if rival is None else ", next layout %s worst max|err| %.3e" % (order[1], rival)
        how = "%d tensor(s) exactly" % exact if worst == 0.0 and exact else "worst max|err| %.3e" % worst
        return 0, "matches layout %s (%s%s)" % (best, how, other)
    return 1, ("matches NO layout: closest is %s with worst max|err| %.3e (> %.3e, or not %.0fx better "
               "than %s) - the container's tiles are not these weights"
               % (best, worst, LAYOUT_MATCH_MAX_ERR, LAYOUT_MATCH_RATIO, order[1] if rival is not None else "-"))
Redundant Code

The _decode_pass function contains duplicated code for loading the GGUF reader and tensor list, which can be refactored to avoid redundancy and improve maintainability.

def _decode_pass(args, layout, quiet=False):
    """One layout over every comparable tensor. Returns (exact, inexact, worst_max_err)."""
    import gguf
    from gguf.quants import dequantize

    hdr, base, mm = load(os.path.join(args.q4nx, "model.q4nx"))
    dec = DECODERS[layout]
    r = gguf.GGUFReader(args.gguf)
    want = [t for t in r.tensors if t.name.endswith(".weight") or t.name in ("output.weight",)]
    if args.tensor:
        want = [t for t in want if args.tensor in t.name]
    if not want:
        print("no tensors matched", file=sys.stderr)
        return 0, 0, 0.0
    exact = inexact = skipped = 0
    worst = (0.0, "")
    # our converter's names, so a GGUF tensor can be looked up in the container
    lay = {"attn_q": "self_attn.q_proj", "attn_k": "self_attn.k_proj", "attn_v": "self_attn.v_proj",
           "attn_output": "self_attn.o_proj", "attn_q_norm": "self_attn.q_norm",
           "attn_k_norm": "self_attn.k_norm", "ffn_gate": "mlp.gate_proj",
           "ffn_up": "mlp.up_proj", "ffn_down": "mlp.down_proj", "attn_norm": "input_layernorm",
           "ffn_norm": "post_attention_layernorm", "token_embd": "model.embed_tokens",
           "output_norm": "model.norm", "output": "lm_head"}

    def q4nx_name(n):
        head, _, field = n.rpartition(".")
        if head.startswith("blk."):
            idx = head.split(".")[1]
            return "model.layers.%s.%s.weight" % (idx, lay[head.split(".", 2)[2]])
        return lay[head] + "." + field

    hdr, base, mm = load(os.path.join(args.q4nx, "model.q4nx"))
    dec = DECODERS[layout]
    r = gguf.GGUFReader(args.gguf)
    want = [t for t in r.tensors if t.name.endswith(".weight") or t.name in ("output.weight",)]
    if args.tensor:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant