Merge pull request #2598 from alicevision/fix/attributeCallbackOnGraphLoad

Discard attribute changed callbacks during graph loading

Author: cbentejac

meshroom/core/exception.py

@@ -12,6 +12,21 @@ class GraphException(MeshroomException):
     pass
 
 
+class GraphCompatibilityError(GraphException):
+    """
+    Raised when node compatibility issues occur when loading a graph.
+    
+    Args:
+        filepath: The path to the file that caused the error.
+        issues: A dictionnary of node names and their respective compatibility issues.
+    """
+    def __init__(self, filepath, issues: dict[str, str]) -> None:
+        self.filepath = filepath
+        self.issues = issues
+        msg = f"Compatibility issues found when loading {self.filepath}: {self.issues}"
+        super().__init__(msg)
+
+
 class UnknownNodeTypeError(GraphException):
     """
     Raised when asked to create a unknown node type.

meshroom/core/graph.py

@@ -15,7 +15,7 @@
 from meshroom.common import BaseObject, DictModel, Slot, Signal, Property
 from meshroom.core import Version
 from meshroom.core.attribute import Attribute, ListAttribute, GroupAttribute
-from meshroom.core.exception import StopGraphVisit, StopBranchVisit
+from meshroom.core.exception import GraphCompatibilityError, StopGraphVisit, StopBranchVisit
 from meshroom.core.node import nodeFactory, Status, Node, CompatibilityNode
 
 # Replace default encoder to support Enums
@@ -214,6 +214,7 @@ def getFeaturesForVersion(fileVersion):
     def __init__(self, name, parent=None):
         super(Graph, self).__init__(parent)
         self.name = name
+        self._loading = False
         self._updateEnabled = True
         self._updateRequested = False
         self.dirtyTopology = False
@@ -246,6 +247,11 @@ def fileFeatures(self):
         """ Get loaded file supported features based on its version. """
         return Graph.IO.getFeaturesForVersion(self.header.get(Graph.IO.Keys.FileVersion, "0.0"))
 
+    @property
+    def isLoading(self):
+        """ Return True if the graph is currently being loaded. """
+        return self._loading
+
     @Slot(str)
     def load(self, filepath, setupProjectFile=True, importProject=False, publishOutputs=False):
         """
