Skip to content

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

Description

@marcobambini

unblock_scale in tools/mxfp4.py:56 applies the per-tile scales an fp8
checkpoint ships alongside its weights. It landed with #26 and has no test.

The only place its name appears under tests/ is a stub that replaces it with
the identity:

# tests/test_convert_resume.py:103
mx.unblock_scale = lambda q, scale, block: q

That stub is correct for what it is there for — resume never reaches a
dequant, and convert.py imports the name, so it has to resolve or every
check in that file fails at module load. But it means the function's real
behaviour is unexercised, and a regression that made unblock_scale return
q unchanged would pass the entire suite.

Why this one and not some other untested helper

Because of how it fails. Getting the block mapping wrong does not raise, does
not change any shape, and does not produce anything that looks wrong
downstream — it applies each scale to the wrong rows and yields plausible
weights. The docstring on the function makes the case itself:

300 rows with 3 scale rows admits both 128 (the truth, with a partial last
tile) and 100 (a clean split), and the wrong one silently applies each scale
to the wrong rows. The shapes agree in both readings, so nothing downstream
notices.

A conversion is hours long and this runs at the front of it. The symptom would
surface as VQ reconstruction error nobody could attribute to a cause.

What is asked

@fab2s#26's description covers exactly this, and says it is already
written:

tools/../test_fp8_blocks.py covers the block mapping on synthetic tensors
(aligned tiles, partial tiles on both axes, a missing companion, a gross
size mismatch) and cross-checks one real tensor bit-exactly against an
index-array dequant; it also documents the case that cannot be caught, a
compatible-but-wrong tile size, which is why the value is read from config.

It did not make it into the diff, and I asked for it in the merge comment.
Filing it here so it is tracked rather than sitting in a closed PR. Under
tests/, wired into tests/run.sh's converter section next to
test_convert_resume.py, and it should SKIP rather than fail where torch
is unavailable — run.sh's rule is that a missing prerequisite says SKIP,
loudly, and is never a silent pass.

The four synthetic cases plus the bit-exact cross-check are the right set. Two
things worth adding if they are not already in what you have:

  • A partial tile on the last row and column together. want is computed
    with a ceiling on each axis independently and the result is cropped with
    [:M, :N] after two repeat_interleaves, so the corner is the place where
    an off-by-one in either axis would hide.
  • The two readers agreeing. ST.__init__ (mxfp4.py:94) and
    ShardReader.__init__ (convert.py:219) each parse
    quantization_config.weight_block_size out of config.json separately, and
    each falls back to (128, 128). Two copies of one rule drift; a check that
    both return the same tensor for the same input pins them together. Read DeepSeek-V3 checkpoints in the converter #26's own
    description notes that ShardReader "is a second reader and was easy to
    miss", which is the same observation from the other side.

No urgency and no obligation — if you would rather not, say so and I will
write it from the description above; you have already done the part that
needed the checkpoint. What should not happen is it staying untested, given
that this is the one path in #26 that fails silently.

Related: #26.

Metadata

Metadata

Assignees

No one assigned

    Labels

    help wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions