-
Notifications
You must be signed in to change notification settings - Fork 72
[pass] Update LiftSubgraphInitializersToMainGraphPass to disallow variable shadowing #2348
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.
Pull Request Overview
This PR updates the LiftSubgraphInitializersToMainGraphPass to disallow variable shadowing in accordance with the ONNX spec and adjusts the tests accordingly.
- Updates test cases to differentiate between unique and duplicated initializer names.
- Modifies the uniqueness check in the requires function to raise a PreconditionError on duplicate names.
- Removes the renaming strategy from the call() method to strictly enforce unique initializer names.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
onnxscript/ir/passes/common/constant_manipulation_test.py | Updated parameterized tests to handle a new test case identifier and error handling logic. |
onnxscript/ir/passes/common/constant_manipulation.py | Added a precondition check for duplicate initializer names and removed renaming logic. |
Comments suppressed due to low confidence (1)
onnxscript/ir/passes/common/constant_manipulation_test.py:314
- Consider adding a test case covering the scenario where an initializer is also a graph input to verify the skipping logic during subgraph lifting.
if then_initializer_name == else_initializer_name:
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Variable shadowing (reusing value names) is disallowed in ONNX across the main graph and subgraphs according to the spec (https://github.com/onnx/onnx/pull/6955/files). This change updates to the logic to check and raise on such cases. A subsequent PR will implement #1432 to allow users to fix names explicitly.