Details
Description
I have an issue with pybind11+afw at runtime with python. I think it doesn't show up for us in 2.2.4, I think because it sees unique_ptr<Background> as a non-default holder and std::shared_ptr as well, so it bypasses a specific check in pybind11 issued at runtime - I think that's what this issue below is discussing:
https://github.com/pybind/pybind11/issues/1317
The code of that check is here (pybind11 2.5.0):
https://github.com/pybind/pybind11/blob/v2.5.0/include/pybind11/attr.h#L266-L273
Our issue is that the base class of Background uses a std::unique_ptr as a holder, and BackgroundMI uses a std::shared_ptr:
https://github.com/lsst/afw/blob/master/python/lsst/afw/math/background/background.cc#L142-L154
This comment says a bit about it why the check exists:
https://github.com/pybind/pybind11/issues/1317#issuecomment-372128683
I tried the fix above that comment and I get a segfault when I did that.
(this is a diff of that fix)
diff --git a/python/lsst/afw/math/background/background.cc b/python/lsst/afw/math/background/background.cc
|
index 13173132c..c2c7d56ba 100644 |
--- a/python/lsst/afw/math/background/background.cc
|
+++ b/python/lsst/afw/math/background/background.cc
|
@@ -70,6 +70,10 @@ void declareGetImage(PyClass &cls, std::string const &suffix) { |
}
|
}
|
|
+// BackgroundMI Deleter to work around different holder types for Background/BackgroundMI |
+template<typename T> struct Deleter { void operator() (T *o) const { delete o; } }; |
+
|
+
|
PYBIND11_MODULE(background, mod) {
|
py::module::import("lsst.afw.image.image"); |
|
@@ -151,7 +155,8 @@ PYBIND11_MODULE(background, mod) { |
clsBackground.def("getBackgroundControl", (std::shared_ptr<BackgroundControl> (Background::*)()) & |
Background::getBackgroundControl);
|
|
- py::class_<BackgroundMI, std::shared_ptr<BackgroundMI>, Background> clsBackgroundMI(mod, "BackgroundMI"); |
+ // Workaround for: https://github.com/pybind/pybind11/issues/1317 |
+ py::class_<BackgroundMI, std::unique_ptr<BackgroundMI, Deleter<BackgroundMI>>, Background> clsBackgroundMI(mod, "BackgroundMI"); |
|
/* Constructors */ |
clsBackgroundMI.def(
|
@@ -179,4 +184,4 @@ PYBIND11_MODULE(background, mod) { |
}
|
}
|
}
|
-} // lsst::afw::math |
\ No newline at end of file
|
+} // lsst::afw::math |
This compiles fine but segfaults on mac and linux I believe, when tested against pybind11 2.5.0. (e.g. pytest tests/test_background.cc will segfault)
Attachments
Issue Links
Activity
in that case the destructor for Background needs to be public, right?
I didn't know we had any unique_ptr holders in the Stack. It's certainliy not recommended (probably for exactly this reason): https://developer.lsst.io/pybind11/style.html#the-shared-ptr-holder-type-should-be-used-for-all-non-trivial-classes
Krzysztof Findeisen
added a comment - - edited I didn't know we had any unique_ptr holders in the Stack. It's certainliy not recommended (probably for exactly this reason): https://developer.lsst.io/pybind11/style.html#the-shared-ptr-holder-type-should-be-used-for-all-non-trivial-classes in that case the destructor for Background needs to be public, right?
Yes, I hadn't realized that was part of the problem at first. And changing that (to a public virtual destructor) is right thing to do anyway: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-virtual
I am addressing this in DM-27064 branch
(for the record - I made Background's destructor public and used a shared_ptr)
I haven't followed up on all of the more recent issues, but I think this is just https://github.com/pybind/pybind11/pull/1161 (something Pim and I tried to get fixed upstream ages ago, but got no traction on), and the newer changes are just better failure modes for the same underlying limitation. If that's right, I think the only real fix is to change the base holder type to shared_ptr instead of changing the derived holder type to unique_ptr.