Repository navigation
Conversation
Signed-off-by: Meng Xin <164952785+mxinO@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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 51 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Security Anti-Patterns PASS. The authoritative diff changes six modelopt Python files and no examples, pyproject.toml, requirements, or lock files. Added lines contain none of the prohibited patterns: unsafe torch.load or n… Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: unified export support for quantized-weight state capture. It is concise and accurately reflects the pull request objectives.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2777 +/- ##
==========================================
+ Coverage 69.48% 78.36% +8.88%
==========================================
Files 648 649 +1
Lines 71791 72005 +214
==========================================
+ Hits 49881 56426 +6545
+ Misses 21910 15579 -6331
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:
|
|
Follow-ups for this two-PR series, carried over from the scope comment on #2251:
These exclusions apply to the new functional API, not ModelOpt's existing offline exporter. Follow-up integration is tracked in Megatron-Bridge #4804; Bridge #5812 and NeMo-RL #3945 consume the complete two-part API. |
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
Nudge: 657 added core-logic lines remain over budget, and the snapshot abstraction needs design rationale and broader capture tests.
Needs action:
- ✂️ Split
export/(649 lines, itself over budget) by file:[1/3]sharedquant_utils.py/quant_format.py/convert_hf_config.pyfixes;[2/3]quantization/plugins/custom.pyrestoration;[3/3]quantized_weight_export.pycapture APIs andexport/__init__.py. - Keep the series acyclic and linearly ordered; document merge order, link siblings, and ensure each slice builds with its own tests and passing CI without later slices.
- Explain in the PR body why extending
quant_utils.pyscale helpers or adaptingget_quantizer_state_dict()is insufficient, and why dedicated dataclass snapshots are preferred over existing Pydantic state representations. - Add capture tests in
tests/unit/torch/export/test_quantized_weight_state.pyfor per-channel/block FP8, dynamic NVFP4, and MXFP4/MXFP8; verify scale values, axis/block metadata, detached-copy independence, and missing-calibration failures.
No action needed:
- The existing mixed-format assertion change is justified by the new exclusion behavior; existing coverage is retained.
- New-file headers match
LICENSE_HEADER. - Tests were inspected, not executed.
Signed-off-by: Meng Xin <164952785+mxinO@users.noreply.github.com>
|
Addressed the capture coverage and rationale feedback from this review. The new cases cover scale values, axis/block metadata, detached-copy independence and required-calibration failures; the independent capture snapshot passes 169 CPU/GPU tests with no skips. The PR body explains the extra layout metadata and use of dataclasses for internal state rather than persisted configuration. Keeping the agreed two-part capture/consumer split for this series instead of another three-way split. Both parts are independently buildable and tested. |
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
Nudge: capture coverage and design rationale are resolved, but 659 added core-logic lines still exceed the review budget.
Needs action:
- ✂️ Split
modelopt/torch/export(651 lines, itself over budget) by file:[1/3]sharedquant_utils.py/quant_format.py/convert_hf_config.pyfixes;[2/3]modelopt/torch/quantization/plugins/custom.pyrestoration;[3/3]quantized_weight_export.pycapture APIs andexport/__init__.py. - 💬 Author retains the two-part capture/consumer series — further subdivide capture; keep dependencies acyclic and linearly ordered, document merge order, link siblings, and give each slice its own tests and passing CI without later slices.
No action needed:
- ✔️ Resolved since the last review: requested format/scale/axis/block, detached-copy and calibration-failure coverage; snapshot design rationale.
- The problem is independent per-weight export capture with layout metadata. Reusing scale helpers, adapting
get_quantizer_state_dict(), and existing Pydantic representations were considered; the body reasonably explains detached dataclass state versus persisted configuration. - The existing mixed-format assertion edit remains justified by retained exclusions; existing coverage is preserved. New headers match
LICENSE_HEADER. - Tests were inspected, not executed.
There was a problem hiding this comment.
My understanding is that this file is capturing the quantization state. Could we rename the file to better reflect that? Maybe something like quantization_export_state.py or quantization_weight_state.py
sugunav14
left a comment
There was a problem hiding this comment.
Left a minor readability comment. Otherwise LGTM!
What does this PR do?
Type of change: New feature and bug fix.
Part 1 of 2: expose exact per-weight quantizer-state capture and metadata-only export specs. Capture is non-mutating and handles numbered grouped weights and supported transposed layouts. Callers can inspect deployment metadata without retaining tensor state.
This also fixes shared format detection, grouped quantizer restoration and HF export metadata, including unquantized-layer exclusions in mixed configurations. Existing offline packing remains on its current path.
Existing scale helpers are reused. The opaque snapshot also records logical weight shape, quantization axes, block sizes and packing layout, which a quantizer-buffer dictionary alone does not provide. It is detached in-memory state, not a new serialized configuration model.
Usage
For a converted and calibrated module:
Capture returns
Nonefor an unquantized weight. The second PR adds consumers of these snapshots. Recapture state for each export when quantizer values may change.Testing
Frozen-source container run: 169 focused CPU/GPU tests passed, no skips. Covers capture/spec, known scale values and axis/block metadata, detached-clone independence, required-calibration failures, ordinary offline export, GPU packing and TE grouped restoration. All changed-file pre-commit checks pass. This validates the extracted API and existing export paths, not training convergence.
Before your PR is "Ready for review"
Additional Information
Split of #2251; the original PR is unchanged. Merge this first, then #2778 ([2/2]), which targets this branch. Megatron-Bridge and NeMo-RL integration require both.
Summary by CodeRabbit