From 92e3e963652801d5ef2fe424c81c6d9f22a071e1 Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Mon, 12 Aug 2024 14:20:18 +0200 Subject: [PATCH 1/7] nanobind: make dumps and loads nb::callable --- src/nrnpython/nrnpy_p2h.cpp | 57 ++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp index 0d765bb6a5..fe10febac6 100644 --- a/src/nrnpython/nrnpy_p2h.cpp +++ b/src/nrnpython/nrnpy_p2h.cpp @@ -1,6 +1,10 @@ #include <../../nrnconf.h> #include + +#include +namespace nb = nanobind; + #include #include #include @@ -607,23 +611,16 @@ static int guigetstr(Object* ho, char** cpp) { return 1; } -static PyObject* loads; -static PyObject* dumps; +static nb::callable loads; +static nb::callable dumps; static void setpickle() { if (dumps) { return; } - PyObject* pickle = PyImport_ImportModule("pickle"); - if (pickle) { - Py_INCREF(pickle); - dumps = PyObject_GetAttrString(pickle, "dumps"); - loads = PyObject_GetAttrString(pickle, "loads"); - if (dumps) { - Py_INCREF(dumps); - Py_INCREF(loads); - } - } + nb::module_ pickle = nb::module_::import_("pickle"); + dumps = pickle.attr("dumps"); + loads = pickle.attr("loads"); if (!dumps || !loads) { hoc_execerror("Neither Python cPickle nor pickle are available", 0); } @@ -632,7 +629,7 @@ static void setpickle() { // note that *size includes the null terminating character if it exists static std::vector pickle(PyObject* p) { PyObject* arg = PyTuple_Pack(1, p); - PyObject* r = nrnpy_pyCallObject(dumps, arg); + PyObject* r = nrnpy_pyCallObject(dumps.ptr(), arg); Py_XDECREF(arg); if (!r && PyErr_Occurred()) { PyErr_Print(); @@ -656,25 +653,21 @@ static std::vector po2pickle(Object* ho) { } } -static PyObject* unpickle(const char* s, std::size_t len) { - PyObject* ps = PyBytes_FromStringAndSize(s, len); - PyObject* arg = PyTuple_Pack(1, ps); - PyObject* po = nrnpy_pyCallObject(loads, arg); - assert(po); - Py_XDECREF(arg); - Py_XDECREF(ps); - return po; +static nb::object unpickle(const char* s, std::size_t len) { + nb::bytes string(s, len); + nb::list args; + args.append(string); + return loads(*args); } -static PyObject* unpickle(const std::vector& s) { +static nb::object unpickle(const std::vector& s) { return unpickle(s.data(), s.size()); } static Object* pickle2po(const std::vector& s) { setpickle(); - PyObject* po = unpickle(s); - Object* ho = nrnpy_pyobject_in_obj(po); - Py_XDECREF(po); + nb::object po = unpickle(s); + Object* ho = nrnpy_pyobject_in_obj(po.ptr()); return ho; } @@ -750,7 +743,7 @@ std::vector call_picklef(const std::vector& fname, int narg) { setpickle(); PyObject* ps = PyBytes_FromStringAndSize(fname.data(), fname.size()); args = PyTuple_Pack(1, ps); - callable = nrnpy_pyCallObject(loads, args); + callable = nrnpy_pyCallObject(loads.ptr(), args); assert(callable); Py_XDECREF(args); Py_XDECREF(ps); @@ -801,7 +794,9 @@ static PyObject* char2pylist(char* buf, int np, int* cnt, int* displ) { Py_INCREF(Py_None); // 'Fatal Python error: deallocating None' eventually PyList_SetItem(plist, i, Py_None); } else { - PyObject* p = unpickle(buf + displ[i], cnt[i]); + nb::object po = unpickle(buf + displ[i], cnt[i]); + po.inc_ref(); + PyObject* p = po.ptr(); PyList_SetItem(plist, i, p); } } @@ -874,7 +869,9 @@ static PyObject* py_broadcast(PyObject* psrc, int root) { nrnmpi_char_broadcast(buf.data(), cnt, root); PyObject* pdest = psrc; if (root != nrnmpi_myid) { - pdest = unpickle(buf); + nb::object po = unpickle(buf); + po.inc_ref(); + pdest = po.ptr(); } else { Py_INCREF(pdest); } @@ -1082,7 +1079,9 @@ static Object* py_alltoall_type(int size, int type) { delete[] sdispl; if (rcnt[0]) { - pdest = unpickle(r); + nb::object po = unpickle(r); + po.inc_ref(); + pdest = po.ptr(); } else { pdest = Py_None; Py_INCREF(pdest); From e2ed3315481e709ebb1e0c99f2b18c1cba84965c Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Mon, 12 Aug 2024 17:21:04 +0200 Subject: [PATCH 2/7] needed otherwise segfault when leaving executable --- src/nrnpython/nrnpy_p2h.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp index fe10febac6..c163f789ce 100644 --- a/src/nrnpython/nrnpy_p2h.cpp +++ b/src/nrnpython/nrnpy_p2h.cpp @@ -624,6 +624,8 @@ static void setpickle() { if (!dumps || !loads) { hoc_execerror("Neither Python cPickle nor pickle are available", 0); } + dumps.inc_ref(); + loads.inc_ref(); } // note that *size includes the null terminating character if it exists From 108ddc3a2dc1a42c4c1a6bd0c2df85c66cdc21ca Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Tue, 13 Aug 2024 16:44:53 +0200 Subject: [PATCH 3/7] Move header for compilation for windows --- src/nrnpython/nrnpy_p2h.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp index c163f789ce..f46287dc9b 100644 --- a/src/nrnpython/nrnpy_p2h.cpp +++ b/src/nrnpython/nrnpy_p2h.cpp @@ -2,9 +2,6 @@ #include -#include -namespace nb = nanobind; - #include #include #include @@ -15,6 +12,10 @@ namespace nb = nanobind; #include "oc_ansi.h" #include "parse.hpp" + +#include +namespace nb = nanobind; + static void nrnpy_decref_defer(PyObject*); static char* nrnpyerr_str(); static PyObject* nrnpy_pyCallObject(PyObject*, PyObject*); From d21724ca47cdd7fd4cfe16700e487a3a041e29f3 Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Wed, 14 Aug 2024 09:48:17 +0200 Subject: [PATCH 4/7] Use release() more often --- src/nrnpython/nrnpy_p2h.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp index f46287dc9b..52d63a0f3e 100644 --- a/src/nrnpython/nrnpy_p2h.cpp +++ b/src/nrnpython/nrnpy_p2h.cpp @@ -625,6 +625,10 @@ static void setpickle() { if (!dumps || !loads) { hoc_execerror("Neither Python cPickle nor pickle are available", 0); } + // We intentionally leak these, because if we don't + // we observe SEGFAULTS during application shutdown. + // Likely because "~nb::callable" runs after the Python + // library cleaned up. dumps.inc_ref(); loads.inc_ref(); } @@ -798,8 +802,7 @@ static PyObject* char2pylist(char* buf, int np, int* cnt, int* displ) { PyList_SetItem(plist, i, Py_None); } else { nb::object po = unpickle(buf + displ[i], cnt[i]); - po.inc_ref(); - PyObject* p = po.ptr(); + PyObject* p = po.release.ptr(); PyList_SetItem(plist, i, p); } } @@ -873,8 +876,7 @@ static PyObject* py_broadcast(PyObject* psrc, int root) { PyObject* pdest = psrc; if (root != nrnmpi_myid) { nb::object po = unpickle(buf); - po.inc_ref(); - pdest = po.ptr(); + pdest = po.release().ptr(); } else { Py_INCREF(pdest); } @@ -1083,8 +1085,7 @@ static Object* py_alltoall_type(int size, int type) { if (rcnt[0]) { nb::object po = unpickle(r); - po.inc_ref(); - pdest = po.ptr(); + pdest = po.release().ptr(); } else { pdest = Py_None; Py_INCREF(pdest); From 3c85dc119849ae6bc6e0c758d8e089aad0173b27 Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Wed, 14 Aug 2024 10:17:10 +0200 Subject: [PATCH 5/7] More typo --- src/nrnpython/nrnpy_p2h.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp index 52d63a0f3e..fddf59ebbe 100644 --- a/src/nrnpython/nrnpy_p2h.cpp +++ b/src/nrnpython/nrnpy_p2h.cpp @@ -802,7 +802,7 @@ static PyObject* char2pylist(char* buf, int np, int* cnt, int* displ) { PyList_SetItem(plist, i, Py_None); } else { nb::object po = unpickle(buf + displ[i], cnt[i]); - PyObject* p = po.release.ptr(); + PyObject* p = po.release().ptr(); PyList_SetItem(plist, i, p); } } From 7c93ae15cd4fe3abbdb900580459916663319ad4 Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Wed, 14 Aug 2024 11:44:40 +0200 Subject: [PATCH 6/7] nanobind: use loads and dumps as callable --- src/nrnpython/nrnpy_p2h.cpp | 46 ++++++++++++------------------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp index fddf59ebbe..00136deca1 100644 --- a/src/nrnpython/nrnpy_p2h.cpp +++ b/src/nrnpython/nrnpy_p2h.cpp @@ -635,19 +635,14 @@ static void setpickle() { // note that *size includes the null terminating character if it exists static std::vector pickle(PyObject* p) { - PyObject* arg = PyTuple_Pack(1, p); - PyObject* r = nrnpy_pyCallObject(dumps.ptr(), arg); - Py_XDECREF(arg); + nb::list args{}; + args.append(nb::borrow(p)); + auto r = nb::borrow(dumps(*args)); if (!r && PyErr_Occurred()) { PyErr_Print(); } assert(r); - assert(PyBytes_Check(r)); - std::size_t size = PyBytes_Size(r); - char* buf = PyBytes_AsString(r); - std::vector ret(buf, buf + size); - Py_XDECREF(r); - return ret; + return std::vector(r.c_str(), r.c_str() + r.size()); } static std::vector po2pickle(Object* ho) { @@ -743,33 +738,24 @@ std::vector call_picklef(const std::vector& fname, int narg) { // fname is a pickled callable, narg is the number of args on the // hoc stack with types double, char*, hoc Vector, and PythonObject // callable return must be pickleable. - PyObject* args = 0; - PyObject* result = 0; - PyObject* callable; - setpickle(); - PyObject* ps = PyBytes_FromStringAndSize(fname.data(), fname.size()); - args = PyTuple_Pack(1, ps); - callable = nrnpy_pyCallObject(loads.ptr(), args); + nb::bytes ps(fname.data(), fname.size()); + nb::list args{}; + args.append(ps); + + auto callable = nb::borrow(loads(*args)); assert(callable); - Py_XDECREF(args); - Py_XDECREF(ps); - args = PyTuple_New(narg); + args.clear(); for (int i = 0; i < narg; ++i) { - PyObject* arg = nrnpy_hoc_pop("call_picklef"); - if (PyTuple_SetItem(args, narg - 1 - i, arg)) { - assert(0); - } - // Py_XDECREF(arg); + nb::object arg = nb::steal(nrnpy_hoc_pop("call_picklef")); + args.append(arg); } - result = nrnpy_pyCallObject(callable, args); - Py_DECREF(callable); - Py_DECREF(args); + nb::object result = callable(*args); if (!result) { char* mes = nrnpyerr_str(); if (mes) { - Fprintf(stderr, "%s\n", mes); + std::cerr << mes << std::endl; free(mes); hoc_execerror("PyObject method call failed:", NULL); } @@ -777,9 +763,7 @@ std::vector call_picklef(const std::vector& fname, int narg) { PyErr_Print(); } } - auto rs = pickle(result); - Py_XDECREF(result); - return rs; + return pickle(result.ptr()); } #include "nrnmpi.h" From 54771d32da04847bcd6507fc315bf1402d5ed336 Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Thu, 15 Aug 2024 09:47:28 +0200 Subject: [PATCH 7/7] don't create list for args anymore --- src/nrnpython/nrnpy_p2h.cpp | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp index 00136deca1..0164c2b667 100644 --- a/src/nrnpython/nrnpy_p2h.cpp +++ b/src/nrnpython/nrnpy_p2h.cpp @@ -635,9 +635,7 @@ static void setpickle() { // note that *size includes the null terminating character if it exists static std::vector pickle(PyObject* p) { - nb::list args{}; - args.append(nb::borrow(p)); - auto r = nb::borrow(dumps(*args)); + auto r = nb::borrow(dumps(nb::borrow(p))); if (!r && PyErr_Occurred()) { PyErr_Print(); } @@ -656,10 +654,7 @@ static std::vector po2pickle(Object* ho) { } static nb::object unpickle(const char* s, std::size_t len) { - nb::bytes string(s, len); - nb::list args; - args.append(string); - return loads(*args); + return loads(nb::bytes(s, len)); } static nb::object unpickle(const std::vector& s) { @@ -740,13 +735,11 @@ std::vector call_picklef(const std::vector& fname, int narg) { // callable return must be pickleable. setpickle(); nb::bytes ps(fname.data(), fname.size()); - nb::list args{}; - args.append(ps); - auto callable = nb::borrow(loads(*args)); + auto callable = nb::borrow(loads(ps)); assert(callable); - args.clear(); + nb::list args{}; for (int i = 0; i < narg; ++i) { nb::object arg = nb::steal(nrnpy_hoc_pop("call_picklef")); args.append(arg);