MODEL-NEMOTRON-H W3 repair: ClaimMamba ignored quantized, and the legacy-alias precedence was INVERTED (#517) - #572
Closed
localai-bot wants to merge 7 commits into
Closed
MODEL-NEMOTRON-H W3 repair: ClaimMamba ignored quantized, and the legacy-alias precedence was INVERTED (#517)#572localai-bot wants to merge 7 commits into
quantized, and the legacy-alias precedence was INVERTED (#517)#572localai-bot wants to merge 7 commits into
Conversation
…fig + 18487-tensor index, headers only (#517) FOLLOWING_AGENTS_PROTOCOL W3 of .agents/specs/nemotron-h-model.md needs a config the parse gate can read and an index the enumeration gate can be checked against, WITHOUT the 20.1 GiB checkpoint. Both are projected from the live NAS copy of nvidia/NVIDIA-Nemotron-3.5-Lightning-30B-A3B-NVFP4 at the pinned revision 29f2d1746d8f41e316523194b19018707749b1b1, reading JSON and safetensors HEADERS only -- no tensor bytes. config.json is the released file MINUS exactly two keys, quantization_config.{config_groups,quantized_layers}: 865 KB of the 1.34 MB file, 5981 entries each, and the surface W1 (#517 W1) owns. `ignore` (72 entries, 2.4 KB) is DELIBERATELY KEPT -- its `mtp*` wildcard is what makes the MTP tower unquantized, which the enumeration has to know and would otherwise have to guess. index.json projects all 18487 tensors to one entry per (layer index, tensor suffix) family with dtype and shape; only the ROUTED-EXPERT index is collapsed to {E}, with `count` recording how many experts share the entry. Layer indices are PRESERVED on purpose, so a wrong layer-type mapping cannot survive the fixture. 707 families, sum(count) == 18487, dtypes BF16 485 / F32 6086 / F8_E4M3 5981 / U8 5935. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…enumerated, KV-shaped (#517) FOLLOWING_AGENTS_PROTOCOL W3 of .agents/specs/nemotron-h-model.md. It makes the architecture known; it runs nothing. The forward REFUSES BY NAME (W4, itself blocked on the Mamba2 SSD CUDA arm #496 W2) and the GGUF arm REFUSES BY NAME (W7, owed per §5b). RED FIRST. The gate was written against declared-but-empty implementations and run before any of it worked, Release -Werror, doctest 2.5.2: [doctest] test cases: 10 | 2 passed | 8 failed | 0 skipped [doctest] assertions: 1258 | 1186 passed | 72 failed | [doctest] Status: FAILURE! failing in exactly the intended shape -- CHECK( 0 == 52 ) for the schedule depth, CHECK( {} == {5, 12, 19, 26, 33, 42} ) for the attention indices, REQUIRE( 0 == 2 ) for the KV groups, "did NOT throw at all!" for the GGUF refusal, 18487 unclaimed tensors for the enumeration. ENUMERATION, the hard number: 18487 of 18487 released tensors CLAIMED by a named consumer. 0 unaccounted, 0 invented, 0 refused. Gated both directions against the committed headers-only projection of the released index, plus a live case that re-verifies that projection against the checkpoint itself when VT_NEMOTRON35_SNAPSHOT names it. Four things the plan had wrong or unstated, settled against source and disk: 1. layers_block_type is the source of truth -- but vLLM VENDORS a NemotronHConfig with the OPPOSITE polarity (hybrid_override_pattern as the ctor arg, layers_block_type derived). That class is imported for TYPE ANNOTATION only; the runtime object comes from transformers AutoConfig, where num_hidden_layers is a property over layers_block_type whose setter discards the checkpoint's value. Both spellings parse, the modern one wins. 2. moe_latent_size does NOT need three states: upstream's predicate is `getattr(config, "moe_latent_size", None) is not None`, so ABSENT and `null` are the same state. std::optional covers both; a real value refuses (§0). 3. The shared detail::ResolveMambaSsmCacheDType is the WRONG reader here and silently HALVES the recurrent state. It is keyed on `mamba_ssm_dtype` (Qwen3.5's spelling); NemotronH ships `mamba_ssm_cache_dtype`, so it returned the CONVOLUTION dtype. Caught by the KV gate as page_size_bytes() 1085440 vs an expected 2134016 -- the SSM state at bf16 instead of f32. Not a Qwen bug; two families, two config keys. Resolved locally, reason recorded at the call site. 4. Upstream's default conv layout is "SD" = (state_len, dim) while ours is (dim, state_len). Same bytes; the local convention is kept so the shared runner sees one orientation, and the divergence is commented rather than left for W4 to rediscover. KV: two groups carrying REAL per-layer names -- 6 attention layers on FullAttentionSpec(2 kv heads, head_size 128), 23 mamba layers on MambaSpec {{6144,3},{64,64,128}} / {bf16,f32}. conv_dim == 6144 is falsified straight off disk by conv1d.weight BF16 [6144,1,4]. The names are load-bearing: kv_cache_utils.cpp:979 and kv_cache_interface.cpp:151-158 both multiply a group's page by layer_names.size(), so a one-element tag under-counts by 23x and 6x. SCOPE: no per-module quant algorithm is resolved -- that is W1, not on main. W3 reads four coarse falsifiable keys (quant_method, quant_algo, kv_cache_scheme, the `mtp*` entry in `ignore`) and derives scale companions STRUCTURALLY; the 18487-tensor gate is what proves the derivation. A non-ModelOpt producer refuses by name. One piece of debt is recorded in the code: a dense `mlp` block's quantized companion layout is DERIVED, because no in-scope checkpoint ships one. nemotron_h.h lives under src/, not include/vllm/: W3 ships nothing on the public ABI and include/vllm/ is a USER_USAGE_PREFIXES surface (#515). RECORDS. docs/FEATURES.md gains an honest scaffold row (registering an architecture IS a feature-surface change, and check-supported-models.py binds the two), the stale "Inventoried but blocked" entry is removed so the page does not say two things, and the three registered counts move 37->38 / 31->32. docs/USAGE.md gains one row in "Architectures that resolve but refuse to run": check-doc-checkpoint classifies the root CMakeLists.txt as a USER_USAGE file, and the obligation is real rather than an artifact -- pointing the CLI at a NemotronH checkpoint now resolves and refuses by name instead of reporting an unsupported architecture. That is one row past the FEATURES.md-only scope I was given, on the operator's own reasoning applied to the surface the checker names. test_model_registry.cpp PINS the registry contents four ways; all four are reconciled, never weakened: the two size assertions 37->38, both byte-order arch sequences, kExampleConfigArchitectures, both pinned "Supported architectures:" refusal strings, and a new explicit NemotronH branch asserting is_hybrid TRUE / supports_multimodal FALSE instead of letting it fall into the pure-text-full-attention else-branch. test_model_loader_gguf.cpp's canonical arch string gains the entry too. GATES, Release -Werror CPU: test_nemotron_h_scaffold 10/10 cases, 38245/38245 assertions, SUCCESS with VT_NEMOTRON35_SNAPSHOT 10/10 cases, 39113/39113 assertions, SUCCESS Debug arm (asserts unmasked) identical full ctest 100% tests passed, 0 tests failed out of 401 (test_voxtral_e2e skipped, no asset) MUTATION-PROVEN (each applied alone, rebuilt, run, reverted, tree verified clean): moe -> attention layer mapping 4 cases / 9 assertions FAILURE attention index set shifted by +1 2 cases / 2 assertions FAILURE conv_dim without 2*n_groups*state_size 4 cases / 8 assertions FAILURE SSM dtype collapsed to the activation 1 case / 2 assertions FAILURE mamba dt_bias left unclaimed 1 case / 2 assertions FAILURE Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…acy-alias precedence was INVERTED (#517) Three review findings on `row/MODEL-NEMOTRON-H-W3C` @ `3295d0c1a` (PR #565). The verdict was PASS; this is the tightening pass. 1. `ClaimMamba` hard-coded its FP8 companions. Every other claimer (`ClaimNvfp4`, `ClaimMoe`, `ClaimMlp`, `ClaimAttention`'s `fp8_kv`) gates on `quantized`; `ClaimFp8` did not. The released config MINUS `quantization_config` -- the shape a released bf16 NemotronH safetensors checkpoint actually ships, which spec 5b already owes -- parsed without refusal and enumerated 92 tensors that checkpoint does not have (23 mamba blocks x 2 projections x 2 companions), first `backbone.layers.0.mixer.in_proj.weight_scale`. Nothing consumes the map yet, so no gated claim was wrong -- but an unimplemented arm is refused BY NAME, never silently mis-enumerated. `quantized` is now threaded through `ClaimFp8`/`ClaimMamba` and both call sites (backbone `quantized`, MTP `mtp_quantized`). The released checkpoint is quantized and its MTP schedule is {attention, moe}, so the 18487-tensor gate is unchanged. 2. `Get{Int,Double,Bool}Aliased` preferred the MODERN key. Upstream does the OPPOSITE for the `mamba_*` scalars: `configuration_nemotron_h.py:145-155` is `self.n_groups = kwargs.pop("mamba_n_groups") if "mamba_n_groups" in kwargs else self.n_groups`, which OVERWRITES an already-populated dataclass field, so LEGACY wins. Re-derived by RUNNING transformers @ 7d06b1a5, not by reading it: `NemotronHConfig(n_groups=8, mamba_n_groups=4, conv_kernel=4, mamba_d_conv=7)` -> `n_groups=4, conv_kernel=7`. The SCHEDULE pair is genuinely modern-wins (`:158-165` consults the pattern only when the list is None; `NemotronHConfig(layer_types=['mamba','mamba'], hybrid_override_pattern='*-')` -> `['mamba','mamba']`), and is left alone. Upstream disagrees with itself between the two families; both polarities are now mirrored and both are stated explicitly where they are implemented. Worse than the behavior was the RECORD: the comment asserted "the modern key wins" as if it were upstream's rule, which is what would mislead the next porter. No released checkpoint ships both spellings of one field, so this was a mirroring defect, not a live one -- which is exactly why it needed a test. 3. "the unported ARMS refuse by name" had one SUBCASE (GGUF). `ForwardNemotronHForCausalLM` is an unconditional `VT_CHECK`, which throws `std::runtime_error`, so it is directly callable with a stub `LoadedModel`. Now asserted, including that the message names W4 and the spec. 4. `NemotronHBlockName` had zero call sites. `BlockFromName` now maps both directions through it and builds its refusal's expected-list from the enum, so a fifth block kind cannot arrive with a message that still lists four. RED first for 1 and 2 on the pre-fix tree: 12 cases, 9 assertions failed -- `companions.empty()` reporting 92 with that exact first name, and `8 == 4`/`4 == 7`/`2 == 9`/`128 == 77`/`true`/`0.001 == 0.5`/`0.1 == 0.6`/ `0.0001 == 0.7`. GREEN after: 12 cases / 38284 assertions / 0 failed, `Status: SUCCESS!` (baseline was 10 / 38245). FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…ep, and two residuals (#517) Records what neither the code nor git carries: which of the fresh review's findings were fixed and how, the two report-only items, and the full mutation proof re-run on the repaired tree. Also CORRECTS 5c. Its reconnaissance item 1 said both spellings are accepted "the modern one winning" as an unqualified rule. That is true of the SCHEDULE pair only. For the `mamba_*` scalars upstream is the OPPOSITE (configuration_nemotron_h.py:145-155 overwrites the already-populated field), re-derived by RUNNING transformers @ 7d06b1a5. A record that states the opposite of upstream is the part that misleads the next porter, so the generalization is scoped and 5d carries the evidence. Residuals recorded, NOT fixed here: - docs/FEATURES.md:171 says "27 of the 32 registered text-generation architectures" while :173 implies 38-3-1 = 34 and test_model_registry.cpp:47 says "34 text archs". W3's +1 was right; the BASE was already stale and no checker validates it. This branch has no docs/ authority, and repairing a pre-existing doc drift inside a scoped repair would hide it. - Fixture DTYPE drift is invisible offline by design: names and shapes are pinned in the committed projection, dtypes only by the live case, and CI has no checkpoint. Named so W4/W6 do not rediscover it. 5b's safetensors-bf16 row now says its enumeration is correct. Gate evidence, disk 68G free (85%) throughout: Release -Werror full build 0 warnings; scaffold 12/12 cases 38284/38284 offline and 12/12 39152/39152 live; Debug (-g0, asserts unmasked) identical on both; full ctest 100% passed, 0 failed out of 403. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…elf -- doc-checkpoint needs docs/ authority (#517) `scripts/agent-preflight.sh --staged` is green on every gate except `doc-checkpoint range`, which fails on `3981de6a4`: it touched `src/vllm/model_executor/models/` without touching `docs/FEATURES.md`. The rule is unconditional and PER-COMMIT -- `check-doc-checkpoint.py:79` classifies any `src/vllm/model_executor/models/` path as `feature_surface`, `:313` requires `docs/FEATURES.md` in the SAME commit, and `commits_in_range` walks `rev-list --reverse --no-merges origin/main..HEAD`, so W3C's own `docs/FEATURES.md` edit in `3295d0c1a` does not cover a later commit. `ci.yml:343` runs the identical invocation on the PR, so the PR is red for the same reason. The checker has no exemption mechanism, by design. This repair branch was dispatched with `docs/` explicitly excluded from its authority, so it does not take the obvious fix and does not weaken the checker to go green. Recorded in the spec with the exact minimal edit the operator can fold into `3981de6a4`: one truthful clause on the `NemotronHForCausalLM` row naming the bf16 enumeration this commit corrected. It disturbs neither W3's existing `docs/` edits nor the separate finding-5 count drift. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…ce (#517) `72e661ae4` is a SPEC-DSPARK (#442) records commit: .agents/benchmark-record.md, .agents/specs/dspark-spec-decode.md, docs/BENCHMARKS.md, docs/STATUS.md. It touches no file this row's gates compile or execute, so the gate evidence recorded in spec 5d stands unchanged rather than being re-asserted from a rebuild it cannot move. That claim is mechanically verified in the commit that follows, not eyeballed. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
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.
Supersedes #565 (
row/MODEL-NEMOTRON-H-W3C@3295d0c1a), which a fresh reviewreturned PASS on with four MINOR/NIT findings and two report-only items.
This branch is that head, with
origin/mainre-merged (W1'sMIXED_PRECISIONresolver having landed as
1bc5ef82c) and the findings repaired.Issue: #517 — W3 of
.agents/specs/nemotron-h-model.md.Record: spec §5d.
The findings
ClaimMambaignored thequantizedflagNemotronHBlockNamehad zero call sitesdocs/FEATURES.md:171arch count is off by two, pre-existingdocs/authority on this branch1. Every other claimer (
ClaimNvfp4,ClaimMoe,ClaimMlp, andClaimAttention'sfp8_kv) gates onquantized;ClaimFp8did not. Thereleased config MINUS
quantization_config— the shape a released bf16NemotronH safetensors checkpoint actually ships, which spec §5b already owes —
parsed without refusal and enumerated 92 tensors that checkpoint does not
have (23 mamba blocks x 2 projections x 2 companions), first
backbone.layers.0.mixer.in_proj.weight_scale. Nothing consumes the map yet, sono gated claim was wrong; but an unimplemented arm is refused BY NAME, never
silently mis-enumerated. The 18487-tensor gate is unchanged — the released
checkpoint is quantized and its MTP schedule is
{attention, moe}.2. Upstream is
self.n_groups = kwargs.pop("mamba_n_groups") if "mamba_n_groups" in kwargs else self.n_groups(
configuration_nemotron_h.py:145-155), which overwrites an already-populateddataclass field — so legacy wins for the
mamba_*scalars. Re-derived byRUNNING transformers @
7d06b1a5, not by reading it:The SCHEDULE pair is genuinely modern-wins and is left alone. The two families
disagree upstream; both polarities are now mirrored and both stated where they
are implemented, each with its own anchor. Worse than the behavior was the
record: the comment and spec §5c asserted "the modern one wins" as upstream's
rule. No released checkpoint ships both spellings of one field, so this was a
mirroring and record defect, not a live one — which is why it needed a test.
Verification
RED first for findings 1 and 2 on the pre-fix tree: 9 failed assertions,
reporting
92companions with that exact first name, and8 == 4/4 == 7/2 == 9/128 == 77/0.001 == 0.5/0.1 == 0.6/0.0001 == 0.7.The review's ten mutations were re-run in full on the repaired tree and all
ten are still RED, plus four this pass adds (forward returns
{};NemotronHBlockNamemislabelskMoe; and a straight regression of each offindings 1 and 2). Each was applied alone, rebuilt, run, then restored with its
SHA-256 re-verified. Two are worth noting: the
mtp*mutation drives theassertion COUNT up by 1036 while going red, and the
NemotronHBlockNamemutation prints
assertions: 28 | 28 passed | 0 failedbeside8 failedtestcases — the cases threw, so
grep 'assertions:'alone reads it as clean. Fulltable in spec §5d.
Local CPU-only host, disk 68G free (85% used) at every measurement:
-Werror, full buildtest_nemotron_h_scaffold, offlineStatus: SUCCESS!test_nemotron_h_scaffold,VT_NEMOTRON35_SNAPSHOTliveStatus: SUCCESS!-g0, asserts unmasked), offline + livectest -j4W3 baselines were 10/38245 offline and 10/39113 live.
For the operator
One gate is RED and this branch cannot clear it itself.
scripts/agent-preflight.sh --stagedis green on everything except:The rule is unconditional and PER-COMMIT:
check-doc-checkpoint.py:79classifies any
src/vllm/model_executor/models/path asfeature_surface,:313requiresdocs/FEATURES.mdin the same commit, andcommits_in_rangewalks
rev-list --reverse --no-merges origin/main..HEAD— so W3C's owndocs/FEATURES.mdedit in3295d0c1adoes not cover a later commit.ci.yml:343runs the identical invocation, so CI is red for the same reason.The checker has no exemption mechanism, deliberately.
This branch was dispatched with
docs/explicitly excluded from its authority,so it neither took the obvious fix nor weakened the checker. The minimal
truthful edit — a real feature-surface fact this commit changed — is one clause
on the
NemotronHForCausalLMrow (docs/FEATURES.md:139):folded into
3981de6a4(amend, or a squash-merge carrying both).Finding 5 is a pre-existing
docs/FEATURES.mddrift this branch deliberatelydid not touch and owes an issue:
:171says "27 of the 32 registeredtext-generation architectures" while
:173implies 38 − 3 Parakeet − 1LlamaModel= 34, andtests/vllm/models/test_model_registry.cpp:47says "34text archs". W3's
+1increment was correct; the base was already stale, and nochecker validates it.
The row stays
INVENTORIED: this makes NemotronH KNOWN, it runs nothing. Theforward (W4), MTP head (W5), e2e token gate (W6) and GGUF arm (W7) all still
refuse by name.
🤖 Generated with Claude Code