Repository navigation
npu: Hy-Memory-30B-A3B (hy_v3) runs on the existing MoE path, read the way its pack states it (Refs #1786) - #2279
Conversation
…e way its pack states it (Refs #1786) hy_v3 routes as DeepSeek-V3 (the fork's hy-v3.cpp), which moe_mlp already runs; the pack said it differently, and each of these alone made the reply ' of of of': - config: moe_router_use_sigmoid (was read as softmax), router_scaling_factor, first_k_dense_replace + the dense width from intermediate_size, expert_hidden_dim (768; moe_intermediate_size says 1536, upstream experts are [768][2048]); - the router stored [H][E] (upstream's transpose) is transposed at load; moe_expert_bias (F32, or bf16) is read; - expert slices on an [N][K] grid despite FLM names, decided from the first MoE layer's own gate stack (vs upstream expert 0: corr 0.9974 against 0.008 for the old [K][N] reading); - a non-MLA MoE's leading dense layer gets DG/DU/DD + dense_mlp; a batched prefill takes the per-token path. Real pack: 'Paris<eos>'; logits vs a numpy hy_v3 forward on its weights corr 1.000000, max |d| 3e-5 of 26.6. New CI arm batch_check_state_hy_v3 (corr 1.0000000); six mutants fail it. ctest 270/270. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ready for review at 3763ff6 |
PR Reviewer Guide 🔍(Review updated until commit fdf4120)Here are some key observations to aid the review process:
|
|
VERDICT: READY at The two symptoms, in the order that makes them useful
Refusal by name is the diagnostic that makes a pack a day's work; The facts that had to be right
Two asks
And the scope line is right"Host-side only; the NPU is off" with |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 3763ff6: CHANGES
Reviewed at 3763ff67a67ffbddf3808a9c5ed91a9fa8aa6cf8. It is on main (0 behind) and merges clean. The diff is 5 files, +332/-12.
Blocking: the [N][K] slice rule also fires on MiniCPM-V-4.7-35B-A3B-NPU2, so "no other MoE container changes reading" does not hold.
The rule's lookup, model_tensor_by_name("model.layers.%d.mlp.gate_exps_proj.weight"), falls back to the other spelling (model_tensor_name_alt). So it finds MiniCPM-V's model.layer.0.mlp.gate_exps_proj.weight, just as it finds hy_v3's. Its own header and config (~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2) satisfy every clause:
| clause | MiniCPM-V-35B |
|---|---|
| gate stack | [4096, 8, 4736], ndim == 3 |
shape[0] % n_expert == 0 |
4096 % 256 = 0 |
shape[0]/E * 32 == expert_im_ |
16 × 32 = 512 = moe_intermediate_size (no expert_hidden_dim) |
shape[1] * 256 == pad_k(H) |
8 × 256 = 2048 = H |
expert_im_ != pad_k(H) |
512 ≠ 2048 |
So this head sets moe_slice_nk_ for MiniCPM-V and reads its expert slices [N][K], where main read them [K][N] (the FLM-name default). That pack is #1788's model, which has its own lead.
The geometry may well say [N][K] is the right reading for MiniCPM-V too, in which case this also fixes a latent permutation. But that is a claim, and it needs the evidence this PR gave hy_v3:
- either a ranged read of one MiniCPM-V expert against its upstream checkpoint, as both
[N][K]and[K][N](the corr 0.9974 vs 0.008 test), plus a before/after answer on the pack; - or narrow the rule, for example to
model_typehy_v3, until MiniCPM-V is measured.
Whichever way it goes, the body's sentence should change.
The same scan found no config on this box that states router_scaling_factor, first_k_dense_replace, expert_hidden_dim, moe_router_use_sigmoid or leading_dense_block_count. The config-keyed changes therefore touch no other pack here; only the shape-keyed slice rule does.
What holds
- The new
batch_check_state_hy_v3arm reportsPASS sequential logits vs the hy_v3 reference on the container's weights: corr 1.0000000 (needs 0.99999), max |d| 1.26e-05 of 8.84, rc 0, withNPU_INFER_HOST_GEMM=1. - The other arms (qwen2, lfm3, qwen3_5, default) pass.
- The mutant table (six mutants, each failing the reference while batched == sequential passes) is the right shape of evidence. I did not re-run it.
CI build at this head was still in progress when I posted.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- each piece is justified by an ablation that the test's own threshold rejects, and the row is a real CI gate
The ablation table is the right kind of evidence. Not "we added sigmoid routing", but:
| mutant | corr |
|---|---|
| softmax gating | 0.9965 |
| no scale | 0.94 |
| bias ignored | 0.87 |
against a correct forward at 1.0000000, and LFM3_MIN_CORR = 0.99999 in the harness. So each of those mutants fails by construction, not by assertion order -- the table is enforced by the threshold, and the closest mutant (0.9965) is still 350x further from the bound than the correct value is. That is the shape I want: the pieces are necessary, measured one at a time, rather than a bundle whose necessity nobody can check.
The sentence that explains why this test had to exist is "batched == sequential cannot see it" -- the existing equality check compares the two paths of one implementation, so a wrong reading inside both passes it silently. That is precisely why a numpy reference forward, on the container's own weights, is the only detector here: it is an independent implementation, and correlation against it is the measurement.
And the row is a genuine gate: batch_check_state_hy_v3 fabricates a two-layer pack in a temp dir rather than needing the 30B container, numpy is installed in the build job, and the script's except ImportError: return 77 pairs with the row's SKIP_RETURN_CODE 77 -- so it runs in CI and skips honestly where numpy is absent. That is the difference between this row and the pack-gated ones next door, and it is worth having on a PR whose subject is a 30B container nobody can put in CI.
The reading details are anchored rather than assumed: the router stored [H][E] (upstream's [E][H] transposed, "checked by a ranged read of the checkpoint"), the expert slices on an [N][K] grid where [N][K] gives corr 0.9974 and the old reading gives 0.008, and expert_hidden_dim (512) taking precedence over moe_intermediate_size (1024) because that is what the real pack does -- the same trap #2265 hit from the other direction.
One suggestion, and it is small because the fixture is synthetic: make the three mutants arms rather than a table. The harness already fabricates the pack and already computes correlation, so three more rows (batch_check_state_hy_v3_mutant_softmax, ..._no_scale, ..._bias_ignored) each asserting corr < LFM3_MIN_CORR would turn "each of these mutants fails it" from a claim in the description into something CI proves on every push -- and it would catch the day someone loosens the threshold, which is the only way the table stops being true.
…B, measured right there too (Refs #1786) Review of #2279 at 3763ff6: the shape rule matches MiniCPM-V-4.7-35B-A3B-NPU2's gate stack ([4096][8][4736], 256 experts of 512). Measured against openbmb/MiniCPM-V-4.7-35B-A3B's own layer-0 gate_up_proj, expert 0: corr 0.997 as [N][K], 0.024 as [K][N] (main's reading). That pack stops at 'MoE designs missing' on this forward with the rule on and with it off, so no answer changes. No other container on the box fires the rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Answer to the CHANGES verdict at 3763ff6. You were right: the
|
|
ready for review at fdf4120 |
|
Persistent review updated to latest commit fdf4120 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at fdf4120: READY
Reviewed at fdf41205e5af12ffb230609c746583c82f0a0d7c. It is 1 behind main and merges clean. The new commit fdf41205 touches only docs/npu.md (+5/−1) and the body; the code is unchanged from 3763ff6.
My CHANGES at 3763ff6 is answered. The body no longer says that no other MoE container changes reading. It now names MiniCPM-V-4.7-35B-A3B-NPU2 as the one other pack the [N][K] rule fires on, which is the pack my header scan found, and gives the measurement I asked for:
- Layer-0 expert 0 against
openbmb/MiniCPM-V-4.7-35B-A3B's owngate_up_proj: corr 0.997 as[N][K]and 0.024 as[K][N], main's reading. So the rule is a fix there too: main read MiniCPM-V's experts permuted. - No answer changes today. The body says the pack "stops at
MoE designs missingon this forward with the rule on and with it off".
Both figures are the author's: the upstream checkpoint is not on this box, and I did not re-run the 35B init. They are stated with the method (a ranged read of the upstream tensor), which is enough to act on.
Carried over from my 3763ff6 review
- The new arm.
batch_check_state_hy_v3holds the forward to a numpy hy_v3 at corr 1.0000000 (max |d| 1.26e-05 of 8.84). The qwen2, lfm3, qwen3_5 and default arms also pass. - The config-keyed changes. No config on this box states
router_scaling_factor,first_k_dense_replace,expert_hidden_dim,moe_router_use_sigmoidorleading_dense_block_count, so they touch no other pack.
For #1788's lead: once the MoE-designs gap closes, MiniCPM-V-35B will be read [N][K] on any build with this PR. By the measurement above, that is the correct reading, and any earlier MiniCPM-V MoE numbers came from the permuted reading.
CI build at this head: completed success, "100% tests passed, 0 tests failed out of 272". batch_check_state_hy_v3 is test #254 there and passed.
What
FastFlowLM's
Hy-Memory-30B-A3B-NPU2(Tencent hy_v3, 30B MoE, 128 experts top 8) now answers on the host. Before, it was refused (moe router), and once loaded its reply wasof of of of. Refs #1786. Host-side only; the NPU is off.hy_v3 routes the way DeepSeek-V3 does (our llama.cpp fork's
src/models/hy-v3.cpp,build_moe_ffnwith sigmoid gating):moe_mlpalready runs that routing. What the forward did not do is read the pack's way of stating it.The pieces
expert_gating_func)moe_router_use_sigmoid: truerouter_scaling_factor(2.826)first_k_dense_replace, widthintermediate_size; DG/DU/DD designs anddense_mlp, as the MLA path hasmoe_intermediate_size= 1536expert_hidden_dim= 768tencent/Hy-Memory-30B-A3B: experts and shared expert are[768][2048][E][H][H][E], transposed at load when that is its shaperouter.gate.weightis exactly its transposemoe_router.bias)moe_expert_bias, F32 (or bf16)mlp.expert_bias[K][N](FLM-name default)[N][K], decided from the first MoE layer's gate stack[E*N/32][K/256][chunk][N][K], 0.008 as[K][N]The upstream checks are ranged reads of the checkpoint's own tensors. A batched prefill of a non-MLA MoE with a leading dense layer takes the per-token path.
Where else the rule applies. The
[N][K]rule needs the stack's per-expert row tiles × 32 to equal the expert width. A transposed Qwen MoE pack has H/32 there (2048 against 768 for Qwen3-Coder-30B). A scan of every*_exps_projcontainer on this box finds one other pack it fires on,MiniCPM-V-4.7-35B-A3B-NPU2(#1788's model, found in review). The reading is right there as well: its layer-0 expert 0 againstopenbmb/MiniCPM-V-4.7-35B-A3B's owngate_up_projgives corr 0.997 as[N][K]and 0.024 as[K][N], main's reading. That pack stops atMoE designs missingon this forward with the rule on and with it off, so no answer changes; the rule only means its experts will be read correctly once that gap closes. The config-keyed changes touch no other config on the box. The leading-dense path needsfirst_k_dense_replaceorleading_dense_block_count.Evidence (host, 2026-11-12)
Paris<eos>. Before the slice fix:of of of of of.q4nx_dequant.c) and this forward, on the same 28-token prompt: logits corr 1.000000, max |d| 3e-5 of 26.6. The top 5 are identical:Paris26.65,巴黎18.79,Paris17.16,France15.1,London12.77.Tests
New CI arm
batch_check_state_hy_v3. It fabricates a two-layer pack written the way this one is:model.layer.N.names, a dense layer 0, and a sigmoid router stored[H][E];[N][K]expert stacks declared 3-D, andexpert_hidden_dim512 against amoe_intermediate_sizedecoy of 1024.It holds the forward's logits to a numpy hy_v3 on those bytes: corr 1.0000000. Each mutant fails it, while batched == sequential passes every time:
[K][N]router_scaling_factormoe_intermediate_sizeexpert gate)The first data set's bias was too small to change a selection, so the bias mutant passed. That also exposed a loader that accepted only an F32 bias; both are fixed.
Suite. Full ctest 270/270, device tests excluded. Every
batch_check_state_ci.pyarm andgptoss_forwardpass. Re-run on the rebased head: build, the 12 cache/MoE/dequant/semantics tests, and the hy_v3 arm.tools/copyright.py --checkrc 0.Not in this PR: a kernel set or device row for this pack.
🤖 Generated with Claude Code