[core] Graph: improved uid conflicts evaluation on deserialization

At the end of the deserialization process, solve node uid conflicts iteratively by node depths,
and only replace the conflicting nodes with a CompatibilityNode.

Add new test suite for testing uid conflict handling.

meshroom/core/graph.py

@@ -4,7 +4,7 @@ import json
 import logging
 import os
 import re
-from typing import Optional
+from typing import Any, Optional
 import weakref
 from collections import defaultdict, OrderedDict
 from contextlib import contextmanager
@@ -18,7 +18,7 @@ from meshroom.core import Version
 from meshroom.core.attribute import Attribute, ListAttribute, GroupAttribute
 from meshroom.core.exception import GraphCompatibilityError, StopGraphVisit, StopBranchVisit
 from meshroom.core.graphIO import GraphIO, GraphSerializer, TemplateGraphSerializer, PartialGraphSerializer
-from meshroom.core.node import Status, Node, CompatibilityNode
+from meshroom.core.node import BaseNode, Status, Node, CompatibilityNode
 from meshroom.core.nodeFactory import nodeFactory
 from meshroom.core.typing import PathLike
 
@@ -291,23 +291,18 @@ class Graph(BaseObject):
             # Create graph edges by resolving attributes expressions
             self._applyExpr()
             
-            # Templates are specific: they contain only the minimal amount of 
+        # Templates are specific: they contain only the minimal amount of 
-            # serialized data to describe the graph structure.
+        # serialized data to describe the graph structure.
-            # They are not meant to be computed: therefore, we can early return here,
+        # They are not meant to be computed: therefore, we can early return here,
-            # as uid conflict evaluation is only meaningful for nodes with computed data.
+        # as uid conflict evaluation is only meaningful for nodes with computed data.
-            if isTemplate:
+        if isTemplate:
-                return
+            return
 
-            # By this point, the graph has been fully loaded and an updateInternals has been triggered, so all the
+        # By this point, the graph has been fully loaded and an updateInternals has been triggered, so all the
-            # nodes' links have been resolved and their UID computations are all complete.
+        # nodes' links have been resolved and their UID computations are all complete.
-            # It is now possible to check whether the UIDs stored in the graph file for each node correspond to the ones
+        # It is now possible to check whether the UIDs stored in the graph file for each node correspond to the ones
-            # that were computed.
+        # that were computed.
-            self.updateInternals()
+        self._evaluateUidConflicts(graphContent)
-            self._evaluateUidConflicts(graphContent)
-            try:
-                self._applyExpr()
-            except Exception as e:
-                logging.warning(e)
 
     def _normalizeGraphContent(self, graphData: dict, fileVersion: Version) -> dict:
         graphContent = graphData.get(GraphIO.Keys.Graph, graphData)
@@ -348,34 +343,48 @@ class Graph(BaseObject):
 
     def _evaluateUidConflicts(self, graphContent: dict):
         """
-        Compare the UIDs of all the nodes in the graph with the UID that is expected in the graph file. If there
+        Compare the computed UIDs of all the nodes in the graph with the UIDs serialized in `graphContent`. If there
         are mismatches, the nodes with the unexpected UID are replaced with "UidConflict" compatibility nodes.
-        Already existing nodes are removed and re-added to the graph identically to preserve all the edges,
+  
-        which may otherwise be invalidated when a node with output edges but a UID conflict is re-generated as a
-        compatibility node.
-
         Args:
-            data (dict): the dictionary containing all the nodes to import and their data
+            graphContent: The serialized Graph content.
         """
-        for nodeName, nodeData in sorted(graphContent.items(), key=lambda x: self.getNodeIndexFromName(x[0])):
-            node = self.node(nodeName)
 
+        def _serializedNodeUidMatchesComputedUid(nodeData: dict, node: BaseNode) -> bool:
+            """Returns whether the serialized UID matches the one computed in the `node` instance."""
+            if isinstance(node, CompatibilityNode):
+                return True
             serializedUid = nodeData.get("uid", None)
-            computedUid = node._uid  # Node's UID from the graph itself
+            computedUid = node._uid
+            return serializedUid is None or computedUid is None or serializedUid == computedUid
 
