Skip to content

fix(quantization): skip MaxPool during FP8 static quantization#28488

Merged
tianleiwu merged 3 commits into
microsoft:mainfrom
Rishi-Dave:rishidave/fix/fp8-static-quant-maxpool
Jun 3, 2026
Merged

fix(quantization): skip MaxPool during FP8 static quantization#28488
tianleiwu merged 3 commits into
microsoft:mainfrom
Rishi-Dave:rishidave/fix/fp8-static-quant-maxpool

Conversation

@Rishi-Dave

Copy link
Copy Markdown
Contributor

Summary

  • Skip MaxPool quantization when the activation type is one of the four ONNX FP8 variants (E4M3FN, E4M3FNUZ, E5M2, E5M2FNUZ), falling back to the existing unquantized passthrough used for opset_version < 12.
  • Add a FLOAT8_TYPES tuple in quant_utils as a single source of truth for the FP8 TensorProto enum values.
  • Add a regression test covering the FP8 MaxPool path.

Motivation

Fixes #21090.

ONNX MaxPool does not accept float8 inputs (per the spec), but the static quantization path was propagating FP8 types into MaxPool nodes. Loading the resulting model failed with:

InvalidGraph: Type 'tensor(float8e4m3fn)' of input parameter
(input_QuantizeLinear_Output) of operator (MaxPool) in node (/MaxPool) is invalid.

The user reproduces this with a simple Conv + MaxPool graph quantized via quantize_static(..., activation_type=QuantType.QFLOAT8E4M3FN, weight_type=QuantType.QFLOAT8E4M3FN, calibrate_method=CalibrationMethod.Distribution).

Changes

  • onnxruntime/python/tools/quantization/quant_utils.py: add FLOAT8_TYPES module-level tuple.
  • onnxruntime/python/tools/quantization/operators/maxpool.py: add an FP8 early-return guard in both QMaxPool.quantize() and QDQMaxPool.quantize() that mirrors the existing opset_version < 12 fallback for each class.
  • onnxruntime/test/python/quantization/test_op_maxpool.py: add TestOpMaxPoolFP8::test_quantize_maxpool_fp8 covering the regression. The test asserts structural correctness (no FP8-typed tensor on MaxPool inputs/outputs, no QuantizeLinear node directly feeding MaxPool) rather than running inference, since QLinearConv-FP8 is not runnable on the CPU EP.

Total non-test LOC: 15 (9 in maxpool.py, 6 in quant_utils.py).

Test Plan

  • python -m pytest onnxruntime/test/python/quantization/test_op_maxpool.py::TestOpMaxPool::test_quantize_maxpool -v — existing INT8 path: PASS.
  • python -m pytest onnxruntime/test/python/quantization/test_op_maxpool.py::TestOpMaxPool::test_quantize_maxpool_s8s8 -v — existing S8S8 path: PASS.
  • python -m pytest onnxruntime/test/python/quantization/test_op_maxpool.py::TestOpMaxPoolFP8::test_quantize_maxpool_fp8 -v — new FP8 regression: PASS.
  • lintrunner on the diff: clean (no patches applied, no warnings or errors).

The calibration-method validation in quantize_static is intentionally left unchanged — per the maintainer comment on the issue, CalibrationMethod.Distribution is the supported FP8 calibration path.

MaxPool does not accept float8 inputs per the ONNX spec, but the static
quantization path was propagating FP8 types into MaxPool nodes, producing
invalid graphs that fail with INVALID_GRAPH at InferenceSession load:

    Type 'tensor(float8e4m3fn)' of input parameter
    (input_QuantizeLinear_Output) of operator (MaxPool) in node (/MaxPool)
    is invalid.

Adds a guard in both QMaxPool.quantize() and QDQMaxPool.quantize() that
falls back to the unquantized passthrough (the same escape hatch used for
opset_version < 12) when the quantizer's activation type is one of the
float8 TensorProto variants. A new module-level FLOAT8_TYPES tuple in
quant_utils provides a single source of truth for the four FP8 enum values.

Adds a regression test in test_op_maxpool.py that runs FP8 static
quantization over a Conv+MaxPool graph and asserts the resulting MaxPool
node neither carries float8 tensor types nor is wrapped by QDQ.

