-
Notifications
You must be signed in to change notification settings - Fork 72
[pass] Implement checker pass and refactor shape inference #2199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
onnxscript/ir/passes/common/shape_inference.py:23
- [nitpick] Consider renaming _merge_func to a more descriptive name such as merge_inferred_model to better reflect its purpose.
def _merge_func(model: ir.Model, inferred_proto: onnx.ModelProto) -> tuple[ir.Model, bool]:
❌ 19 Tests Failed:
View the top 2 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
initializer_values = tuple(model.graph.initializers.values()) | ||
tensors = {v.name: v.const_value for v in initializer_values} | ||
original_inputs_len = len(model.graph.inputs) | ||
initializer_names = {v.name for v in initializer_values} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Looks like the check in line 73 is not really being used effectively? Wouldn't it be better to ensure that we create only a list of the temporarily removed initializers in the loop below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the function to use a dictionary and removed the check. Does it look like what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a new CheckerPass to perform ONNX model checks and refactors the shape inference pass to better handle large models by offloading initializer management to a shared utility. Key changes include:
- Adding CheckerPass and its corresponding tests in onnx_checker.py and onnx_checker_test.py.
- Refactoring shape inference in shape_inference.py to use a helper (partial_infer_shapes) and common initializer management via _c_api_utils.
- Updating tests in shape_inference_test.py to validate the in-place nature of the pass and adjust initializer handling.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
onnxscript/ir/passes/common/shape_inference_test.py | Added test for in-place property and updated initializer tests reflecting the refactor. |
onnxscript/ir/passes/common/shape_inference.py | Refactored the inference pass and merged inferred shapes using a dedicated helper. |
onnxscript/ir/passes/common/onnx_checker_test.py | Introduced tests for the new CheckerPass ensuring proper model checking without side effects. |
onnxscript/ir/passes/common/onnx_checker.py | Added a CheckerPass to run ONNX model checks using the C API utilities. |
onnxscript/ir/passes/common/_c_api_utils.py | Extracted common functionality for managing large initializer values during API calls. |
Comments suppressed due to low confidence (1)
onnxscript/ir/passes/common/shape_inference_test.py:129
- Consider adding or reintroducing some assertions to validate that initializers and input lists are correctly restored after performing shape inference. This will help ensure that the refactored initializer handling via _c_api_utils does not inadvertently modify the model.
Removed assertions checking that the original model is not modified after shape inference
Uh oh!
There was an error while loading. Please reload this page.