From 3e9d81269e6fd717f3c6500e35a33d2ded4f0dd8 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Wed, 5 Mar 2025 16:53:28 -0800 Subject: [PATCH 01/11] add problematic test Signed-off-by: Jade Abraham --- .../Python/correctness/reuseInterpreter.chpl | 39 +++++++++++++++++++ .../Python/correctness/reuseInterpreter.good | 2 + .../correctness/reuseInterpreter.numlocales | 1 + 3 files changed, 42 insertions(+) create mode 100644 test/library/packages/Python/correctness/reuseInterpreter.chpl create mode 100644 test/library/packages/Python/correctness/reuseInterpreter.good create mode 100644 test/library/packages/Python/correctness/reuseInterpreter.numlocales diff --git a/test/library/packages/Python/correctness/reuseInterpreter.chpl b/test/library/packages/Python/correctness/reuseInterpreter.chpl new file mode 100644 index 000000000000..2d623993ebc3 --- /dev/null +++ b/test/library/packages/Python/correctness/reuseInterpreter.chpl @@ -0,0 +1,39 @@ +use Python; +use BlockDist; + +var interpreters = blockDist.createArray(0.. Date: Wed, 5 Mar 2025 20:36:49 -0800 Subject: [PATCH 02/11] fix threading Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 456 ++++++++++++++++++++++++++--------- 1 file changed, 339 insertions(+), 117 deletions(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index f88f06511e5f..d4a6b01e7c03 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -434,6 +434,8 @@ module Python { var objgraph: PyObjectPtr = nil; @chpldoc.nodoc var isSubInterpreter: bool; + @chpldoc.nodoc + var done: sync bool = false; @chpldoc.nodoc proc init(isSubInterpreter: bool = false) { @@ -443,96 +445,144 @@ module Python { @chpldoc.nodoc proc postinit() throws { if this.isSubInterpreter then return; + + // + // make sure 'done' is empty + // + done.readFE(); + + // + // make sure 'setupDone' is empty + // + var setupDone: sync bool = false; + setupDone.readFE(); + + // + // Do all Python initialization/finalization in a single task. + // Initialization will be done, then this thread will block on 'done'. + // When the deinit is called, it will unblock this thread and finalize + // + begin { + // preinit + var preconfig: PyPreConfig; + PyPreConfig_InitIsolatedConfig(c_ptrTo(preconfig)); + preconfig.utf8_mode = 1; + checkPyStatus(Py_PreInitialize(c_ptrTo(preconfig))); + + // init + var config_: PyConfig; + var cfgPtr = c_ptrTo(config_); + // we want an isolated config for things like LC_ALL, but we still want + // to use things like PYTHONPATH + PyConfig_InitIsolatedConfig(cfgPtr); + config_.isolated = 0; + config_.use_environment = 1; + config_.user_site_directory = 1; + config_.site_import = 1; + defer PyConfig_Clear(cfgPtr); + + // check VIRTUAL_ENV, if its set, make it the executable + var venv = getenv("VIRTUAL_ENV".c_str()); + if venv != nil { + // ideally this just sets config_.home + // but by setting executable we can reuse the python logic to + // determine the locale (in the string sense, not the chapel sense) + const executable = string.createBorrowingBuffer(venv) + "/bin/python"; + const wideExecutable = executable.c_wstr(); + checkPyStatus( + PyConfig_SetString( + cfgPtr, c_ptrTo(config_.executable), wideExecutable)); + deallocate(wideExecutable); + } - // preinit - var preconfig: PyPreConfig; - PyPreConfig_InitIsolatedConfig(c_ptrTo(preconfig)); - preconfig.utf8_mode = 1; - checkPyStatus(Py_PreInitialize(c_ptrTo(preconfig))); - - // init - var config_: PyConfig; - var cfgPtr = c_ptrTo(config_); - // we want an isolated config for things like LC_ALL, but we still want - // to use things like PYTHONPATH - PyConfig_InitIsolatedConfig(cfgPtr); - config_.isolated = 0; - config_.use_environment = 1; - config_.user_site_directory = 1; - config_.site_import = 1; - defer PyConfig_Clear(cfgPtr); - - // check VIRTUAL_ENV, if its set, make it the executable - var venv = getenv("VIRTUAL_ENV".c_str()); - if venv != nil { - // ideally this just sets config_.home - // but by setting executable we can reuse the python logic to determine - // the locale (in the string sense, not the chapel sense) - const executable = string.createBorrowingBuffer(venv) + "/bin/python"; - const wideExecutable = executable.c_wstr(); - checkPyStatus( - PyConfig_SetString( - cfgPtr, c_ptrTo(config_.executable), wideExecutable)); - deallocate(wideExecutable); - } + // initialize + checkPyStatus(Py_InitializeFromConfig(cfgPtr)); - // initialize - checkPyStatus(Py_InitializeFromConfig(cfgPtr)); + var sys = PyImport_ImportModule("sys"); + this.checkException(); + defer Py_DECREF(sys); - var sys = PyImport_ImportModule("sys"); - this.checkException(); - defer Py_DECREF(sys); + // setup sys.path + { + var path = PyObject_GetAttrString(sys, "path"); + this.checkException(); + defer Py_DECREF(path); + + // add paths from PYTHONPATH to the front of PATH + var pythonPathCstr = getenv("PYTHONPATH".c_str()); + if pythonPathCstr != nil { + const pythonPath = string.createBorrowingBuffer(pythonPathCstr); + + var elms = pythonPath.split(getOsPathSepHelper(this)); + for elm in elms { + if elm.size == 0 then continue; + PyList_Append(path, Py_BuildValue("s", elm.c_str())); + this.checkException(); + } + } - // setup sys.path - { - var path = PyObject_GetAttrString(sys, "path"); - this.checkException(); - defer Py_DECREF(path); + // add the current directory to the python path + PyList_Insert(path, 0, Py_BuildValue("s", ".")); + this.checkException(); + } - // add paths from PYTHONPATH to the front of PATH - var pythonPathCstr = getenv("PYTHONPATH".c_str()); - if pythonPathCstr != nil { - const pythonPath = string.createBorrowingBuffer(pythonPathCstr); + if !ArrayTypes.createArrayTypes() { + throwChapelException("Failed to create Python array types for Chapel arrays"); + } + if !ArrayTypes.registerArrayTypeEnum() { + throwChapelException("Failed to register Python array types for Chapel arrays"); + } - var elms = pythonPath.split(getOsPathSepHelper(this)); - for elm in elms { - if elm.size == 0 then continue; - PyList_Append(path, Py_BuildValue("s", elm.c_str())); + // + // grab the GIL and enter a state where we can invoke threads + // + var gil = PyGILState_Ensure(); + var ts = PyEval_SaveThread(); + + if pyMemLeaks { + // import objgraph + this.objgraph = this.importModuleInternal("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); } } - // add the current directory to the python path - PyList_Insert(path, 0, Py_BuildValue("s", ".")); - this.checkException(); - } + // + // setup is done + // + setupDone.writeEF(true); - if !ArrayTypes.createArrayTypes() { - throwChapelException("Failed to create Python array types for Chapel arrays"); - } - if !ArrayTypes.registerArrayTypeEnum() { - throwChapelException("Failed to register Python array types for Chapel arrays"); - } + // + // wait for deinit to be called + // + done.readFE(); + + // + // restore the thread state so we can release the gil and exit + // + PyEval_RestoreThread(ts); + PyGILState_Release(gil); + Py_Finalize(); - if pyMemLeaks { - // import objgraph - this.objgraph = this.importModuleInternal("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); - } } + + // + // blocks until setup is done, once setup is done we can return + // + setupDone.readFE(); } @chpldoc.nodoc proc deinit() { + writeln("hello"); if this.isSubInterpreter then return; if pyMemLeaks && this.objgraph != nil { @@ -560,7 +610,12 @@ module Python { Py_DECREF(this.objgraph); } - Py_Finalize(); + writeln("can i get here"); + + // + // we are done, the thread keeping the interpreter alive can finish + // + done.writeEF(true); } /* @@ -630,6 +685,8 @@ module Python { return values. */ proc run(code: string) throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); PyRun_SimpleString(code.c_str()); this.checkException(); } @@ -776,6 +833,9 @@ module Python { */ inline proc checkException() throws { if Python.checkExceptions { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var exc = chpl_PyErr_GetRaisedException(); if exc then throw PythonException.build(this, exc); } @@ -787,6 +847,9 @@ module Python { displayed in the correct order. */ inline proc flush(flushStderr: bool = false) throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var stdout = PySys_GetObject("stdout"); if stdout == nil then throw new ChapelException("stdout not found"); @@ -822,7 +885,13 @@ module Python { Most users should not need to call this directly, except when writing a :type:`TypeConverter`. */ - proc toPython(const val: ?t): PyObjectPtr throws { + proc toPython(const val): PyObjectPtr throws { + var gil = PyGILState_Ensure(); + defer PyGILState_Release(gil); + return toPythonInner(val); + } + @chpldoc.nodoc + proc toPythonInner(const val: ?t): PyObjectPtr throws { for converter in this.converters { if converter.handlesType(t) { return converter.toPython(this, t, val); @@ -891,6 +960,12 @@ module Python { a :type:`TypeConverter`. */ proc fromPython(type t, obj: PyObjectPtr): t throws { + var gil = PyGILState_Ensure(); + defer PyGILState_Release(gil); + return fromPythonInner(t, obj); + } + @chpldoc.nodoc + proc fromPythonInner(type t, obj: PyObjectPtr): t throws { if isClassType(t) && (!isSharedClassType(t) && !isOwnedClassType(t) && @@ -1421,7 +1496,8 @@ module Python { /* - Represents a Python value, it handles reference counting and is owned by default. + Represents a Python value, it handles reference counting and is owned by + default. */ class Value { /* @@ -1438,9 +1514,12 @@ module Python { :arg interpreter: The interpreter that this object is associated with. :arg obj: The :type:`~CTypes.c_ptr` to the existing object. - :arg isOwned: Whether this object owns the Python object. This is true by default. + :arg isOwned: Whether this object owns the Python object. + This is true by default. */ - proc init(in interpreter: borrowed Interpreter, obj: PyObjectPtr, isOwned: bool = true) { + proc init(in interpreter: borrowed Interpreter, + obj: PyObjectPtr, + isOwned: bool = true) { this.interpreter = interpreter; this.obj = obj; this.isOwned = isOwned; @@ -1458,6 +1537,8 @@ module Python { this.interpreter = interpreter; this.isOwned = true; init this; + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); this.obj = this.interpreter.loadPickle(pickleData); } /* @@ -1480,8 +1561,13 @@ module Python { @chpldoc.nodoc proc deinit() { - if this.isOwned then + writeln("calling deinit"); + if this.isOwned { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); Py_DECREF(this.obj); + } + writeln("done calling deinit"); } /* @@ -1500,9 +1586,11 @@ module Python { Equivalent to calling ``str(obj)`` in Python. */ proc str(): string throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); var pyStr = PyObject_Str(this.obj); this.check(); - var res = interpreter.fromPython(string, pyStr); + var res = interpreter.fromPythonInner(string, pyStr); return res; } @@ -1536,6 +1624,9 @@ module Python { const args..., kwargs:?=none): retType throws where kwargs.isAssociative() { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArg = this.packTuple((...args)); defer Py_DECREF(pyArg); return callInternal(retType, this.getPyObject(), pyArg, kwargs); @@ -1545,6 +1636,9 @@ module Python { proc this(const args..., kwargs:?=none): owned Value throws where kwargs.isAssociative() { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArg = this.packTuple((...args)); defer Py_DECREF(pyArg); return callInternal(owned Value, this.getPyObject(), pyArg, kwargs); @@ -1553,6 +1647,9 @@ module Python { @chpldoc.nodoc proc this(type retType, kwargs:?=none): retType throws where kwargs.isAssociative() { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArgs = Py_BuildValue("()"); defer Py_DECREF(pyArgs); return callInternal(retType, this.getPyObject(), pyArgs, kwargs); @@ -1560,6 +1657,9 @@ module Python { pragma "last resort" @chpldoc.nodoc proc this(kwargs:?=none): owned Value throws where kwargs.isAssociative() { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArgs = Py_BuildValue("()"); defer Py_DECREF(pyArgs); return callInternal(owned Value, this.getPyObject(), pyArgs, kwargs); @@ -1567,6 +1667,9 @@ module Python { @chpldoc.nodoc proc this(type retType, const args...): retType throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArg = this.packTuple((...args)); defer Py_DECREF(pyArg); return callInternal(retType, this.getPyObject(), pyArg, none); @@ -1576,10 +1679,13 @@ module Python { return this(owned Value, (...args)); @chpldoc.nodoc proc this(type retType = owned Value): retType throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyRes = PyObject_CallNoArgs(this.getPyObject()); this.check(); - var res = interpreter.fromPython(retType, pyRes); + var res = interpreter.fromPythonInner(retType, pyRes); return res; } @@ -1587,7 +1693,7 @@ module Python { proc packTuple(const args...) throws { var pyArgs: args.size * PyObjectPtr; for param i in 0..#args.size { - pyArgs(i) = interpreter.toPython(args(i)); + pyArgs(i) = interpreter.toPythonInner(args(i)); } defer for pyArg in pyArgs do Py_DECREF(pyArg); @@ -1602,13 +1708,13 @@ module Python { pyArg: PyObjectPtr, kwargs: ?t): retType throws { var pyKwargs; - if t != nothing then pyKwargs = interpreter.toPython(kwargs); + if t != nothing then pyKwargs = interpreter.toPythonInner(kwargs); else pyKwargs = nil; var pyRes = PyObject_Call(pyFunc, pyArg, pyKwargs); this.check(); - var res = interpreter.fromPython(retType, pyRes); + var res = interpreter.fromPythonInner(retType, pyRes); return res; } @@ -1647,10 +1753,13 @@ module Python { @chpldoc.nodoc proc get(type t, attr: string): t throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyAttr = PyObject_GetAttrString(this.getPyObject(), attr.c_str()); interpreter.checkException(); - var res = interpreter.fromPython(t, pyAttr); + var res = interpreter.fromPythonInner(t, pyAttr); return res; } @chpldoc.nodoc @@ -1665,7 +1774,10 @@ module Python { :arg value: The value to set the attribute/field to. */ proc set(attr: string, value) throws { - var pyValue = interpreter.toPython(value); + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + var pyValue = interpreter.toPythonInner(value); defer Py_DECREF(pyValue); PyObject_SetAttrString(this.getPyObject(), attr.c_str(), pyValue); @@ -1695,6 +1807,9 @@ module Python { const args..., kwargs:?=none): retType throws where kwargs.isAssociative() { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArg = this.packTuple((...args)); defer Py_DECREF(pyArg); @@ -1710,6 +1825,9 @@ module Python { proc call(method: string,const args..., kwargs:?=none): owned Value throws where kwargs.isAssociative() { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArg = this.packTuple((...args)); defer Py_DECREF(pyArg); @@ -1724,20 +1842,23 @@ module Python { @chpldoc.nodoc proc call(type retType, method: string, const args...): retType throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArgs: args.size * PyObjectPtr; for param i in 0..#args.size { - pyArgs(i) = interpreter.toPython(args(i)); + pyArgs(i) = interpreter.toPythonInner(args(i)); } defer for pyArg in pyArgs do Py_DECREF(pyArg); - var methodName = interpreter.toPython(method); + var methodName = interpreter.toPythonInner(method); defer Py_DECREF(methodName); var pyRes = PyObject_CallMethodObjArgs( this.getPyObject(), methodName, (...pyArgs), nil); interpreter.checkException(); - var res = interpreter.fromPython(retType, pyRes); + var res = interpreter.fromPythonInner(retType, pyRes); return res; } @chpldoc.nodoc @@ -1746,13 +1867,16 @@ module Python { @chpldoc.nodoc proc call(type retType, method: string): retType throws { - var methodName = interpreter.toPython(method); + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + var methodName = interpreter.toPythonInner(method); defer Py_DECREF(methodName); var pyRes = PyObject_CallMethodNoArgs(this.getPyObject(), methodName); interpreter.checkException(); - var res = interpreter.fromPython(retType, pyRes); + var res = interpreter.fromPythonInner(retType, pyRes); return res; } @chpldoc.nodoc @@ -1765,6 +1889,9 @@ module Python { proc call(type retType, method: string, kwargs:?=none): retType throws where kwargs.isAssociative() { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var pyArgs = Py_BuildValue("()"); defer Py_DECREF(pyArgs); @@ -1790,9 +1917,12 @@ module Python { object, however it does not consume the object. */ proc value(type value) throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + // fromPython will decrement the ref count, so we need to increment it Py_INCREF(this.obj); - return interpreter.fromPython(value, this.obj); + return interpreter.fromPythonInner(value, this.obj); } /* @@ -1853,11 +1983,15 @@ module Python { Import a Python module by name. See :proc:`Interpreter.importModule`. */ proc init(interpreter: borrowed Interpreter, in modName: string) { - super.init(interpreter, - PyImport_ImportModule(modName.c_str()), isOwned=true); + super.init(interpreter, nil: PyObjectPtr, isOwned=true); this.modName = modName; + init this; + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + this.obj = PyImport_ImportModule(modName.c_str()); } + /* Import a Python module from a string using an arbitrary name. See :proc:`Interpreter.importModule`. @@ -1867,6 +2001,8 @@ module Python { super.init(interpreter, nil: PyObjectPtr, isOwned=true); this.modName = modName; init this; + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); this.obj = interpreter.compileModule(moduleContents, modName); } @@ -1879,6 +2015,8 @@ module Python { super.init(interpreter, nil: PyObjectPtr, isOwned=true); this.modName = modName; init this; + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); this.obj = interpreter.compileModule(moduleBytecode, modName); } } @@ -1896,10 +2034,12 @@ module Python { This is equivalent to ``mod.get(className)``. See :proc:`Value.get`. */ proc init(mod: borrowed Value, in fnName: string) { - super.init(mod.interpreter, - PyObject_GetAttrString(mod.getPyObject(), fnName.c_str()), - isOwned=true); + super.init(mod.interpreter, nil: PyObjectPtr, isOwned=true); this.fnName = fnName; + init this; + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + this.obj = PyObject_GetAttrString(mod.getPyObject(), fnName.c_str()); } /* Takes ownership of an existing Python object, pointed to by ``obj``. @@ -1933,6 +2073,8 @@ module Python { super.init(interpreter, nil: PyObjectPtr, isOwned=true); this.fnName = ""; init this; + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); this.obj = interpreter.compileLambdaInternal(lambdaFn); } @chpldoc.nodoc @@ -1960,10 +2102,12 @@ module Python { :arg className: The name of the class. */ proc init(mod: borrowed Value, in className: string) { - super.init(mod.interpreter, - PyObject_GetAttrString(mod.getPyObject(), className.c_str()), - isOwned=true); + super.init(mod.interpreter, nil: PyObjectPtr, isOwned=true); this.className = className; + init this; + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + this.obj = PyObject_GetAttrString(mod.getPyObject(), className.c_str()); } @chpldoc.nodoc @@ -1992,6 +2136,9 @@ module Python { :returns: The size of the list. */ proc size: int throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var size = PySequence_Size(this.getPyObject()); this.check(); return size; @@ -2010,9 +2157,13 @@ module Python { @chpldoc.nodoc proc get(type T, idx: int): T throws { - var item = PySequence_GetItem(this.getPyObject(), idx.safeCast(Py_ssize_t)); + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + var item = PySequence_GetItem(this.getPyObject(), + idx.safeCast(Py_ssize_t)); this.check(); - return interpreter.fromPython(T, item); + return interpreter.fromPythonInner(T, item); } @chpldoc.nodoc proc get(idx: int): owned Value throws do @@ -2026,6 +2177,9 @@ module Python { :arg item: The item to set. */ proc set(idx: int, item: ?) throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + PySequence_SetItem(this.getPyObject(), idx.safeCast(Py_ssize_t), interpreter.toPython(item)); @@ -2050,6 +2204,9 @@ module Python { :returns: The size of the dict. */ proc size: int throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var size = PyDict_Size(this.getPyObject()); this.check(); return size; @@ -2068,7 +2225,11 @@ module Python { @chpldoc.nodoc proc get(type T, key: ?): T throws { - var item = PyDict_GetItem(this.getPyObject(), interpreter.toPython(key)); + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + var item = PyDict_GetItem(this.getPyObject(), + interpreter.toPythonInner(key)); this.check(); Py_INCREF(item); return interpreter.fromPython(T, item); @@ -2085,8 +2246,12 @@ module Python { :arg item: The item to set. */ proc set(key: ?, item: ?) throws { - var val = interpreter.toPython(item); - PyDict_SetItem(this.getPyObject(), interpreter.toPython(key), + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + var val = interpreter.toPythonInner(item); + PyDict_SetItem(this.getPyObject(), + interpreter.toPythonInner(key), val); Py_DECREF(val); this.check(); @@ -2101,6 +2266,9 @@ module Python { :throws KeyError: If the key did not exist in the dict. */ proc del(key: ?) throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + PyDict_DelItem(this.getPyObject(), interpreter.toPython(key)); this.check(); } @@ -2110,6 +2278,9 @@ module Python { in Python. */ proc clear() throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + PyDict_Clear(this.getPyObject()); this.check(); } @@ -2119,6 +2290,9 @@ module Python { ``obj.copy()`` in Python. */ proc copy(): PyDict throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var c = PyDict_Copy(this.getPyObject()); this.check(); return new PyDict(this.interpreter, c); @@ -2131,8 +2305,11 @@ module Python { :arg key: The key to check for membership in the dict. */ proc contains(key: ?): bool throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var result = PyDict_Contains(this.getPyObject(), - interpreter.toPython(key)); + interpreter.toPythonInner(key)); this.check(); return result: bool; } @@ -2155,6 +2332,9 @@ module Python { :returns: The size of the set. */ proc size: int throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var size = PySequence_Size(this.getPyObject()); this.check(); return size; @@ -2167,7 +2347,10 @@ module Python { :arg item: The item to add to the set. */ proc add(item: ?) throws { - PySet_Add(this.getPyObject(), interpreter.toPython(item)); + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + PySet_Add(this.getPyObject(), interpreter.toPythonInner(item)); this.check(); } @@ -2178,8 +2361,11 @@ module Python { :arg item: The item to check for membership in the set. */ proc contains(item: ?): bool throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var result = PySet_Contains(this.getPyObject(), - interpreter.toPython(item)); + interpreter.toPythonInner(item)); this.check(); return result: bool; } @@ -2191,7 +2377,11 @@ module Python { :arg item: The item to discard from the set. */ proc discard(item: ?) throws { - PySet_Discard(this.getPyObject(), interpreter.toPython(item)); + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + PySet_Discard(this.getPyObject(), + interpreter.toPythonInner(item)); this.check(); } @@ -2206,9 +2396,12 @@ module Python { control which element `pop` will return. */ proc pop(type T = owned Value): T throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var popped = PySet_Pop(this.getPyObject()); this.check(); - return interpreter.fromPython(T, popped); + return interpreter.fromPythonInner(T, popped); } /* @@ -2216,6 +2409,9 @@ module Python { in Python */ proc clear() throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + PySet_Clear(this.getPyObject()); this.check(); } @@ -2273,6 +2469,9 @@ module Python { } @chpldoc.nodoc proc postinit() throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + if PyObject_CheckBuffer(this.getPyObject()) == 0 { throw new ChapelException("Object does not support buffer protocol"); } @@ -2305,6 +2504,9 @@ module Python { } proc deinit() { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + PyBuffer_Release(c_ptrTo(this.view)); } @@ -2369,13 +2571,17 @@ module Python { :arg arr: The Chapel array to create the object from. */ proc init(in interpreter: borrowed Interpreter, ref arr: []) throws - where isSupportedArrayType(arr) { - super.init(interpreter, ArrayTypes.createArray(arr), isOwned=true); + where isSupportedArrayType(arr) { + super.init(interpreter, nil: PyObjectPtr, isOwned=true); this.eltType = arr.eltType; + init this; + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + this.obj = ArrayTypes.createArray(arr); } @chpldoc.nodoc proc init(in interpreter: borrowed Interpreter, ref arr: []) throws - where !isSupportedArrayType(arr) { + where !isSupportedArrayType(arr) { compilerError("Only 1D local rectangular arrays are currently supported"); this.eltType = nothing; } @@ -2393,6 +2599,9 @@ module Python { :returns: The size of the array. */ proc size: int throws { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + var size = PySequence_Size(this.getPyObject()); this.check(); return size; @@ -2406,9 +2615,13 @@ module Python { :returns: The item at the given index. */ proc get(idx: int): eltType throws { - var pyObj = PySequence_GetItem(this.getPyObject(), idx.safeCast(Py_ssize_t)); + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + var pyObj = PySequence_GetItem(this.getPyObject(), + idx.safeCast(Py_ssize_t)); this.check(); - return interpreter.fromPython(eltType, pyObj); + return interpreter.fromPythonInner(eltType, pyObj); } /* Set an item in the array. Equivalent to calling ``obj[idx] = item`` in @@ -2418,13 +2631,18 @@ module Python { :arg item: The item to set. */ proc set(idx: int, item: eltType) throws { - var pyItem = interpreter.toPython(item); + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); + + var pyItem = interpreter.toPythonInner(item); PySequence_SetItem(this.getPyObject(), idx.safeCast(Py_ssize_t), pyItem); this.check(); } } + // TODO: now that we handle this on every call to the python interpreter, + // is this still needed as a user facing type? /* Represents the Global Interpreter Lock, this is used to ensure that only one thread is executing python code at a time. Each thread should have its own @@ -2477,6 +2695,8 @@ module Python { } } + // TODO: now that we handle this on every call to the python interpreter, + // is this still needed as a user facing type? /* Represents the current thread state. This saves and restores the current thread state. @@ -2634,6 +2854,8 @@ module Python { extern type PyThreadState; type PyThreadStatePtr = c_ptr(PyThreadState); extern type PyGILState_STATE; + extern type PyInterpreterState; + type PyInterpreterStatePtr = c_ptr(PyInterpreterState); /* PyConfig From 5d55a6a9d8a0f56ea6027a9207ca628c5f761b88 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 08:32:42 -0800 Subject: [PATCH 03/11] add manual pthread Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 331 ++++++++++++++++++++++------------- 1 file changed, 211 insertions(+), 120 deletions(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index d4a6b01e7c03..edd97e3d3c71 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -353,6 +353,7 @@ module Python { private import this.ArrayTypes; private use CWChar; private use OS.POSIX only getenv; + private import this.PThread; /* Use 'objgraph' to detect memory leaks in the Python code. Care should be @@ -406,6 +407,85 @@ module Python { throw new ChapelException(message); } + @chpldoc.nodoc + record signalPair { + var setupDone: c_ptr(PThread.pthreadSignal); + var done: c_ptr(PThread.pthreadSignal); + } + @chpldoc.nodoc + var interpreterThreadError: owned Error? = nil; + private proc interpreterThread(arg: c_ptr(void)): c_ptr(void) { + + var signals = (arg:c_ptr(signalPair)).deref(); + + try { + // preinit + var preconfig: PyPreConfig; + PyPreConfig_InitIsolatedConfig(c_ptrTo(preconfig)); + preconfig.utf8_mode = 1; + checkPyStatus(Py_PreInitialize(c_ptrTo(preconfig))); + + // init + var config_: PyConfig; + var cfgPtr = c_ptrTo(config_); + // we want an isolated config for things like LC_ALL, but we still want + // to use things like PYTHONPATH + PyConfig_InitIsolatedConfig(cfgPtr); + config_.isolated = 0; + config_.use_environment = 1; + config_.user_site_directory = 1; + config_.site_import = 1; + defer PyConfig_Clear(cfgPtr); + + // check VIRTUAL_ENV, if its set, make it the executable + var venv = getenv("VIRTUAL_ENV".c_str()); + if venv != nil { + // ideally this just sets config_.home + // but by setting executable we can reuse the python logic to + // determine the locale (in the string sense, not the chapel sense) + const executable = string.createBorrowingBuffer(venv) + "/bin/python"; + const wideExecutable = executable.c_wstr(); + checkPyStatus( + PyConfig_SetString( + cfgPtr, c_ptrTo(config_.executable), wideExecutable)); + deallocate(wideExecutable); + } + + // initialize + checkPyStatus(Py_InitializeFromConfig(cfgPtr)); + } catch e { + // if any errors occur, signal that setup is done and exit + interpreterThreadError = e; + signals.setupDone.deref().signal(); + return nil; + } + + // + // grab the GIL and enter a state where we can invoke threads + // + var gil = PyGILState_Ensure(); + var ts = PyEval_SaveThread(); + + // + // setup is done + // + signals.setupDone.deref().signal(); + + // + // wait for deinit to be called + // + signals.done.deref().wait(); + + // + // restore the thread state so we can release the gil and exit + // + PyEval_RestoreThread(ts); + PyGILState_Release(gil); + Py_Finalize(); + return nil; + + } + /* Represents the python interpreter. All code using the Python module should create and maintain a single instance of this class. @@ -435,7 +515,9 @@ module Python { @chpldoc.nodoc var isSubInterpreter: bool; @chpldoc.nodoc - var done: sync bool = false; + var done: PThread.pthreadSignal; + @chpldoc.nodoc + var thread: PThread.pthread_t; @chpldoc.nodoc proc init(isSubInterpreter: bool = false) { @@ -445,144 +527,92 @@ module Python { @chpldoc.nodoc proc postinit() throws { if this.isSubInterpreter then return; - - // - // make sure 'done' is empty - // - done.readFE(); - // - // make sure 'setupDone' is empty - // - var setupDone: sync bool = false; - setupDone.readFE(); + var setupDone = new PThread.pthreadSignal(); + var signals = new signalPair(c_ptrTo(setupDone), c_ptrTo(this.done)); // // Do all Python initialization/finalization in a single task. // Initialization will be done, then this thread will block on 'done'. // When the deinit is called, it will unblock this thread and finalize + // NOTE: this is done with a manual pthread to work around + // https://github.com/chapel-lang/chapel/issues/26855 // - begin { - // preinit - var preconfig: PyPreConfig; - PyPreConfig_InitIsolatedConfig(c_ptrTo(preconfig)); - preconfig.utf8_mode = 1; - checkPyStatus(Py_PreInitialize(c_ptrTo(preconfig))); - - // init - var config_: PyConfig; - var cfgPtr = c_ptrTo(config_); - // we want an isolated config for things like LC_ALL, but we still want - // to use things like PYTHONPATH - PyConfig_InitIsolatedConfig(cfgPtr); - config_.isolated = 0; - config_.use_environment = 1; - config_.user_site_directory = 1; - config_.site_import = 1; - defer PyConfig_Clear(cfgPtr); - - // check VIRTUAL_ENV, if its set, make it the executable - var venv = getenv("VIRTUAL_ENV".c_str()); - if venv != nil { - // ideally this just sets config_.home - // but by setting executable we can reuse the python logic to - // determine the locale (in the string sense, not the chapel sense) - const executable = string.createBorrowingBuffer(venv) + "/bin/python"; - const wideExecutable = executable.c_wstr(); - checkPyStatus( - PyConfig_SetString( - cfgPtr, c_ptrTo(config_.executable), wideExecutable)); - deallocate(wideExecutable); - } + PThread.pthread_create(c_ptrTo(this.thread), + nil, + c_ptrTo(interpreterThread), + c_ptrTo(signals)); + + // + // blocks until setup is done, once setup is done we can return + // + setupDone.wait(); - // initialize - checkPyStatus(Py_InitializeFromConfig(cfgPtr)); + // check for an error from the pthread + if interpreterThreadError != nil { + throw interpreterThreadError; + } - var sys = PyImport_ImportModule("sys"); - this.checkException(); - defer Py_DECREF(sys); - // setup sys.path - { - var path = PyObject_GetAttrString(sys, "path"); - this.checkException(); - defer Py_DECREF(path); - - // add paths from PYTHONPATH to the front of PATH - var pythonPathCstr = getenv("PYTHONPATH".c_str()); - if pythonPathCstr != nil { - const pythonPath = string.createBorrowingBuffer(pythonPathCstr); - - var elms = pythonPath.split(getOsPathSepHelper(this)); - for elm in elms { - if elm.size == 0 then continue; - PyList_Append(path, Py_BuildValue("s", elm.c_str())); - this.checkException(); - } - } + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); - // add the current directory to the python path - PyList_Insert(path, 0, Py_BuildValue("s", ".")); - this.checkException(); - } - if !ArrayTypes.createArrayTypes() { - throwChapelException("Failed to create Python array types for Chapel arrays"); - } - if !ArrayTypes.registerArrayTypeEnum() { - throwChapelException("Failed to register Python array types for Chapel arrays"); - } + var sys = PyImport_ImportModule("sys"); + this.checkException(); + defer Py_DECREF(sys); - // - // grab the GIL and enter a state where we can invoke threads - // - var gil = PyGILState_Ensure(); - var ts = PyEval_SaveThread(); - - if pyMemLeaks { - // import objgraph - this.objgraph = this.importModuleInternal("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); + // setup sys.path + { + var path = PyObject_GetAttrString(sys, "path"); + this.checkException(); + defer Py_DECREF(path); + + // add paths from PYTHONPATH to the front of PATH + var pythonPathCstr = getenv("PYTHONPATH".c_str()); + if pythonPathCstr != nil { + const pythonPath = string.createBorrowingBuffer(pythonPathCstr); + + var elms = pythonPath.split(getOsPathSepHelper(this)); + for elm in elms { + if elm.size == 0 then continue; + PyList_Append(path, Py_BuildValue("s", elm.c_str())); this.checkException(); - Py_DECREF(res); - Py_DECREF(growth); } } - // - // setup is done - // - setupDone.writeEF(true); - - // - // wait for deinit to be called - // - done.readFE(); + // add the current directory to the python path + PyList_Insert(path, 0, Py_BuildValue("s", ".")); + this.checkException(); + } - // - // restore the thread state so we can release the gil and exit - // - PyEval_RestoreThread(ts); - PyGILState_Release(gil); - Py_Finalize(); + if !ArrayTypes.createArrayTypes() { + throwChapelException("Failed to create Python array types for Chapel arrays"); + } + if !ArrayTypes.registerArrayTypeEnum() { + throwChapelException("Failed to register Python array types for Chapel arrays"); + } + if pyMemLeaks { + // import objgraph + this.objgraph = this.importModuleInternal("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); + } } - // - // blocks until setup is done, once setup is done we can return - // - setupDone.readFE(); } @chpldoc.nodoc proc deinit() { - writeln("hello"); if this.isSubInterpreter then return; if pyMemLeaks && this.objgraph != nil { @@ -610,12 +640,11 @@ module Python { Py_DECREF(this.objgraph); } - writeln("can i get here"); - // // we are done, the thread keeping the interpreter alive can finish // - done.writeEF(true); + this.done.signal(); + PThread.pthread_join(this.thread, nil); } /* @@ -1561,13 +1590,11 @@ module Python { @chpldoc.nodoc proc deinit() { - writeln("calling deinit"); if this.isOwned { var g = PyGILState_Ensure(); defer PyGILState_Release(g); Py_DECREF(this.obj); } - writeln("done calling deinit"); } /* @@ -3219,4 +3246,68 @@ module Python { } } + + /* + A small subset of the pthread API + */ + @chpldoc.nodoc + module PThread { + private use CTypes; + + extern type pthread_t; + extern type pthread_attr_t; + extern type pthread_mutex_t; + extern type pthread_mutexattr_t; + extern type pthread_cond_t; + extern type pthread_condattr_t; + extern proc pthread_create(thread: c_ptr(pthread_t), + attr: c_ptr(pthread_attr_t), + start_routine: c_fn_ptr, + arg: c_ptr(void)): c_int; + + extern proc pthread_join(thread: pthread_t, + retval: c_ptr(c_ptr(void))): c_int; + extern proc pthread_mutex_init(mutex: c_ptr(pthread_mutex_t), + attr: c_ptr(pthread_mutexattr_t)): c_int; + extern proc pthread_mutex_destroy(mutex: c_ptr(pthread_mutex_t)): c_int; + extern proc pthread_mutex_lock(mutex: c_ptr(pthread_mutex_t)): c_int; + extern proc pthread_mutex_unlock(mutex: c_ptr(pthread_mutex_t)): c_int; + extern proc pthread_cond_init(cond: c_ptr(pthread_cond_t), + attr: c_ptr(pthread_condattr_t)): c_int; + extern proc pthread_cond_destroy(cond: c_ptr(pthread_cond_t)): c_int; + extern proc pthread_cond_wait(cond: c_ptr(pthread_cond_t), + mutex: c_ptr(pthread_mutex_t)): c_int; + extern proc pthread_cond_signal(cond: c_ptr(pthread_cond_t)): c_int; + + record pthreadSignal { + var mutex: pthread_mutex_t; + var cond: pthread_cond_t; + var signaled: bool = false; + proc init() { + init this; + pthread_mutex_init(c_ptrTo(mutex), nil); + pthread_cond_init(c_ptrTo(cond), nil); + } + proc ref deinit() { + pthread_mutex_destroy(c_ptrTo(mutex)); + pthread_cond_destroy(c_ptrTo(cond)); + } + proc ref signal() { + pthread_mutex_lock(c_ptrTo(mutex)); + signaled = true; + pthread_cond_signal(c_ptrTo(cond)); + pthread_mutex_unlock(c_ptrTo(mutex)); + } + proc ref wait() { + pthread_mutex_lock(c_ptrTo(mutex)); + while !signaled { + pthread_cond_wait(c_ptrTo(cond), c_ptrTo(mutex)); + } + signaled = false; + pthread_mutex_unlock(c_ptrTo(mutex)); + } + } + + } + } From e5aff95390094ed9b40359b4089d590fafc5ea1b Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 09:02:12 -0800 Subject: [PATCH 04/11] fix whitespace Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 42 ++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index edd97e3d3c71..334b11c9416e 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -460,25 +460,25 @@ module Python { return nil; } - // + // // grab the GIL and enter a state where we can invoke threads - // + // var gil = PyGILState_Ensure(); var ts = PyEval_SaveThread(); - // + // // setup is done - // + // signals.setupDone.deref().signal(); - // + // // wait for deinit to be called - // + // signals.done.deref().wait(); - // + // // restore the thread state so we can release the gil and exit - // + // PyEval_RestoreThread(ts); PyGILState_Release(gil); Py_Finalize(); @@ -531,21 +531,21 @@ module Python { var setupDone = new PThread.pthreadSignal(); var signals = new signalPair(c_ptrTo(setupDone), c_ptrTo(this.done)); - // + // // Do all Python initialization/finalization in a single task. // Initialization will be done, then this thread will block on 'done'. // When the deinit is called, it will unblock this thread and finalize // NOTE: this is done with a manual pthread to work around // https://github.com/chapel-lang/chapel/issues/26855 - // + // PThread.pthread_create(c_ptrTo(this.thread), nil, c_ptrTo(interpreterThread), c_ptrTo(signals)); - // + // // blocks until setup is done, once setup is done we can return - // + // setupDone.wait(); // check for an error from the pthread @@ -640,9 +640,9 @@ module Python { Py_DECREF(this.objgraph); } - // + // // we are done, the thread keeping the interpreter alive can finish - // + // this.done.signal(); PThread.pthread_join(this.thread, nil); } @@ -864,7 +864,7 @@ module Python { if Python.checkExceptions { var g = PyGILState_Ensure(); defer PyGILState_Release(g); - + var exc = chpl_PyErr_GetRaisedException(); if exc then throw PythonException.build(this, exc); } @@ -1782,7 +1782,7 @@ module Python { proc get(type t, attr: string): t throws { var g = PyGILState_Ensure(); defer PyGILState_Release(g); - + var pyAttr = PyObject_GetAttrString(this.getPyObject(), attr.c_str()); interpreter.checkException(); @@ -1803,7 +1803,7 @@ module Python { proc set(attr: string, value) throws { var g = PyGILState_Ensure(); defer PyGILState_Release(g); - + var pyValue = interpreter.toPythonInner(value); defer Py_DECREF(pyValue); @@ -1836,7 +1836,7 @@ module Python { where kwargs.isAssociative() { var g = PyGILState_Ensure(); defer PyGILState_Release(g); - + var pyArg = this.packTuple((...args)); defer Py_DECREF(pyArg); @@ -1854,7 +1854,7 @@ module Python { where kwargs.isAssociative() { var g = PyGILState_Ensure(); defer PyGILState_Release(g); - + var pyArg = this.packTuple((...args)); defer Py_DECREF(pyArg); @@ -1871,7 +1871,7 @@ module Python { proc call(type retType, method: string, const args...): retType throws { var g = PyGILState_Ensure(); defer PyGILState_Release(g); - + var pyArgs: args.size * PyObjectPtr; for param i in 0..#args.size { pyArgs(i) = interpreter.toPythonInner(args(i)); @@ -1918,7 +1918,7 @@ module Python { kwargs:?=none): retType throws where kwargs.isAssociative() { var g = PyGILState_Ensure(); defer PyGILState_Release(g); - + var pyArgs = Py_BuildValue("()"); defer Py_DECREF(pyArgs); From e433acfa4ec1250d29b9988a2c5652f9bc59da2f Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 09:12:41 -0800 Subject: [PATCH 05/11] remove manual gil Signed-off-by: Jade Abraham --- .../Python/correctness/compareIterationPatterns.chpl | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/library/packages/Python/correctness/compareIterationPatterns.chpl b/test/library/packages/Python/correctness/compareIterationPatterns.chpl index 3c5e9f1d1aea..ddf352825139 100644 --- a/test/library/packages/Python/correctness/compareIterationPatterns.chpl +++ b/test/library/packages/Python/correctness/compareIterationPatterns.chpl @@ -27,12 +27,9 @@ proc serialPythonApply(interp: borrowed, type t, arr, l) { proc parallelPythonApply(interp: borrowed, type t, arr, l) { var lambdaFunc = new Python.Function(interp, l); var res: [arr.domain] t; - var ts = new threadState(); - ts.save(); - forall i in arr.domain with (var gil = new Python.GIL()) { + forall i in arr.domain { res(i) = lambdaFunc(t, arr(i)); } - ts.restore(); return res; } // From a05ee2bd74608537a2d039abd94246b45a487721 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 09:13:00 -0800 Subject: [PATCH 06/11] add manual gil on files using internal functions Signed-off-by: Jade Abraham --- .../Python/correctness/loadingCode/fromCloudPickle.chpl | 4 +++- .../packages/Python/correctness/loadingCode/fromPickle.chpl | 3 +++ .../Python/correctness/loadingCode/moduleFromBytes.chpl | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/test/library/packages/Python/correctness/loadingCode/fromCloudPickle.chpl b/test/library/packages/Python/correctness/loadingCode/fromCloudPickle.chpl index dac6c3ea00fa..233dd7d2c0f1 100644 --- a/test/library/packages/Python/correctness/loadingCode/fromCloudPickle.chpl +++ b/test/library/packages/Python/correctness/loadingCode/fromCloudPickle.chpl @@ -2,13 +2,15 @@ use Python, IO; var interp = new Interpreter(); - +// must manually hold the GIL, since `loadPickle` is an internal function +var g = new GIL(); var MyClass = new Class(interp, interp.loadPickle(openReader("MyClass.pkl").readAll(bytes))); var my_function = new Function(interp, interp.loadPickle(openReader("my_function.pkl").readAll(bytes))); +g.release(); // run my_function var res = my_function(); diff --git a/test/library/packages/Python/correctness/loadingCode/fromPickle.chpl b/test/library/packages/Python/correctness/loadingCode/fromPickle.chpl index 4b62f9478057..5293023c51d3 100644 --- a/test/library/packages/Python/correctness/loadingCode/fromPickle.chpl +++ b/test/library/packages/Python/correctness/loadingCode/fromPickle.chpl @@ -3,6 +3,9 @@ use Python, IO; var interp = new Interpreter(); +// must manually hold the GIL, since `loadPickle` is an internal function +var g = new GIL(); var x = new Value(interp, interp.loadPickle(openReader("x.pkl").readAll(bytes))); +g.release(); writeln(x); diff --git a/test/library/packages/Python/correctness/loadingCode/moduleFromBytes.chpl b/test/library/packages/Python/correctness/loadingCode/moduleFromBytes.chpl index c4870bdbe904..546069df8b04 100644 --- a/test/library/packages/Python/correctness/loadingCode/moduleFromBytes.chpl +++ b/test/library/packages/Python/correctness/loadingCode/moduleFromBytes.chpl @@ -4,7 +4,10 @@ config const pycPath: string; use Python; var interp = new Interpreter(); +// must manually hold the GIL, since `loadPycFile` is an internal function +var g = new GIL(); var modBytes = interp.loadPycFile(pycPath); +g.release(); var mod = interp.importModule("mod", modBytes); // get MyClass and my_function From da49ccafca0c63bfe27af75495df0a34ae908772 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 09:14:19 -0800 Subject: [PATCH 07/11] fix memLeaks Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index 334b11c9416e..7dcd86403c06 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -616,6 +616,8 @@ module Python { if this.isSubInterpreter then return; if pyMemLeaks && this.objgraph != nil { + var g = PyGILState_Ensure(); + defer PyGILState_Release(g); // note: try! is used since we can't have a throwing deinit // run gc.collect() before showing growth From 44a457a7a2e14cc3f7796fd6510be3d744c57fe6 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 11:36:16 -0800 Subject: [PATCH 08/11] update docs Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 143 ++++------------------------------- 1 file changed, 14 insertions(+), 129 deletions(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index 7dcd86403c06..db98b0d7c0fa 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -60,123 +60,19 @@ Parallel Execution ------------------ - Running any Python code in parallel from Chapel requires special care, due - to the Global Interpreter Lock (GIL) in the Python interpreter. - This module supports two ways of doing this. + Running any Python code in parallel from Chapel will likely be no faster than + serial execution due to the Global Interpreter Lock (GIL) in the Python + interpreter. This module automatically handles the GIL in the public API. .. note:: Newer Python versions offer a free-threading mode that allows multiple threads concurrently. This currently requires a custom build of Python. If - you are using a Python like this, you should be able to use this module - freely in parallel code. + you are using a Python interpreter like this, the GIL handling code in this + module will become a no-op. - Using Multiple Interpreters - ~~~~~~~~~~~~~~~~~~~~~~~~~~~ - - .. warning:: - - Sub-interpreter support in Chapel is highly experimental and currently has - undefined behavior. - - The most performant way to run Python code in parallel is to use multiple - sub-interpreters. Each sub-interpreter is isolated from the others with its - own GIL. This allows multiple threads to run Python code concurrently. Note - that communication between sub-interpreters is severely limited and it is - strongly recommend to limit the amount of data shared between - sub-interpreters. - - .. note:: - - This feature is only available in Python 3.12 and later. Attempting to use - sub-interpreters with earlier versions of Python will result in a runtime - exception. - - The following demonstrates using sub-interpreters in a ``coforall`` loop: - - .. - START_TEST - FILENAME: CoforallTestSub.chpl - NOEXEC - START_GOOD - END_GOOD - - .. code-block:: chapel - - use Python; - - var code = """ - import sys - def hello(): - print('Hello from a task') - sys.stdout.flush() - """; - - proc main() { - var interpreter = new Interpreter(); - coforall 0..#4 { - var subInterpreter = new SubInterpreter(interpreter); - var m = subInterpreter.importModule('myMod', code); - var hello = m.get('hello'); - hello(); - } - } - - .. - END_TEST - - To make use of a sub-interpreter in a ``forall`` loop, the sub-interpreter - should be created as a task private variable. It is recommended that users - wrap the sub-interpreter in a ``record`` to initialize their Python objects, - to prevent duplicated work. For example, the following code creates multiple - sub-interpreters in a ``forall`` loop, where each task gets its own copy of - the module. - - .. - START_TEST - FILENAME: TaskPrivateSubInterp.chpl - NOEXEC - START_GOOD - END_GOOD - - .. code-block:: chapel - - use Python; - - record perTaskModule { - var i: owned SubInterpreter?; - var m: owned Module?; - proc init(parent: borrowed Interpreter, code: string) { - init this; - i = try! (new SubInterpreter(parent)); - m = try! (i!.importModule("anon", code)); - } - } - - proc main() { - var interpreter = new Interpreter(); - forall i in 1..10 - with (var mod = - new perTaskModule(interpreter, "x = 10")) { - writeln(mod.m!.get(int, "x")); - } - } - - .. - END_TEST - - Using A Single Interpreter - ~~~~~~~~~~~~~~~~~~~~~~~~~~ - - A single Python interpreter can execute code in parallel, as long as the GIL - is properly handled. Before any parallel execution with Python code can occur, - the thread state needs to be saved. After the parallel execution, the thread - state must to be restored. Then for each thread, the GIL must be acquired and - released. This is necessary to prevent segmentation faults and deadlocks in - the Python interpreter. - - The following demonstrates this when explicit tasks are introduced with a - ``coforall``: + The following demonstrates executing multiple Chapel tasks using a `coforall` + and a single Python interpreter: .. START_TEST @@ -194,21 +90,16 @@ var interp = new Interpreter(); var func = interp.compileLambda("lambda x,: x * x"); - var ts = new threadState(); - ts.save(); // save the thread state coforall tid in 1..10 { - var gil = new GIL(); // acquire the GIL + // the call to 'func' automatically acquires and releases the GIL Arr[tid] = func(int, tid); - // GIL is automatically released at the end of the block } - ts.restore(); // restore the thread state writeln(Arr); .. END_TEST - When using a data-parallel ``forall``, the GIL should be acquired and released - as a task private variable. + The code works similarly with a data-parallel ``forall`` loop: .. START_TEST @@ -226,20 +117,17 @@ var interp = new Interpreter(); var func = interp.compileLambda("lambda x,: x * x"); - var ts = new threadState(); - ts.save(); // save the thread state - forall tid in 1..10 with (var gil = new GIL()) { + forall tid in 1..10 { + // the call to 'func' automatically acquires and releases the GIL Arr[tid] = func(int, tid); } - ts.restore(); // restore the thread state writeln(Arr); .. END_TEST - In the examples above, because the GIL is being acquired and released for the - entirety of each task, these examples will be no faster than running the tasks - serially. + Although these examples use Chapel's task parallelism constructs, + they will be no faster than running the tasks serially due to the GIL. Using Python Modules With Distributed Code ------------------------------------------- @@ -272,12 +160,9 @@ const interp = new Interpreter(); const func = interp.compileLambda("lambda x,: x + 1"); - var ts = new threadState(); - ts.save(); forall i in Arr.localSubdomain() with (var gil = new GIL()) { Arr[i] = func(Arr.eltType, Arr[i]); } - ts.restore(); } } writeln(Arr); @@ -286,7 +171,7 @@ END_TEST In this example, ``interp`` and ``func`` only exist for the body of the - ``on`` block,Python objects can be made to persist beyond the scope of a + ``on`` block, Python objects can be made to persist beyond the scope of a given ``on`` block by creating a distributed array of Python objects. Defining Custom Types From 0c9a5d02e3fb4767d7d89d537ffd84e743b4e877 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 14:36:42 -0800 Subject: [PATCH 09/11] use defer for allocate Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index db98b0d7c0fa..57cafd466090 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -330,10 +330,10 @@ module Python { // determine the locale (in the string sense, not the chapel sense) const executable = string.createBorrowingBuffer(venv) + "/bin/python"; const wideExecutable = executable.c_wstr(); + defer deallocate(wideExecutable); checkPyStatus( PyConfig_SetString( cfgPtr, c_ptrTo(config_.executable), wideExecutable)); - deallocate(wideExecutable); } // initialize From e00b604b8487f7f854f58b7527fe7bb26e7a1f41 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 14:38:18 -0800 Subject: [PATCH 10/11] add comment Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index 57cafd466090..5d26d85cd00c 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -399,6 +399,9 @@ module Python { var objgraph: PyObjectPtr = nil; @chpldoc.nodoc var isSubInterpreter: bool; + + // the done signal is a field so it has the same lifetime as the + // helper thread @chpldoc.nodoc var done: PThread.pthreadSignal; @chpldoc.nodoc From 145012ab8a086767809151ab6322932e653beeb9 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 6 Mar 2025 15:15:35 -0800 Subject: [PATCH 11/11] apply feedback Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index 5d26d85cd00c..34d536e81670 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -322,7 +322,7 @@ module Python { config_.site_import = 1; defer PyConfig_Clear(cfgPtr); - // check VIRTUAL_ENV, if its set, make it the executable + // check VIRTUAL_ENV, if it is set, make it the executable var venv = getenv("VIRTUAL_ENV".c_str()); if venv != nil { // ideally this just sets config_.home @@ -2488,7 +2488,7 @@ module Python { :arg arr: The Chapel array to create the object from. */ proc init(in interpreter: borrowed Interpreter, ref arr: []) throws - where isSupportedArrayType(arr) { + where isSupportedArrayType(arr) { super.init(interpreter, nil: PyObjectPtr, isOwned=true); this.eltType = arr.eltType; init this; @@ -2498,7 +2498,7 @@ module Python { } @chpldoc.nodoc proc init(in interpreter: borrowed Interpreter, ref arr: []) throws - where !isSupportedArrayType(arr) { + where !isSupportedArrayType(arr) { compilerError("Only 1D local rectangular arrays are currently supported"); this.eltType = nothing; }