feat(models): complete tie_word_embeddings guards for #2512 families#2896
Conversation
…ing families Closes the tie-audit follow-up. Reject tie=True on the untied-default families (mistral3_vlm, step3p5, step3p7, nemotron_omni), verified against their published configs; reject tie=False on the tied-default gemma4_moe and flip the NVIDIA-NeMo#2601 untied tests to expect the raise. Adds the symmetric reject_unsupported_untied_word_embeddings helper. Refs NVIDIA-NeMo#2512 Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR completes the tie_word_embeddings audit by adding a symmetric guard for tied-default architectures and wiring fast-fail validation into additional model families so unsupported tying/untying configurations are rejected at construction time.
Changes:
- Added
reject_unsupported_untied_word_embeddings()to explicitly rejecttie_word_embeddings=Falsefor tied-default models (Gemma4 MoE). - Applied
reject_unsupported_tied_word_embeddings()to additional untied-default model families (mistral3_vlm, step3p5, step3p7, nemotron_omni) to rejecttie_word_embeddings=True. - Updated and expanded unit tests to cover the new guard and to flip Gemma4 MoE “untied” tests to assert rejection.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/utils/test_checkpoint_utils.py | Adds unit tests for the new untied-guard helper and minor formatting cleanup. |
| tests/unit_tests/models/gemma4_moe/test_gemma4_moe_tied_weights.py | Updates Gemma4 MoE tests to assert tie_word_embeddings=False is rejected and adds pytest dependency. |
| nemo_automodel/components/models/step3p7/model.py | Adds a constructor-time guard rejecting tie_word_embeddings=True for an untied-default family. |
| nemo_automodel/components/models/step3p5/model.py | Adds a constructor-time guard rejecting tie_word_embeddings=True for an untied-default family. |
| nemo_automodel/components/models/nemotron_omni/model.py | Adds a constructor-time guard rejecting tie_word_embeddings=True for an untied-default family. |
| nemo_automodel/components/models/mistral3_vlm/model.py | Adds a constructor-time guard rejecting tie_word_embeddings=True for the supported untied Mistral3 VLM checkpoint. |
| nemo_automodel/components/models/gemma4_moe/model.py | Adds a constructor-time guard rejecting tie_word_embeddings=False for tied-default Gemma4 MoE. |
| nemo_automodel/components/checkpoint/utils.py | Introduces the new symmetric reject_unsupported_untied_word_embeddings() helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Raises: | ||
| NotImplementedError: if the controlling ``tie_word_embeddings`` flag is unset. | ||
| """ |
There was a problem hiding this comment.
Fixed — updated the docstring to say it raises when the controlling flag evaluates to False.
tie_word_embeddings audit (#2512) — both flip directions and the remaining families.…review) Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
|
/ok to test b37ec0f |
|
Thanks @Achyuthan-S. Will approve and merge after ci passed. I verified the guards against the actual checkpoint configs (including the nemotron_omni nested One follow-up I've decided on, tracked in #2935: replace the two-helper API with a single constructor-facing guard — each model class declares a three-state You're welcome to pick it up if you're interested. Otherwise I'll take it. Either way, no action needed on this PR. |
|
Thanks @yuhezhang-ai — glad the checkpoint verification lined up, and thanks for the merge. I'll pick up #2935 as the follow-up (plan posted over there , on the issue page). |
What
tie=Trueon the untied-default families —mistral3_vlm,step3p5,step3p7,nemotron_omni— via the existingreject_unsupported_tied_word_embeddingsguard.tie=Falseon the tied-defaultgemma4_moe, via a new symmetricreject_unsupported_untied_word_embeddingshelper. Gemma4 is now tied-only; the fix(gemma4_moe): re-tie lm_head to active embed_tokens on MoE path #2601 untied tests are flipped to assert the rejection (untying Gemma4 was never a tested load/conversion path — fresh construction leavinglm_headseparate doesn't prove a tied checkpoint can materialize a correct separatelm_head.weight).Verified per family (config inspection only, no weights)
mistralai/Mistral-Medium-3.5-128Btie_word_embeddings=False(top & nested) → untiedtie=Truestepfun-ai/Step-3.5-Flashvalidate_layer_type; flagFalseper inspection)tie=Truestepfun-ai/Step-3.7-Flashlm_head→ untiedtie=Truenvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-BF16llm_configtie=False→ untiedtie=Truetie=FalseTests
reject_unsupported_untied_word_embeddingshelper.gemma4_moeuntied tests updated to assert the rejection.__init__on the top-level config, so they fail fast before any build.Closes #2512.
cc @yuhezhang-ai