[OMNIML-5105] Fix torch import error to remove circular dependency & move Nemotron configs #1606
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReorders ChangesTorch Package Import Ordering
Nemotron PTQ recipes and launcher examples
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modelopt/torch/__init__.py (1)
26-40: ⚡ Quick winMake the package exports explicit here.
This import-order fix looks fine, but
modelopt.torchstill relies on implicit package exports. Please add an explicit__all__for the intended public subpackages here, and if this package is meant to re-export submodule symbols, align the__init__.pywith the repo’sfrom .module import *pattern as well.♻️ Minimal direction
"""Model optimization and deployment subpackage for torch.""" +__all__ = [ + "opt", + "distill", + "nas", + "peft", + "prune", + "quantization", + "sparsity", + "speculative", + "utils", +] + import importlib import warnings as _warningsAs per coding guidelines,
**/*.py: "Define the public API with__all__at the top of each Python module and re-export submodules in__init__.pyfiles usingfrom .module import *to keep the public API explicit and make star-imports safe".🤖 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 `@modelopt/torch/__init__.py` around lines 26 - 40, Add an explicit public API to modelopt.torch by defining __all__ in this __init__.py listing the exported subpackages (e.g. "opt", "distill", "nas", "peft", "prune", "quantization", "sparsity", "speculative", "utils"), and if any of these subpackages are intended to re-export symbols at package-level follow the repo pattern by adding corresponding from .<module> import * lines for those modules (keep the import-order fix intact and ensure __all__ reflects exactly the names you re-export).
🤖 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.
Nitpick comments:
In `@modelopt/torch/__init__.py`:
- Around line 26-40: Add an explicit public API to modelopt.torch by defining
__all__ in this __init__.py listing the exported subpackages (e.g. "opt",
"distill", "nas", "peft", "prune", "quantization", "sparsity", "speculative",
"utils"), and if any of these subpackages are intended to re-export symbols at
package-level follow the repo pattern by adding corresponding from .<module>
import * lines for those modules (keep the import-order fix intact and ensure
__all__ reflects exactly the names you re-export).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 30f55e52-9764-4583-9c50-4a144a1491ca
📒 Files selected for processing (3)
modelopt/torch/__init__.pymodelopt_recipes/huggingface/Nemotron-3-Super-120B-A12B/super-nvfp4-max-calib.yamlmodelopt_recipes/huggingface/Nemotron-3-Super-120B-A12B/super-nvfp4.yaml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1606 +/- ##
===========================================
+ Coverage 62.88% 76.73% +13.84%
===========================================
Files 511 511
Lines 56634 56634
===========================================
+ Hits 35615 43456 +7841
+ Misses 21019 13178 -7841
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:
|
| @@ -13,8 +13,8 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| # Mirrors the published nvidia/NVIDIA-Nemotron-3-Super-120B-A12B-NVFP4 hf_quant_config.json: | |||
| # but with ONE major difference: use max calibration instead of MSE | |||
| # Approximately mirrors the published nvidia/NVIDIA-Nemotron-3-Super-120B-A12B-NVFP4 hf_quant_config.json: | |||
There was a problem hiding this comment.
why do we not just publish the same MSE based recipe?
There was a problem hiding this comment.
that's AutoQuant derived, very specific on certain layers and not generalizable
| @@ -13,8 +13,8 @@ | |||
| # See the License for the specific language governing permissions and | |||
There was a problem hiding this comment.
let's move into huggingface/nemotron_h/ptq/
There was a problem hiding this comment.
this is model specific, I would like to keep the model specific folder
There was a problem hiding this comment.
sorry, I actually mean huggingface/nemotron_h/Nemotron-3-Super-120B-A12B/ptq/
Nemotron-3-Super-120B-A12B is of nemotron_h model type
There was a problem hiding this comment.
that will make the recipe name super long .. I don't understand why we need a huggingface/ folder when we could just store models in top level modelopt_recipes so that the recipe would look like HuggingFace structure e.g. meta-llama/llama-3.1-8b
we also only have PTQ recipes right now so adding the ptq/ is redundant
There was a problem hiding this comment.
I agree with Jenny. It should just be exactly the same as the hugging face hub id. For example,
modelopt_recipes/nvidia/NVIDIA-Nemotron-3-Super-120B-A12B-BF16/nvfp4_max_calib_hf_ptq.yaml
jenchen13
force-pushed
the
jennifchen/fix_import
branch
from
5353089 to
2b147c5
Compare
jenchen13
changed the title
and Move Nemotron recipes to Hugging Face paths Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
jenchen13
force-pushed
the
jennifchen/fix_import
branch
from
2b147c5 to
0f25f65
Compare
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
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
🤖 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
`@tools/launcher/examples/nvidia/NVIDIA-Nemotron-3-Ultra-550B-A55B-BF16/megatron_lm_ptq.yaml`:
- Around line 31-32: MLM_MODEL_CFG currently points to
nvidia/NVIDIA-Nemotron-3-Ultra-550B-A55B-BF16 while QUANT_CFG references
huggingface/models/nvidia/Nemotron-3-Super-120B-A12B/ptq/nvfp4-max-calib which
is a recipe mismatch; either update QUANT_CFG to an Ultra-specific recipe when
available or add an inline comment next to QUANT_CFG clarifying that the
Ultra-550B reuses the Super-120B nvfp4-max-calib recipe intentionally (reference
MLM_MODEL_CFG and QUANT_CFG and the nvfp4-max-calib recipe name in the comment).
🪄 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: 198d214d-ba62-44cf-88f1-6c55ecb78776
📒 Files selected for processing (7)
CHANGELOG.rstmodelopt/torch/__init__.pymodelopt_recipes/huggingface/models/nvidia/Nemotron-3-Nano-4B/ptq/nvfp4_w4a16.yamlmodelopt_recipes/huggingface/models/nvidia/Nemotron-3-Super-120B-A12B/ptq/nvfp4-max-calib.yamlmodelopt_recipes/huggingface/models/nvidia/Nemotron-3-Super-120B-A12B/ptq/nvfp4-mse.yamltools/launcher/examples/nvidia/NVIDIA-Nemotron-3-Super-120B-A12B-BF16/megatron_lm_ptq.yamltools/launcher/examples/nvidia/NVIDIA-Nemotron-3-Ultra-550B-A55B-BF16/megatron_lm_ptq.yaml
✅ Files skipped from review due to trivial changes (3)
- modelopt_recipes/huggingface/models/nvidia/Nemotron-3-Super-120B-A12B/ptq/nvfp4-max-calib.yaml
- CHANGELOG.rst
- modelopt_recipes/huggingface/models/nvidia/Nemotron-3-Super-120B-A12B/ptq/nvfp4-mse.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- modelopt/torch/init.py
Signed-off-by: Jenny Chen <jennifchen@nvidia.com>
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
…tests (#1970) ### What does this PR do? **Type of change:** documentation (+ new tests) `modelopt_recipes/ptq.md` is meant to track every shipped PTQ recipe, but recent recipe PRs did not update it. This PR brings the doc back in sync and adds unit tests so it can't drift again. **Doc sync — general recipes:** - Add `nvfp4_experts_only_input_scale1-kv_fp8_cast` (#1947) to the shipped-recipes table (now 20 recipes) and document the `input_scale1` variant (constant amax 2688 → exported NVFP4 `input_scale == 1.0`, expert activations uncalibrated). - Add a pointer that the PTQ recipes' `quantize` sections also drive QAT/QAD, with the LSQ / Dual-LSQ QAD recipes under `general/qad/` (#1884). **Doc sync — model-specific recipes:** - Add `gemma4` (algorithm override, #1690), `diffusion_gemma` (extra `*self_conditioning*` exclusion, #1707), `vit` (FP8 with attention BMM quantizers for Torch-TRT, #1569), and the qwen3_5 / qwen3_5_moe `w4a16_nvfp4_mse-fp8_attn-kv_fp8_cast` twins (#1620). - Rewrite the checkpoint-mirror section for the `huggingface/models/nvidia/` tier: Super recipe rename (`super-nvfp4.yaml` → `nvfp4-mse.yaml` / `nvfp4-max-calib.yaml`), Nano-4B GGUF Q4_K_M mirror (#1327/#1606), Ultra-550B `nvfp4-4o6` (#1684); fix stale `models/<checkpoint>` paths. **Enforcement — new `tests/unit/recipe/test_recipe_docs.py`:** 1. Every `general/ptq/*.yaml` stem must appear (backticked) in `ptq.md`. 2. Every recipe row in the shipped-recipes table must exist on disk (catches renames/removals). 3. The "All N `general/ptq/` recipes" count must match the file count. 4. Every model folder under `huggingface/` containing `ptq/*.yaml` must be mentioned in the doc. ### Usage ```bash pytest tests/unit/recipe/test_recipe_docs.py -q ``` ### Testing - `pytest tests/unit/recipe/ -q` → 218 passed (includes the 4 new tests). - Verified enforcement bites: against the pre-update `ptq.md`, 3 of the 4 new tests fail with actionable messages. - `pre-commit run --files modelopt_recipes/ptq.md tests/unit/recipe/test_recipe_docs.py` → all hooks pass. ### Before your PR is "*Ready for review*" Make sure you read and follow [Contributor guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md) and your commits are signed (`git commit -s -S`). Make sure you read and follow the [Security Best Practices](https://github.com/NVIDIA/Model-Optimizer/blob/main/SECURITY.md#security-coding-practices-for-contributors) (e.g. avoiding hardcoded `trust_remote_code=True`, `torch.load(..., weights_only=False)`, `pickle`, etc.). - Is this change backward compatible?: ✅ (docs + tests only; no runtime code changed) - If you copied code from any other sources or added a new PIP dependency, did you follow guidance in `CONTRIBUTING.md`: N/A - Did you write any new necessary tests?: ✅ — `tests/unit/recipe/test_recipe_docs.py` - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: N/A (documentation/test change) - Did you get Claude approval on this PR?: ❌ (pending) ### Additional Information Follow-up to the recipe guide added in #1662. The doc-consistency tests could also be wired into `tools/precommit/check_modelopt_recipes.py` for commit-time feedback if reviewers prefer both layers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added the `nvfp4_experts_only_input_scale1-kv_fp8_cast` recipe to the general PTQ catalog. * Documented the new `input_scale1` calibration behavior and related quantization/caching details. * Expanded and reworked PTQ documentation, including updated navigation, “Beyond PTQ” guidance, new/updated model-specific recipes, and expanded checkpoint mirror entries. * Updated the general PTQ shipped-recipe count to 20. * **Tests** * Added automated consistency checks between on-disk shipped recipe YAML files and the PTQ documentation (coverage, existence, and counts). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chenjie Luo <chenjiel@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
5 participants
What does this PR do?
Type of change: Bug fix
when running Megatron-LM modelopt example
generate.pya circular import causes an error. the cause was because it import modelopt.torch.quantization which imports modelopt/torch -->Usage
# Add a code snippet demonstrating how to use thisTesting
Before 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
Summary by CodeRabbit
Chores
nvfp4-mseandnvfp4-max-calibquantization variants, including matching export and vLLM checkpoint/output paths.Documentation