[Pass] Support lift constants to initializers pass #2160
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2160 +/- ##
==========================================
+ Coverage 74.03% 74.13% +0.09%
==========================================
Files 222 224 +2
Lines 29239 29355 +116
Branches 3399 3414 +15
==========================================
+ Hits 21647 21762 +115
- Misses 6439 6440 +1
Partials 1153 1153 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
onnxscript/ir/passes/common/lift_constants_to_initializers_test.py
Outdated
Show resolved
Hide resolved
| const_value=tensor, | ||
| ) | ||
| assert node.graph is not None | ||
| assert isinstance(node.graph, ir.Graph) |
There was a problem hiding this comment.
@justinchuby does this make sense? I think there should not be any ir.Function node coming out from recursive iterator?
There was a problem hiding this comment.
Does make sense. Thanks! The reason why it was annotated with graph | function is that the “owning graph” can be a function when the node is part of a function. Maybe there are better ways to do it 🤔
There was a problem hiding this comment.
(Unrelated to this PR): Isn't a Function object a wrapper around a Graph object? Does node.graph not return that graph object even in the case of function nodes?
There was a problem hiding this comment.
Technically the graph in a function is private and not used directly. It is currently an implementation detail
There was a problem hiding this comment.
sorry I was wrong. It is pointed to a graph when we call function.append, but it is not when we call ir.Node(graph=function). I need to figure out how to reconcile this. Suggestions appreciated. #2181
titaiwangms
force-pushed
the
titaiwang/add_constant_to_initilizer_pass
branch
from
c3cec70 to
a2e9d2a
Compare
Co-authored-by: Justin Chu <[email protected]>
Fix #2156