-            if serializedUid and computedUid and serializedUid != computedUid:
+        uidConflictingNodes = [
-                # Different UIDs, remove the existing node from the graph and replace it with a CompatibilityNode
+            node
-                logging.debug("UID conflict detected for {}".format(nodeName))
+            for node in self.nodes
-                self.removeNode(nodeName)
+            if not _serializedNodeUidMatchesComputedUid(graphContent[node.name], node)
-                n = nodeFactory(nodeData, nodeName, expectedUid=computedUid)
+        ]
-                self._addNode(n, nodeName)
+
-            else:
+        if not uidConflictingNodes:
-                # f connecting nodes have UID conflicts and are removed/re-added to the graph, some edges may be lost:
+            return
-                # the links will be erroneously updated, and any further resolution will fail.
+
-                # Recreating the entire graph as it was ensures that all edges will be correctly preserved.
+        logging.warning("UID Compatibility issues found: recreating conflicting nodes as CompatibilityNodes.")
-                self.removeNode(nodeName)
+
-                n = nodeFactory(nodeData, nodeName)
+        # A uid conflict is contagious: if a node has a uid conflict, all of its downstream nodes may be 
-                self._addNode(n, nodeName)
+        # impacted as well, as the uid flows through connections.
+        # Therefore, we deal with conflicting uid nodes by depth: replacing a node with a CompatibilityNode restores
+        # the serialized uid, which might solve "false-positives" downstream conflicts as well.
+        nodesSortedByDepth = sorted(uidConflictingNodes, key=lambda node: node.minDepth)
+        for node in nodesSortedByDepth:
+            nodeData = graphContent[node.name]
+            # Evaluate if the node uid is still conflicting at this point, or if it has been resolved by an
+            # upstream node replacement.
+            if _serializedNodeUidMatchesComputedUid(nodeData, node):
+                continue
+            expectedUid = node._uid
+            compatibilityNode = nodeFactory(graphContent[node.name], node.name, expectedUid=expectedUid)
+            # This operation will trigger a graph update that will recompute the uids of all nodes,
+            # allowing the iterative resolution of uid conflicts.
+            self.replaceNode(node.name, compatibilityNode)
 
 
     def importGraphContentFromFile(self, filepath: PathLike) -> list[Node]:

tests/test_compatibility.py

@@ -472,3 +472,155 @@ class TestGraphTemplateLoading:
             replaceNodeTypeDesc(SampleNodeV1.__name__, SampleNodeV2)
 
             loadGraph(graph.filepath, strictCompatibility=True)
