Skip to content

test: cover fp8 block-scale dequantization - #40

Merged
marcobambini merged 3 commits into
sqliteai:mainfrom
tomatotomata:codex/fp8-block-scale-tests
Aug 13, 2026
Merged

test: cover fp8 block-scale dequantization#40
marcobambini merged 3 commits into
sqliteai:mainfrom
tomatotomata:codex/fp8-block-scale-tests

Conversation

@tomatotomata

Copy link
Copy Markdown
Contributor

Resolves #39

What changed

  • Added tests/test_fp8_blocks.py with aligned tiles, a partial final row and column, missing scale companions, and gross shape mismatch cases.
  • Added a bit-exact index-based reference check for the block mapping.
  • Asserted that mxfp4.ST and convert.ShardReader agree when both read the configured block size.
  • Wired the test into tests/run.sh, with an explicit SKIP when torch is unavailable.

Validation

  • python tests/test_fp8_blocks.py passes all 5 checks.
  • python tests/test_convert_resume.py passes.
  • python tests/test_convert_chat.py passes.
  • git diff --check passes.

The test stays synthetic and does not require model weights or a multi-hour conversion.

@marcobambini

Copy link
Copy Markdown
Member

Reviewed. The test is good and I checked it by trying to break it rather than
by reading it — all four mutants I planted in unblock_scale were caught:

mutation result
return q unchanged (the exact regression the resume stub simulates) killed
transpose the block dims, repeat_interleave(bn, 0)…(bm, 1) killed
drop the [:M, :N] crop killed
remove the scale-shape guard killed

expected_dequant being an independent index-array implementation rather than
a second call to the thing under test is what gives it that power, and the 3x5
against block (2,3) case puts a partial tile on both axes at once, which is the
corner where the two independent ceilings and the crop meet. Both were what #39
asked for, and the reader-agreement check pins ST and ShardReader together,
which is the drift #26 itself warned about.

Two things before this comes out of draft.

1. It never runs, on any machine that tests this project

run.sh invokes it as bare python3, and torch is not a system package here —
per CLAUDE.md it is never a repo dependency and every torch checker goes
through uv run --with torch --no-project. So on my machine, with uv installed
and every other torch check running, this reports:

converter
  SKIP  fp8 block-scale mapping — torch not installed

It skips in CI too, and that is the part that decides it: the Linux build job
is the one job that installs uv, so with the right wiring this check would
actually execute there — and #39 exists because the fp8 path is the one with a
silent failure mode. A test that never runs is the same amount of coverage as
no test.

Confirmed the fix, with the command -v uv guard the other torch sites use:

if ! command -v uv >/dev/null 2>&1; then
    sk "fp8 block-scale mapping" "uv not installed"
else
    out=$(run_uv run --quiet --with torch --no-project \
              python tests/test_fp8_blocks.py 2>&1); rc=$?

SKIP becomes PASS with that change and all five checks execute. run_uv is
new on main since you opened this — it puts a deadline on uv, which returns
124 on a hang, and your * case already treats that as a failure. Note the
guard has to be on uv rather than on python3: without uv present, run_uv
exits 127 and your * branch would report FAIL where it should SKIP.

2. One reader's guard is uncovered

test_missing_scale_companion_is_rejected exercises ST only. I deleted
ShardReader's own if sname not in self.wm: raise KeyError — the one in
convert.py — and made it return the tensor unscaled instead. The suite
stayed green.
That is precisely the silent-wrong-answer shape this file
exists to prevent, in the reader #26's description called out as "a second
reader and was easy to miss".

write_safetensors_model(root, block, include_scale=False) already builds the
directory; it needs the ShardReader(root).get("weight") half added alongside
the ST one.

