Introduce insert_nodes convenience function

[Johansmm]

Convenience function to insert a set of nodes in an ir.Value(s).

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.85%. Comparing base (d794fca) to head (2e69b61).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/onnx_ir/_convenience/__init__.py	90.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
+ Coverage   76.74%   76.85%   +0.11%     
==========================================
  Files          40       40              
  Lines        4816     4852      +36     
  Branches      952      961       +9     
==========================================
+ Hits         3696     3729      +33     
- Misses        827      829       +2     
- Partials      293      294       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[justinchuby]

Could you share a use case for this function? I am trying to think what the best api for it should be. Right now the order of appearance of the node inputs implicitly matches the values, which may not be ideal. Also the function name may be improved to be more accurate and succinct.

If we generalize this method, what should an “insert_nodes” api do?

[Johansmm]

I am thinking in some case that is needed to insert new nodes without remove previous ones. Following: microsoft/onnxscript#2064 discussion, this is not possible to do this with onnxscript.rewriter, since rewrite_pattern have to redefine target_pattern.

Moreover I believe with this helper it is easer to include new nodes with something like

for node in ir_model.graph:
    if node.op_type == 'expected_op_type':
        insert_node(node.outputs[0], [new_node])

I found this ease to understand/use.

[Johansmm]

For your other comment (about order), do you have any suggestion ? When len(values) > 1, I though was a good idea to infer the inputs for the list of nodes keeping the order, being needed to change the position of nodes in the list if values need to be re-order.

Maybe it would be enough if it is better specified in the use of the function, right?

[justinchuby]

I wonder if something like this would make sense:

def insert_nodes(values, nodes, incoming_values, outgoing_values)

or if the new nodes are connected linearly

def insert_nodes(values, nodes)

(we can constrain that the inputs only be the first node, and the outputs only be produced by the last node so that the intention is clear)

[Johansmm]

I wonder if something like this would make sense:

def insert_nodes(values, nodes, incoming_values, outgoing_values)

or if the new nodes are connected linearly

def insert_nodes(values, nodes)

(we can constrain that the inputs only be the first node, and the outputs only be produced by the last node so that the intention is clear)

@justinchuby I was thinking about this suggestion. I think I understand your point, but at the same time I believe it is unnecessary to pass both information (values and nodes).

Therefore, how about changing the definition of the function to:

def insert_nodes(values, incoming_values, outgoing_values):

and then deduce all nodes through something like:

nodes = []
new_nodes = ivalue.consumers()
while end_condition(new_nodes, outgoing_values): 
    nodes.extend(new_nodes)
    new_nodes = new_nodes_next_step(new_nodes)

Please let me know what do you think.

[justinchuby]

Thanks for the reminder - I will need to think about this more carefully once I am done with the thing I am currently working on. I will get back to you hopefully by the end of this week? Your comment makes sense. Are there special consideration needed to ensure node order? (topologically sorted and deterministic). Also we need to ensure there is no loops and the values fully encloses the subgraph?

[Johansmm]

Thanks for the reminder - I will need to think about this more carefully once I am done with the thing I am currently working on. I will get back to you hopefully by the end of this week? Your comment makes sense. Are there special consideration needed to ensure node order? (topologically sorted and deterministic). Also we need to ensure there is no loops and the values fully encloses the subgraph?

Sorry @justinchuby I forget to answer you:

1. I do not think so. To deduce the nodes through incoming_values and outgoing_values It should be enough to go from incoming_values in the graph to outgoing_values, right? in this process the nodes would be added in a topological way.

In the same way when there are multiple incoming/outgoing, I think traversing the graph to deduce the nodes would force to sort them indirectly.
2. For the loops yes, we should verify there are no enclosures. I believe this should not be a problem.

[Johansmm]

Hi @justinchuby, I'm back with this idea (sorry for the delay). I'd like to know how to rethink the tool to make this possible.

I just did a push force to rebase in last main and removing graph.sort().

[justinchuby]

My apology - This is a very useful feature which we want, but I am not sure when again I will have time to look into it due to my other priorities. I will try to find time when I can. My high level comment is that we would like (1) simple apis (2) efficient runtime and (3) fault tolerant behavior (can raise error if needed, but we should avoid silent errors). Graph traversal would be fine, as long as it is robust