+
+
+class UidTestingNodeV1(desc.Node):
+    inputs = [
+        desc.File(name="input", label="Input", description="", value="", invalidate=True),
+    ]
+    outputs = [desc.File(name="output", label="Output", description="", value=desc.Node.internalFolder)]
+
+
+class UidTestingNodeV2(desc.Node):
+    """ 
+    Changes from SampleNodeBV1:
+        * 'param' has been added
+    """
+
+    inputs = [
+        desc.File(name="input", label="Input", description="", value="", invalidate=True),
+        desc.ListAttribute(
+            name="param",
+            label="Param",
+            elementDesc=desc.File(
+                name="file",
+                label="File",
+                description="",
+                value="",
+            ),
+            description="",
+        ),
+    ]
+    outputs = [desc.File(name="output", label="Output", description="", value=desc.Node.internalFolder)]
+
+
+class UidTestingNodeV3(desc.Node):
+    """
+    Changes from SampleNodeBV2:
+        * 'input' is not invalidating the UID.
+    """
+
+    inputs = [
+        desc.File(name="input", label="Input", description="", value="", invalidate=False),
+        desc.ListAttribute(
+            name="param",
+            label="Param",
+            elementDesc=desc.File(
+                name="file",
+                label="File",
+                description="",
+                value="",
+            ),
+            description="",
+        ),
+    ]
+    outputs = [desc.File(name="output", label="Output", description="", value=desc.Node.internalFolder)]
+
+
+class TestUidConflict:
+    def test_changingInvalidateOnAttributeDescCreatesUidConflict(self, graphSavedOnDisk):
+        with registeredNodeTypes([UidTestingNodeV2]):
+            graph: Graph = graphSavedOnDisk
+            node = graph.addNewNode(UidTestingNodeV2.__name__)
+
+            graph.save()
+            replaceNodeTypeDesc(UidTestingNodeV2.__name__, UidTestingNodeV3)
+
+            with pytest.raises(GraphCompatibilityError):
+                loadGraph(graph.filepath, strictCompatibility=True)
+
+            loadedGraph = loadGraph(graph.filepath)
+            loadedNode = loadedGraph.node(node.name)
+            assert isinstance(loadedNode, CompatibilityNode)
+            assert loadedNode.issue == CompatibilityIssue.UidConflict
+
+    def test_uidConflictingNodesPreserveConnectionsOnGraphLoad(self, graphSavedOnDisk):
+        with registeredNodeTypes([UidTestingNodeV2]):
+            graph: Graph = graphSavedOnDisk
+            nodeA = graph.addNewNode(UidTestingNodeV2.__name__)
+            nodeB = graph.addNewNode(UidTestingNodeV2.__name__)
+
+            nodeB.param.append("")
+            graph.addEdge(nodeA.output, nodeB.param.at(0))
+
+            graph.save()
+            replaceNodeTypeDesc(UidTestingNodeV2.__name__, UidTestingNodeV3)
+
+            loadedGraph = loadGraph(graph.filepath)
+            assert len(loadedGraph.compatibilityNodes) == 2
+
+            loadedNodeA = loadedGraph.node(nodeA.name)
+            loadedNodeB = loadedGraph.node(nodeB.name)
+
+            assert loadedNodeB.param.at(0).linkParam == loadedNodeA.output
+
+    def test_upgradingConflictingNodesPreserveConnections(self, graphSavedOnDisk):
+        with registeredNodeTypes([UidTestingNodeV2]):
+            graph: Graph = graphSavedOnDisk
+            nodeA = graph.addNewNode(UidTestingNodeV2.__name__)
+            nodeB = graph.addNewNode(UidTestingNodeV2.__name__)
+
+            # Double-connect nodeA.output to nodeB, on both a single attribute and a list attribute
+            nodeB.param.append("")
+            graph.addEdge(nodeA.output, nodeB.param.at(0))
+            graph.addEdge(nodeA.output, nodeB.input)
+
+            graph.save()
+            replaceNodeTypeDesc(UidTestingNodeV2.__name__, UidTestingNodeV3)
+
+            def checkNodeAConnectionsToNodeB():
+                loadedNodeA = loadedGraph.node(nodeA.name)
+                loadedNodeB = loadedGraph.node(nodeB.name)
+                return (
+                    loadedNodeB.param.at(0).linkParam == loadedNodeA.output
+                    and loadedNodeB.input.linkParam == loadedNodeA.output
+                )
+
+            loadedGraph = loadGraph(graph.filepath)
+            loadedGraph.upgradeNode(nodeA.name)
+
+            assert checkNodeAConnectionsToNodeB()
+            loadedGraph.upgradeNode(nodeB.name)
+
+            assert checkNodeAConnectionsToNodeB()
+            assert len(loadedGraph.compatibilityNodes) == 0
+
+
+    def test_uidConflictDoesNotPropagateToValidDownstreamNodeThroughConnection(self, graphSavedOnDisk):
+        with registeredNodeTypes([UidTestingNodeV1, UidTestingNodeV2]):
+            graph: Graph = graphSavedOnDisk
+            nodeA = graph.addNewNode(UidTestingNodeV2.__name__)
+            nodeB = graph.addNewNode(UidTestingNodeV1.__name__)
+
+            graph.addEdge(nodeA.output, nodeB.input)
+
+            graph.save()
+            replaceNodeTypeDesc(UidTestingNodeV2.__name__, UidTestingNodeV3)
+
+            loadedGraph = loadGraph(graph.filepath)
+            assert len(loadedGraph.compatibilityNodes) == 1
+
+    def test_uidConflictDoesNotPropagateToValidDownstreamNodeThroughListConnection(self, graphSavedOnDisk):
+        with registeredNodeTypes([UidTestingNodeV2, UidTestingNodeV3]):
+            graph: Graph = graphSavedOnDisk
+            nodeA = graph.addNewNode(UidTestingNodeV2.__name__)
+            nodeB = graph.addNewNode(UidTestingNodeV3.__name__)
+
+            nodeB.param.append("")
+            graph.addEdge(nodeA.output, nodeB.param.at(0))
+
+            graph.save()
+            replaceNodeTypeDesc(UidTestingNodeV2.__name__, UidTestingNodeV3)
+
+            loadedGraph = loadGraph(graph.filepath)
+            assert len(loadedGraph.compatibilityNodes) == 1