-
Notifications
You must be signed in to change notification settings - Fork 72
[IR] Record owning graph for input/output/initializers #2282
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
59ca12c
to
e64efe1
Compare
❌ 5 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.
Pull Request Overview
This PR updates the IR to record the owning graph for a value when it is used as a graph input/output. The key changes are:
- Introducing tracked container classes (GraphInputs and GraphOutputs) to manage and validate graph inputs/outputs.
- Modifying the Graph initialization to use these tracked containers.
- Adding the owning_graph() method to the Value class to help determine graph ownership.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
onnxscript/ir/_tracked_containers.py | New container classes for graph inputs and outputs with invariance checking and graph assignment logic. |
onnxscript/ir/_core.py | Updated Graph initialization to use tracked containers; added owning_graph() method with warning checks. |
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 addresses issue #1440 by ensuring that graph inputs, outputs, and initializers properly maintain their associations with the owning Graph through a tracked list. Key changes include modifying the constant folding logic to “free” graph outputs by copying and clearing them, updating tests to reflect new graph input behavior, and refactoring initializer handling to avoid incorrect graph ownership.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
onnxscript/optimizer/_constant_folding.py | Copies graph outputs into a temporary list and clears the original outputs to allow value movement. |
onnxscript/ir/passes/common/constant_manipulation_test.py | Updates unit tests to reflect that certain graph inputs are now removed when initializers are lifted. |
onnxscript/ir/passes/common/constant_manipulation.py | Iterates safely over initializers by converting the dict keys to a tuple before modification. |
onnxscript/ir/external_data_test.py | Wraps initializer values in new Value objects to reset their graph associations. |
onnxscript/ir/_core.py | Updates the Graph API to use the new GraphInput/Output/Initializer containers and adds a .graph property on Value. |
Comments suppressed due to low confidence (1)
onnxscript/ir/external_data_test.py:320
- Wrapping the initializer value in a new Value resets its graph association, ensuring proper ownership transfer. Verify that all relevant metadata from the original initializer is preserved in the new Value object.
model.graph.initializers["tensor_same_file"] = ir.Value(
Fix #1440 by pointing graph input, output and initializers back to the Graph using a tracked list.
Users can now check if a value is a graph input/output/initializer, and find the owning graph of a value with
.graph
.