Improve Python module testing and cleanup memory leaks (#26353)

Make more of the Python package module tests run more frequently. While
doing this, I also cleaned up as many of the Python memory leaks in the
Python module as I could find.

Some of the tests rely on external Python dependencies like torch and
numba. Previously, these tests were always skipped and a "compileOnly"
version was tested. This PR adds a step to the skipif to install the
dependencies locally if they are missing. This allows the tests to
actually be executed in more cases

This PR also adds support for checking for unfree'ed references,
controlled by the `--pyMemLeaks` flag. This relies on an external python
dependency, so this uses similar external Python dependencies testing
support as above

Testing
- [x] `start_test test/library/packages/Python`
- [x] `start_test -memleaks test/library/packages/Python`

Future Work:
- remove global `toFree` list

[Reviewed by @DanilaFe]

modules/packages/Python.chpl

@@ -34,8 +34,6 @@
 // for example, a List, Set, Dict, Tuple, etc.
 // these should provide native like operation, so `for i in pyList` should work
 
-// TODO: there are decrefs missing all over the place
-
 // TODO: using the Py*_Check, we may be able to avoid needing to specify the type of the return value
 
 // TODO: implement operators as dunder methods
@@ -227,6 +225,8 @@ module Python {
   private use CWChar;
   private use OS.POSIX only getenv;
 
+  config const pyMemLeaks = false;
+
   // TODO: this must be first to avoid use-before-def, but that makes it first in the docs
   // is there a way to avoid this?
   /* Represents the python NoneType */
@@ -267,6 +267,10 @@ module Python {
   class Interpreter {
     @chpldoc.nodoc
     var converters: List.list(owned TypeConverter);
+    @chpldoc.nodoc
+    var toFree: List.list(PyObjectPtr);
+    @chpldoc.nodoc
+    var objgraph: PyObjectPtr = nil;
 
     @chpldoc.nodoc
     proc init() throws {
@@ -332,9 +336,55 @@ module Python {
       // I think we can do this by setting sys.stdout and sys.stderr to a python
       // object that looks like a python file but forwards calls like write to
       // Chapel's write
+
+      if pyMemLeaks {
+        // import objgraph
+        this.objgraph = this.importModule("objgraph");
+        if this.objgraph == nil {
+          writeln("objgraph not found, memory leak detection disabled. Install objgraph with 'pip install objgraph'");
+        } else {
+          // objgraph.growth()
+          var growth = PyObject_GetAttrString(this.objgraph, "growth");
+          this.checkException();
+          var res = PyObject_CallFunctionObjArgs(growth, Py_None, nil);
+          this.checkException();
+          Py_DECREF(res);
+          Py_DECREF(growth);
+        }
+      }
     }
     @chpldoc.nodoc
-    proc deinit() {
+    proc deinit()  {
+
+      for obj in this.toFree {
+        Py_CLEAR(c_ptrTo(obj));
+      }
+
+      if pyMemLeaks && this.objgraph != nil {
+        // note: try! is used since we can't have a throwing deinit
+
+        // run gc.collect() before showing growth
+        var gc = PyImport_ImportModule("gc");
+        try! this.checkException();
+        var collect = PyObject_GetAttrString(gc, "collect");
+        try! this.checkException();
+        PyObject_CallNoArgs(collect);
+        try! this.checkException();
+        Py_DECREF(collect);
+        Py_DECREF(gc);
+
+
+        // objgraph.show_growth()
+        var show_growth = PyObject_GetAttrString(this.objgraph, "show_growth");
+        try! this.checkException();
+
+        PyObject_CallOneArg(show_growth, Py_None);
+        try! this.checkException();
+        Py_DECREF(show_growth);
+
+        Py_DECREF(this.objgraph);
+      }
+
       Py_Finalize();
     }
     /*
@@ -400,12 +450,19 @@ module Python {
     inline proc checkException() throws {
       var exc = chpl_PyErr_GetRaisedException();
       if exc {
-        var py_str = PyObject_Str(exc);
-        var str = this.fromPython(string, py_str);
-        Py_DECREF(py_str);
-        Py_DECREF(exc);
-        throw new PythonException(str);
+        throw PythonException.build(this, exc);
+      }
+    }
+
+    @chpldoc.nodoc
+    inline proc importModule(in modName: string): PyObjectPtr throws {
+      var mod = PyImport_ImportModule(modName.c_str());
+      try {
+        this.checkException();
+      } catch e: ImportError {
+        return nil;
       }
+      return mod;
     }
 
     /*
@@ -504,7 +561,17 @@ module Python {
         this.checkException();
         return v;
       } else if isArrayType(t) {
-        return fromList(t, obj);
+        // we need to create a dummy array to determine the type
+        pragma "no init"
+        var dummy: t;
+
+        if dummy.rank == 1 && dummy.isDefaultRectangular() {
+          return fromList(t, obj);
+        } else if dummy.isAssociative() {
+          return fromDict(t, obj);
+        } else {
+          halt("Unsupported fromPython array type: '" + t:string + "'");
+        }
       } else if isSubtype(t, List.list) {
         return fromList(t, obj);
       } else if isSubtype(t, Function) {
@@ -528,6 +595,7 @@ module Python {
       where isArrayType(arr.type) && arr.rank == 1 && arr.isDefaultRectangular() {
       var pyList = PyList_New(arr.size);
       this.checkException();
+      this.toFree.pushBack(pyList);
       for i in 0..<arr.size {
         const idx = arr.domain.orderToIndex(i);
         PyList_SetItem(pyList, i, toPython(arr[idx]));
@@ -543,6 +611,7 @@ module Python {
     proc toList(l: List.list(?)): PyObjectPtr throws {
       var pyList = PyList_New(l.size);
       this.checkException();
+      this.toFree.pushBack(pyList);
       for i in 0..<l.size {
         PyList_SetItem(pyList, i, toPython(l(i)));
         this.checkException();
@@ -561,13 +630,16 @@ module Python {
         throw new ChapelException("Can only convert a sequence with a known size to an array");
 
       // if its a sequence with a known size, we can just iterate over it
-      var res: T = noinit;
+      var res: T;
       if PySequence_Size(obj) != res.size {
         throw new ChapelException("Size mismatch");
       }
       for i in 0..<res.size {
         const idx = res.domain.orderToIndex(i);
-        res[idx] = fromPython(res.eltType, PySequence_GetItem(obj, i));
+        var elm = PySequence_GetItem(obj, i);
+        this.checkException();
+        this.toFree.pushBack(elm);
+        res[idx] = fromPython(res.eltType, elm);
         this.checkException();
       }
       return res;
@@ -585,6 +657,7 @@ module Python {
         for i in 0..<PySequence_Size(obj) {
           var item = PySequence_GetItem(obj, i);
           this.checkException();
+          this.toFree.pushBack(item);
           res.pushBack(fromPython(T.eltType, item));
         }
         return res;
@@ -594,6 +667,7 @@ module Python {
         while true {
           var item = PyIter_Next(obj);
           this.checkException();
+          this.toFree.pushBack(item);
           if item == nil {
             break;
           }
@@ -632,6 +706,7 @@ module Python {
       where isArrayType(arr.type) && arr.isAssociative() {
       var pyDict = PyDict_New();
       this.checkException();
+      this.toFree.pushBack(pyDict);
       for key in arr.domain {
         var pyKey = toPython(key);
         var pyValue = toPython(arr[key]);
@@ -646,9 +721,31 @@ module Python {
     /*
       Converts a python dictionary to an associative array
     */
-    // proc fromDict(type T, obj: PyObjectPtr): T throws {
-      // TODO
-    // }
+    proc fromDict(type T, obj: PyObjectPtr): T throws
+      where isArrayType(T) {
+
+      // rebuild the array with a modifiable domain
+      var dom = chpl__domainFromArrayRuntimeType(T);
+      type eltType = chpl__eltTypeFromArrayRuntimeType(T);
+      var arr: [dom] eltType;
+
+      type keyType = arr.idxType;
+      type valType = arr.eltType;
+      var keys = PyDict_Keys(obj);
+      defer Py_DECREF(keys);
+      this.checkException();
+      for i in 0..<PyList_Size(keys) {
+        var key = PyList_GetItem(keys, i);
+        this.checkException();
+        var val = PyDict_GetItem(obj, key);
+        this.checkException();
+
+        var keyVal = this.fromPython(keyType, key);
+        dom.add(keyVal);
+        arr[keyVal] = this.fromPython(valType, val);
+      }
+      return arr;
+    }
 
     // TODO: convert python dict to chapel map
 
@@ -672,6 +769,27 @@ module Python {
     proc init(in message: string) {
       super.init(message);
     }
+    @chpldoc.nodoc
+    proc type build(interp: borrowed Interpreter, exc: PyObjectPtr): owned PythonException throws {
+      assert(exc != nil);
+      var py_str = PyObject_Str(exc);
+      var str = interp.fromPython(string, py_str);
+      Py_DECREF(py_str);
+      Py_DECREF(exc);
+      if PyErr_GivenExceptionMatches(exc, PyExc_ImportError) != 0 {
+        return new ImportError(str);
+      } else {
+        throw new PythonException(str);
+      }
+    }
+  }
+  /*
+    Represents an ImportError in the Python code
+  */
+  class ImportError: PythonException {
+    proc init(in message: string) {
+      super.init(message);
+    }
   }
   /*
     Represents an exception caused by code in the Chapel module, not forwarded from Python.
@@ -844,13 +962,15 @@ module Python {
       else
         pyRes = PyObject_CallFunctionObjArgs(this.fn.get(), (...pyArgs), nil);
       interpreter.checkException();
+      interpreter.toFree.pushBack(pyRes);
 
       var res = interpreter.fromPython(retType, pyRes);
       return res;
     }
     proc this(type retType): retType throws {
       var pyRes = PyObject_CallNoArgs(this.fn.get());
       interpreter.checkException();
+      interpreter.toFree.pushBack(pyRes);
 
       var res = interpreter.fromPython(retType, pyRes);
       return res;
@@ -873,6 +993,7 @@ module Python {
 
       var pyRes = PyObject_Call(this.fn.get(), pyArg, pyKwargs);
       interpreter.checkException();
+      interpreter.toFree.pushBack(pyRes);
 
       var res = interpreter.fromPython(retType, pyRes);
       return res;
@@ -892,12 +1013,12 @@ module Python {
 
       var pyRes = PyObject_Call(this.fn.get(), pyArg, pyKwargs);
       interpreter.checkException();
+      interpreter.toFree.pushBack(pyRes);
 
       var res = interpreter.fromPython(retType, pyRes);
       return res;
     }
 
-
     proc getAttr(type t, attr: string): t throws {
       var pyAttr = PyObject_GetAttrString(this.fn.get(), attr.c_str());
       interpreter.checkException();
@@ -1381,6 +1502,7 @@ module Python {
     extern proc Py_Finalize();
     extern proc Py_INCREF(obj: PyObjectPtr);
     extern "chpl_Py_DECREF" proc Py_DECREF(obj: PyObjectPtr);
+    extern "chpl_Py_CLEAR" proc Py_CLEAR(obj: c_ptr(PyObjectPtr));
     extern proc PyObject_Str(obj: PyObjectPtr): PyObjectPtr; // `str(obj)`
     extern proc PyImport_ImportModule(name: c_ptrConst(c_char)): PyObjectPtr;
 
@@ -1413,6 +1535,8 @@ module Python {
     */
     extern proc PyErr_Occurred(): PyObjectPtr;
     extern proc chpl_PyErr_GetRaisedException(): PyObjectPtr;
+    extern proc PyErr_GivenExceptionMatches(given: PyObjectPtr, exc: PyObjectPtr): c_int;
+    extern const PyExc_ImportError: PyObjectPtr;
 
     /*
       Values
@@ -1489,6 +1613,7 @@ module Python {
     extern proc PyDict_GetItemString(dict: PyObjectPtr,
                                      key: c_ptrConst(c_char)): PyObjectPtr;
     extern proc PyDict_Size(dict: PyObjectPtr): c_long;
+    extern proc PyDict_Keys(dict: PyObjectPtr): PyObjectPtr;
 
     extern proc PyObject_GetAttrString(obj: PyObjectPtr,
                                        name: c_ptrConst(c_char)): PyObjectPtr;

modules/packages/PythonHelper/ChapelPythonHelper.h

@@ -53,6 +53,7 @@ static inline PyObject* chpl_PyErr_GetRaisedException(void) {
 }
 
 static inline void chpl_Py_DECREF(PyObject* o) { Py_DECREF(o); }
+static inline void chpl_Py_CLEAR(PyObject** o) { Py_CLEAR(*o); }
 
 static inline int chpl_PyList_Check(PyObject* o) { return PyList_Check(o); }
 static inline int chpl_PyGen_Check(PyObject* o) { return PyGen_Check(o); }

test/library/packages/Python/.gitignore

@@ -1 +1,2 @@
 __pycache__
+python_libs

test/library/packages/Python/CLEANFILES

@@ -1 +1,2 @@
 __pycache__
+python_libs

test/library/packages/Python/EXECENV

@@ -0,0 +1,15 @@
+#!/usr/bin/env bash
+
+# if CHPL_TEST_VENV_DIR is set and not none, make sure to set VIRTUAL_ENV to match
+if [ -n "$CHPL_TEST_VENV_DIR" ] && [ "$CHPL_TEST_VENV_DIR" != "none" ]; then
+  echo "VIRTUAL_ENV=$CHPL_TEST_VENV_DIR"
+fi
+
+# make sure to add the libs dir to the PYTHONPATH if it exists
+FILE_DIR=$(cd $(dirname ${BASH_SOURCE[0]}) ; pwd)
+MY_LIB_DIR=$FILE_DIR/python_libs
+if [ -d "$MY_LIB_DIR" ]; then
+  echo "PYTHONPATH=$MY_LIB_DIR:$PYTHONPATH"
+fi
+
+echo ""

test/library/packages/Python/correctness/CLEANFILES

@@ -0,0 +1 @@
+../CLEANFILES

test/library/packages/Python/correctness/COMPOPTS

@@ -0,0 +1 @@
+../COMPOPTS

test/library/packages/Python/correctness/EXECENV

@@ -0,0 +1 @@
+../EXECENV

test/library/packages/Python/examples/bs4/CLEANFILES

@@ -0,0 +1 @@
+../../CLEANFILES

test/library/packages/Python/examples/bs4/EXECENV

@@ -1 +1 @@
-../EXECENV
+../../EXECENV

test/library/packages/Python/examples/bs4/findLinks.skipif

@@ -1,16 +1,4 @@
 #!/usr/bin/env bash
 
-
-# respect CHPL_TEST_VENV_DIR if it is set and not none
-if [ -n "$CHPL_TEST_VENV_DIR" ] && [ "$CHPL_TEST_VENV_DIR" != "none" ]; then
-  chpl_python=$CHPL_TEST_VENV_DIR/bin/python3
-else
-  chpl_python=$($CHPL_HOME/util/config/find-python.sh)
-fi
-
-# try and import bs4, if it fails, skip the test
-if ! $chpl_python -c "import bs4" &>/dev/null; then
-  echo "True"
-else
-  echo "False"
-fi
+FILE_DIR=$(cd $(dirname ${BASH_SOURCE[0]}) ; pwd)
+$FILE_DIR/../../skipIfAndInstallPackage.sh $FILE_DIR bs4