Fixes microsoft#21090
@tianleiwu
tianleiwu requested a review from Copilot May 20, 2026 16:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes invalid ONNX graphs produced by FP8 static quantization by skipping MaxPool quantization when FP8 activation types are used, and adds a regression test to ensure FP8 tensors are not propagated into MaxPool.

Changes:

  • Add a shared FLOAT8_TYPES constant (TensorProto enum values) for FP8 detection.
  • Skip MaxPool quantization for FP8 activation types in both QOperator and QDQ paths.
  • Add a regression test that checks graph structure for the FP8 Conv -> MaxPool case.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
onnxruntime/test/python/quantization/test_op_maxpool.py Adds FP8 regression coverage to ensure MaxPool remains unquantized and does not receive FP8 tensors.
onnxruntime/python/tools/quantization/quant_utils.py Introduces FLOAT8_TYPES tuple as a single source of truth for FP8 TensorProto types.
onnxruntime/python/tools/quantization/operators/maxpool.py Guards MaxPool quantization to avoid emitting unsupported FP8-typed MaxPool inputs/outputs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread onnxruntime/test/python/quantization/test_op_maxpool.py
Comment thread onnxruntime/test/python/quantization/test_op_maxpool.py
Comment thread onnxruntime/python/tools/quantization/quant_utils.py
Comment thread onnxruntime/python/tools/quantization/operators/maxpool.py Fixed
CodeQL flagged super(Direct8BitOp, self).quantize() inside
QMaxPool.quantize() — the first argument to super() must be the
enclosing class. The intent of both call sites is to bypass
Direct8BitOp.quantize() and fall through to the
QuantOperatorBase passthrough, so call QuantOperatorBase.quantize(self)
explicitly. This is behaviorally identical and resilient to future
changes in the MRO between Direct8BitOp and QuantOperatorBase.
@Rishi-Dave

Copy link
Copy Markdown
Contributor Author

The CodeQL alert ("First argument to super() is not enclosing class") was raised against commit 69095c5, where QMaxPool.quantize retained the legacy super(Direct8BitOp, self).quantize() call to skip its own parent and hit QuantOperatorBase.quantize. That two-argument form named the parent class instead of the enclosing class, which is what CodeQL flagged.

Follow-up commit 9facab2 already addresses this. Both call sites in onnxruntime/python/tools/quantization/operators/maxpool.py (the opset_version < 12 path and the new FP8 early-return) now use an explicit QuantOperatorBase.quantize(self) call. That preserves the original intent of bypassing Direct8BitOp.quantize while removing the MRO ambiguity, and a plain zero-arg super().quantize() is used everywhere else where the immediate parent is the right target.

The alert should clear on the next CodeQL re-scan of the PR head.

@Rishi-Dave

Copy link
Copy Markdown
Contributor Author

The CodeQL py/super-not-enclosing-class alert on onnxruntime/python/tools/quantization/operators/maxpool.py is addressed in commit 9facab2. The original super(Direct8BitOp, self).quantize() was deliberately skipping Direct8BitOp.quantize() to fall through to the base implementation, but spelling it that way passes a non-enclosing class as the first arg to super(), which CodeQL (correctly) flags as fragile. The fix replaces both call sites in QMaxPool.quantize() with an explicit QuantOperatorBase.quantize(self) call and adds the corresponding import from base_operator. Same runtime behavior, no MRO games, and the alert should resolve on the next scan.

Comment thread onnxruntime/python/tools/quantization/operators/maxpool.py Dismissed

@tianleiwu tianleiwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, correct, well-scoped fix for #21090. The FP8 early-return guard in both QMaxPool.quantize() and QDQMaxPool.quantize() mirrors the existing opset_version < 12 passthrough and keeps MaxPool unquantized, which is what avoids propagating FP8 types into the MaxPool input. self.quantizer.activation_qType is the resolved integer TensorProto enum (base_quantizer.py sets self.activation_qType = getattr(activation_qType, "tensor_type", activation_qType)), so the in FLOAT8_TYPES membership test is a correct int-vs-tuple comparison. Replacing super(Direct8BitOp, self).quantize() with the explicit QuantOperatorBase.quantize(self) is clearer and equivalent.

