fix: raise ValueError for unknown awq() algorithm instead of silent no-op#1908
fix: raise ValueError for unknown awq() algorithm instead of silent no-op#1908arham766 wants to merge 2 commits into
Conversation
…o-op awq() checked the algorithm string with two if-in tests and no else, so a typo like "awq-lite" returned an uncalibrated model with no signal. awq is public API; only the mtq.quantize config layer validates the Literal. Raise before any model mutation, matching the module's fail-fast style. Internal callers audited: svdquant hardcodes a valid value and mode.py values are pydantic-validated. Part of the findings in NVIDIA#1902. Signed-off-by: arham766 <arhamislam766@yahoo.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesAWQ Algorithm Validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modelopt/torch/quantization/model_calib.py (1)
1158-1173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting
algorithmand the newValueError.The docstring lists only
modelandforward_loopinArgs, omittingalgorithmand the fail-fastValueErrorthis change introduces. As per coding guidelines, "Document public APIs with docstrings, including examples when useful."📝 Suggested docstring addition
Args: model: Model to be calibrated. forward_loop: A callable which takes the model as argument and forwards calibration data through the model. + algorithm: One of "awq_lite", "awq_clip", or "awq_full". + + Raises: + ValueError: If `algorithm` is not one of the supported values.🤖 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/quantization/model_calib.py` around lines 1158 - 1173, Update the apply AWQ public API docstring in model_calib.py to document the algorithm parameter alongside model and forward_loop, and add a Raises section for the ValueError thrown when algorithm is not one of awq_lite, awq_clip, or awq_full. Use the existing apply AWQ function and its algorithm validation block as the reference points, and keep the wording aligned with the current AWQFullCalibConfig docs.Source: Coding guidelines
🤖 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/quantization/model_calib.py`:
- Around line 1158-1173: Update the apply AWQ public API docstring in
model_calib.py to document the algorithm parameter alongside model and
forward_loop, and add a Raises section for the ValueError thrown when algorithm
is not one of awq_lite, awq_clip, or awq_full. Use the existing apply AWQ
function and its algorithm validation block as the reference points, and keep
the wording aligned with the current AWQFullCalibConfig docs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 578676b8-cc2f-4eb9-8f69-d6f6d20bd9c1
📒 Files selected for processing (2)
modelopt/torch/quantization/model_calib.pytests/unit/torch/quantization/test_calib.py
Signed-off-by: arham766 <arhamislam766@yahoo.com>
What does this PR do?
Type of change: Bug fix
Bug 2 in #1902: awq() silently no-ops on an unknown algorithm string (public all API; only the mtq.quantize config layer Literal-validates). Now raises ValueError before any model mutation, listing the valid algorithms. Internal callers audited: svdquant hardcodes awq_lite; mode.py values are pydantic-validated. Regression test asserts the raise and that the valid path still works.
Usage
N/A
Testing
Regression tests included (added to existing unit test files; each was verified to fail with the fix reverted). Full tests/unit/torch quantization+export+utils+opt battery passes locally with all sibling fixes applied (962 passed). Note: the test-suite PRs #1903-#1907 contain behavior-documenting NOTE tests that pin the OLD behavior fixed here — whichever lands second will be rebased to flip those assertions (happy to do so).
Before your PR is "Ready for review"
Additional Information
Issue: #1902
Summary by CodeRabbit
Bug Fixes
ValueError.forward_loopisn’t provided in supported scenarios, emitting aUserWarninginstead of failing unexpectedly.Tests
forward_loop=Nonewarning behavior during AWQ calibration.