test: direct unit suite for the quantization conversion layer (63 tests)#1903
test: direct unit suite for the quantization conversion layer (63 tests)#1903arham766 wants to merge 2 commits into
Conversation
Hermetic CPU-only unit tests; adversarially reviewed and verified to kill seeded mutations of the target module. Part of the coverage initiative 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)
📝 WalkthroughWalkthroughAdds a new unit test suite for quantization conversion, covering module replacement, registration, cfg-driven quantizer updates, attribute APIs, state restoration, conversion entrypoints, context-managed overrides, and a deprecated helper. ChangesConversion module test suite
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 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)
tests/unit/torch/quantization/test_conversion.py (1)
189-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWeak assertion for the "forward routes through quantizer" claim.
The comment on Line 199 states the forward call must route through the quantizer, but the test only checks that the call doesn't raise. By default
TensorQuantizerhasif_quant=True, if_calib=Falseand computes a dynamic amax per call, so a broken routing (quantizer silently bypassed) wouldn't necessarily raise here — the assertion doesn't actually pin the claimed behavior.Consider a stronger check, e.g. patching/spying on
input_quantizer.forwardor comparing output against a manually-unquantized forward pass, to make this a real regression guard.♻️ Example approach
- model(torch.randn(2, 4)) # forward must route through the quantizer + called = [] + orig_forward = model[1].input_quantizer.forward + model[1].input_quantizer.forward = lambda x: (called.append(True), orig_forward(x))[1] + model(torch.randn(2, 4)) + assert called, "forward did not route through the quantizer"🤖 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/quantization/test_conversion.py` around lines 189 - 201, The forward-call assertion in test_register_replace_unregister_cycle is too weak to prove routing through TensorQuantizer. Strengthen the check around model(torch.randn(2, 4)) by spying on or patching _QuantPlainAct.input_quantizer.forward, or by asserting the output differs from a manually computed unquantized pass, so the test in test_register_replace_unregister_cycle and the replace_quant_module/register/unregister path actually verify quantizer usage rather than just lack of exceptions.
🤖 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 `@tests/unit/torch/quantization/test_conversion.py`:
- Around line 189-201: The forward-call assertion in
test_register_replace_unregister_cycle is too weak to prove routing through
TensorQuantizer. Strengthen the check around model(torch.randn(2, 4)) by spying
on or patching _QuantPlainAct.input_quantizer.forward, or by asserting the
output differs from a manually computed unquantized pass, so the test in
test_register_replace_unregister_cycle and the
replace_quant_module/register/unregister path actually verify quantizer usage
rather than just lack of exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ce486ed-389a-4f8c-8ac4-04ca3170e3f5
📒 Files selected for processing (1)
tests/unit/torch/quantization/test_conversion.py
…lamp assertion; update header year Signed-off-by: arham766 <arhamislam766@yahoo.com>
What does this PR do?
Type of change: new tests
Part of the unit-coverage initiative in #1902 (one PR per module; this one is self-contained). Covers: quantize/restore mode layer: replace_quant_module insertion rules, register/unregister lifecycle, set_quantizer_by_cfg full-replacement vs partial-merge semantics across all three accepted formats (legacy formats pinned under pytest.warns(DeprecationWarning)), SequentialQuantizer creation/downgrade (guarded non-empty collection helpers), restore error directions (Unmatched vs Extra keys), cfg context-manager revert paths. Documents two real defects from the fused-experts name normalization (see issue).
Usage
N/A — tests only.
Testing
Suite is hermetic (CPU-only, no network, deterministic, <3s) and passes locally; the full collectable tests/unit tree passes with it (1999 passed). The suite was adversarially reviewed and verified to kill seeded mutations of the target module (details in #1902).
Before your PR is "Ready for review"
Additional Information
Issue: #1902
Summary by CodeRabbit