Repository navigation
Conversation
_convert_weight treated every scale as per-axis: it compared scale.shape[0] against the weight's quantization axis and reshaped the scale along that axis. A per-tensor scale is the ONNX default for QuantizeLinear/DequantizeLinear and has scalar or single-element shape, so a scalar scale raised IndexError on scale.shape[0] and a (1,) scale raised a spurious shape mismatch. Either way the conversion of a per-tensor quantized weight failed. Decide per-tensor vs per-axis before any scale indexing: a scale with a single element is broadcast over the whole weight, and only a multi-element scale is validated against the axis length. The per-axis path and its error are unchanged. Signed-off-by: Yi-111-a <153097222+Yi-111-a@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
MatMul QDQ conversion tests tests/unit/onnx/quantization/test_qdq_utils.py |
A fixture builds MatMul QDQ models with selected scale and zero-point shapes. Tests expect scalar and one-element scales to remove QuantizeLinear, convert the weight initializer to int8, and preserve outputs within atol=1e-5. A mismatch case expects a RuntimeError identifying the scale and weight shape mismatch. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other · Severity of issue fixed: Medium
Merge Risk: 
8ce37
-
No actionable merge-blocking risk is established in the test-only change.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the added ONNX tests for per-tensor weight scales in qdq_to_dq. Linked Issues check Issue #2561requires successful conversion for scalar and(1,)per-tensor scales, numerical equivalence, and rejection of an invalid per-axis scale. The reviewed head contains the per-tensor convers…Out of Scope Changes check The pull request adds only focused QDQ conversion tests and their test helpers. The tests directly cover issue #2561requirements and preserve the per-axis validation case. No unrelated change is show…Security Anti-Patterns PASS. The PR changes only tests/unit/onnx/quantization/test_qdq_utils.py; it adds nomodeloptorexamplesPython code and changes no dependency manifest. Added lines contain none of the specifie…Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Full details: Linked Issues check
-
Explanation
Issue
#2561requires successful conversion for scalar and(1,)per-tensor scales, numerical equivalence, and rejection of an invalid per-axis scale. The reviewed head contains the per-tensor conversion implementation.TestQdqToDqPerTensorScalecovers both scale shapes, checks removal ofQuantizeLinear, checks anint8weight, compares ONNX Runtime outputs, and verifies rejection of a mismatched(8,)per-axis scale. The reported focused test run passed.
Full details: Out of Scope Changes check
-
Explanation
The pull request adds only focused QDQ conversion tests and their test helpers. The tests directly cover issue
#2561requirements and preserve the per-axis validation case. No unrelated change is shown in the whole-pull-request diff.
Full details: Security Anti-Patterns
-
Explanation
PASS. The PR changes only
tests/unit/onnx/quantization/test_qdq_utils.py; it adds nomodeloptorexamplesPython code and changes no dependency manifest. Added lines contain none of the specified security anti-patterns: unsafetorch.load,numpy.load(..., allow_pickle=True), hardcodedtrust_remote_code=True, external-inputeval/exec, or# nosec.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Autopilot is currently an internal CodeRabbit preview.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Heads-up on an overlap, since I filed #2561 and have changes pending in the same function — flagging it so whoever merges does not get a surprise, not to hold this up. #2554 touches the lines immediately above this diff. It already introduces if scale_array.size > 1:
# per-axis: take the axis the Q/DQ pair declares
axis = 1 if dq_axis is None else dq_axis
...
else:
axis = axis_map[op_type]This PR adds the same distinction a few lines down, for the shape validation and the #2575 is unrelated, but #2567 also touches this function slightly lower down (it derives the stored weight type and clip range from the zero point instead of hardcoding I am happy to rebase mine on top of this once it lands, or to reorder — entirely the maintainers' call which goes first. Just did not want two near-identical per-tensor checks to land independently. |
A one-element scale was routed to the consumer's axis_map entry, which only converts when that dimension happens to be 1. A single-output projection is the counterexample: weight (1, 32) with a (1,) scale picked MatMul axis 1 and failed the length check against 32. It fails the same way without a Transpose, so the gate was wrong rather than the tracing. A single scale covers the whole weight. It broadcasts, and no axis applies to it, so leave reshape_dims all ones and run the length check only for a per-axis scale. That leaves nothing reading axis_map's values -- the per-axis branch takes its axis from the Q/DQ pair -- so it becomes the set of supported consumers it was really being used as, and the Gemm transB lookup that fed it goes with it. One consequence worth flagging: a scalar scale now converts as well, which is the subject of NVIDIA#2613. Signed-off-by: Siddhardha Nanda <99672439+SID-6921@users.noreply.github.com>
…in qdq_to_dq (#2554) ### What does this PR do? Type of change: Bug fix > Rewritten 2026-10-02 to describe what the branch actually does. The first version described a composed-permutation approach that review replaced; that machinery is gone. `qdq_to_dq()` assumed `DequantizeLinear` feeds `Conv`/`Gemm`/`MatMul` directly, tolerating only a `Cast` in between. A weight that reaches `MatMul` transposed — as in MaxViT's attention blocks — put `Transpose` in front of `_convert_weight`, which has no entry for it: ``` RuntimeError: Failed to convert node .../qkv/weight_quantizer/QuantizeLinear: Unsupported op_type for real weight quantization: Transpose ``` **Two changes, and the second is the one that matters.** **1. Trace `Cast`/`Transpose` to reach the real consumer.** That walk now exists only to find the node whose op type decides whether this conversion supports the weight at all. **2. Take the quantization axis from the Q/DQ pair, not from the consumer.** After conversion the `DequantizeLinear` stays in the graph with its input rewired to the weight initializer, so *its* axis is what indexes the stored weight — regardless of any `Transpose` downstream. The conversion follows that axis, defaulting to ONNX's `axis = 1` when the attribute is omitted. No permutation is composed and there is no consumer-axis fallback. That also fixes a case that is broken on `main` today and has nothing to do with `Transpose`. Measured end to end under onnxruntime, square weight into `MatMul`: | Q/DQ axis | Transpose | on `main` | this PR | |---|---|---|---| | 0 | no | **silently wrong** (max diff 4.62) | correct | | 0 | yes | raises `Unsupported op_type` | correct | | 1 | no | correct | correct | | 1 | yes | n/a (raised before this PR) | correct | A wrong axis on a square weight still satisfies the scale-length check, so nothing raises and the model simply computes something else. **Two smaller consequences of making the Q/DQ pair authoritative:** - A **single-element scale** covers the whole weight, so it broadcasts and no axis applies to it. It no longer borrows the consumer's axis, which only worked when that dimension happened to be 1 — a `(1, 32)` single-output projection failed before. A scalar scale converts as well, which is also the subject of #2613. - A **per-axis Q/DQ pair that disagrees** on the axis now raises instead of silently following the DQ and dropping the Q. Effective axes are compared, so declaring `axis` on only one side is not a disagreement. With nothing left reading the per-op axis values, `axis_map` became the set of supported consumers it was really being used as. ### Testing `TestQdqToDqTranspose`, 10 cases, each failing on `main`: - `test_conversion_matches_qdq` — 5 combinations of shape x axis x transpose x declared/omitted axis, comparing onnxruntime output before and after conversion - `test_dq_transpose_without_perm_is_traced` — ONNX lets `Transpose` omit `perm` - `test_one_element_scale_broadcasts_through_transpose` — `(1, 32)` and `(32, 1)` - `test_one_element_scale_is_not_per_axis` — single-output `Conv` - `test_disagreeing_qdq_axes_raise` On the model from #2442: a quantized `maxvit_tiny_rw_256` exported with `dq_only=True` raises on `main` and exports here, and the resulting DQ-only graph is **bit-identical to the QDQ graph under onnxruntime — max abs diff `0.0e+00`** across 457 quantizers. `tests/unit/onnx` is 659 passed locally with 11 failures that also occur on pristine `main` in this environment (int4 paths needing cupy, three `TestColumnMajorTransformation` tests needing MSVC build tools). `ruff check` reports the same 3 pre-existing `PLR0917` findings before and after. ### Before your PR is "*Ready for review*" - Is this change backward compatible?: ✅ — a correctly emitted per-channel Q/DQ pair already declares the axis this conversion now reads, so well-formed graphs convert bit-identically; only graphs where the two disagreed change behaviour - 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?: ✅ - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: ✅ - Did you get Claude approval on this PR?: N/A ### Additional Information Fixes #2442. #2613 edits `_convert_weight` against the lines this PR restructures, and #2567 touches the same function lower down. Happy to rebase in whatever order suits you. --------- Signed-off-by: SID <99672439+SID-6921@users.noreply.github.com> Signed-off-by: Siddhardha Nanda <99672439+SID-6921@users.noreply.github.com> Co-authored-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
|
Thanks for the contribution! This PR now has merge conflicts with We've added the |
main now handles single-element scales in _convert_weight (NVIDIA#2554, NVIDIA#2678), so take main's implementation and keep only the per-tensor regression tests for a plain MatMul weight with a scalar or (1,) scale. Signed-off-by: Yi-111-a <153097222+Yi-111-a@users.noreply.github.com>
Yi-111-a
changed the title
|
Thanks @kevalmorabia97, merged current |
What does this PR do?
Type of change: Bug fix
Fixes #2561.
_convert_weight()assumed every scale was per-axis. It comparedscale.shape[0]against the weight's quantization axis and reshaped the scale along that axis, which breaks for a per-tensor scale — the ONNX default forQuantizeLinear/DequantizeLinear, where a scalar (or single-element) scale broadcasts across the whole weight:main()IndexError: tuple index out of range(1,)ValueError: Scale shape (1,) does not match weight shape (16, 32) along axis 1Both spellings are valid ONNX — they pass
onnx.checker.check_modeland run under onnxruntime before conversion — so any--dq_only/dq_only=Trueexport of a model quantized per-tensor failed. (The scalar expansion suggested in the earlier #581 turns theIndexErrorinto the(1,)case's shape mismatch rather than fixing it.)The change decides per-tensor vs per-axis before anything indexes the scale: a scale with a single element is broadcast over the whole weight, and only a multi-element scale is validated against the axis length. The per-axis path and its error message are unchanged.
Usage
No API change. Existing per-tensor exports now convert:
Testing
TestQdqToDqPerTensorScalecovering both per-tensor spellings (parametrized over scalar and(1,)) plus a guard that a per-axis scale of the wrong length is still rejected. The equivalence test runs the real model through onnxruntime and compares the converted DQ-only graph against the original QDQ graph — output is bit-identical (atol=1e-5) for both spellings, and the rewritten weight isint8with the Q node removed.pytest tests/unit/onnx/quantization/test_qdq_utils.py— 31 passed. The 3TestColumnMajorTransformationfailures in this file also fail on unmodifiedmain(333ace1) in this environment; they are unrelated to this change.pytest tests/unit/onnx— 652 passed, 9 failed. The same 9 failures occur on unmodifiedmain(649 passed there; the +3 are the new tests).ruff check/ruff format --checkclean on both touched files (the 3 remainingPLR0917hits are on untouched lines).Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Summary by CodeRabbit