Fix saving pruned Nemotron-3-Nano hybrid_override_pattern with MTP or Pipe symbols - #2061
Conversation
Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
📝 WalkthroughWalkthroughThe pruning export reconstructs ChangesMTP-aware hybrid pattern export
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/examples/megatron_bridge/test_prune_minitron.py (1)
43-51: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the normalized pattern in the HF output.
The fixture enables one MTP prediction layer, but the HF branch only checks that
config.jsonexists and that the parameter count is below the target. Add assertions on the reloaded configuration to verify thathybrid_override_patterncontains only main-layer characters, hasnum_hidden_layersentries, and has MTP disabled. This prevents regressions that save the old MTP suffix or PP boundaries.As per coding guidelines, tests must exercise the behavior they claim to validate. As per path instructions, integration tests must protect regressions and document expected behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/examples/megatron_bridge/test_prune_minitron.py` around lines 43 - 51, Update the HF-output assertions in the test case using create_tiny_nemotron_h_dir to reload config.json and validate hybrid_override_pattern contains only main-layer characters, has exactly num_hidden_layers entries, and confirms MTP is disabled; retain the existing file-existence and parameter-count checks.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/megatron_bridge/prune_minitron.py`:
- Around line 664-667: Update the hybrid pattern normalization block around
hybrid_override_pattern to write the normalized value to
text_cfg.hybrid_override_pattern when provider is a VLM (_HYBRID_PROVIDER_TYPES)
and that field exists. Preserve the existing normalization via
parse_main_layer_chars and continue using the root hf_cfg field for non-VLM
configurations.
---
Nitpick comments:
In `@tests/examples/megatron_bridge/test_prune_minitron.py`:
- Around line 43-51: Update the HF-output assertions in the test case using
create_tiny_nemotron_h_dir to reload config.json and validate
hybrid_override_pattern contains only main-layer characters, has exactly
num_hidden_layers entries, and confirms MTP is disabled; retain the existing
file-existence and parameter-count checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a4609c66-e8af-4634-97e2-28d37dec6bbe
📒 Files selected for processing (2)
examples/megatron_bridge/prune_minitron.pytests/examples/megatron_bridge/test_prune_minitron.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2061 +/- ##
==========================================
+ Coverage 65.51% 66.97% +1.45%
==========================================
Files 521 521
Lines 59812 59812
==========================================
+ Hits 39185 40058 +873
+ Misses 20627 19754 -873
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Every other language-model field in the pruned-HF-config block writes to text_cfg (== hf_cfg for non-VLMs, hf_cfg.text_config for VLMs). The hybrid pattern used hf_cfg, so a hybrid VLM whose pattern lives under text_config failed the hasattr guard and never got its pattern updated after pruning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
|
#2041 #2031 #2061 #1983 #1628 (#2062) ## Cherry-picked PRs - #2022 - #2026 - #2032 - #1981 - #2010 - #2043 - #2042 - #2038 - #2050 - #2041 - #2031 - #2061 - #1983 - #1628 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added recipe-driven ONNX quantization and export for embedding and reranking models. * Added launcher support for inline commands, package requirements, and configurable Docker users. * Added QAD workflows, Nemotron data recipes, pruning, quantization, and evaluation examples. * Added evaluation accuracy and pruning score thresholds. * **Bug Fixes** * Improved ONNX control-flow conversion, compressed NVFP4 export, dataset loading, and multi-GPU DiffusionGemma handling. * **Documentation** * Updated CUDA 12/13 installation guidance and vLLM Blackwell deployment instructions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: vipandya <vipandya@nvidia.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com> Signed-off-by: Fridah-nv <201670829+Fridah-nv@users.noreply.github.com> Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com> Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com> Signed-off-by: Meng Xin <mxin@nvidia.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com> Signed-off-by: Chenjie Luo <108829653+cjluo-nv@users.noreply.github.com> Signed-off-by: Juhi Mittal <juhim@nvidia.com> Signed-off-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com> Signed-off-by: Gwenaelle Cunha Sergio <gcunhasergio@nvidia.com> Co-authored-by: vishalpandya1990 <vishalpandya1990@gmail.com> Co-authored-by: Frida Hou <201670829+Fridah-nv@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Wei-Ming Chen <17592131+meenchen@users.noreply.github.com> Co-authored-by: Ajinkya Rasane <131806219+ajrasane@users.noreply.github.com> Co-authored-by: OpenAI Codex <noreply@openai.com> Co-authored-by: mxinO <164952785+mxinO@users.noreply.github.com> Co-authored-by: Chenjie Luo <108829653+cjluo-nv@users.noreply.github.com> Co-authored-by: Juhi Mittal <39641197+juhi10071998@users.noreply.github.com> Co-authored-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
Saving pruned Nemotron-3-Nano (with MTP) to HF format raised an assertion which is fixed here
Tested on nemo:26.04 with transformers 4.57
Summary by CodeRabbit
Bug Fixes
Tests