-
Notifications
You must be signed in to change notification settings - Fork 72
[IR] Convenience constructor for Node #2126
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 refactors the IR constructors by moving them into a dedicated module and adding a convenience constructor for creating Node objects.
- Adds a new module (onnxscript/ir/_convenience/_constructors.py) with constructors for both tensor and node objects.
- Updates the IR init.py file to export the new constructors and refactors tests to point to the new module.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
onnxscript/ir/_convenience/_constructors.py | Introduces convenience constructors for tensor and node, enabling easier instantiation of IR objects. |
onnxscript/ir/init.py | Updates public API exports to reference the new node and tensor constructors. |
onnxscript/ir/_convenience/_constructors_test.py | Adjusts tests to use the refactored tensor constructor from the new module. |
❌ 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 |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[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.
Would be better to include the motivation of this PR, and a demo of constructing a node and a tensor.
Done by updating the PR description and added doctests |
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.
LGTM, thanks
Create a convenience constructor for `Node`. Refactor the constructors to a separate module. ## Motivation Currently users when interacting with the IR needs to use the raw `ir.Node` constructor for creating nodes. This constructor is designed for performance and not ease-of-use. For users I created a new `ir.node` that exposes a more natural calling style that supports plain python values as attributes and an optional `domain` argument. --------- Co-authored-by: Copilot <[email protected]>
Create a convenience constructor for `Node`. Refactor the constructors to a separate module. ## Motivation Currently users when interacting with the IR needs to use the raw `ir.Node` constructor for creating nodes. This constructor is designed for performance and not ease-of-use. For users I created a new `ir.node` that exposes a more natural calling style that supports plain python values as attributes and an optional `domain` argument. --------- Co-authored-by: Copilot <[email protected]>
Create a convenience constructor for `Node`. Refactor the constructors to a separate module. ## Motivation Currently users when interacting with the IR needs to use the raw `ir.Node` constructor for creating nodes. This constructor is designed for performance and not ease-of-use. For users I created a new `ir.node` that exposes a more natural calling style that supports plain python values as attributes and an optional `domain` argument. --------- Co-authored-by: Copilot <[email protected]>
Create a convenience constructor for
Node
. Refactor the constructors to a separate module.Motivation
Currently users when interacting with the IR needs to use the raw
ir.Node
constructor for creating nodes. This constructor is designed for performance and not ease-of-use. For users I created a newir.node
that exposes a more natural calling style that supports plain python values as attributes and an optionaldomain
argument.