fix(model): stop building a DeepSeek query norm HF does not define - #5262
Conversation
build_conversion_tasks is declared -> List[None | WeightConversionTask] and does leave None slots: the backfill loop skips any global parameter whose mapping lookup returns None, which is expected whenever the Megatron model owns a parameter the HF architecture has no counterpart for. All three consumers dereferenced the task anyway, so an unmapped parameter surfaced as AttributeError with no parameter name in the traceback, despite the builder having already warned about each one by name. Signed-off-by: Minho Ryu <ryumin93@gmail.com>
ac6e35a to
b2bf7ed
Compare
MCore derives Q and KV normalization from a single qk_layernorm flag. DeepSeek needs it enabled for kv_a_layernorm, which every checkpoint ships. When q_lora_rank is None that same flag also fuses a query normalization into linear_q_proj, but DeepseekV3Attention builds a bare q_proj in that case, so the HF checkpoint has nothing to load into it. The result was a trainable per-layer parameter with no HF counterpart: it stayed at its initialization on load, made the weight load crash on its own None task slot, optimized as an extra degree of freedom during training, and was dropped on export. Build MLA without the query norm when there is no query LoRA, keeping the KV norm. Signed-off-by: Minho Ryu <ryumin93@gmail.com>
The resolver-level tests did not check that the bridges behind the two affected models actually route through the corrected spec builder. Assert that both DeepSeekV2Bridge and DeepSeekV3Bridge install it while keeping qk_layernorm enabled for the KV norm. Signed-off-by: Minho Ryu <ryumin93@gmail.com>
yaoyu-33
left a comment
There was a problem hiding this comment.
The central TE/TP=1/no-MTP query-norm substitution is independently validated, including a bit-exact affected-branch HF→MCore→HF round trip. I am requesting changes for the remaining P1 issues: preserve and test virtual-stage routing; retain strict failure for genuinely unmapped global parameters instead of silently skipping them; support or intentionally gate the optional local backend; and commit a real affected state-dict/round-trip regression test. The standalone-MTP fallback is also still open as P2. Please update the linked inline threads and request re-review on the exact new head; CI approval should wait for that revision.
The override only reached the Transformer Engine backend, a pipeline without virtual stages, and an MTP layer that shares a stage with decoder layers. Outside those it was either bypassed or fatal. - Match get_gpt_decoder_block_spec's signature exactly. GPTModelProvider.provide() decides whether to pass vp_stage by inspecting the callable, so omitting it left interleaved pipeline parallelism calling MCore's layer-offset helper without a virtual stage, which asserts. - Neutralise a standalone q_layernorm before delegating to the parent resolver. The local (non-TE) MLA spec supplies one whenever qk_layernorm is set, and the parent rejects that combination outright when there is no query LoRA, so the affected checkpoints could not be built at all without Transformer Engine. - Re-apply the attention swap on a standalone MTP stage. That stage owns no decoder layers, so mtp_block_spec re-derives its layer spec straight from MCore and never calls the DeepSeek builder; the MTP layer regained the query norm the decoder layers had just dropped. Added GPTModelProvider.mtp_layer_spec_transform as the hook, since any model with a non-stock layer spec loses it on the same branch. - Raise by name when build_conversion_tasks finds no mapping for a real parameter, instead of warning and leaving a None slot. The consumer-side skips stay: a None slot has a second, legitimate producer in the same builder, an HF checkpoint that does not carry a weight the mapping names. Tests cover the signature parity, vp_stage forwarding, the local backend, the standalone MTP transform on a bare layer spec, and an unmapped parameter failing by name. Signed-off-by: Minho Ryu <ryumin93@gmail.com> Signed-off-by: ryan.u <ryan.u@kakaocorp.com>
d121a63 to
b67b8d9
Compare
|
All five threads are answered on the new head, b67b8d9, and the description is updated to match. Four are implemented as asked. The local backend is the exception: MCore has no non-fused path for |
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test ee9f7d9 |
|
/ok to test b6a6612 |
The new mapping validation turned three functional tests red. In each one the toy is built from a HF class whose current save layout differs from the released checkpoint the bridge maps, so the mapped weights were absent and the runs were comparing parameters that had never been loaded. exaone: ExaoneMoeForCausalLM declares _keys_to_ignore_on_load_unexpected = [r"mtp.*"], so released checkpoints carry MTP weights but the class does not build them. Setting num_nextn_predict_layers made the provider build MTP layers and the bridge register 15 MTP mappings against weights the toy could never save. Drop MTP from the toy config. nemotronh: saving writes backbone.embedding.weight while the released Nemotron-3-Nano index has backbone.embeddings.weight. Rename it after save. qwen_audio: saving nests the language model one level deeper than the released Qwen2-Audio checkpoint, language_model.model.model.* against language_model.model.*. Collapse it after save. The audio tower, projector and head already match. Verified: exaone roundtrip at TP=2 matched 89/89 weights; the nemotronh and qwen_audio toys now save the keys their bridges map. Signed-off-by: Minho Ryu <ryumin93@gmail.com>
125e946 to
abd5d21
Compare
|
The three functional failures turned out to be the same problem in different places, and ee9f7d9 was right to flag all of them. Fixed in abd5d21. Each of those toys is built from a HF class whose current save layout no longer matches the released checkpoint the bridge maps, so the mapped weights were not in the file at all and the runs were comparing parameters that had never been loaded. That is what the new check exists to catch, so I fixed the toys rather than the check. exaone was the clearest of them. The other two are naming. nemotronh saves I ran what I could locally on two A100s. qwen_audio passes outright, exaone's roundtrip matches 89/89 weights, and nemotronh clears the validation and then stops on the APEX fused wgrad extension, which my container lacks and CI has. gemma_vl only failed in that one run, with EADDRINUSE on port 52177 and no mapping error anywhere in the log, so I left it alone. What I did not touch is that this mismatch is not only a test artifact. Anyone who re-saves a Nemotron-H or Qwen2-Audio checkpoint with the pinned transformers ends up with a file these bridges cannot map, and before ee9f7d9 it would have loaded quietly with those weights left at their initialization. Whether the bridges should accept both layouts felt bigger than this PR and not really mine to decide for those families, but I can pick it up if you would rather have it here. |
|
/ok to test bdbe7eb |
|
Everything is green now, including the four jobs that were failing before. The exaone, nemotronh and qwen_audio fixes went in with the last branch update, and gemma_vl passed on the rerun, so that port collision was a one off. @yaoyu-33 @kamran-nvidia @adityavavreNVDA this is only waiting on review at this point. The earlier review shows as dismissed, so it needs another pass from whoever has a moment. Thanks for keeping the branch current and running CI while I was chasing down the fixtures. |
See NVIDIA-NeMo/Megatron-Bridge#5262 Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
See NVIDIA-NeMo/Megatron-Bridge#5262 Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
What does this PR do ?
Fixes #5261
Stop building a per-layer query normalization that the HF DeepSeek architecture does not define, and stop the resulting unmapped parameter from crashing the weight load.
Bug and root cause
MCore derives Q and KV normalization from a single
qk_layernormflag. DeepSeek needs it enabled forkv_a_layernorm, which every checkpoint ships, so both bridges setprovider.qk_layernorm = Trueunconditionally.When
q_lora_rank is Nonethere is no query LoRA, and that same flag makes MCore fuse a query normalization intolinear_q_projinstead:Both lines come from the same resolver,
mla_qk_norm_config.py::_resolve_mla_qk_layernorm, and this repo maps the second toself_attn.q_a_layernorm.weight(models/deepseek/common.py:57). HF defines no query-side norm when there is no query LoRA —modeling_deepseek_v3.py:384-389builds a bareq_proj— so nothing can be loaded into the first.Dumping the converted model showed the parameter sitting at its init while every neighbouring norm took its checkpoint value (source norms randomized to
uniform(0.5, 1.5)to tell the two apart):requires_grad=True, so training optimized a degree of freedom the source model does not have, and export dropped it silently. On top of that the missing mapping produced aNoneconversion-task slot, which the load path dereferenced.Changelog
MLASelfAttentionWithoutQueryNorm, which keeps the KV norm and drops the query norm whenq_lora_rank is None, plusget_deepseek_decoder_block_specto install it.get_gpt_decoder_block_spec.get_gpt_decoder_block_specparameter for parameter, includingvp_stageandpp_rank.GPTModelProvider.provide()inspects the callable's signature and only forwardsvp_stagewhen it is declared, so omitting it routed interleaved pipeline parallelism into the layer-offset helper without a virtual stage.replace_mla_self_attentionand add amtp_layer_spec_transformprovider field, so a standalone MTP stage applies it on the fallback path wheremtp_block_specre-derives the layer spec from MCore.q_lora_rank=Nonestate-dict round trip in the DeepSeek functional group. The existing toy fixture usesq_lora_rank=512, which is the unaffected control, so nothing in the suite reached the changed branch.Scope of the
Nonetask slotbuild_conversion_taskshas two producers of aNoneslot, and they are not the samecondition. A Megatron parameter with no registry entry is a wrong model: the parameter
stays at its initial value on import and is dropped on export. That now raises by name and
points at
_HCAlphaSecondaryMappingas the way to declare a deliberate no-op.The second producer is an HF checkpoint that does not carry a weight the mapping names. It
is benign and pre-existing, so the consumer-side skips stay. Removing them would turn a
missing optional weight into a crash in shared conversion code for every model family.
Backend support
The local backend cannot express this architecture, and could not before this PR.
_resolve_mla_qk_layernormbuildslinear_q_projfrom the backend's fused norm+linearimplementation whenever
q_lora_rank is None, andLocalSpecProviderhas none, so_require_linearraises. Keeping the query norm instead trips_raise_unused_q_norm. Bothbranches are closed, and DeepSeek needs
qk_layernormon forkv_a_layernorm.Reproducing MCore's resolver inside the bridge to work around this would fork logic that
belongs upstream, so the bridge fails early with an explicit Transformer Engine
requirement.
qk_layernorm=Falseis not an alternative: it also removeslinear_kv_up_proj.layer_norm_weight, whichcommon.py:40maps to a real HF weight. Two spec-level workarounds are closed as well —_reject_disabled_normrejects a fused KV projection while QK norm is disabled, and_mla_fused_linear_or_defaultoverrides an explicitly non-fusedlinear_q_projback to the fused class. Overriding_resolve_qk_norm_configon the attention module is the one hook that composes, and it needs no Megatron-LM change (upstreammainis byte-identical to the pinned submodule here).Validation
Model construction, TP=1, tiny
DeepseekV3ForCausalLMcheckpoints:linear_q_projnormlinear_kv_up_projnormq_projnormq_lora_rank=Noneq_lora_rank=64(control)q_lora_rank=None, MTP=1No
No mapping found for megatron_paramwarnings remain. The MTP row above is the shared-stage layout, wheremtp_block_specreuses the corrected decoder spec. A standalone MTP stage owns no decoder layers and re-derives its spec straight from MCore, which is why the substitution is also registered asmtp_layer_spec_transformrather than relying on the shared spec.Logit parity,
compare_hf_and_megatron/compare.py, TP=1:q_lora_rank=64(control)q_lora_rank=None, before this PRq_lora_rank=None, after this PRRemoving the parameter does not move inference. The defect was never an inference-time divergence — it was a phantom trainable parameter — and an earlier revision of #5261 that claimed otherwise has been corrected.
Conversion-loop regression, against the parent commit:
AttributeError: 'NoneType' object has no attribute 'megatron_module'and... 'param_weight', 2 failed.Unit tests:
Run against the Megatron-Core commit pinned by this branch, not the submodule checkout,
so the QK-norm resolver under discussion is the one exercised.
The whole of
tests/unit_tests/models/was run on this branch and on the parent commit inthe same environment, to check that raising on an unmapped parameter does not disturb other
model families:
bd25f3ed3Same failures and errors, nine more passes, which are the tests added here. The pre-existing
failures are environment-related and unrelated to this change.
State-dict round trip, one GPU:
HF has
q_proj.weightandkv_a_layernorm.weightand no query LoRA. Megatron haslinear_q_proj.weightandlinear_kv_up_proj.layer_norm_weightand nolinear_q_proj.layer_norm_weight. Every original weight survives HF to Megatron to HFbit-exactly, and no
q_a_projorq_a_layernormkey is invented. The structuralexpectations come from the HF architecture, not from the conversion registry.
Coverage note: the unit tests reach the resolver and both affected bridges, but no test loads a real affected checkpoint.
kakaocorp/kanana-2-30b-a3b-thinkingis too large for the 2-GPU functional budget, anddeepseek-ai/DeepSeek-V2-Litecannot be loaded at all under the pinnedtransformers— itstrust_remote_codemodeling file callsDynamicCache.from_legacy_cache, which no longer exists. The model-construction and parity evidence above therefore comes from tinyDeepseekV3ForCausalLMcheckpoints built to the same shape (q_projpresent,q_a_proj/q_a_layernormabsent) rather than from CI. The round trip is now committed asTestDeepSeekWithoutQueryLoRAon a generated toy checkpoint, so the changed branch has end-to-end coverage even though no real affected checkpoint fits the budget.Static checks:
ruff check src testspassed,ruff format --checkclean on changed files,git diff --checkpassed.Environment caveat: these runs used a CUDA 12.9 / A100 container built from
uv.lock, not the CImegatron-bridgeimage —nvidia-resiliency-extpublishes glibc 2.39 wheels only (this host is glibc 2.35), the CUDA source-build extras do not compile here, and Transformer Engine had to be built from the pinned commit withNVTE_WITH_NCCL_EP=0forsm80.gradient_accumulation_fusionwas disabled for the parity runs because APEX is absent; it selects the weight-gradient kernel and does not affect the forward pass. The toy checkpoints are 2-layer and randomly initialized, so they do not speak to drift across a deep trained stack. CI on the real image remains the authoritative run.Scope
q_lora_rank is None; theq_lora_rankpath resolves identically to before._HCAlphaSecondaryMappingindeepseek_v4_bridge.pyis the existing way to declare a deliberate no-op. The separateNo mapping found for global_namewarning thattest_fp8_param_export.pyasserts is a different site and is untouched, as are the consumer-side skips for a weight the HF checkpoint does not carry.GitHub Actions CI
See the CI section in the Contributing doc for how to trigger the CI. A Nvidia developer will need to approve and trigger the CI for external contributors.
Before your PR is "Ready for review"
Pre checks:
Additional Information