Skip to content

tools: a container with no Q8 tiles no longer claims the Q8 code order was verified - #1045

Merged
bong-water-water-bong merged 1 commit into
mainfrom
1bit/q4nx-noq8-claim
Nov 5, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
1bit/q4nx-noq8-claim

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Follow-up to the review nit on #1037 (pi-prs: "the no-Q8 case still prints the 'Q8 tiles hold the code order' sentence").

Root cause

main() closed with

elif args.against:
    print("chunk layout verified, and the Q8 tiles hold the code order npu/q4nx.cpp decodes")

i.e. the closing verdict was keyed on --against being passed, not on anything having been compared.
check_q8_layout() returns ([], []) for a container with no Q8 tensor (it prints "no Q8 tensors, nothing to
check against the source weights" per file), so a Q4-only container exited 0 with a line claiming a code-order
verification that never happened — the same class of silent-success the previous commit in #1037 was about, one
level up in the control flow.

Fix

Count the Q8 tensors that were actually decoded and correlated (compared += len(rows) over all files) and key
the closing line on that:

case before after
Q8 container, right GGUF chunk layout verified, and the Q8 tiles hold the code order … unchanged
Q8 container, wrong GGUF chunk layout NOT verified, rc 1 unchanged
container with no Q8 tiles chunk layout verified, and the Q8 tiles hold the code order …, rc 0 chunk layout verified; no Q8 tile was decoded, so no code order was checked against the source weights, rc 0
no --against unchanged unchanged

The default path and the JSON summary are untouched.

Checked

run result
tools/check_q4nx_dir.py ~/models/npu/Qwen3-0.6B --against …/ZR1-1.5B-bf16.gguf (no Q8 tensor) rc 0, new line, no success claim
…/ZR1-1.5B-npu-q4km-q8 --against …/ZR1-1.5B-bf16.gguf rc 0, A corr +0.9998, success line unchanged
same container, --against Qwen3-0.6B's GGUF rc 1, both tensors named, chunk layout NOT verified
tools/check_q4nx_dir.py --self-test PASS — new row drives main() on a synthetic no-Q8 container and fails if the claim reappears
tests/gguf_to_q4nx_test.py (the existing CI step) PASS
tools/copyright.py --check rc 0

No issue to close: this is the review nit from #1037 (merged at 2fad106f), not a separate defect report.

…order was checked

--against printed its success line ("...and the Q8 tiles hold the code order
npu/q4nx.cpp decodes") whenever the chunk-layout check passed, including for a
container with no Q8 tensor at all: the per-file line said "nothing to check"
and the closing line said the code order had been verified.  The closing line now
depends on whether any Q8 tile was actually decoded and correlated, and says
plainly that no code order was checked when none was.  Self-test gains a row that
drives main() on a no-Q8 container and fails if the claim comes back.
@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 🔶

1037 - Partially compliant

Compliant requirements:

  • The tool now verifies Q8 tile code order against the source GGUF using both code orders
  • The tool correctly handles containers with no Q8 tensors and does not claim verification occurred
  • The tool imports numpy and gguf only when --against is used
  • The tool includes tests for both correct and incorrect code order cases
  • The tool correctly reports when no Q8 tensors are present and no verification was performed

Non-compliant requirements:

  • The tool does not include documentation updates in the same PR as the behavior changes (docs, README, wiki)

Requires further human verification:

  • Verification that the tool's behavior matches the hardware measurements mentioned in the ticket (e.g., ZR1-1.5B results)
  • Confirmation that the new tests cover all relevant edge cases and are integrated into CI
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect Success Message for No-Q8 Containers

The tool previously printed a success message claiming Q8 code order verification was performed even when no Q8 tensors were present in the container. This could mislead users into thinking the verification was successful when it was not actually run. The fix correctly checks if any Q8 tensors were actually decoded and correlated before printing the success message.

elif args.against and compared:
    print("chunk layout verified, and the Q8 tiles hold the code order npu/q4nx.cpp decodes")
elif args.against:
    # No Q8 tensor was decoded, so nothing was compared: say that instead of
    # printing the success line for a check that did not run.
    print("chunk layout verified; no Q8 tile was decoded, so no code order was checked "
          "against the source weights")

@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 66336de7 — my nit on #1037 is closed, and the fix is keyed on the right thing (compared, not args.against).

Re-ran the three container cases on this box, plus the self-test:

# container with no Q8 tiles, --against passed
$ tools/check_q4nx_dir.py ~/models/npu/Qwen3-0.6B --against ~/models/zyphra-zr1/ZR1-1.5B-bf16.gguf
  ...: no Q8 tensors, nothing to check against the source weights
  chunk layout verified; no Q8 tile was decoded, so no code order was checked against the source weights    (rc 0)
# the misleading line is gone; the Q8 container is unchanged
$ tools/check_q4nx_dir.py ~/models/npu/ZR1-1.5B-npu-q4km-q8 --against ~/models/zyphra-zr1/ZR1-1.5B-bf16.gguf
  A corr +0.9998 / B corr +0.0011 ... chunk layout verified, and the Q8 tiles hold the code order npu/q4nx.cpp decodes   (rc 0)
# wrong source weights still refuse
$ tools/check_q4nx_dir.py ~/models/npu/ZR1-1.5B-npu-q4km-q8 --against ~/models/Qwen3-0.6B-Q4_K_M.gguf
  FAIL ... not the layout npu/q4nx.cpp decodes ... chunk layout NOT verified    (rc 1)
$ tools/check_q4nx_dir.py --self-test        # passes, and the new row fails if the claim reappears
$ tools/copyright.py --check                  # rc 0

The self-test row is a real test rather than an assertion about the string: it drives main() on a synthetic no-Q8 container with --against, so it exercises the compared == 0 branch through the same control flow the tool uses.

One nit, pre-existing and not from this PR: --against a path that is not a GGUF dies with a raw traceback (ValueError: GGUF magic invalid from gguf_reader) instead of a one-line message, rc 1 either way. Worth a try around the reader the next time this file is open; not a blocker.

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