FIX: Fix python errors not raising in Python

- Remove PyErr_Print to avoid swallowing errors which should raise in
  the Python interpreter
- Early return when Python is currently raising to avoid
  corrupting the traceback
- Add error print for all failing callbacks
- Improve memory management

Author: Thibault-Pelletier

LayerDM/MRMLDM/vtkMRMLLayerDMObjectEventObserverScripted.cxx

@@ -11,13 +11,21 @@ vtkMRMLLayerDMObjectEventObserverScripted::vtkMRMLLayerDMObjectEventObserverScri
   this->SetUpdateCallback(
     [this](vtkObject* node, unsigned long eventId, void* callData) -> void
     {
-      if (!Py_IsInitialized())
+      if (!vtkMRMLLayerDMPythonUtil::IsValidPythonContext())
       {
         return;
       }
 
       vtkPythonScopeGilEnsurer gilEnsurer;
-      vtkMRMLLayerDMPythonUtil::CallPythonObject(this->m_object, vtkMRMLLayerDMPythonUtil::ToPyArgs(node, eventId, callData));
+      if (auto result = vtkMRMLLayerDMPythonUtil::CallPythonObject(this->m_object, vtkMRMLLayerDMPythonUtil::ToPyArgs(node, eventId, callData)))
+      {
+        Py_DECREF(result);
+      }
+      else
+      {
+        auto errorMsg = std::string(__func__) + ": Failed to call : " + vtkMRMLLayerDMPythonUtil::GetObjectStr(this->m_object) + ":";
+        vtkMRMLLayerDMPythonUtil::PrintErrorTraceback(this, errorMsg);
+      }
     });
 }
 

LayerDM/MRMLDM/vtkMRMLLayerDMPipelineScriptedCreator.cxx

