Add New Metadata and Pattern Features#2271
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import onnx | ||
|
|
||
| from onnxscript import ir | ||
| from onnxscript import rewriter |
Check notice
Code scanning / CodeQL
Unused import Note
| inputs.add(ninput) | ||
| elif any(ninput is init for init in node.graph.initializers): | ||
| initializers.add(ninput) | ||
| elif ninput.producer() == None: |
Check notice
Code scanning / CodeQL
Testing equality to None Note
| @@ -0,0 +1,190 @@ | |||
| import pytest | |||
Check notice
Code scanning / CodeQL
Unused import Note test
| import ast | ||
|
|
||
| import onnx | ||
| from onnxscript import script |
Check notice
Code scanning / CodeQL
Unused import Note test
| # for rule in tracer.best_matches_map: | ||
| # matches = tracer.best_matches_map[rule] | ||
| # for match in matches: | ||
| # print(f'Reason: {match.match_result.reason}') | ||
| # print(f'root_node: {match.root_node}') | ||
| # pdb.set_trace() |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| # for node in ir.traversal.RecursiveGraphIterator(mypipeline_model.graph): | ||
| # if node.domain == '': | ||
| # print(node) |
Check notice
Code scanning / CodeQL
Commented-out code Note test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2271 +/- ##
==========================================
- Coverage 70.38% 62.59% -7.80%
==========================================
Files 222 225 +3
Lines 26619 27229 +610
Branches 2661 2770 +109
==========================================
- Hits 18736 17044 -1692
- Misses 6969 9342 +2373
+ Partials 914 843 -71 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
lintrunner found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| else: | ||
| return self.class_metadata[depth] | ||
|
|
||
| class PytorchHierarchyNode: |
There was a problem hiding this comment.
This is useful! Let me see how we can provide the functionality to users.
| def append_output_to_node(node, output): | ||
| output._producer = node | ||
| output._index = node.outputs[-1]._index + 1 | ||
| node._outputs = (*node._outputs, output) | ||
| node._num_outputs = len(node._outputs) | ||
|
|
||
| def prepend_output_to_node(node, output): | ||
| output._producer = node | ||
| output._index = 0 | ||
| for outp in node._outputs: | ||
| outp._index += 1 | ||
| node._outputs = (output, *node._outputs) | ||
| node._num_outputs = len(node._outputs) |
There was a problem hiding this comment.
We should add this to node so there is no need to modify internal states
There was a problem hiding this comment.
In fact we recommend creating new nodes if you want to update inputs and outputs. This way invariance of the graph is maintained. You may consider accumulating the inputs and outputs into two lists before constructing the node.
| Ident = ir.Node(domain='', | ||
| op_type='Identity', | ||
| inputs = [cinput], | ||
| outputs = [noutput], | ||
| num_outputs =1) | ||
| LoopBody.function.append(Ident) | ||
|
|
||
| #Add Output to Function Call Nodes | ||
| for i,node in enumerate(nodes): | ||
| output_copy = copy.copy(noutput) | ||
|
|
||
| #preserve single_assignment | ||
| output_copy.name += f'_{i}' | ||
| append_output_to_node(node,output_copy) |
There was a problem hiding this comment.
Is there a way to collect all outputs before constructing the node?
| vmap[input] = ValuePattern(input.name) | ||
|
|
||
| for init in graph.initializers: | ||
| vmap[init] = ValuePattern(init.name) |
There was a problem hiding this comment.
Looks like you want to map constants and initializers to unconstrained variables in the pattern? I wonder if it would make sense to map them to "Constants" in the pattern that require a matching contstant-value in the graph for a successful match? That makes reasonable, at least for simple and small constants. If it should be abstracted, wouldn't it be better for the user themselves to do that explicitly by mapping them to graph inputs?
There was a problem hiding this comment.
Yes. Thanks for this suggestion. I think we should do this. This would provide a good level of control for the user.
| ninputs.append(vmap[ninput]) | ||
|
|
||
| #if len(node.outputs) > 1: | ||
| vp_outputs = builder.__getattr__(node.op_type)(*ninputs,_domain=node.domain, _outputs=len(node.outputs)) |
There was a problem hiding this comment.
So, the attributes of the node are abstracted away, and not matched?
There was a problem hiding this comment.
Good point. We ought to match the attributes. I'll make these changes.
| node._outputs = (output, *node._outputs) | ||
| node._num_outputs = len(node._outputs) | ||
|
|
||
| def prepend_input_to_node(node, input): |
There was a problem hiding this comment.
Modifying private fields is not recommended. I would consider initializing a new node
| golden_results = ort_run_graph(args.filename, input_dict, outputs[0].name) | ||
|
|
||
|
|
||
| LoopBody = LoopBodyTemplate(args.patternfilename) |
There was a problem hiding this comment.
| LoopBody = LoopBodyTemplate(args.patternfilename) | |
| loop_body = LoopBodyTemplate(args.patternfilename) |
note: always use snake_case for variable names
| return [output, used_output] | ||
|
|
||
|
|
||
| def bGraphView(name, nodes): |
There was a problem hiding this comment.
nit: snake case for function names. Is this build_graph_view?
| usage.add("EXTERNAL") | ||
| return usage | ||
|
|
||
| def find_subgraph_inputs(nodes): |
There was a problem hiding this comment.
This function is useful. I wonder if we should put it in ir.convenience https://github.com/onnx/ir-py/blob/main/src/onnx_ir/_convenience/__init__.py
There was a problem hiding this comment.
As well as the one below for outputs and bGraphView
3 participants
This PR will add new metadata and pattern features.