From 1f1344a9aff407c96a80aa1489ac2ff1e1639be6 Mon Sep 17 00:00:00 2001 From: Antoine Lambert Date: Thu, 23 Nov 2023 01:14:03 +0100 Subject: [PATCH] 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. --- library/talipot-core/src/Graph.cpp | 7 ++- .../bindings/talipot-core/Algorithm.sip | 2 +- .../bindings/talipot-core/ExportModule.sip | 2 +- .../bindings/talipot-core/Graph.sip | 49 +++++++++++++++++++ .../bindings/talipot-core/ImportModule.sip | 2 +- .../bindings/talipot-core/talipot.sip | 6 +++ .../talipot-python/modules/talipotplugins.py | 27 ++++------ tests/python/test_tgf_import.py | 30 ++++++++++-- 8 files changed, 98 insertions(+), 27 deletions(-) diff --git a/library/talipot-core/src/Graph.cpp b/library/talipot-core/src/Graph.cpp index ec6a850752..17d70783ff 100644 --- 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 ¶meters, 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 index 9f95c8c75f..219117701d 100644 --- 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 index e8499bea18..51c7737ab0 100644 --- 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 index 8b7e05babf..860b61b4cf 100644 --- 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(*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(*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(*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(*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(*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 index 8ba7cd9358..0048b653ee 100644 --- 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 index 71252ba342..4f9c7b32c0 100644 --- 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 #include diff --git a/library/talipot-python/modules/talipotplugins.py b/library/talipot-python/modules/talipotplugins.py index 4a28b8a2e6..9701cdb7aa 100644 --- 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 index ebfb39c988..d615a2d181 100644 --- 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" + )