fix(export): [NVBug 6525534] preserve nested VLM namespaces - #2032
Conversation
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughWalkthroughThe change filters shadowed reverse weight-renaming rules using registered module namespaces, preventing nested VLM sibling capture and double-prefixing while preserving text-only nested mappings. Regression tests cover both namespace configurations, and the changelog records the fix. ChangesReverse rename scoping
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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2032 +/- ##
===========================================
+ Coverage 66.87% 77.91% +11.04%
===========================================
Files 519 519
Lines 59101 59123 +22
===========================================
+ Hits 39521 46066 +6545
+ Misses 19580 13057 -6523
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:
|
Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com>
meenchen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: 2
🤖 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 `@CHANGELOG.rst`:
- Around line 88-89: Convert the nested submodel reverse-mappings entry in
CHANGELOG.rst into a separate bullet by adding the changelog list marker at its
start, keeping the existing text unchanged.
In `@tests/unit/torch/export/test_quant_aware_conversion.py`:
- Around line 269-270: Document the optional Transformers dependency before both
local WeightRenaming imports in
tests/unit/torch/export/test_quant_aware_conversion.py at lines 269-270 and
297-298. Add a brief rationale stating that Transformers is optional and the
test is skipped when unavailable; no other import behavior needs to change.
🪄 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: 26e031e9-4198-4140-8c3c-f8eaa03b1b2d
📒 Files selected for processing (3)
CHANGELOG.rstmodelopt/torch/export/quant_aware_conversion.pytests/unit/torch/export/test_quant_aware_conversion.py
| Nested submodel reverse mappings are now scoped against registered model namespaces, preventing text-only mappings from capturing an already nested VLM's ``model.visual.*`` namespace or double-prefixing ``model.language_model.*`` (observed on Qwen3.5). | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make this a separate changelog bullet.
Without the - marker, this text continues the preceding bug-fix entry instead of creating the new item described by the PR.
Suggested fix
- Nested submodel reverse mappings are now scoped against registered model namespaces, preventing text-only mappings from capturing an already nested VLM's ``model.visual.*`` namespace or double-prefixing ``model.language_model.*`` (observed on Qwen3.5).
+- Nested submodel reverse mappings are now scoped against registered model namespaces, preventing text-only mappings from capturing an already nested VLM's ``model.visual.*`` namespace or double-prefixing ``model.language_model.*`` (observed on Qwen3.5).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Nested submodel reverse mappings are now scoped against registered model namespaces, preventing text-only mappings from capturing an already nested VLM's ``model.visual.*`` namespace or double-prefixing ``model.language_model.*`` (observed on Qwen3.5). | |
| - Nested submodel reverse mappings are now scoped against registered model namespaces, preventing text-only mappings from capturing an already nested VLM's ``model.visual.*`` namespace or double-prefixing ``model.language_model.*`` (observed on Qwen3.5). |
🤖 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 `@CHANGELOG.rst` around lines 88 - 89, Convert the nested submodel
reverse-mappings entry in CHANGELOG.rst into a separate bullet by adding the
changelog list marker at its start, keeping the existing text unchanged.
| pytest.importorskip("transformers.core_model_loading") | ||
| from transformers.core_model_loading import WeightRenaming |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the optional dependency for both local imports.
The conditional placement is valid, but each local import needs a brief comment explaining that Transformers is optional and the test is skipped when unavailable. As per coding guidelines and path instructions, optional in-function imports require this justification.
- tests/unit/torch/export/test_quant_aware_conversion.py#L269-L270: add the rationale before the
WeightRenamingimport. - tests/unit/torch/export/test_quant_aware_conversion.py#L297-L298: add the same rationale before the
WeightRenamingimport.
📍 Affects 1 file
tests/unit/torch/export/test_quant_aware_conversion.py#L269-L270(this comment)tests/unit/torch/export/test_quant_aware_conversion.py#L297-L298
🤖 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/unit/torch/export/test_quant_aware_conversion.py` around lines 269 -
270, Document the optional Transformers dependency before both local
WeightRenaming imports in tests/unit/torch/export/test_quant_aware_conversion.py
at lines 269-270 and 297-298. Add a brief rationale stating that Transformers is
optional and the test is skipped when unavailable; no other import behavior
needs to change.
Sources: Coding guidelines, Path instructions
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
The fix is focused and appears correct. It detects broad reverse prefix renames whose destination is already an explicitly registered child namespace, preventing Qwen3.5 VLM sibling capture and duplicate language_model nesting while retaining the rename for standalone text models. The two regression tests cover both sides of that behavior, and the changelog is updated. No licensing text is changed.
|
/ok to test 362c1e4 |
2 participants
What does this PR do?
Type of change: Bug fix
Prevents recursively collected text-submodel reverse mappings from rewriting an already nested multimodal model namespace during unified Hugging Face export.
Transformers reverses the Qwen3.5 text mapping into a broad
^model.->model.language_model.rename. ModelOpt previously applied that rule to every key in the full VLM state dict, movingmodel.visual.*under the language model and nestingmodel.language_model.*twice. This change drops the reverse rule only when its target child namespace is already registered. Standalone text models continue to use the conversion.Usage
# Existing export_hf_checkpoint usage is unchanged.Testing
python -m pytest -q tests/unit/torch/export(112 passed,1 skippedbecause optional Diffusers is not installed)Qwen3_5MoeForConditionalGenerationmeta-device model-tree probe passedpython -m pre_commit run --files modelopt/torch/export/quant_aware_conversion.py tests/unit/torch/export/test_quant_aware_conversion.pyBefore your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: N/AAdditional Information
No API or dependency changes.
Summary by CodeRabbit
Bug Fixes
Tests