[DRAFT] Merge metadata props when fusing #2375
Conversation
bmehta001
force-pushed
the
merge-docstrings-when-fusing
branch
from
403c9f3 to
0ffe2bd
Compare
bmehta001
force-pushed
the
merge-docstrings-when-fusing
branch
from
0ffe2bd to
cd8ea57
Compare
onnxscript/rewriter/_rewrite_rule.py
Outdated
| fused_metadata_props = "Fused: " + "\t\n".join( | ||
| n.metadata_props for n in delta.match.nodes if getattr(n, "metadata_props", None) | ||
| ) |
There was a problem hiding this comment.
n will always have metadata_props.
onnxscript/rewriter/_rewrite_rule.py
Outdated
| n.metadata_props for n in delta.match.nodes if getattr(n, "metadata_props", None) | ||
| ) | ||
| # Assign to all new nodes (or just the first, depending on your policy) | ||
| delta.new_nodes[0].metadata_props += fused_metadata_props |
There was a problem hiding this comment.
Prefer metadata_props.update()
onnxscript/rewriter/_rewrite_rule.py
Outdated
| # If we are fusing nodes, update the metadata props of the new node(s) | ||
| if delta.match.nodes and delta.new_nodes: | ||
| # Concatenate metadata props from all original nodes | ||
| fused_metadata_props = "Fused: " + "\t\n".join( |
There was a problem hiding this comment.
I would consider creating special reconciliation logic for known fields
There was a problem hiding this comment.
+1. I am also confused a bit here. Isn't metadata_props effectively a dict[str, str]? I am not sure how the proto is represented in onnx-ir, but assuming it is a dictionary. We should be merging the dictionaries, using special merging logic for known entries in the dictionary.
This may require us to extend the definition/documentation of specific metadata fields. For example, we may need a way to represent a set of source-locations, instead of a single source-location, for a source-location metadata.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2375 +/- ##
==========================================
+ Coverage 70.15% 70.28% +0.12%
==========================================
Files 197 199 +2
Lines 24985 25233 +248
Branches 2669 2691 +22
==========================================
+ Hits 17529 17735 +206
- Misses 6529 6566 +37
- Partials 927 932 +5 ☔ View full report in Codecov by Sentry. |
| @@ -531,14 +531,22 @@ def _apply_to_graph_or_function( | |||
| f = ir.Function(domain, name, overload, graph=graph, attributes=()) | |||
| model.functions[f.identifier()] = f | |||
|
|
|||
| # If we are fusing nodes, update the metadata props of the new node(s) | |||
| # If we are fusing nodes, update the docstring of the new node(s) | |||
| attributes = ["namespace", "pkg.torch.onnx.class_hierarchy", "pkg.torch.onnx.fx_node", "pkg.torch.onnx.name_scopes", "pkg.torch.onnx.stack_trace"] | |||
There was a problem hiding this comment.
For namespace, class_hierarchy and name_scopes, we should parse the info and find the common leading scopes.
No description provided.