-
Notifications
You must be signed in to change notification settings - Fork 64
[pass] Create topological sort pass #2191
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
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
onnxscript/ir/passes/common/topological_sort.py:20
- Verify that the sort() method on model.graph is implemented to perform a topological sort, as relying on a default sort may not achieve the intended graph ordering.
model.graph.sort()
onnxscript/ir/passes/common/topological_sort.py:29
- [nitpick] Consider replacing the manual node-by-node comparison with a direct list comparison (e.g., using 'modified = nodes != new_nodes') to handle any potential length differences and improve clarity.
for node, new_node in zip(nodes, new_nodes):
* **Test for modified=True**: Add a test to check if `modified` is True when the input model needs sorting. * **Test for modified=False**: Add a test to check if `modified` is False when the input model is already sorted.
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 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
onnxscript/ir/passes/common/topological_sort_test.py:25
- Consider adding an assertion that verifies the actual order of nodes in the model after sorting to ensure that the pass not only signals modification but also results in the expected order.
pass_result = topological_sort.TopologicalSortPass()(model)
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 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
onnxscript/ir/passes/common/topological_sort.py:19
- [nitpick] Consider renaming 'nodes' to 'original_nodes' and 'new_nodes' to 'sorted_nodes' (line 21) for improved clarity regarding the state of the node list before and after sorting.
nodes = list(model.graph)
Simply expose the
sort()
api as a pass for composability.