-
Notifications
You must be signed in to change notification settings - Fork 90
Clear initializers in constant folding pass #2668
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
Clear unused initializers on the fly to prevent memory usage jump due to intermediate folded tensors. Signed-off-by: Justin Chu <[email protected]>
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 adds automatic cleanup of unused initializers during constant folding and bumps the package version to 0.5.6.
- Adds functionality to detect and remove unused initializers when nodes are removed during constant folding
- Updates package version from 0.5.5 to 0.5.6
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| onnxscript/optimizer/_constant_folding.py | Adds _clear_unused_initializers helper function and integrates it into the replace_node method to clean up unused initializers after node replacement |
| VERSION | Bumps package version from 0.5.5 to 0.5.6 |
Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2668 +/- ##
==========================================
+ Coverage 70.42% 70.45% +0.02%
==========================================
Files 224 224
Lines 26729 26751 +22
Branches 2673 2678 +5
==========================================
+ Hits 18825 18847 +22
Misses 6973 6973
Partials 931 931 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Justin Chu <[email protected]>
|
@titaiwangms fixed tests. PTAL. Also cc @gramalingam |
|
|
||
| if isinstance(root, ir.Graph): | ||
| # The old node should now be detached from the graph | ||
| assert node.graph is None |
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.
Do we need this assertion? This might be true in the current implementation ... but just wondering if we might ever want to replace [oldnode] by [newnode1, oldnode, newnode2], for example ... not sure of replace_nodes_and_values supports such a scenario ... but the logic here seems to be robust even in that case. But the assert would fail in that case. Anyway ... just a minor suggestion.
Also: in principle, this logic could go into replace_nodes_and_values, can't it?
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.
this logic could go into replace_nodes_and_values, can't it?
Good point - I agree. But also replace_nodes_and_values is more general. So checking value users may be more expensive there.
replace [oldnode] by [newnode1, oldnode, newnode2]
Not sure? replace_nodes_and_values will disconnect the old nodes (which breaks all the value connections) so I don't think it will work?
Clear unused initializers on the fly to prevent memory usage jump due to intermediate folded tensors.