fix(minimax-h3): the fp32-island rule now BINDS the bf16 GGUF host loader (#244) - #272
Merged
Merged
Conversation
…ader (#244) #253 fixed the INSTANCE — rope.inv_freq and adaln_t_table stopped being rounded into bf16 bits that Ptr<float>() then reinterprets as garbage — but it did so by hardcoding those two names. MiniMaxH3IsFp32IslandTensor covers SEVEN: both patch projections, time_embedder.*, both output heads, and those two. Five islands were still silently down-converted on this path. Severity, stated precisely because the issue reads worse than it is: those five are NOT garbage. BindMiniMaxH3DitViews types each tensor from the storage map it lands in, so a bf16 island is correctly TYPED bf16 and consumed as such. rope.inv_freq was uniquely dangerous because the forward reads it through a raw Ptr<float>(), bypassing the dtype entirely. What remained was a silent PRECISION divergence from upstream on one loader, not corruption — and it is the CPU --dequant-bf16 path only; the production GPU stream was never affected. The fix consults the single source instead of re-listing names. The rule's own contract says "all four staging paths must agree"; this HOST loader is a FIFTH that no gate covered, which is exactly how the two descriptions drifted apart. The gate is the load-bearing half: it walks every tensor the loader placed and requires island => storage (f32), non-island => bf16_storage, with an `islands >= 6` floor so a predicate matching nothing cannot pass vacuously. It asserts on the loader's OWN storage split rather than the bound views, because that map IS the decision under test. MUTATION-VERIFIED. Reverting the loader to the hardcoded two-name check turns the suite RED — 78/79 cases, 24 failed assertions, exactly the five missed island categories. Restored byte-for-byte afterwards. Suite: 79/79 cases, 57,395 assertions (was 57,299; the gate adds 96). FOLLOWING_AGENTS_PROTOCOL Assisted-by: Claude Code:claude-opus-5 [ClaudeCode]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #244.
What #253 fixed, and what it left
#253 fixed the instance —
rope.inv_freqandadaln_t_tablestopped being rounded into bf16 bits thatPtr<float>()then reinterprets as garbage. But it did so by hardcoding those two names.MiniMaxH3IsFp32IslandTensorcovers seven: both patch projections,time_embedder.*, both output heads, and those two. Five islands were still silently down-converted on this path.Severity, stated precisely
The issue reads worse than it is, so: those five are not garbage.
BindMiniMaxH3DitViewstypes each tensor from the storage map it lands in, so a bf16 island is correctly typed bf16 and consumed as such.rope.inv_freqwas uniquely dangerous because the forward reads it through a rawPtr<float>(), bypassing the dtype.What remained is a silent precision divergence from upstream on one loader, not corruption — and on the CPU
--dequant-bf16path only. The production GPU stream was never affected, which is why every render this session was correct.The fix
Consult the single source instead of re-listing names. The rule's own contract says "all four staging paths must agree" — this host loader is a fifth that no gate covered, which is exactly how the two descriptions drifted apart.
The gate is the load-bearing half
It walks every tensor the loader placed and requires island ⇒
storage(f32), non-island ⇒bf16_storage, with anislands >= 6floor so a predicate matching nothing can't pass vacuously. It asserts on the loader's own storage split rather than the bound views, because that map is the decision under test.Mutation-verified
The 24 are exactly the five island categories the hardcoded check missed. Tree restored byte-for-byte afterwards.
Suite grew 57,299 → 57,395 (the gate adds 96 assertions), so it is genuinely executing rather than passing vacuously.
Pattern
Third instance today of a rule documented as universal that one code path quietly ignores — after
decoder_tiling"defaults false" (#251) andvllm_server_maincompiled out underVLLM_CPP_SERVER=OFF(#202). Cheap to fix, expensive to rediscover.