Merge pull request #2923 from akohlmey/python-finalize-take2

Treat calling Py_Finalize() more like MPI_Finalize() and avoid crashes

doc/src/Library_create.rst

@@ -10,6 +10,7 @@ This section documents the following functions:
 - :cpp:func:`lammps_mpi_init`
 - :cpp:func:`lammps_mpi_finalize`
 - :cpp:func:`lammps_kokkos_finalize`
+- :cpp:func:`lammps_python_finalize`
 
 --------------------
 
@@ -104,3 +105,13 @@ calling program.
 
 .. doxygenfunction:: lammps_mpi_finalize
    :project: progguide
+
+-----------------------
+
+.. doxygenfunction:: lammps_kokkos_finalize
+   :project: progguide
+
+-----------------------
+
+.. doxygenfunction:: lammps_python_finalize
+   :project: progguide

src/PYTHON/python_impl.cpp

@@ -1,4 +1,3 @@
-// clang-format off
 /* ----------------------------------------------------------------------
    LAMMPS - Large-scale Atomic/Molecular Massively Parallel Simulator
    https://www.lammps.org/, Sandia National Laboratories
@@ -25,8 +24,8 @@
 #include "python_utils.h"
 #include "variable.h"
 
-#include <cstring>
 #include <Python.h>    // IWYU pragma: export
+#include <cstring>
 
 #ifdef MLIAP_PYTHON
 #include "mliap_model_python.h"
@@ -40,9 +39,6 @@ using namespace LAMMPS_NS;
 
 enum { NONE, INT, DOUBLE, STRING, PTR };
 
-#define VALUELENGTH 64               // also in variable.cpp
-
-
 /* ---------------------------------------------------------------------- */
 
 PythonImpl::PythonImpl(LAMMPS *lmp) : Pointers(lmp)
@@ -65,10 +61,6 @@ PythonImpl::PythonImpl(LAMMPS *lmp) : Pointers(lmp)
 #endif
 #endif
 
-  // one-time initialization of Python interpreter
-  // pyMain stores pointer to main module
-  external_interpreter = Py_IsInitialized();
-
 #ifdef MLIAP_PYTHON
   // Inform python intialization scheme of the mliappy module.
   // This -must- happen before python is initialized.
@@ -82,9 +74,7 @@ PythonImpl::PythonImpl(LAMMPS *lmp) : Pointers(lmp)
   // With Python 3.7 this function is now called by Py_Initialize()
   // Deprecated since version 3.9, will be removed in version 3.11
 #if PY_MAJOR_VERSION < 3 || PY_MINOR_VERSION < 7
-  if (!PyEval_ThreadsInitialized()) {
+  if (!PyEval_ThreadsInitialized()) { PyEval_InitThreads(); }
-    PyEval_InitThreads();
-  }
 #endif
 
   PyUtils::GIL lock;
@@ -110,12 +100,6 @@ PythonImpl::~PythonImpl()
     }
   }
 
-  // shutdown Python interpreter
-  if (!external_interpreter) {
-    PyGILState_Ensure();
-    Py_Finalize();
-  }
-
   memory->sfree(pfuncs);
 }
 
@@ -133,10 +117,8 @@ void PythonImpl::command(int narg, char **arg)
 
     char *str = nullptr;
     if (pfuncs[ifunc].noutput) {
-      str = input->variable->pythonstyle(pfuncs[ifunc].ovarname,
+      str = input->variable->pythonstyle(pfuncs[ifunc].ovarname, pfuncs[ifunc].name);
-                                         pfuncs[ifunc].name);
+      if (!str) error->all(FLERR, "Python variable does not match Python function");
-      if (!str)
-        error->all(FLERR,"Python variable does not match Python function");
     }
 
     invoke_function(ifunc, str);