A couple of non-blocking points:

  • Single-source-of-truth not fully realized: operators/matmul.py (lines 178-183) still hardcodes the identical 4-element FP8 tuple for its domain selection. Reusing the new FLOAT8_TYPES constant there (if self.quantizer.weight_qType in FLOAT8_TYPES) would complete the stated goal and prevent future drift. Optional here since matmul.py is outside this diff.
  • Test coverage: Only the QDQ path (QDQMaxPool) is exercised. The QMaxPool (QOperator-format) guard is untested; a short note or QOperator-format assertion would document intent. (FP8 + QOperator may not be reachable via the public quantize_static API today.)

The automated-review note about onnx_proto.TensorProto vs TensorProto is a false positive — both names refer to the same object in this module, and onnx_proto.TensorProto matches the existing precedent in matmul.py.

Comment thread onnxruntime/python/tools/quantization/quant_utils.py
…ol path

- Replace hardcoded FP8 tuple in QLinearMatMul.quantize with the shared
  FLOAT8_TYPES constant from quant_utils to keep a single source of truth.
- Drop the now-unused onnx_proto import in matmul.py.
- Add test_quantize_maxpool_fp8_qoperator to exercise the QOperator-format
  MaxPool skip guard, mirroring the existing QDQ-format coverage.

Addresses follow-up review feedback on microsoft#28488.
@Rishi-Dave

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Both follow-up suggestions are addressed in e8e27ef:

  1. operators/matmul.py now imports FLOAT8_TYPES from quant_utils and uses it for the FP8 domain check, replacing the hardcoded 4-element tuple at lines 178-183. The now-unused onnx_proto import was dropped (lintrunner-clean). Membership semantics are identical — FLOAT8_TYPES is the same four TensorProto values.

  2. test_op_maxpool.py gains test_quantize_maxpool_fp8_qoperator, which runs quantize_static with QuantFormat.QOperator + QuantType.QFLOAT8E4M3FN to exercise the QMaxPool.quantize() guard in operators/maxpool.py. It asserts exactly one MaxPool node survives and that none of its tensors carry an FP8 elem type, mirroring the existing QDQ-format test.

Local lintrunner -a is clean and all 4 tests in test_op_maxpool.py pass.

Comment thread onnxruntime/python/tools/quantization/operators/matmul.py

@tianleiwu tianleiwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct and well-scoped for #21090. The FP8 early-return guard in both QMaxPool.quantize() and QDQMaxPool.quantize() mirrors the existing opset_version < 12 passthrough, keeping MaxPool unquantized so FP8 types are no longer propagated into its input. self.quantizer.activation_qType is the resolved integer TensorProto enum, so the in FLOAT8_TYPES membership test is a correct int-vs-tuple comparison.

The earlier request to complete the single-source-of-truth goal is now addressed — operators/matmul.py consumes the new FLOAT8_TYPES constant instead of re-hardcoding the 4-element FP8 set, so the two lists can no longer drift. Resolving that thread.

Two minor, non-blocking items remain (both already anchored inline by the automated reviewers):

  • QMaxPool.quantize() FP8 branchreturn QuantOperatorBase.quantize(self) returns None (the base quantize() has no return value), which is what CodeQL flags. For consistency with the bare return used in the QDQMaxPool guard and the opset_version < 12 branch just above it, drop the return keyword: call QuantOperatorBase.quantize(self) then return. Cosmetic only.

  • Test assertions — in both new tests the FP8-type checks are guarded by if tensor_name in tensor_types:. If a MaxPool input/output never appears in value_info/graph IO (e.g., it is only an initializer or its type is not materialized), the assertNotIn is silently skipped and the test can pass vacuously. Consider also indexing initializers into tensor_types, or asserting the tensor was found, so the guard genuinely fails when an FP8 type leaks onto MaxPool.

Neither item blocks merge.

@tianleiwu
tianleiwu enabled auto-merge (squash) June 3, 2026 23:25
@tianleiwu
tianleiwu merged commit e2789cd into microsoft:main Jun 3, 2026
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] support FP8 calibraion method and quantization

4 participants