Validate non-empty cfg when enabling quantizers in quant_cfg#1192
Validate non-empty cfg when enabling quantizers in quant_cfg#1192shengliangxu wants to merge 11 commits intomainfrom
Conversation
…g_list
An entry with enable=True and an empty or invalid cfg (e.g., cfg={},
cfg=[], or cfg=42) would pass normalize_quant_cfg_list but later crash
in set_quantizer_by_cfg when QuantizerAttributeConfig(enable=True) was
constructed with no actual quantizer attributes.
Add early validation in normalize_quant_cfg_list to reject such entries
at config time rather than surfacing a confusing Pydantic ValidationError
deep in the conversion pipeline.
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/torch/quantization/test_config_validation.py (1)
166-205: Add regression cases for invalid list elements in enabledcfgNice coverage for empty/invalid top-level
cfg; please also add cases forcfg=[{}]andcfg=[42]withenable=Trueto lock in early rejection of malformed sequential configs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/torch/quantization/test_config_validation.py` around lines 166 - 205, Add two regression tests to ensure invalid list elements inside an enabled cfg are rejected: create tests similar to existing ones that call normalize_quant_cfg_list with [{"quantizer_name": "*weight_quantizer", "cfg": [{}], "enable": True}] and [{"quantizer_name": "*weight_quantizer", "cfg": [42], "enable": True}] and assert they raise ValueError matching "non-empty dict or list"; place them alongside the other test_error_* cases referencing the normalize_quant_cfg_list function so malformed sequential configs are explicitly covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelopt/torch/quantization/config.py`:
- Around line 1669-1680: The validation currently only checks the top-level cfg
container; update the check in the block using entry/cfg/enable so that when
enable is True and cfg is a list you also validate every element: require each
element to be a dict and non-empty (reject non-dict items like 42 and empty
dicts like {}), raising the same ValueError (include raw) if any element is
invalid; keep the existing dict-or-list check for top-level cfg and add the
per-item check (refer to variables entry, cfg, enable and the
QuantizerAttributeConfig-related error path) so malformed list payloads are
rejected early.
---
Nitpick comments:
In `@tests/unit/torch/quantization/test_config_validation.py`:
- Around line 166-205: Add two regression tests to ensure invalid list elements
inside an enabled cfg are rejected: create tests similar to existing ones that
call normalize_quant_cfg_list with [{"quantizer_name": "*weight_quantizer",
"cfg": [{}], "enable": True}] and [{"quantizer_name": "*weight_quantizer",
"cfg": [42], "enable": True}] and assert they raise ValueError matching
"non-empty dict or list"; place them alongside the other test_error_* cases
referencing the normalize_quant_cfg_list function so malformed sequential
configs are explicitly covered.
🪄 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: Pro
Run ID: 6ff10f3d-df75-4a3b-bb2b-efac0193fc02
📒 Files selected for processing (2)
modelopt/torch/quantization/config.pytests/unit/torch/quantization/test_config_validation.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
modelopt/torch/quantization/config.py (1)
1680-1691:⚠️ Potential issue | 🟠 MajorEnabled list
cfgis still under-validated at item level.Line 1686 validates only the top-level container. Cases like
cfg=[42]orcfg=[{}]still pass this block and can fail later, which weakens the “early rejection” goal.Suggested fix
cfg = entry.get("cfg") enable = entry.get("enable", True) if enable and cfg is not None: - if not isinstance(cfg, (dict, list)) or len(cfg) == 0: + if isinstance(cfg, dict): + is_invalid = len(cfg) == 0 + elif isinstance(cfg, list): + is_invalid = len(cfg) == 0 or any( + not isinstance(item, dict) or len(item) == 0 for item in cfg + ) + else: + is_invalid = True + if is_invalid: raise ValueError( f"Invalid quant_cfg entry: {raw!r} — 'cfg' must be a non-empty dict " - "or list when enabling a quantizer. Either provide quantizer " - "attributes in 'cfg' or remove 'cfg' and set 'enable' explicitly." + "or a non-empty list of non-empty dicts when enabling a quantizer. " + "Either provide quantizer attributes in 'cfg' or remove 'cfg' and " + "set 'enable' explicitly." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/torch/quantization/config.py` around lines 1680 - 1691, The current validation only checks that cfg is a non-empty dict or list but doesn’t validate list elements, allowing invalid entries like cfg=[42] or cfg=[{}]; update the validation in the block handling entry.get("cfg")/enable to iterate when cfg is a list and ensure each element is a non-empty dict with the expected structure (i.e., dict type and len(item) > 0), and raise a ValueError (including raw) if any element is not a non-empty dict; also keep the existing non-list branch to ensure a dict cfg is non-empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@modelopt/torch/quantization/config.py`:
- Around line 1680-1691: The current validation only checks that cfg is a
non-empty dict or list but doesn’t validate list elements, allowing invalid
entries like cfg=[42] or cfg=[{}]; update the validation in the block handling
entry.get("cfg")/enable to iterate when cfg is a list and ensure each element is
a non-empty dict with the expected structure (i.e., dict type and len(item) >
0), and raise a ValueError (including raw) if any element is not a non-empty
dict; also keep the existing non-list branch to ensure a dict cfg is non-empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db068ac8-55d1-4e78-b057-191388182afc
📒 Files selected for processing (1)
modelopt/torch/quantization/config.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1192 +/- ##
==========================================
- Coverage 75.42% 66.20% -9.22%
==========================================
Files 353 353
Lines 40603 40612 +9
==========================================
- Hits 30623 26887 -3736
- Misses 9980 13725 +3745
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
1. Correctness
[SUGGESTION] Error message could include the actual cfg type for easier debugging
When the validation rejects an invalid cfg, the error shows the raw entry but not the specific type/value that failed. For quick diagnosis, consider including it:
raise ValueError(
f"Invalid quant_cfg entry: {raw!r} — 'cfg' must be a non-empty dict "
f"or list when enabling a quantizer (got {type(cfg).__name__}: {cfg!r}). "
"Either provide quantizer attributes in 'cfg' or set 'enable' explicitly."
)2. Edge Case
[QUESTION] cfg=None explicitly provided with enable=True
The validation skips when cfg is None, which is correct for the default path (cfg not provided). But {"quantizer_name": "*", "cfg": None, "enable": True} also passes — is this a valid config downstream, or should it be caught here too?
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
resolved
This is intentionally allowed — it's the canonical way to express "enable this quantizer with default/inherited settings" (same as {"quantizer_name": "*", "enable": True} with no cfg). The code path entry.setdefault("cfg", None) normalizes missing cfg to None, so explicitly passing cfg=None is equivalent. |
2 participants
What does this PR do?
An entry with enable=True and an empty or invalid cfg (e.g., cfg={}, cfg=[], or cfg=42) would pass normalize_quant_cfg_list but later crash in set_quantizer_by_cfg when QuantizerAttributeConfig(enable=True) was constructed with no actual quantizer attributes.
Add early validation in normalize_quant_cfg_list to reject such entries at config time rather than surfacing a confusing Pydantic ValidationError deep in the conversion pipeline.
Testing
new unit tests added
Summary by CodeRabbit