talipot-python: Rework exception management in talipot python plugins

When a talipot plugin written in Python is invoked from the classical Python interpreter and not from the talipot GUI, prefer to let exception that might be raised by its implementation continue its way instead of caughting it. This behavior is much more pythonic than simply printing the error traceback and clearing the python error indicator as client code calling the plugin can catch the errors it might raise.
anlambert · Nov 23, 2023 · 1f1344a · 1f1344a
1 parent 17dd9ea
commit 1f1344a
Show file tree

Hide file tree

Showing 8 changed files with 98 additions and 27 deletions.
diff --git a/library/talipot-core/src/Graph.cpp b/library/talipot-core/src/Graph.cpp
@@ -353,13 +353,14 @@ Graph *tlp::importGraph(const std::string &format, DataSet &parameters, PluginPr
   setlocale(LC_NUMERIC, "C");
 
   // If the import failed and we created the graph then delete the graph
+  string pluginProgLang = newImportModule->programmingLanguage();
   if (!newImportModule->importGraph()) {
     if (newGraphP) {
       delete graph;
     }
     graph = nullptr;
     string errMsg = tmpProgress->getError();
-    if (!errMsg.empty()) {
+    if (!errMsg.empty() && !(pluginProgLang == "Python" && deletePluginProgress)) {
       tlp::error() << "[" << format << "] " << errMsg << std::endl;
     }
   } else {
@@ -411,11 +412,13 @@ bool tlp::exportGraph(Graph *graph, std::ostream &outputStream, const std::strin
     graph->setAttribute("file", filename);
   }
 
+  string pluginProgLang = newExportModule->programmingLanguage();
+
   result = newExportModule->exportGraph(outputStream);
 
   if (!result) {
     string errMsg = tmpProgress->getError();
-    if (!errMsg.empty()) {
+    if (!errMsg.empty() && !(pluginProgLang == "Python" && deletePluginProgress)) {
       tlp::error() << "[" << format << "] " << errMsg << std::endl;
     }
   }

diff --git a/library/talipot-python/bindings/talipot-core/Algorithm.sip b/library/talipot-python/bindings/talipot-core/Algorithm.sip
@@ -50,7 +50,7 @@ public:
 
   virtual ~Algorithm();
 
-  virtual bool run() = 0;
+  virtual bool run() = 0 /VirtualErrorHandler=pass_through/;
 %Docstring
 tlp.Algorithm.run()
 

diff --git a/library/talipot-python/bindings/talipot-core/ExportModule.sip b/library/talipot-python/bindings/talipot-core/ExportModule.sip
@@ -87,7 +87,7 @@ saves to.
 
   virtual std::string category() const;
 
-  virtual bool exportGraph(std::ostream &os) = 0;
+  virtual bool exportGraph(std::ostream &os) = 0 /VirtualErrorHandler=pass_through/;
 %Docstring
 tlp.ExportModule.exportGraph(os)
 

diff --git a/library/talipot-python/bindings/talipot-core/Graph.sip b/library/talipot-python/bindings/talipot-core/Graph.sip
@@ -84,6 +84,10 @@ bool callGraphPropertyAlgorithm(tlp::Graph *graph, const std::string &algoName,
     msg += ".";
     PyErr_SetString(PyExc_Exception, msg.c_str());
   }
+  if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+    sipIsErr = -1;
+  }
   return ret;
 }
 
@@ -357,6 +361,10 @@ if (pluginExists<tlp::Algorithm>(*a0)) {
   msg += ".";
   PyErr_SetString(PyExc_Exception, msg.c_str());
 }
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
 %End
 
 //===========================================================================================
@@ -5139,8 +5147,13 @@ Returns the provided graph or :const:`None` if the import fails.
 :rtype:
    :class:`tlp.Graph`
 %End
+
 %MethodCode
 sipRes = tlp::loadGraph(*a0, nullptr, a1);
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
 %End
 
 //===========================================================================================
@@ -5177,6 +5190,14 @@ Returns a new graph or :const:`None` if the import fails.
    :class:`tlp.Graph`
 %End
 
+%MethodCode
+sipRes = tlp::loadGraph(*a0);
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
+%End
+
 //===========================================================================================
 
 bool saveGraph(tlp::Graph *graph, const std::string &filename);
@@ -5212,6 +5233,14 @@ Returns whether the export was successful or not.
    boolean
 %End
 
+%MethodCode
+sipRes = tlp::saveGraph(a0, *a1);
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
+%End
+
 //===========================================================================================
 
 bool saveGraph(tlp::Graph *graph);
@@ -5250,6 +5279,10 @@ if (ok) {
   msg += "\".";
   PyErr_SetString(PyExc_Exception, msg.c_str());
 }
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
 %End
 
 //===========================================================================================
@@ -5267,6 +5300,10 @@ if (pluginExists<tlp::ImportModule>(*a0)) {
   msg += ".";
   PyErr_SetString(PyExc_Exception, msg.c_str());
 }
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
 %End
 
 tlp::Graph *importGraph(const std::string &importPluginName, tlp::DataSet dataSet /GetWrapper/ ) /TransferBack/;
@@ -5318,6 +5355,10 @@ if (pluginExists<tlp::ImportModule>(*a0)) {
   msg += ".";
   PyErr_SetString(PyExc_Exception, msg.c_str());
 }
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
 %End
 
 //===========================================================================================
@@ -5373,6 +5414,10 @@ if (pluginExists<tlp::ImportModule>(*a0)) {
   msg += ".";
   PyErr_SetString(PyExc_Exception, msg.c_str());
 }
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
 %End
 
 //===========================================================================================
@@ -5441,6 +5486,10 @@ if (pluginExists<tlp::ExportModule>(*a0)) {
   msg += ".";
   PyErr_SetString(PyExc_Exception, msg.c_str());
 }
+if (PyErr_Occurred()) {
+    // propagate exceptions raised by talipot plugins written in Python
+  sipIsErr = -1;
+}
 %End
 
 //===========================================================================================

diff --git a/library/talipot-python/bindings/talipot-core/ImportModule.sip b/library/talipot-python/bindings/talipot-core/ImportModule.sip
@@ -87,7 +87,7 @@ This method returns all the extensions (normal and gzipped) of file formats this
    list of string
 %End
 
-  virtual bool importGraph() = 0;
+  virtual bool importGraph() = 0 /VirtualErrorHandler=pass_through/;
 %Docstring
 tlp.ImportModule.importGraph()
 

diff --git a/library/talipot-python/bindings/talipot-core/talipot.sip b/library/talipot-python/bindings/talipot-core/talipot.sip
@@ -32,6 +32,12 @@
   PyRun_SimpleString("import sys;globals()['talipot'] = sys.modules['talipot']");
 %End
 
+%VirtualErrorHandler pass_through
+// pass through virtual error handler used to not clear Python error indicator
+// in some Python implementation of C++ virtual methods
+%End
+
+
 %ModuleHeaderCode
 #include <talipot/Graph.h>
 #include <talipot/PluginsManager.h>

diff --git a/library/talipot-python/modules/talipotplugins.py b/library/talipot-python/modules/talipotplugins.py
@@ -85,15 +85,12 @@ def runPlugin(plugin):
     except Exception:
         if plugin.pluginProgress:
             plugin.pluginProgress.setError(traceback.format_exc())
-        # Case where the plugin execution has not been launched through the
-        # Talipot GUI, so print the stack trace to stderr
         from talipot import tlp
 
         if type(plugin.pluginProgress) == tlp.SimplePluginProgress:
-            sys.stdout.write(('There was an error when running Python plugin '
-                              'named "%s". See stack trace below.\n') %
-                             plugin.name())
-            traceback.print_exc()
+            # case where the plugin execution has not been launched through the
+            # Talipot GUI, raise exception again in that case
+            raise
     return ret
 
 
@@ -104,15 +101,12 @@ def importGraph(plugin):
     except Exception:
         if plugin.pluginProgress:
             plugin.pluginProgress.setError(traceback.format_exc())
-        # Case where the plugin execution has not been launched through
-        # the Talipot GUI, so print the stack trace to stderr
         from talipot import tlp
 
         if type(plugin.pluginProgress) == tlp.SimplePluginProgress:
-            sys.stdout.write(('There was an error when running Python plugin '
-                              'named "%s". See stack trace below.\n') %
-                             plugin.name())
-            traceback.print_exc()
+            # case where the plugin execution has not been launched through the
+            # Talipot GUI, raise exception again in that case
+            raise
     return ret
 
 
@@ -123,15 +117,12 @@ def exportGraph(plugin, os):
     except Exception:
         if plugin.pluginProgress:
             plugin.pluginProgress.setError(traceback.format_exc())
-        # Case where the plugin execution has not been launched through
-        # the Talipot GUI, so print the stack trace to stderr
         from talipot import tlp
 
         if type(plugin.pluginProgress) == tlp.SimplePluginProgress:
-            sys.stdout.write(('There was an error when running Python plugin '
-                              'named "%s". See stack trace below.\n') %
-                             plugin.name())
-            traceback.print_exc()
+            # case where the plugin execution has not been launched through the
+            # Talipot GUI, raise exception again in that case
+            raise
     return ret
 
 

diff --git a/tests/python/test_tgf_import.py b/tests/python/test_tgf_import.py
@@ -109,8 +109,19 @@ def test_load_tgf_graph_invalid_edge(self):
                         """
                     )
                 )
-            graph = tlp.importGraph(self.tgfPluginName, {"filename": tgfFile})
-            self.assertTrue(graph is None)
+            with self.assertRaises(ValueError) as cm:
+                tlp.importGraph(self.tgfPluginName, {"filename": tgfFile})
+            self.assertEqual(
+                cm.exception.args[0],
+                "Invalid edge definition: 4"
+            )
+
+            with self.assertRaises(ValueError) as cm:
+                tlp.loadGraph(tgfFile)
+            self.assertEqual(
+                cm.exception.args[0],
+                "Invalid edge definition: 4"
+            )
 
     def test_load_tgf_graph_invalid_node_id(self):
         with tempfile.TemporaryDirectory() as tgfDir:
@@ -128,5 +139,16 @@ def test_load_tgf_graph_invalid_node_id(self):
                         """
                     )
                 )
-            graph = tlp.importGraph(self.tgfPluginName, {"filename": tgfFile})
-            self.assertTrue(graph is None)
+            with self.assertRaises(ValueError) as cm:
+                tlp.importGraph(self.tgfPluginName, {"filename": tgfFile})
+            self.assertEqual(
+                cm.exception.args[0],
+                "Invalid node identifier found when parsing edges: 4"
+            )
+
+            with self.assertRaises(ValueError) as cm:
+                tlp.loadGraph(tgfFile)
+            self.assertEqual(
+                cm.exception.args[0],
+                "Invalid node identifier found when parsing edges: 4"
+            )