Improve Python module testing and cleanup memory leaks

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 {
@@ -339,9 +343,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();
     }
     /*
@@ -407,12 +457,21 @@ 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;
+      } catch e {
+        throw e;
       }
+      return mod;
     }
 
     /*
@@ -511,7 +570,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) {
@@ -535,6 +604,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]));
@@ -550,6 +620,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();
@@ -568,13 +639,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;
@@ -592,6 +666,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;
@@ -601,6 +676,7 @@ module Python {
         while true {
           var item = PyIter_Next(obj);
           this.checkException();
+          this.toFree.pushBack(item);
           if item == nil {
             break;
           }
@@ -639,6 +715,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]);
@@ -653,9 +730,33 @@ 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) {
+
+      // no init causes segfaults :(
+      var dummy: T;
+
+      // rebuild the array with a modifiable domain
+      var dom = dummy.domain;
+      var arr: [dom] dummy.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
 
@@ -679,6 +780,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.
@@ -851,13 +973,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;
@@ -880,6 +1004,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;
@@ -899,12 +1024,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();
@@ -1388,6 +1513,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;
 
@@ -1420,6 +1546,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
@@ -1496,6 +1624,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