@@ -259,6 +265,13 @@ def load(self, filepath, setupProjectFile=True, importProject=False, publishOutp
                            of opened.
             publishOutputs: True if "Publish" nodes from templates should not be ignored.
         """
+        self._loading = True
+        try:
+            self._load(filepath, setupProjectFile, importProject, publishOutputs)
+        finally:
+            self._loading = False
+
+    def _load(self, filepath, setupProjectFile, importProject, publishOutputs):
         if not importProject:
             self.clear()
         with open(filepath) as jsonFile:
@@ -1633,11 +1646,27 @@ def setVerbose(self, v):
     canComputeLeaves = Property(bool, lambda self: self._canComputeLeaves, notify=canComputeLeavesChanged)
 
 
-def loadGraph(filepath):
+def loadGraph(filepath, strictCompatibility: bool = False) -> Graph:
     """
+    Load a Graph from a Meshroom Graph (.mg) file.
+
+    Args:
+        filepath: The path to the Meshroom Graph file.
+        strictCompatibility: If True, raise a GraphCompatibilityError if the loaded Graph has node compatibility issues.
+
+    Returns:
+        Graph: The loaded Graph instance.
+
+    Raises:
+        GraphCompatibilityError: If the Graph has node compatibility issues and `strictCompatibility` is True.
     """
     graph = Graph("")
     graph.load(filepath)
+
+    compatibilityIssues = len(graph.compatibilityNodes) > 0
+    if compatibilityIssues and strictCompatibility:
+        raise GraphCompatibilityError(filepath, {n.name: str(n.issue) for n in graph.compatibilityNodes})
+
     graph.update()
     return graph
 

meshroom/core/node.py

@@ -964,6 +964,10 @@ def _onAttributeChanged(self, attr: Attribute):
         if attr.value is None:
             # Discard dynamic values depending on the graph processing.
             return
+        
+        if self.graph and self.graph.isLoading:
+            # Do not trigger attribute callbacks during the graph loading.
+            return
 
         callback = self._getAttributeChangedCallback(attr)
 

tests/test_compatibility.py

@@ -8,7 +8,7 @@
 
 import meshroom.core
 from meshroom.core import desc, registerNodeType, unregisterNodeType
-from meshroom.core.exception import NodeUpgradeError
+from meshroom.core.exception import GraphCompatibilityError, NodeUpgradeError
 from meshroom.core.graph import Graph, loadGraph
 from meshroom.core.node import CompatibilityNode, CompatibilityIssue, Node
 
@@ -395,3 +395,24 @@ def test_conformUpgrade():
 
     unregisterNodeType(SampleNodeV5)
     unregisterNodeType(SampleNodeV6)
+
+
+class TestGraphLoadingWithStrictCompatibility:
+
+    def test_failsOnNodeDescriptionCompatibilityIssue(self, graphSavedOnDisk):
+        registerNodeType(SampleNodeV1)
+        registerNodeType(SampleNodeV2)
+
+        graph: Graph = graphSavedOnDisk
+        graph.addNewNode(SampleNodeV1.__name__)
+        graph.save()
+
+        # Replace saved node description by V2
+        meshroom.core.nodesDesc[SampleNodeV1.__name__] = SampleNodeV2
+
+        with pytest.raises(GraphCompatibilityError):
+            loadGraph(graph.filepath, strictCompatibility=True)
+
+        unregisterNodeType(SampleNodeV1)
+        unregisterNodeType(SampleNodeV2)
+

tests/test_nodeAttributeChangedCallback.py

@@ -156,7 +156,7 @@ def test_loadingGraphDoesNotTriggerCallback(self, graphSavedOnDisk):
         node.affectedInput.value = 2
         graph.save()
 
-        loadedGraph = loadGraph(graph.filepath)
+        loadedGraph = loadGraph(graph.filepath, strictCompatibility=True)
         loadedNode = loadedGraph.node(node.name)
         assert loadedNode
         assert loadedNode.affectedInput.value == 2
@@ -170,11 +170,13 @@ def test_loadingGraphDoesNotTriggerCallbackForConnectedAttributes(
 
         graph.addEdge(nodeA.input, nodeB.input)
         nodeA.input.value = 5
+        assert nodeB.affectedInput.value == nodeB.input.value * 2
+
         nodeB.affectedInput.value = 2
 
         graph.save()
 
-        loadedGraph = loadGraph(graph.filepath)
+        loadedGraph = loadGraph(graph.filepath, strictCompatibility=True)
         loadedNodeB = loadedGraph.node(nodeB.name)
         assert loadedNodeB
         assert loadedNodeB.affectedInput.value == 2
@@ -407,3 +409,25 @@ def test_clearingDynamicOutputValueDoesNotTriggerDownstreamAttributeChangedCallb
         nodeA.clearData()
         assert nodeA.output.value == nodeB.input.value is None
         assert nodeB.affectedInput.value == expectedPreClearValue
+
+    def test_loadingGraphWithComputedDynamicOutputValueDoesNotTriggerDownstreamAttributeChangedCallback(
+        self, graphSavedOnDisk
+    ):
+        graph: Graph = graphSavedOnDisk
+        nodeA = graph.addNewNode(NodeWithDynamicOutputValue.__name__)
+        nodeB = graph.addNewNode(NodeWithAttributeChangedCallback.__name__)
+
+        nodeA.input.value = 10
+        graph.addEdge(nodeA.output, nodeB.input)
+        executeGraph(graph)
+
+        assert nodeA.output.value == nodeB.input.value == 20
+        assert nodeB.affectedInput.value == 0
+
+        graph.save()
+
+        loadGraph(graph.filepath, strictCompatibility=True)
+
+        assert nodeB.affectedInput.value == 0
+
+