[BUG]: staticly import Python modules in C++ global scope #4263
Comments
|
I found a workaround (metaopt/optree@b0c4212). It works fine for Python 3.6-3.10 on Windows (https://github.com/metaopt/optree/actions/runs/3290405945/jobs/5423476779). Change: // header.h
static const py::module_ PyCollectionsModule = py::module_::import("collections");
static const py::object PyOrderedDictTypeObject = py::getattr(PyCollectionsModule, "OrderedDict");
static const py::object PyDefaultDictTypeObject = py::getattr(PyCollectionsModule, "defaultdict");
static const py::object PyDequeTypeObject = py::getattr(PyCollectionsModule, "deque");to // header.h
#ifdef _WIN32 // Windows
const py::object& ImportOrderedDict();
const py::object& ImportDefaultDict();
const py::object& ImportDeque();
#define PyOrderedDictTypeObject ImportOrderedDict()
#define PyDefaultDictTypeObject ImportDefaultDict()
#define PyDequeTypeObject ImportDeque()
#else // UNIX
static const py::module_ PyCollectionsModule = py::module_::import("collections");
static const py::object PyOrderedDictTypeObject = py::getattr(PyCollectionsModule, "OrderedDict");
static const py::object PyDefaultDictTypeObject = py::getattr(PyCollectionsModule, "defaultdict");
static const py::object PyDequeTypeObject = py::getattr(PyCollectionsModule, "deque");
#endif// impl.h
#ifdef _WIN32 // Windows
const py::object &ImportOrderedDict() {
static const py::module_ collections = py::module_::import("collections");
static const py::object object = py::getattr(collections, "OrderedDict");
return object;
}
const py::object &ImportDefaultDict() {
static const py::module_ collections = py::module_::import("collections");
static const py::object object = py::getattr(collections, "defaultdict");
return object;
}
const py::object &ImportDeque() {
static const py::module_ collections = py::module_::import("collections");
static const py::object object = py::getattr(collections, "deque");
return object;
}
#endif
|
|
I'm surprised your original code works on Linux at all. Calling cpython functions like import in a static context (outside of your module init functions) is generally a really bad idea because the Python interpreter might be in an inconsistent state. You original code was broken, not pybind11. Also, your fix (to function statics) would be greatly improved by switching to py::module_ pointers instead of direct py::module_s to avoid similar issues with running destructors in a bad context. As a general rule in C++, never use a class with a constructor or destructor as a global static due to initialization and deallocation issues. Thanks for the report though, pybind11 should definitely have better error reporting here. I'll see if I can write a PR with better error messages. At the very least, documentation should probably be improved. |
|
#4246 will probably fix the lack of good error messaging here |
|
Fixed with function static variables: #define PyCollectionsModule (*ImportCollections())
#define PyOrderedDictTypeObject (*ImportOrderedDict())
#define PyDefaultDictTypeObject (*ImportDefaultDict())
#define PyDequeTypeObject (*ImportDeque())
inline py::module_* ImportCollections() {
static auto collectionsUptr = std::make_unique<py::module_>(
py::reinterpret_borrow<py::module_>(py::module_::import("collections")));
return collectionsUptr.get();
}
inline py::object* ImportOrderedDict() {
static auto OrderedDictUptr = std::make_unique<py::object>(
py::reinterpret_borrow<py::object>(py::getattr(PyCollectionsModule, "OrderedDict")));
return OrderedDictUptr.get();
}
inline py::object* ImportDefaultDict() {
static auto defaultdictUptr = std::make_unique<py::object>(
py::reinterpret_borrow<py::object>(py::getattr(PyCollectionsModule, "defaultdict")));
return defaultdictUptr.get();
}
inline py::object* ImportDeque() {
static auto dequeUptr = std::make_unique<py::object>(
py::reinterpret_borrow<py::object>(py::getattr(PyCollectionsModule, "deque")));
return dequeUptr.get();
} |
|
@XuehaiPan That code is still not right. You absolutely should not be using unique_ptrs for those variables as you are not supposed to deallocate them in the global scope. Use simply raw pointers that you intentionally leak. |
@lalaland Thanks for the advice. To my best knowledge, as the references are explicitly borrowed (
Both #define PyCollectionsModule (ImportCollections())
#define PyOrderedDictTypeObject (ImportOrderedDict())
#define PyDefaultDictTypeObject (ImportDefaultDict())
#define PyDequeTypeObject (ImportDeque())
inline const py::module_& ImportCollections() {
static const py::module_* ptr = new py::module_{py::module_::import("collections")};
return *ptr;
}
inline const py::object& ImportOrderedDict() {
static const py::object* ptr = new py::object{py::getattr(PyCollectionsModule, "OrderedDict")};
return *ptr;
}
inline const py::object& ImportDefaultDict() {
static const py::object* ptr = new py::object{py::getattr(PyCollectionsModule, "defaultdict")};
return *ptr;
}
inline const py::object& ImportDeque() {
static const py::object* ptr = new py::object{py::getattr(PyCollectionsModule, "deque")};
return *ptr;
}The raw pointer version looks cleaner and I refactored my code in metaopt/optree#16. |
This is incorrect and might indicate a documentation issue on our side. Borrowed references do not mean that the PyObject is alive on std::unique_ptr deallocation. The borrowed vs stolen reference distinction is about whether the reference count for the PyObject gets increased when the py::object C++ wrapper is created. Reference counts are always decreased when py::objects are deallocated, whether it was originally from a stolen or borrowed reference. And when that reference count is decreased, we call a CPython function to do that. And that CPython function cannot be called in a global static context. Thus you cannot deallocate any py::objects in a global static context. So you cannot use unique_ptr here. (Not to mention that if the reference count actually drops to zero, it will call a ton of CPython functions to actually free that PyObject ...)
Your code might appear to work work, but it's undefined behavior. It might be silently corrupting data or change behavior whenever you change compilers, CPython versions, OSes, etc, etc. |
|
(Also, as a PS, that library for optimized PyTrees looks really nice. I use JAX quite frequently and I'll have to check it out!) |
XuehaiPan
changed the title
Required prerequisites
Problem description
Failed to load a
.pydlibrary if it contains global imports with Python 3.8-3.10 (Python 3.6-3.7 works fine). See https://github.com/metaopt/optree/actions/runs/3288272968/jobs/5418431116 for more details.Related code:
https://github.com/metaopt/optree/blob/fa1bb70f2e92f5a3c110c00fc1790dd20d36b72d/include/utils.h#L51-L56
For Python 3.8-3.10, the import fails if there are C++ global-level imports:
Everything works fine if I change this into a function static variable:
Reproducible example code
Directory structure:
Source files:
Command-line and results:
Python 3.7:
Python 3.10:
The text was updated successfully, but these errors were encountered: