Merge pull request #2947 from alicevision/fix/reloadPlugins

Correctly catch syntax errors in node descriptions when reloading plugins

Author: cbentejac

meshroom/core/plugins.py

@@ -476,7 +476,12 @@ def reload(self) -> bool:
                          f"at {self.path} has not been modified since the last load.")
             return False
 
-        updated = importlib.reload(sys.modules.get(self.nodeDescriptor.__module__))
+        try:
+            updated = importlib.reload(sys.modules.get(self.nodeDescriptor.__module__))
+        except Exception as exc:
+            logging.error(f"[Reload] {self.nodeDescriptor.__name__}: {exc} ({type(exc).__name__})")
+            self.status = NodePluginStatus.DESC_ERROR
+            return False
         descriptor = getattr(updated, self.nodeDescriptor.__name__)
 
         if not descriptor:

meshroom/ui/reconstruction.py

@@ -19,6 +19,7 @@
 from meshroom.core.node import Node, CompatibilityNode, Status, Position, CompatibilityIssue
 from meshroom.core.taskManager import TaskManager
 from meshroom.core.evaluation import MathEvaluator
+from meshroom.core.plugins import NodePluginStatus
 
 from meshroom.ui import commands
 from meshroom.ui.graph import UIGraph
@@ -445,17 +446,25 @@ def _reloadPlugins(self):
         The nodes in the graph will be updated to match the changes in the description, if
         there was any.
         """
-        nodeTypes: list[str] = []
+        reloadedNodes: list[str] = []
+        errorNodes: list[str] = []
         for plugin in meshroom.core.pluginManager.getPlugins().values():
             for node in plugin.nodes.values():
                 if node.reload():
-                    nodeTypes.append(node.nodeDescriptor.__name__)
-        self.pluginsReloaded.emit(nodeTypes)
+                    reloadedNodes.append(node.nodeDescriptor.__name__)
+                else:
+                    if node.status == NodePluginStatus.DESC_ERROR or node.status == NodePluginStatus.ERROR:
+                        errorNodes.append(node.nodeDescriptor.__name__)
+
+        self.pluginsReloaded.emit(reloadedNodes, errorNodes)
 
     @Slot(list)
-    def _onPluginsReloaded(self, nodeTypes: list):
-        self._graph.reloadNodePlugins(nodeTypes)
-        self.parent().showMessage("Plugins reloaded!", "ok")
+    def _onPluginsReloaded(self, reloadedNodes: list, errorNodes: list):
+        self._graph.reloadNodePlugins(reloadedNodes)
+        if len(errorNodes) > 0:
+            self.parent().showMessage(f"Some plugins failed to reload: {', '.join(errorNodes)}", "error")
+        else:
+            self.parent().showMessage("Plugins reloaded!", "ok")
 
     @Slot(result=bool)
     @Slot(str, result=bool)
@@ -957,7 +966,7 @@ def setBuildingIntrinsics(self, value):
     displayedAttrs3DChanged = Signal()
     displayedAttrs3D = Property(QObject, lambda self: self._displayedAttrs3D, notify=displayedAttrs3DChanged)
 
-    pluginsReloaded = Signal(list)
+    pluginsReloaded = Signal(list, list)
 
     @Slot(QObject)
     def setActiveNode(self, node, categories=True, inputs=True):

tests/test_plugins.py

@@ -158,7 +158,7 @@ def test_loadedPlugin(self):
         assert name == "sharedTemplate"
         assert plugin.templates[name] == os.path.join(str(plugin.path), "sharedTemplate.mg")
 
-    def test_reloadNodePlugin(self):
+    def test_reloadNodePluginInvalidDescrpition(self):
         plugin = pluginManager.getPlugin("pluginB")
         assert plugin == self.plugin
         node = plugin.nodes["PluginBNodeB"]
@@ -217,6 +217,39 @@ def test_reloadNodePlugin(self):
         assert node.status == NodePluginStatus.DESC_ERROR  # Not NOT_LOADED
         assert not pluginManager.isRegistered(nodeName)
 
+    def test_reloadNodePluginSyntaxError(self):
+        plugin = pluginManager.getPlugin("pluginB")
+        assert plugin == self.plugin
+        node = plugin.nodes["PluginBNodeA"]
+        nodeName = node.nodeDescriptor.__name__
+
+        # Check that the node has been registered
+        assert node.status == NodePluginStatus.LOADED
+        assert pluginManager.isRegistered(nodeName)
+
+        # Introduce a syntax error in the description
+        originalFileContent = None
+        with open(node.path, "r") as f:
+            originalFileContent = f.read()
+
+        replaceFileContent = originalFileContent.replace('name="input",', 'name="input"')
+        with open(node.path, "w") as f:
+            f.write(replaceFileContent)
+
+        # Reload the node and assert it is invalid but still registered
+        node.reload()
+        assert node.status == NodePluginStatus.DESC_ERROR
+        assert pluginManager.isRegistered(nodeName)
+
+        # Restore the node file to its original state (with a description error)
+        with open(node.path, "w") as f:
+            f.write(originalFileContent)
+
+        # Assert the status is correct and the node is still registered
+        node.reload()
+        assert node.status == NodePluginStatus.NOT_LOADED
+        assert pluginManager.isRegistered(nodeName)
+
 
 class TestPluginsConfiguration:
     CONFIG_PATH = ("CONFIG_PATH", "sharedTemplate.mg", "config.json")