[5597849] Fix for 'SymbolicShapeInference' error #453
Conversation
WalkthroughRenamed helper Changes
Sequence Diagram(s)sequenceDiagram
participant Finder as find_nodes_from_matmul_to_exclude
participant Excluder as _exclude_matmuls_by_shape_inference
participant Infer as infer_shapes
Finder->>Excluder: call(matmul_nodes, calibration_shapes, model)
alt calibration_shapes provided
Excluder->>Infer: infer_shapes(model, input_shapes)
Infer-->>Excluder: model_with_inferred_shapes
Excluder->>Excluder: evaluate shapes -> collect exclusions
else calibration_shapes absent
Excluder->>Excluder: use existing value_info -> collect exclusions
end
Excluder-->>Finder: return excluded_matmul_node_names
sequenceDiagram
participant Quantize as quantize()
participant Loader as get_input_shapes
Quantize->>Quantize: has calibrate_per_node?
alt calibrate_per_node == true and calibration_shapes missing
Quantize->>Loader: get_input_shapes(model)
Loader-->>Quantize: input_shapes
else
Quantize->>Quantize: skip automatic input shape loading
end
Quantize-->>Quantize: proceed with calibration using available shapes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)modelopt/onnx/quantization/graph_utils.py (1)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modelopt/onnx/quantization/graph_utils.py (2)
968-976: Use explicit None-check to select the shape-inference path.Truthiness check can misroute when calibration_shapes is {} or "".
- if calibration_shapes: + if calibration_shapes is not None: nodes_to_exclude = _exclude_matmuls_by_shape_inference( model, matmul_nodes, calibration_shapes )
1061-1113: Shape inference path: add robustness (output lookup, input validation, minor wording).
- Include graph outputs in shape lookup; current code only checks value_info and may falsely error if a MatMul/Gemm output is a graph output.
- Validate calibration_shapes dims are positive integers.
- Minor: adjust comment to “shape inference” for consistency.
def _exclude_matmuls_by_shape_inference( model: onnx.ModelProto, matmul_nodes: list, calibration_shapes: str | dict | None = None ) -> list[str]: """Use shape inference to find MatMuls with dimension 1.""" - # Prepare model for symbolic inference + # Prepare model for shape inference for graph_input in model.graph.input: for dim in graph_input.type.tensor_type.shape.dim: if dim.HasField("dim_param"): dim.Clear() dim.dim_value = 1 # Apply calibration shapes if provided input_shapes = {} if calibration_shapes: input_shapes = ( parse_shapes_spec(calibration_shapes) if isinstance(calibration_shapes, str) else calibration_shapes ) for graph_input in model.graph.input: if graph_input.name in input_shapes: input_shape = input_shapes[graph_input.name] tensor_shape = graph_input.type.tensor_type.shape.dim if len(tensor_shape) != len(input_shape): raise ValueError( f"{graph_input.name} expects shape of rank {len(tensor_shape)}, " f"but calibration shape of rank {len(input_shape)} was passed." ) - for dim, new_dim_value in zip(tensor_shape, input_shape): - dim.dim_value = new_dim_value + for dim, new_dim_value in zip(tensor_shape, input_shape): + # Require positive, concrete dims to keep ONNX inference stable. + if int(new_dim_value) <= 0: + raise ValueError( + f"calibration_shapes for '{graph_input.name}' must be > 0; got {new_dim_value}" + ) + dim.dim_value = int(new_dim_value) - model = infer_shapes(model) - value_info_map = {vi.name: vi for vi in model.graph.value_info} + model = infer_shapes(model) nodes_to_exclude = [] for matmul_node in matmul_nodes: output_name = matmul_node.outputs[0].name - value_info = value_info_map.get(output_name) - if not value_info: - raise RuntimeError(f"Shape inference did not find shape for {output_name}.") + # Search inputs, outputs, or value_info + value_info = get_tensor_from_name(model.graph, output_name) + if value_info is None: + raise RuntimeError(f"Shape inference did not find shape for {output_name}.") dims = value_info.type.tensor_type.shape.dim if all(isinstance(inp, Variable) for inp in matmul_node.inputs): if len(dims) < 2: raise RuntimeError(f"Shape for {output_name} is incorrect.") if dims[-1].dim_value == 1 or dims[-2].dim_value == 1: nodes_to_exclude.append(matmul_node.name) elif len(dims) < 3 and any(out.dim_value == 1 for out in dims): nodes_to_exclude.append(matmul_node.name) return nodes_to_exclude
🧹 Nitpick comments (1)
modelopt/onnx/quantization/graph_utils.py (1)
949-951: Docstring wording: “symbolic shape inference” → “shape inference”.This path now uses ONNX shape inference via infer_shapes, not ORT SymbolicShapeInference. Update the docstring for accuracy.
- calibration_shapes: Model input shapes for inference. If provided, symbolic shape inference will be used - instead of calibration_data_reader. + calibration_shapes: Model input shapes for inference. If provided, ONNX shape inference will be used + instead of calibration_data_reader.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modelopt/onnx/quantization/graph_utils.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modelopt/onnx/quantization/graph_utils.py (1)
modelopt/onnx/utils.py (2)
infer_shapes(723-736)parse_shapes_spec(234-247)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: build-docs
- GitHub Check: code-quality
🔇 Additional comments (1)
modelopt/onnx/quantization/graph_utils.py (1)
38-41: Let me check ifgraph_utils.pyitself has any remaining references toSymbolicShapeInference:Good switch to internal ONNX shape inference helper.
Using
modelopt.onnx.utils.infer_shapesavoids the ORT SymbolicShapeInference dependency and handles >2GB models. The change ingraph_utils.pysuccessfully removes the ORT dependency without leaving dangling references. ✓
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
gcunhase
force-pushed
the
dev/gcunhasergio/calib_shapes_fix_5597849
branch
from
7cf0d8f to
543c2ca
Compare
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
==========================================
- Coverage 73.42% 73.39% -0.04%
==========================================
Files 180 180
Lines 17975 17976 +1
==========================================
- Hits 13199 13193 -6
- Misses 4776 4783 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
What does this PR do?
Type of change: Bug fix
Overview: Fixed issue with
--calibration_shapesusage withSymbolicShapeInference. Replaced with onnxinfer_shapesfunction.Usage
Testing
Model in bug 5597849.
Before your PR is "Ready for review"
Summary by CodeRabbit
Refactor
Chores