Node Renaming Causes Graph Corruption

Required prerequisites

• Make sure you've read the documentation. Your issue may be addressed there.
• Search the issue tracker and discussions to verify that this hasn't already been reported. +1 or comment there if it has.

What commit version of aidge do you use

• aidge_core: 0.4.0
• aidge_onnx:0.3.0

Problem description

My Graph consists of two ONNX models connected by a generic operation. I use the aidge_onnx.export_onnx function, but an error occurs when renaming nodes that have duplicate names.

Warning: node name "Gather" is a duplicate, renaming to Gather_2.
Warning: node name "onnx__Conv_868" is a duplicate, renaming to onnx__Conv_868_1.
Warning: node name "onnx__Conv_871" is a duplicate, renaming to onnx__Conv_871_1.
Warning: node name "onnx__Conv_874" is a duplicate, renaming to onnx__Conv_874_1.
Warning: node name "onnx__Conv_877" is a duplicate, renaming to onnx__Conv_877_1.
Warning: node name "onnx__Conv_880" is a duplicate, renaming to onnx__Conv_880_1.
Warning: node name "onnx__Conv_883" is a duplicate, renaming to onnx__Conv_883_1.
Warning: node name "onnx__Conv_886" is a duplicate, renaming to onnx__Conv_886_1.
Warning: node name "onnx__Conv_889" is a duplicate, renaming to onnx__Conv_889_1.
Warning: node name "Constant" is a duplicate, renaming to Constant_20.
Warning: node name "Constant_1" is a duplicate, renaming to Constant_1_1.
Warning: node name "Constant_2" is a duplicate, renaming to Constant_2_1.
Warning: node name "Unsqueeze_output_0" is a duplicate, renaming to Unsqueeze_output_0_1.
Warning: node name "Constant_3" is a duplicate, renaming to Constant_3_1.
Warning: node name "Constant_4" is a duplicate, renaming to Constant_4_1.
Warning: node name "Constant_5" is a duplicate, renaming to Constant_5_1.
Warning: node name "Unsqueeze_1_output_0" is a duplicate, renaming to Unsqueeze_1_output_0_1.
Warning: node name "Unsqueeze_2_output_0" is a duplicate, renaming to Unsqueeze_2_output_0_1.
Warning: node name "Concat" is a duplicate, renaming to Concat_4.
Warning: node name "Reshape" is a duplicate, renaming to Reshape_2.
Warning: node name "Transpose" is a duplicate, renaming to Transpose_1.
Warning: node name "Gather_1" is a duplicate, renaming to Gather_1_1.
Warning: node name "Unsqueeze_3_output_0" is a duplicate, renaming to Unsqueeze_3_output_0_1.
Exporting ...
[ERROR] - Assertion failed: it != mNodeRegistry.end() in
[ERROR]   aidge/aidge_core/src/graph/GraphView.cpp:972
[FATAL] - No node named Gather_2 in graph , the graph may be corrupted !
Traceback (most recent call last):
  File "/ssdmv7_ONNX_export.py", line 217, in <module>
    aidge_onnx.export_onnx(
  File "/env/lib/python3.10/site-packages/aidge_onnx/onnx_export.py", line 100, in export_onnx
    remove_duplicate_names(graph_view)
  File "/env/lib/python3.10/site-packages/aidge_onnx/onnx_export.py", line 53, in remove_duplicate_names
    node.set_name(new_name)
RuntimeError: No node named Gather_2 in graph , the graph may be corrupted !

I added custom logging inside onnx_export.py in the remove_duplicate_names function:

for name, node_list in name_map.items():
    if len(node_list) > 1:
        # We need another recursive call to check current modifications doesn't invalidate the graph
        print(node_list)
        name_updated = True
        for idx, node in enumerate(node_list):
            new_name = f"{name}_{idx}"
            print(f"Renaming '{name}' -> '{new_name}'")
            node.set_name(new_name)

Output :

[Node(name='Gather_2', optype='Gather', parents: [1, 1], children: [[1]]), Node(name='Gather_2', optype='Gather', parents: [1, 1], children: [[1, 1, 1]])]
Renaming 'Gather_2' -> 'Gather_2_0'
Renaming 'Gather_2' -> 'Gather_2_1'

• Does renaming create a mismatch in the GraphView?
• Does it come from the lack of a function that checks whether the modifications invalidate the graph, as suggested in the comment?

added Bug 🐛 Help Wanted LanguagePython OSLinux labels

assigned to @olivierbichler and @pineapple

The chain of events seems clear from your inputs: you have two nodes named Gather_2 in your graph! So when you try to rename one, both are actually renamed, that is why for the second one you get the error.

To ensure that the first node is not renamed unnecessarily, shouldn't we modify the function as follows:

    for name, node_list in name_map.items():
        if len(node_list) > 1:
            # We need another recursive call to check current modifications doesn't invalidate the graph
            name_updated = True
            for idx, node in enumerate(node_list[1:]):
                node.set_name(name + f"_{idx}")

closed

reopened

After closer examination with @nthm, there is still an issue: when a GraphView is added to another GraphView, the nodes name registry is not updated, leading to the following assertion being triggered:

AIDGE_ASSERT(it != mNodeRegistry.end(), "No node named {} in graph {}, the graph may be corrupted !", node->name(), name());

This code block is supposed to do the check: https://gitlab.eclipse.org/eclipse/aidge/aidge_core/-/blob/main/src/graph/GraphView.cpp?ref_type=heads#L810-818

Yes, I does the check, but does not add the added node name into the registry.

mentioned in commit aidge_core@611131eb

@olivierbichler I am currently working on an experimental branch that contains a lot of branch that are waiting to be merged.

I have made a commit aidge_core@611131eb to fix the issue described by @nthm the problem was that:

• 1 Check if the name is unique in the graph to add
• 2 If not create a unique name in the graph current graph
• 3 update the name

The problem is that there was no check between step 2 and 3 if the new name was unique in the graph to add causing the issue.

@bhalimi Tagging you since we talked about it ^^

mentioned in commit aidge_core@e8178478

mentioned in merge request aidge_core!348 (merged)

mentioned in commit aidge_core@515535eb

mentioned in commit aidge_core@76ccc349

closed