Smaller

  • Rebase on main: it has moved a fair bit today (0.6.8, plus five fixes from
    UCRT64 + first real Kimi-Linear container on Windows: whole-model oracle PASS on AVX2, and five gaps no CI job can reach #36 that touch run.sh). It still merges clean, but run_uv only exists
    after that.
  • The bytes(...view(torch.uint8).flatten().tolist()) round-trip works but
    .numpy().tobytes() is not available without numpy, so if you want it
    shorter, bytes(q.view(torch.uint8).flatten().tolist()) is already the
    portable form — no change needed, just noting I checked it rather than
    assuming.
  • Read DeepSeek-V3 checkpoints in the converter #26's description mentioned the test would also document the case that
    cannot be caught — a compatible-but-wrong tile size, which is why the value
    is read from config rather than inferred. A comment saying so would put the
    reasoning next to the checks that cannot express it.

Nothing here is structural, and the mutation results say the core is right.
Happy to take it as soon as it runs. Thanks for picking this up — it was filed
this morning against a different contributor's PR and you got there first.

marcobambini and others added 2 commits August 13, 2026 10:20
Two things on top of tomatotomata's checks, which are unchanged and were
right: the mutants I planted in unblock_scale — returning q untouched,
transposing the block dims, dropping the [:M, :N] crop, removing the shape
guard — were all caught, and expected_dequant being an independent index-array
implementation rather than a second call to the code under test is what gives
it that.

It never ran. run.sh invoked it as bare `python3`, and torch is not a system
package on the machines that test this: CLAUDE.md says it is never a repo
dependency and every other torch checker goes through uv. So the whole thing
reported "torch not installed" and skipped everywhere, CI included — where the
Linux job is the one that installs uv, which is to say the one place it does
get to run. Through run_uv now, with the guard on uv rather than python3,
since without uv run_uv exits 127 and the catch-all would report FAIL where a
SKIP is meant. 56 passed / 0 failed / 2 skipped becomes 57 / 0 / 2.

And the missing-companion check exercised ST only. Deleting ShardReader's own
`raise` in convert.py, so it returns the tensor unscaled instead, kept the
suite green — a silent wrong answer in the reader sqliteai#26's description called out
as "a second reader and was easy to miss". Both readers now, and that mutant
is killed.

The same mutation against ST survives, and should: without its guard, raw()
still raises KeyError naming weight_scale_inv, so the contract the test asserts
— refuses, and says which tensor — holds either way. That guard buys a better
message, not a different decision, and asserting its exact prose would test the
wording rather than the behaviour.

Also wrote down the case nothing here can catch, which is the reason the tile
size is read from config rather than inferred: 300 rows against 3 scale rows
admits both 128 and 100, both pass every shape check, and the wrong one applies
each scale to the wrong rows.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@marcobambini

Copy link
Copy Markdown
Member

Both points fixed and pushed to this branch —
494f53e, on top of your
commit, which is unchanged. Rebased on main at the same time, since it moved
a good deal today.

1. It now runs. Through run_uv run --with torch --no-project, with the
guard on uv rather than python3. The suite goes from 56 passed / 0 failed /
2 skipped to 57 / 0 / 2 against real containers, and — the part that
actually decides it — here is the Linux CI job, which is the one that installs
uv:

converter
  PASS  fp8 block scales, partial tiles, missing companions, and reader agreement

That line did not exist before. Under bare python3 it read
SKIP — torch not installed, everywhere, CI included.

2. Both readers. test_missing_scale_companion_is_rejected now loops over
ST and ShardReader. The mutation that survived in my review — deleting
convert.py's own raise so it returns the tensor unscaled — is killed.

One thing I checked and deliberately did not change: the same mutation
against ST still survives, and it should. Without that guard, raw() raises
KeyError: 'weight_scale_inv' anyway, so the contract the test asserts —
refuses, and names the missing tensor — holds either way. That guard buys a
better message, not a different decision, and pinning its exact prose would
test the wording rather than the behaviour.

I also wrote down the case nothing here can catch, since #26 described it and
it is the reason unblock_scale takes block as an argument instead of
deriving it: 300 rows against 3 scale rows admits both 128 and 100, both pass
every shape check, and the wrong one applies each scale to the wrong rows.

CI is 9/9 on the branch. I approved the workflow run — it needed a maintainer
for a first-time contributor, and I had read and executed the whole diff
locally by then.

Over to you to take it out of draft; I did not want to flip that on your
PR. Once you do it can go straight in.

For the record, your checks were the right ones and they were right on the
first try: expected_dequant as an independent index-array implementation
rather than a second call to the code under test is what gave the file its
teeth, and 3x5 against block (2,3) puts a partial tile on both axes at once,
which is the corner where the two ceilings and the crop meet. Everything above
is wiring and one missing reader. Thanks for picking this up the morning it was
filed.

@marcobambini
marcobambini marked this pull request as ready for review August 13, 2026 08:43
@marcobambini
marcobambini merged commit 7f1fbba into sqliteai:main Aug 13, 2026
9 checks passed
@marcobambini

Copy link
Copy Markdown
Member

Merged, and #39 closed with it.

CI 9/9 on the merge commit, and the check runs rather than skips on the Linux job — which was the whole difference between this being coverage and being a file.

Thanks. Filed in the morning, correct checks by the afternoon, and the mutation results were the part that made it easy to take: four planted bugs in unblock_scale all caught, including the one the resume stub had been silently simulating since #26 landed.

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.

The fp8 block-scale mapping has no test, and the only stub naming it replaces it with the identity

2 participants