@@ -20,7 +20,7 @@ vtkMRMLLayerDMPipelineScriptedCreator::vtkMRMLLayerDMPipelineScriptedCreator()
   this->SetCallback(
     [this](vtkMRMLAbstractViewNode* viewNode, vtkMRMLNode* node) -> vtkSmartPointer<vtkMRMLLayerDMPipelineI>
     {
-      if (!Py_IsInitialized())
+      if (!vtkMRMLLayerDMPythonUtil::IsValidPythonContext())
       {
         return nullptr;
       }
@@ -30,6 +30,8 @@ vtkMRMLLayerDMPipelineScriptedCreator::vtkMRMLLayerDMPipelineScriptedCreator()
         this->m_object, vtkMRMLLayerDMPythonUtil::ToPyArgs({ vtkMRMLLayerDMPythonUtil::ToPyObject(viewNode), vtkMRMLLayerDMPythonUtil::ToPyObject(node) }));
       if (!result)
       {
+        auto errorMsg = std::string(__func__) + ": Failed to call : " + vtkMRMLLayerDMPythonUtil::GetObjectStr(this->m_object) + ":";
+        vtkMRMLLayerDMPythonUtil::PrintErrorTraceback(this, errorMsg);
         return nullptr;
       }
       return vtkMRMLLayerDMPipelineI::SafeDownCast(vtkPythonUtil::GetPointerFromObject(result, "vtkMRMLLayerDMPipelineI"));

LayerDM/MRMLDM/vtkMRMLLayerDMPythonUtil.cxx

@@ -1,4 +1,5 @@
 #include "vtkMRMLLayerDMPythonUtil.h"
+
 #include <vtkObjectFactory.h>
 
 vtkStandardNewMacro(vtkMRMLLayerDMPythonUtil);
@@ -62,7 +63,7 @@ vtkSmartPyObject vtkMRMLLayerDMPythonUtil::ToPyArgs(vtkObject* obj, unsigned lon
 PyObject* vtkMRMLLayerDMPythonUtil::CastCallData(PyObject* object, int vtkType)
 {
   vtkPythonScopeGilEnsurer gilEnsurer;
-  if (!Py_IsInitialized())
+  if (!IsValidPythonContext())
   {
     return nullptr;
   }
@@ -126,16 +127,24 @@ PyObject* vtkMRMLLayerDMPythonUtil::CastCallData(PyObject* object, int vtkType)
 
 PyObject* vtkMRMLLayerDMPythonUtil::CallPythonMethod(PyObject* object, const vtkSmartPyObject& pyArgs, const std::string& fName)
 {
-  if (!Py_IsInitialized() || !object)
+  if (!IsValidPythonContext() || !object)
   {
     return nullptr;
   }
 
+  vtkPythonScopeGilEnsurer gilEnsurer;
   PyObject* method = PyObject_GetAttrString(object, fName.c_str());
-  if (!method || !PyCallable_Check(method))
+  if (!method)
+  {
+    // Don't modify Python's error reporting
+    return nullptr;
+  }
+
+  if (!PyCallable_Check(method))
   {
-    const std::string errorString = "Method is not callable : " + fName;
-    PyErr_SetString(PyExc_ValueError, errorString.c_str());
+    // PyCallable_Check doesn't raise any errors. Raise called attribute isn't callable.
+    const auto errorString = std::string("vtkMRMLLayerDMPythonUtil::") + __func__ + ": Attribute is not callable : '" + fName + "' of object : " + GetObjectStr(object);
+    PyErr_SetString(PyExc_TypeError, errorString.c_str());
     return nullptr;
   }
 
@@ -144,24 +153,26 @@ PyObject* vtkMRMLLayerDMPythonUtil::CallPythonMethod(PyObject* object, const vtk
 
 PyObject* vtkMRMLLayerDMPythonUtil::CallPythonObject(PyObject* object, const vtkSmartPyObject& pyArgs)
 {
-  if (!Py_IsInitialized() || !object || !PyCallable_Check(object))
+  if (!IsValidPythonContext() || !object)
   {
     return nullptr;
   }
 
   vtkPythonScopeGilEnsurer gilEnsurer;
-  PyObject* result = PyObject_CallObject(object, pyArgs);
-  if (!result)
+  if (!PyCallable_Check(object))
   {
-    PyErr_Print();
+    // PyCallable_Check doesn't raise any errors. Raise called attribute isn't callable.
+    const auto errorString = std::string("vtkMRMLLayerDMPythonUtil::") + __func__ + ": Object is not callable : " + GetObjectStr(object);
+    PyErr_SetString(PyExc_TypeError, errorString.c_str());
     return nullptr;
   }
-  return result;
+
+  return PyObject_CallObject(object, pyArgs);
 }
 
 void vtkMRMLLayerDMPythonUtil::SetPythonObject(PyObject** destObject, PyObject* object)
 {
-  if (!Py_IsInitialized())
+  if (!IsValidPythonContext())
   {
     return;
   }
@@ -188,3 +199,99 @@ void vtkMRMLLayerDMPythonUtil::DeletePythonObject(PyObject** destObject)
   Py_XDECREF(*destObject);
   *destObject = nullptr;
 }
+
+std::string vtkMRMLLayerDMPythonUtil::GetObjectStr(PyObject* object)
+{
+  if (!Py_IsInitialized())
+  {
+    return {};
+  }
+
+  if (!object)
+  {
+    return "None";
+  }
+
+  // Save current errors to avoid changing the current python error stack if any
+  PyObject *type, *value, *traceback;
+  PyErr_Fetch(&type, &value, &traceback);
+
+  std::string objectString{ "INVALID_OBJECT_STR" };
+  if (auto strObj = PyObject_Str(object))
+  {
+    objectString = PyUnicode_AsUTF8(strObj);
+    Py_DECREF(strObj);
+  }
+
+  // Restore the python error stack
+  PyErr_Restore(type, value, traceback);
+  return objectString;
+}
+
+bool vtkMRMLLayerDMPythonUtil::IsValidPythonContext()
+{
+  if (!Py_IsInitialized())
+  {
+    return false;
+  }
+
+  vtkPythonScopeGilEnsurer gilEnsurer;
+  return !PyErr_Occurred();
+}
+
+std::string vtkMRMLLayerDMPythonUtil::FormatExceptionTraceback()
+{
+  // Don't use IsValidPythonContext here as it checks if no error has occurred
+  if (!Py_IsInitialized())
+  {
+    return {};
+  }
+
+  vtkPythonScopeGilEnsurer gilEnsurer;
+  if (!PyErr_Occurred())
+  {
+    // No error has occurred
+    return {};
+  }
+
+  // Get error string from the traceback module
+  PyObject *type, *value, *traceback;
+  PyErr_Fetch(&type, &value, &traceback);
+  PyErr_NormalizeException(&type, &value, &traceback);
+
+  PyObject* tracebackModule = PyImport_ImportModule("traceback");
+  PyObject* formatExceptionFunc = PyObject_GetAttrString(tracebackModule, "format_exception");
+  PyObject* args = PyTuple_Pack(3, type, value, traceback);
+  PyObject* formattedList = PyObject_CallObject(formatExceptionFunc, args);
+  PyObject* emptyString = PyUnicode_FromString("");
+  PyObject* formatted = PyUnicode_Join(emptyString, formattedList);
+  std::string exceptionTraceback = PyUnicode_AsUTF8(formatted);
+
+  // Cleanup
+  PyErr_Restore(type, value, traceback);
+  Py_XDECREF(formatted);
+  Py_XDECREF(emptyString);
+  Py_XDECREF(formattedList);
+  Py_XDECREF(args);
+  Py_XDECREF(formatExceptionFunc);
+  Py_XDECREF(tracebackModule);
+  return exceptionTraceback;
+}
+
+void vtkMRMLLayerDMPythonUtil::PrintErrorTraceback(const vtkObject* object, const std::string& errorMsg)
+{
+  // If the traceback is not empty, print the traceback using vtkErrorMacro
+  const auto traceback = FormatExceptionTraceback();
+  if (traceback.empty())
+  {
+    return;
+  }
+
+  std::string errorString{ errorMsg };
+  if (!errorString.empty())
+  {
+    errorString += "\n";
+  }
+  errorString += traceback;
+  vtkErrorWithObjectMacro(object, "" << traceback.c_str());
+}

LayerDM/MRMLDM/vtkMRMLLayerDMPythonUtil.h

@@ -8,6 +8,7 @@
 #include <vtkSmartPyObject.h>
 
 // STD includes
+#include <string>
 #include <vector>
 
 /// \brief Utility class for Python/C++ interoperability in VTK MRML Layer Displayable Manager
@@ -82,6 +83,30 @@ class VTK_SLICER_LAYERDM_MODULE_MRMLDISPLAYABLEMANAGER_EXPORT vtkMRMLLayerDMPyth
   /// \param destObject Pointer to PyObject pointer to delete and set to nullptr
   static void DeletePythonObject(PyObject** destObject);
 
+  /// \brief true is python is initialized and no error has occurred.
+  static bool IsValidPythonContext();
+
+  /// \brief returns the formatted exception string if any exception occurred.
+  /// Can be used to warn on current errors without swallowing the exception.
+  static std::string FormatExceptionTraceback();
+
+  /// \brief Outputs a vtkErrorMacro containing the traceback in case it's not empty.
+  ///
+  /// Does nothing if the python environment is not initialized.
+  /// Should be used by Methods that cannot properly forward Python results back to the interpreter to avoid
+  /// silent errors happening in the Python layer (or errors being raised in unrelated parts of the executable).
+  ///
+  /// This method doesn't swallow the underlying python exception.
+  /// Python level exceptions should be handled at the python level where they are raised to avoid silent errors from happening.
+  static void PrintErrorTraceback(const vtkObject* object, const std::string& errorMsg);
+
+  /// \brief returns the input's PyObject's string representation as std::string.
+  /// If the python environment is not initialized, returns empty string.
+  /// If the input object is nullptr, returns "None"
+  /// If the input object string representation fails, return "INVALID_OBJECT_STR"
+  /// Calling this method doesn't change the current python error stack.
+  static std::string GetObjectStr(PyObject* object);
+
 protected:
   vtkMRMLLayerDMPythonUtil();
   ~vtkMRMLLayerDMPythonUtil() override;