@@ -187,7 +169,6 @@ void PythonImpl::command(int narg, char **arg)
     } else if (strcmp(arg[iarg], "return") == 0) {
       if (iarg + 2 > narg) error->all(FLERR, "Invalid python command");
       noutput = 1;
-      delete[] ostr;
       ostr = arg[iarg + 1];
       iarg += 2;
     } else if (strcmp(arg[iarg], "format") == 0) {
@@ -211,7 +192,8 @@ void PythonImpl::command(int narg, char **arg)
     } else if (strcmp(arg[iarg], "exists") == 0) {
       existflag = 1;
       iarg++;
-    } else error->all(FLERR,"Invalid python command");
+    } else
+      error->all(FLERR, "Invalid python command");
   }
 
   if (pyfile && herestr) error->all(FLERR, "Invalid python command");
@@ -261,14 +243,12 @@ void PythonImpl::command(int narg, char **arg)
 
   if (!pFunc) {
     PyUtils::Print_Errors();
-    error->all(FLERR,"Could not find Python function {}",
+    error->all(FLERR, "Could not find Python function {}", pfuncs[ifunc].name);
-                                 pfuncs[ifunc].name);
   }
 
   if (!PyCallable_Check(pFunc)) {
     PyUtils::Print_Errors();
-    error->all(FLERR,"Python function {} is not callable",
+    error->all(FLERR, "Python function {} is not callable", pfuncs[ifunc].name);
-                                 pfuncs[ifunc].name);
   }
 
   pfuncs[ifunc].pFunc = (void *) pFunc;
@@ -295,20 +275,14 @@ void PythonImpl::invoke_function(int ifunc, char *result)
   int ninput = pfuncs[ifunc].ninput;
   PyObject *pArgs = PyTuple_New(ninput);
 
