From ebe21ec024db51a88075a5753a407728dbfabb77 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Wed, 8 Jan 2025 11:06:12 -0800 Subject: [PATCH 1/8] remove toFree Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 58 ++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 16 deletions(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index 409539cf897d..bded9fee7f5b 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -265,8 +265,6 @@ module Python { @chpldoc.nodoc var converters: List.list(owned TypeConverter); @chpldoc.nodoc - var toFree: List.list(PyObjectPtr); - @chpldoc.nodoc var objgraph: PyObjectPtr = nil; @chpldoc.nodoc @@ -353,10 +351,6 @@ module Python { @chpldoc.nodoc 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 @@ -585,7 +579,6 @@ module Python { where isArrayType(arr.type) && arr.rank == 1 && arr.isDefaultRectangular() { var pyList = PyList_New(arr.size.safeCast(c_size_t)); this.checkException(); - this.toFree.pushBack(pyList); for i in 0.. Date: Wed, 8 Jan 2025 11:06:19 -0800 Subject: [PATCH 2/8] add notest Signed-off-by: Jade Abraham --- test/library/packages/Python/examples/bs4/python_libs.notest | 0 test/library/packages/Python/examples/numba/python_libs.notest | 0 test/library/packages/Python/examples/torch/python_libs.notest | 0 3 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 test/library/packages/Python/examples/bs4/python_libs.notest create mode 100644 test/library/packages/Python/examples/numba/python_libs.notest create mode 100644 test/library/packages/Python/examples/torch/python_libs.notest diff --git a/test/library/packages/Python/examples/bs4/python_libs.notest b/test/library/packages/Python/examples/bs4/python_libs.notest new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/test/library/packages/Python/examples/numba/python_libs.notest b/test/library/packages/Python/examples/numba/python_libs.notest new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/test/library/packages/Python/examples/torch/python_libs.notest b/test/library/packages/Python/examples/torch/python_libs.notest new file mode 100644 index 000000000000..e69de29bb2d1 From d38468033ebf1994f3ad7deefcee9204ec6ac3ad Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 9 Jan 2025 14:41:40 -0800 Subject: [PATCH 3/8] cleanups for decref Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 217 +++++++++++++---------------------- 1 file changed, 77 insertions(+), 140 deletions(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index bded9fee7f5b..9b52e4c0c1fb 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -246,7 +246,6 @@ module Python { var sepPy = PyObject_GetAttrString(os, "pathsep"); interp.checkException(); - defer Py_DECREF(sepPy); var sep = interp.fromPython(string, sepPy); return sep; @@ -458,6 +457,8 @@ module Python { /* Convert a Chapel value to a python object. This clones the Chapel value. + This returns a new reference to a Python object. + .. note:: Most users should not need to call this directly, except when writing @@ -501,6 +502,7 @@ module Python { } else if isSubtype(t, List.list) { return toList(val); } else if isSubtype(t, Value) { + Py_INCREF(val.get()); return val.get(); } else if t == NoneType { return Py_None; @@ -512,6 +514,10 @@ module Python { /* Convert a Python object to a Chapel value. This clones the Python value. + This steals a reference to the Python object, so the Chapel object will + either own the Python object or it will decrement the reference count + when it is done with it. + .. note:: Most users should not need to call this directly, except when writing @@ -538,12 +544,9 @@ module Python { return v: t; // cast to the real type } else if isBoolType(t) { return if obj == Py_True then true else false; - } else if t == c_ptrConst(c_char) { - var v = PyUnicode_AsUTF8(obj); - this.checkException(); - return v; } else if t == string { var v = string.createCopyingBuffer(PyUnicode_AsUTF8(obj)); + Py_DECREF(obj); this.checkException(); return v; } else if isArrayType(t) { @@ -565,6 +568,9 @@ module Python { } else if isSubtype(t, Value) || isSubtype(t, Value?) { return new t(this, obj); } else if t == NoneType { + // returning NoneType can be used to ignore a return value + // but if its not actually None, we still need to decrement the reference count + if obj != Py_None then Py_DECREF(obj); return None; } else { halt("Unsupported fromPython type: '" + t:string + "'"); @@ -572,7 +578,7 @@ module Python { } /* - Converts an array to a python list + Converts an array to a python list. Returns new reference */ @chpldoc.nodoc proc toList(arr): PyObjectPtr throws @@ -588,7 +594,7 @@ module Python { } /* - Convert a chapel list to a python list + Convert a chapel list to a python list. Returns a new reference */ @chpldoc.nodoc proc toList(l: List.list(?)): PyObjectPtr throws { @@ -602,7 +608,7 @@ module Python { } /* - Converts a python list to an array + Converts a python list to an array. Steals a reference to obj. */ @chpldoc.nodoc proc fromList(type T, obj: PyObjectPtr): T throws @@ -628,7 +634,7 @@ module Python { } /* - Convert a python list to a Chapel list + Convert a python list to a Chapel list. Steals a reference to obj. */ @chpldoc.nodoc proc fromList(type T, obj: PyObjectPtr): T throws where isSubtype(T, List.list) { @@ -681,7 +687,7 @@ module Python { /* - Converts an associative array to a python dictionary + Converts an associative array to a python dictionary. Returns a new reference. */ @chpldoc.nodoc proc toDict(arr): PyObjectPtr throws @@ -700,7 +706,7 @@ module Python { // TODO: convert chapel map to python dict /* - Converts a python dictionary to an associative array + Converts a python dictionary to an associative array. Steals a reference to obj. */ proc fromDict(type T, obj: PyObjectPtr): T throws where isArrayType(T) { @@ -757,7 +763,6 @@ module Python { 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); @@ -854,6 +859,8 @@ module Python { } + var counter = 0; + /* Represents a Python value, it handles reference counting and is owned by default. */ @@ -911,15 +918,6 @@ module Python { */ proc get() do return this.obj; - /* - Returns the Chapel value of the object. - - This is a shortcut for calling :proc:`~Interpreter.fromPython` on this object. - */ - proc value(type value) throws { - return interpreter.fromPython(value, this.obj); - } - /* Stop owning val, return the underlying ptr. */ @@ -965,120 +963,72 @@ module Python { This handles conversion from Chapel types to Python types and back, by copying the Chapel types to Python types and back. + The keyword arguments should be passed as an associative Chapel array. + For example: + + .. code-block:: chapel + + var res = myObj(int, arg1, arg2, kwargs=["key1" => 42, "key2" => 17]); + :arg retType: The Chapel type of the return value. If the callable returns nothing, use :type:`NoneType`. :arg args: The arguments to pass to the callable. + :arg kwargs: The keyword arguments to pass to the callable. */ + pragma "last resort" + proc this(type retType, + const args..., + kwargs:?=none): retType throws + where kwargs.isAssociative() { + var pyArg = this.packTuple((...args)); + defer Py_DECREF(pyArg); + return callInternal(retType, pyArg, kwargs); + } + pragma "last resort" + @chpldoc.nodoc + proc this(type retType, + kwargs:?=none): retType throws where kwargs.isAssociative() { + var pyArgs = Py_BuildValue("()"); + defer Py_DECREF(pyArgs); + return callInternal(retType, pyArgs, kwargs); + } + @chpldoc.nodoc proc this(type retType, const args...): retType throws { - var pyArgs: args.size * PyObjectPtr; - for param i in 0..#args.size { - pyArgs(i) = interpreter.toPython(args(i)); - } - defer { - for param i in 0..#args.size { - Py_DECREF(pyArgs(i)); - } - } - - var pyRes; - if pyArgs.size == 1 then - pyRes = PyObject_CallOneArg(this.get(), pyArgs(0)); - else - pyRes = PyObject_CallFunctionObjArgs(this.get(), (...pyArgs), nil); - interpreter.checkException(); - - var res = interpreter.fromPython(retType, pyRes); - return res; + var pyArg = this.packTuple((...args)); + defer Py_DECREF(pyArg); + return callInternal(retType, pyArg, none); } - /* - Treat this value as a callable and call it with no arguments. - - See :proc:`~Value.this` for more information. - */ + @chpldoc.nodoc proc this(type retType): retType throws { var pyRes = PyObject_CallNoArgs(this.get()); - interpreter.checkException(); + this.check(); var res = interpreter.fromPython(retType, pyRes); return res; } - /* - Treat this value as a callable and call it with the given arguments and keyword arguments. - - The keyword arguments should be passed as an associative Chapel array. - For example: - - .. code-block:: chapel - - var res = myObj(int, arg1, arg2, kwargs=["key1" => 42, "key2" => 17]); - See :proc:`~Value.this` for more information. - */ - pragma "last resort" - proc this(type retType, const args..., kwargs:?t=none): retType throws - where kwargs.isAssociative() { + @chpldoc.nodoc + proc packTuple(const args...) throws { var pyArgs: args.size * PyObjectPtr; for param i in 0..#args.size { pyArgs(i) = interpreter.toPython(args(i)); } - var pyArg = PyTuple_Pack(args.size.safeCast(c_size_t), (...pyArgs)); - defer { - for param i in 0..#args.size { - Py_DECREF(pyArgs(i)); - } - Py_DECREF(pyArg); - } + defer for pyArg in pyArgs do Py_DECREF(pyArg); - var pyKwargs; - if t != nothing { - pyKwargs = interpreter.toPython(kwargs); - } else { - pyKwargs = nil; - } - defer { - if pyKwargs != nil { - Py_DECREF(pyKwargs); - } - } - - var pyRes = PyObject_Call(this.get(), pyArg, pyKwargs); + var pyArg = PyTuple_Pack(args.size.safeCast(c_size_t), (...pyArgs)); interpreter.checkException(); - var res = interpreter.fromPython(retType, pyRes); - return res; + return pyArg; } - /* - Treat this value as a callable and call it with the given keyword arguments. - - The keyword arguments should be passed as an associative Chapel array. - For example: - - .. code-block:: chapel - - var res = myObj(int, kwargs=["key1" => 42, "key2" => 17]); - - See :proc:`~Value.this` for more information. - */ - pragma "last resort" - proc this(type retType, kwargs:?t=none): retType throws - where kwargs.isAssociative() { - - var pyArg = Py_BuildValue("()"); - + @chpldoc.nodoc + proc callInternal(type retType, + pyArg: PyObjectPtr, kwargs: ?t): retType throws { var pyKwargs; - if t != nothing { - pyKwargs = interpreter.toPython(kwargs); - } else { - pyKwargs = nil; - } - defer { - if pyKwargs != nil { - Py_DECREF(pyKwargs); - } - } + if t != nothing then pyKwargs = interpreter.toPython(kwargs); + else pyKwargs = nil; - var pyRes = PyObject_Call(this.get(), pyArg, pyKwargs); - interpreter.checkException(); + var pyRes = PyObject_Call(this.obj, pyArg, pyKwargs); + this.check(); var res = interpreter.fromPython(retType, pyRes); return res; @@ -1111,7 +1061,7 @@ module Python { Import a Python module by name. */ proc init(interpreter: borrowed Interpreter, in modName: string) { - super.init(interpreter, PyImport_ImportModule(modName.c_str())); + super.init(interpreter, PyImport_ImportModule(modName.c_str()), isOwned=true); this.modName = modName; } } @@ -1124,15 +1074,15 @@ module Python { var fnName: string; proc init(mod: borrowed Module, in fnName: string) { - super.init(mod.interpreter, PyObject_GetAttrString(mod.get(), fnName.c_str())); + super.init(mod.interpreter, PyObject_GetAttrString(mod.get(), fnName.c_str()), isOwned=true); this.fnName = fnName; } - proc init(interpreter: borrowed Interpreter, in fnName: string, in obj: PyObjectPtr) { - super.init(interpreter, obj); + proc init(interpreter: borrowed Interpreter, in fnName: string, in obj: PyObjectPtr, isOwned: bool = true) { + super.init(interpreter, obj, isOwned=isOwned); this.fnName = fnName; } proc init(interpreter: borrowed Interpreter, in lambdaFn: string) throws { - super.init(interpreter, nil:PyObjectPtr); + super.init(interpreter, nil:PyObjectPtr, isOwned=true); this.fnName = ""; init this; this.obj = interpreter.compileLambda(lambdaFn); @@ -1146,32 +1096,22 @@ module Python { class Class: Value { var className: string; proc init(mod: borrowed Module, in className: string) { - super.init(mod.interpreter, PyObject_GetAttrString(mod.get(), className.c_str())); + super.init(mod.interpreter, PyObject_GetAttrString(mod.get(), className.c_str()), isOwned=true); this.className = className; } @chpldoc.nodoc proc newInstance(const args...): PyObjectPtr throws { - var pyArgs: args.size * PyObjectPtr; - for param i in 0..#args.size { - pyArgs(i) = interpreter.toPython(args(i)); - } - defer { - for param i in 0..#args.size { - Py_DECREF(pyArgs(i)); - } - } - - var pyRes = PyObject_CallFunctionObjArgs(this.get(), (...pyArgs), nil); - interpreter.checkException(); - + var pyArg = packTuple((...args)); + defer Py_DECREF(pyArg); + var pyRes = PyObject_Call(this.get(), pyArg, nil); + this.check(); return pyRes; } @chpldoc.nodoc proc newInstance(): PyObjectPtr throws { var pyRes = PyObject_CallNoArgs(this.get()); - interpreter.checkException(); - + this.check(); return pyRes; } @@ -1191,8 +1131,8 @@ module Python { */ class ClassObject: Value { @chpldoc.nodoc - proc init(interp: borrowed Interpreter, in obj: PyObjectPtr) { - super.init(interp, obj); + proc init(interp: borrowed Interpreter, in obj: PyObjectPtr, isOwned: bool = true) { + super.init(interp, obj, isOwned=isOwned); } proc setAttr(attr: string, value) throws { @@ -1208,11 +1148,7 @@ module Python { for param i in 0..#args.size { pyArgs(i) = interpreter.toPython(args(i)); } - defer { - for param i in 0..#args.size { - Py_DECREF(pyArgs(i)); - } - } + defer for pyArg in pyArgs do Py_DECREF(pyArg); var methodName = interpreter.toPython(method); defer Py_DECREF(methodName); @@ -1453,6 +1389,7 @@ module Python { 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 PyMem_Free(ptr: c_ptr(void)); extern proc PyObject_Str(obj: PyObjectPtr): PyObjectPtr; // `str(obj)` extern proc PyImport_ImportModule(name: c_ptrConst(c_char)): PyObjectPtr; From e801b4462e49a62b64b16990f00b25d63a03c9b7 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 9 Jan 2025 14:41:51 -0800 Subject: [PATCH 4/8] improve myModel test Signed-off-by: Jade Abraham --- test/library/packages/Python/examples/torch/myModel.chpl | 4 +++- test/library/packages/Python/examples/torch/mymodel.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/library/packages/Python/examples/torch/myModel.chpl b/test/library/packages/Python/examples/torch/myModel.chpl index e3574b24a2e0..6f7f020618f0 100644 --- a/test/library/packages/Python/examples/torch/myModel.chpl +++ b/test/library/packages/Python/examples/torch/myModel.chpl @@ -17,7 +17,8 @@ proc main() { // create the model var model = myModelClass(); - var input_tensor = tensor(owned Value, [[1.0,]], kwargs=["requires_grad" => true]); + var input_tensor = tensor(owned Value, [[1.0,]], kwargs=["requires_grad" => false]); + writeln("Input tensor: ", input_tensor); // init model weights to 0.9 { @@ -34,6 +35,7 @@ proc main() { // compute the loss var loss_fn = MSELoss(); var target = tensor(owned Value, [[2.0,]]); + writeln("Target: ", target); var loss = loss_fn(owned ClassObject, pred, target); loss.call(NoneType, "backward"); diff --git a/test/library/packages/Python/examples/torch/mymodel.py b/test/library/packages/Python/examples/torch/mymodel.py index 6066896732b4..ff214df8e273 100644 --- a/test/library/packages/Python/examples/torch/mymodel.py +++ b/test/library/packages/Python/examples/torch/mymodel.py @@ -11,7 +11,8 @@ def forward(self, x): def main(): # create the model model = MyModel() - input_tensor = torch.tensor([[1.0]], requires_grad=True) + input_tensor = torch.tensor([[1.0]], requires_grad=False) + print("Input tensor:", input_tensor) # init model weights to 0.9 for p in model.parameters(): @@ -23,6 +24,7 @@ def main(): # compute the loss loss_fn = torch.nn.MSELoss() target = torch.tensor([[2.0]]) + print("Target:", target) loss = loss_fn(pred, target) loss.backward() From 8e89d07a49194f4d1fff2d568f4423d2407d700e Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 9 Jan 2025 14:42:00 -0800 Subject: [PATCH 5/8] improve lambdas test Signed-off-by: Jade Abraham --- test/library/packages/Python/memleaks/lambdas.chpl | 14 +++++++++++++- test/library/packages/Python/memleaks/lambdas.good | 3 +++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/test/library/packages/Python/memleaks/lambdas.chpl b/test/library/packages/Python/memleaks/lambdas.chpl index 2920ea97c718..5388ea8560c7 100644 --- a/test/library/packages/Python/memleaks/lambdas.chpl +++ b/test/library/packages/Python/memleaks/lambdas.chpl @@ -8,6 +8,18 @@ proc main() { writeln(l(list(int), 1)); writeln(l(list(string), "hi")); - // this has a an extra lis + // this has an extra list writeln(l(list(list(int)), [1, 2, 3])); + + // chapelList([4,5,6]) + var l2 = new Function(interp, "lambda x,y,z,: [x,y,z]"); + // printing this out will cause a memleak, the __str__ method for list leaks + // e.g., 'print([[1,2,3])' in python leaks memory + l2(list(owned Value?), 1, "hi", [4, 5, 6]); + writeln("skipping potentially leaking print"); + + + var l3 = new Function(interp, "lambda x,: x"); + writeln(l3(list(int), [1,])); + writeln(l3(NoneType, [1,])); } diff --git a/test/library/packages/Python/memleaks/lambdas.good b/test/library/packages/Python/memleaks/lambdas.good index cea54b84f82e..af3cdb4bfafe 100644 --- a/test/library/packages/Python/memleaks/lambdas.good +++ b/test/library/packages/Python/memleaks/lambdas.good @@ -1,3 +1,6 @@ [1, 1] [hi, hi] [[1, 2, 3], [1, 2, 3]] +skipping potentially leaking print +[1] +() From 3209bd24845c9e030c2a0357aca8f6a56545bc4f Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 9 Jan 2025 14:54:54 -0800 Subject: [PATCH 6/8] add back Value.value Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index 9b52e4c0c1fb..210054c4fae8 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -918,6 +918,17 @@ module Python { */ proc get() do return this.obj; + /* + Returns the Chapel value of the object. + + This is a shortcut for calling :proc:`~Interpreter.fromPython` on this object, however it does not consume the object. + */ + proc value(type value) throws { + // fromPython will decrement the reference count, so we need to increment it + Py_INCREF(this.obj); + return interpreter.fromPython(value, this.obj); + } + /* Stop owning val, return the underlying ptr. */ From 966c622dff68a5ee41d2bad7e407ca110ff39ac2 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 14 Jan 2025 13:20:52 -0800 Subject: [PATCH 7/8] remove debug code Signed-off-by: Jade Abraham --- modules/packages/Python.chpl | 2 -- 1 file changed, 2 deletions(-) diff --git a/modules/packages/Python.chpl b/modules/packages/Python.chpl index 210054c4fae8..0f12efbe78fe 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -859,8 +859,6 @@ module Python { } - var counter = 0; - /* Represents a Python value, it handles reference counting and is owned by default. */ From 8b9c31c6283c25d8f1ad9f9b1dc0f50ddc38b8b0 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 14 Jan 2025 13:21:14 -0800 Subject: [PATCH 8/8] fix formatting 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 0f12efbe78fe..42b2cf774ed7 100644 --- a/modules/packages/Python.chpl +++ b/modules/packages/Python.chpl @@ -1398,7 +1398,7 @@ module Python { 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 PyMem_Free(ptr: c_ptr(void)); + extern proc PyMem_Free(ptr: c_ptr(void)); extern proc PyObject_Str(obj: PyObjectPtr): PyObjectPtr; // `str(obj)` extern proc PyImport_ImportModule(name: c_ptrConst(c_char)): PyObjectPtr;