-  if (!pArgs) {
+  if (!pArgs) { error->all(FLERR, "Could not create Python function arguments"); }
-    error->all(FLERR,"Could not create Python function arguments");
-  }
 
   for (int i = 0; i < ninput; i++) {
     int itype = pfuncs[ifunc].itype[i];
     if (itype == INT) {
       if (pfuncs[ifunc].ivarflag[i]) {
         str = input->variable->retrieve(pfuncs[ifunc].svalue[i]);
-
+        if (!str) { error->all(FLERR, "Could not evaluate Python function input variable"); }
-        if (!str) {
-          error->all(FLERR,"Could not evaluate Python function input variable");
-        }
-
         pValue = PY_INT_FROM_LONG(atoi(str));
       } else {
         pValue = PY_INT_FROM_LONG(pfuncs[ifunc].ivalue[i]);
@@ -316,11 +290,7 @@ void PythonImpl::invoke_function(int ifunc, char *result)
     } else if (itype == DOUBLE) {
       if (pfuncs[ifunc].ivarflag[i]) {
         str = input->variable->retrieve(pfuncs[ifunc].svalue[i]);
-
+        if (!str) { error->all(FLERR, "Could not evaluate Python function input variable"); }
-        if (!str) {
-          error->all(FLERR,"Could not evaluate Python function input variable");
-        }
-
         pValue = PyFloat_FromDouble(atof(str));
       } else {
         pValue = PyFloat_FromDouble(pfuncs[ifunc].dvalue[i]);
@@ -328,10 +298,7 @@ void PythonImpl::invoke_function(int ifunc, char *result)
     } else if (itype == STRING) {
       if (pfuncs[ifunc].ivarflag[i]) {
         str = input->variable->retrieve(pfuncs[ifunc].svalue[i]);
-        if (!str) {
+        if (!str) { error->all(FLERR, "Could not evaluate Python function input variable"); }
-          error->all(FLERR,"Could not evaluate Python function input variable");
-        }
-
         pValue = PY_STRING_FROM_STRING(str);
       } else {
         pValue = PY_STRING_FROM_STRING(pfuncs[ifunc].svalue[i]);
@@ -369,7 +336,8 @@ void PythonImpl::invoke_function(int ifunc, char *result)
       const char *pystr = PY_STRING_AS_STRING(pValue);
       if (pfuncs[ifunc].longstr)
         strncpy(pfuncs[ifunc].longstr, pystr, pfuncs[ifunc].length_longstr);
-      else strncpy(result,pystr,VALUELENGTH-1);
+      else
+        strncpy(result, pystr, Variable::VALUELENGTH - 1);
     }
   }
   Py_CLEAR(pValue);
@@ -386,8 +354,7 @@ int PythonImpl::find(const char *name)
 
 /* ------------------------------------------------------------------ */
 
-int PythonImpl::variable_match(const char *name, const char *varname,
+int PythonImpl::variable_match(const char *name, const char *varname, int numeric)
-                               int numeric)
 {
   int ifunc = find(name);
   if (ifunc < 0) return -1;
@@ -406,8 +373,8 @@ char *PythonImpl::long_string(int ifunc)
 
 /* ------------------------------------------------------------------ */
 
-int PythonImpl::create_entry(char *name, int ninput, int noutput, int length_longstr,
+int PythonImpl::create_entry(char *name, int ninput, int noutput, int length_longstr, char **istr,
-                             char **istr, char *ostr, char *format)
+                             char *ostr, char *format)
 {
   // ifunc = index to entry by name in pfuncs vector, can be old or new
   // free old vectors if overwriting old pfunc
@@ -417,10 +384,10 @@ int PythonImpl::create_entry(char *name, int ninput, int noutput, int length_lon
   if (ifunc < 0) {
     ifunc = nfunc;
     nfunc++;
-    pfuncs = (PyFunc *)
+    pfuncs = (PyFunc *) memory->srealloc(pfuncs, nfunc * sizeof(struct PyFunc), "python:pfuncs");
-      memory->srealloc(pfuncs,nfunc*sizeof(struct PyFunc),"python:pfuncs");
     pfuncs[ifunc].name = utils::strdup(name);
-  } else deallocate(ifunc);
+  } else
+    deallocate(ifunc);
 
   pfuncs[ifunc].ninput = ninput;
   pfuncs[ifunc].noutput = noutput;
@@ -471,10 +438,10 @@ int PythonImpl::create_entry(char *name, int ninput, int noutput, int length_lon
     } else if (type == 'p') {
       pfuncs[ifunc].ivarflag[i] = 0;
       pfuncs[ifunc].itype[i] = PTR;
-      if (strcmp(istr[i],"SELF") != 0)
+      if (strcmp(istr[i], "SELF") != 0) error->all(FLERR, "Invalid python command");
-        error->all(FLERR,"Invalid python command");
 
-    } else error->all(FLERR,"Invalid python command");
+    } else
+      error->all(FLERR, "Invalid python command");
   }
 
   // process output as value or variable
@@ -484,15 +451,18 @@ int PythonImpl::create_entry(char *name, int ninput, int noutput, int length_lon
   if (!noutput) return ifunc;
 
   char type = format[ninput];
-  if (type == 'i') pfuncs[ifunc].otype = INT;
+  if (type == 'i')
-  else if (type == 'f') pfuncs[ifunc].otype = DOUBLE;
+    pfuncs[ifunc].otype = INT;
-  else if (type == 's') pfuncs[ifunc].otype = STRING;
+  else if (type == 'f')
-  else error->all(FLERR,"Invalid python command");
+    pfuncs[ifunc].otype = DOUBLE;
+  else if (type == 's')
+    pfuncs[ifunc].otype = STRING;
+  else
+    error->all(FLERR, "Invalid python command");
 
   if (length_longstr) {
     if (pfuncs[ifunc].otype != STRING)
-      error->all(FLERR,"Python command length keyword "
+      error->all(FLERR, "Python command length keyword cannot be used unless output is a string");
-                 "cannot be used unless output is a string");
     pfuncs[ifunc].length_longstr = length_longstr;
     pfuncs[ifunc].longstr = new char[length_longstr + 1];
     pfuncs[ifunc].longstr[length_longstr] = '\0';
@@ -534,8 +504,7 @@ void PythonImpl::deallocate(int i)
   delete[] pfuncs[i].ivarflag;
   delete[] pfuncs[i].ivalue;
   delete[] pfuncs[i].dvalue;
-  for (int j = 0; j < pfuncs[i].ninput; j++)
+  for (int j = 0; j < pfuncs[i].ninput; j++) delete[] pfuncs[i].svalue[j];
-    delete [] pfuncs[i].svalue[j];
   delete[] pfuncs[i].svalue;
   delete[] pfuncs[i].ovarname;
   delete[] pfuncs[i].longstr;
@@ -547,3 +516,10 @@ bool PythonImpl::has_minimum_version(int major, int minor)
 {
   return (PY_MAJOR_VERSION == major && PY_MINOR_VERSION >= minor) || (PY_MAJOR_VERSION > major);
 }
+
+/* ------------------------------------------------------------------ */
+
+void PythonImpl::finalize()
+{
+  if (Py_IsInitialized()) Py_Finalize();
+}

src/PYTHON/python_impl.h

@@ -20,9 +20,8 @@
 namespace LAMMPS_NS {
 
 class PythonImpl : protected Pointers, public PythonInterface {
- public:
-  bool external_interpreter;
 
+ public:
   PythonImpl(class LAMMPS *);
   ~PythonImpl();
   void command(int, char **);
@@ -33,6 +32,7 @@ class PythonImpl : protected Pointers, public PythonInterface {
   int execute_string(char *);
   int execute_file(char *);
   bool has_minimum_version(int major, int minor);
+  static void finalize();
 
  private:
   void *pyMain;

src/library.cpp

@@ -33,17 +33,18 @@
 #include "group.h"
 #include "info.h"
 #include "input.h"
+#include "lmppython.h"
 #include "memory.h"
 #include "modify.h"
 #include "molecule.h"
 #include "neigh_list.h"
 #include "neighbor.h"
-#include "region.h"
-#include "respa.h"
 #include "output.h"
 #if defined(LMP_PLUGIN)
 #include "plugin.h"
 #endif
+#include "region.h"
+#include "respa.h"
 #include "thermo.h"
 #include "timer.h"
 #include "universe.h"
@@ -339,6 +340,8 @@ function no more MPI calls may be made.
 
 .. versionadded:: 18Sep2020
 
+*See also*
+   :cpp:func:`lammps_kokkos_finalize`, :cpp:func:`lammps_python_finalize`
 \endverbatim */
 
 void lammps_mpi_finalize()
@@ -369,6 +372,8 @@ After calling this function no Kokkos functionality may be used.
 
 .. versionadded:: 2Jul2021
 
+*See also*
+   :cpp:func:`lammps_mpi_finalize`, :cpp:func:`lammps_python_finalize`
 \endverbatim */
 
 void lammps_kokkos_finalize()
@@ -376,6 +381,42 @@ void lammps_kokkos_finalize()
   KokkosLMP::finalize();
 }
 
+/* ---------------------------------------------------------------------- */
+
+/** Clear the embedded Python environment
+ *
+\verbatim embed:rst
+
+This function resets and clears an embedded Python environment
+by calling the `Py_Finalize() function
+<https://docs.python.org/3/c-api/init.html#c.Py_FinalizeEx>`_
+of the embedded Python library, if enabled.
+This call would free up all allocated resources and release
+loaded shared objects.
+
+However, this is **not** done when a LAMMPS instance is deleted because
+a) LAMMPS may have been used through the Python module and thus
+the Python interpreter is external and not embedded into LAMMPS
+and therefore may not be reset by LAMMPS b) some Python modules
+and extensions, most notably NumPy, are not compatible with being
+initialized multiple times, which would happen if additional
+LAMMPS instances using Python would be created *after*
+after calling Py_Finalize().
+
+This function can be called to explicitly clear the Python
+environment in case it is safe to do so.
+
+.. versionadded:: TBD
+
+*See also*
+   :cpp:func:`lammps_mpi_finalize`, :cpp:func:`lammps_kokkos_finalize`
+\endverbatim */
+
+void lammps_python_finalize()
+{
+  Python::finalize();
+}
+
 // ----------------------------------------------------------------------
 // Library functions to process commands
 // ----------------------------------------------------------------------

src/library.h

@@ -95,6 +95,7 @@ void lammps_close(void *handle);
 void lammps_mpi_init();
 void lammps_mpi_finalize();
 void lammps_kokkos_finalize();
+void lammps_python_finalize();
 
 /* ----------------------------------------------------------------------
  * Library functions to process commands

src/lmppython.cpp

@@ -1,4 +1,3 @@
-// clang-format off
 /* ----------------------------------------------------------------------
    LAMMPS - Large-scale Atomic/Molecular Massively Parallel Simulator
    https://www.lammps.org/, Sandia National Laboratories
@@ -39,9 +38,7 @@ Python::~Python()
 
 /* ---------------------------------------------------------------------- */
 
-PythonInterface::~PythonInterface()
+PythonInterface::~PythonInterface() {}
-{
-}
 
 /* ---------------------------------------------------------------------- */
 
@@ -55,7 +52,8 @@ void Python::init()
 }
 
 /* ---------------------------------------------------------------------- */
-bool Python::is_enabled() const {
+bool Python::is_enabled() const
+{
 #if defined(LMP_PYTHON)
   return true;
 #else
@@ -126,3 +124,12 @@ bool Python::has_minimum_version(int major, int minor)
   init();
   return impl->has_minimum_version(major, minor);
 }
+
+/* ------------------------------------------------------------------ */
+
+void Python::finalize()
+{
+#if defined(LMP_PYTHON)
+  PythonImpl::finalize();
+#endif
+}

src/lmppython.h

@@ -47,6 +47,7 @@ class Python : protected Pointers {
 
   bool is_enabled() const;
   void init();
+  static void finalize();
 
  private:
   PythonInterface *impl;

src/main.cpp

@@ -15,6 +15,8 @@
 
 #include "accelerator_kokkos.h"
 #include "input.h"
+#include "lmppython.h"
+
 #if defined(LAMMPS_EXCEPTIONS)
 #include "exceptions.h"
 #endif
@@ -79,15 +81,18 @@ int main(int argc, char **argv)
     delete lammps;
   } catch (LAMMPSAbortException &ae) {
     KokkosLMP::finalize();
+    Python::finalize();
     MPI_Abort(ae.universe, 1);
   } catch (LAMMPSException &e) {
     KokkosLMP::finalize();
+    Python::finalize();
     MPI_Barrier(lammps_comm);
     MPI_Finalize();
     exit(1);
   } catch (fmt::format_error &fe) {
     fprintf(stderr, "fmt::format_error: %s\n", fe.what());
     KokkosLMP::finalize();
+    Python::finalize();
     MPI_Abort(MPI_COMM_WORLD, 1);
     exit(1);
   }
@@ -99,11 +104,13 @@ int main(int argc, char **argv)
   } catch (fmt::format_error &fe) {
     fprintf(stderr, "fmt::format_error: %s\n", fe.what());
     KokkosLMP::finalize();
+    Python::finalize();
     MPI_Abort(MPI_COMM_WORLD, 1);
     exit(1);
   }
 #endif
   KokkosLMP::finalize();
+  Python::finalize();
   MPI_Barrier(lammps_comm);
   MPI_Finalize();
 }

src/variable.cpp

@@ -49,7 +49,6 @@ using namespace MathConst;
 #define MAXLEVEL 4
 #define MAXLINE 256
 #define CHUNK 1024
-#define VALUELENGTH 64               // also in python.cpp
 #define MAXFUNCARG 6
 
 #define MYROUND(a) (( a-floor(a) ) >= .5) ? ceil(a) : floor(a)

src/variable.h

@@ -72,6 +72,7 @@ class Variable : protected Pointers {
     PYTHON,
     INTERNAL
   };
+  static constexpr int VALUELENGTH = 64;
 
  private:
   int me;

tools/swig/lammps.i

@@ -64,6 +64,7 @@ extern void   lammps_close(void *handle);
 extern void   lammps_mpi_init();
 extern void   lammps_mpi_finalize();
 extern void   lammps_kokkos_finalize();
+extern void   lammps_python_finalize();
 extern void   lammps_file(void *handle, const char *file);
 extern char  *lammps_command(void *handle, const char *cmd);
 extern void   lammps_commands_list(void *handle, int ncmd, const char **cmds);
@@ -199,6 +200,7 @@ extern void   lammps_close(void *handle);
 extern void   lammps_mpi_init();
 extern void   lammps_mpi_finalize();
 extern void   lammps_kokkos_finalize();
+extern void   lammps_python_finalize();
 extern void   lammps_file(void *handle, const char *file);
 extern char  *lammps_command(void *handle, const char *cmd);
 extern void   lammps_commands_list(void *handle, int ncmd, const char **cmds);

unittest/python/CMakeLists.txt

@@ -4,11 +4,6 @@
 # availability of the PYTHON package is tested for inside the tester.
 
 set(TEST_INPUT_FOLDER ${CMAKE_CURRENT_SOURCE_DIR})
-add_executable(test_python_package test_python_package.cpp)
-target_link_libraries(test_python_package PRIVATE lammps GTest::GMock GTest::GTest)
-target_compile_definitions(test_python_package PRIVATE -DTEST_INPUT_FOLDER=${TEST_INPUT_FOLDER})
-add_test(NAME PythonPackage COMMAND test_python_package WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
-set_tests_properties(PythonPackage PROPERTIES ENVIRONMENT "LAMMPS_POTENTIALS=${LAMMPS_POTENTIALS_DIR};PYTHONPATH=${CMAKE_CURRENT_BINARY_DIR}:${LAMMPS_PYTHON_DIR}:$ENV{PYTHONPATH};PYTHONUNBUFFERED=1")
 
 # we must have shared libraries enabled for testing the python module
 if(NOT BUILD_SHARED_LIBS)
@@ -22,9 +17,20 @@ if(CMAKE_VERSION VERSION_LESS 3.12)
     set(Python_EXECUTABLE ${PYTHON_EXECUTABLE})
   endif()
 else()
-  find_package(Python3 COMPONENTS Interpreter)
+  find_package(Python3 COMPONENTS Interpreter Development)
 endif()
 
+add_executable(test_python_package test_python_package.cpp)
+target_link_libraries(test_python_package PRIVATE lammps GTest::GMock GTest::GTest)
+target_compile_definitions(test_python_package PRIVATE -DTEST_INPUT_FOLDER=${TEST_INPUT_FOLDER})
+# this requires CMake 3.12. don't care to add backward compatibility for this.
+if(Python3_Development_FOUND)
+  target_compile_definitions(test_python_package PRIVATE -DTEST_HAVE_PYTHON_DEVELOPMENT=1)
+  target_link_libraries(test_python_package PRIVATE Python::Python)
+endif()
+add_test(NAME PythonPackage COMMAND test_python_package WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+set_tests_properties(PythonPackage PROPERTIES ENVIRONMENT "LAMMPS_POTENTIALS=${LAMMPS_POTENTIALS_DIR};PYTHONPATH=${CMAKE_CURRENT_BINARY_DIR}:${LAMMPS_PYTHON_DIR}:$ENV{PYTHONPATH};PYTHONUNBUFFERED=1")
+
 if(Python_EXECUTABLE)
   # prepare to augment the environment so that the LAMMPS python module and the shared library is found.
   set(PYTHON_TEST_ENVIRONMENT PYTHONPATH=${LAMMPS_PYTHON_DIR}:$ENV{PYTHONPATH})

unittest/python/test_python_package.cpp

@@ -27,6 +27,10 @@
 #include "../testing/core.h"
 #include "../testing/systems/melt.h"
 
+#if defined(TEST_HAVE_PYTHON_DEVELOPMENT)
+#include <Python.h>
+#endif
+
 // location of '*.py' files required by tests
 #define STRINGIFY(val) XSTR(val)
 #define XSTR(val) #val
@@ -88,6 +92,23 @@ TEST_F(PythonPackageTest, InvokeFunctionFromFile)
     ASSERT_THAT(output, HasSubstr("2.25\n"));
 }
 
+#if defined(TEST_HAVE_PYTHON_DEVELOPMENT)
+TEST_F(PythonPackageTest, InvokeInitialized)
+{
+    // execute python function from file
+    HIDE_OUTPUT([&] {
+        command("python printnum file ${input_dir}/func.py");
+    });
+    ASSERT_TRUE(Py_IsInitialized());
+    HIDE_OUTPUT([&] {
+        command("clear");
+    });
+    ASSERT_TRUE(Py_IsInitialized());
+    lammps_python_finalize();
+    ASSERT_FALSE(Py_IsInitialized());
+}
+#endif
+
 TEST_F(PythonPackageTest, InvokeFunctionPassInt)
 {
     // execute python function, passing integer as argument