DM-25877: Change pybind11 to use a single importable module #26

timj · 2020-07-09T17:27:32Z

This is a rebase of the old pybind11-fat-wrapper branch from @smonkewitz . It builds fine but currently there is a segv in the pixelization test.

timj · 2020-07-09T17:37:57Z

It's the pixel method that is failing in HTM and Q3C (there is no test in Q3c):

>>> from lsst.sphgeom import HtmPixelization as H
>>> H(1)
HtmPixelization(1)
>>> H(1).pixel(10)
Segmentation fault: 11

The other methods from Pixelization seem to be okay.

timj · 2020-07-09T17:48:39Z

Not sure if this helps anyone:

>>> from lsst.sphgeom import HtmPixelization as H
>>> H(1)
HtmPixelization(1)
>>> H(1).pixel(10)
sphgeom.so was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 27552 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xa)
    frame #0: 0x00000001039ae11f sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(pybind11::detail::instance*, void const*) [inlined] long std::__1::__libcpp_atomic_refcount_increment<long>(__t=<unavailable>) at memory:3375:12 [opt]
   3372	__libcpp_atomic_refcount_increment(_Tp& __t) _NOEXCEPT
   3373	{
   3374	#if defined(_LIBCPP_HAS_BUILTIN_ATOMIC_SUPPORT) && !defined(_LIBCPP_HAS_NO_THREADS)
-> 3375	    return __atomic_add_fetch(&__t, 1, __ATOMIC_RELAXED);
   3376	#else
   3377	    return __t += 1;
   3378	#endif
Target 0: (python) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xa)
  * frame #0: 0x00000001039ae11f sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(pybind11::detail::instance*, void const*) [inlined] long std::__1::__libcpp_atomic_refcount_increment<long>(__t=<unavailable>) at memory:3375:12 [opt]
    frame #1: 0x00000001039ae11f sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(pybind11::detail::instance*, void const*) [inlined] std::__1::__shared_count::__add_shared(this=0x0000000000000002) at memory:3435 [opt]
    frame #2: 0x00000001039ae11f sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(pybind11::detail::instance*, void const*) [inlined] std::__1::__shared_weak_count::__add_shared(this=0x0000000000000002) at memory:3474 [opt]
    frame #3: 0x00000001039ae11f sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(pybind11::detail::instance*, void const*) [inlined] std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>::shared_ptr(this=<unavailable>, __r=std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>::element_type @ 0x00000001018503d0 strong=1 weak=1) at memory:4060 [opt]
    frame #4: 0x00000001039ae10a sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(pybind11::detail::instance*, void const*) [inlined] std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>::shared_ptr(this=<unavailable>, __r=std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>::element_type @ 0x00000001018503d0 strong=1 weak=1) at memory:4058 [opt]
    frame #5: 0x00000001039ae10a sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(pybind11::detail::instance*, void const*) [inlined] pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_holder_from_existing(v_h=<unavailable>, holder_ptr=0x00007ffeefbf2ce8) at pybind11.h:1283 [opt]
    frame #6: 0x00000001039ae106 sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(pybind11::detail::instance*, void const*) [inlined] pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_holder(inst=0x0000000101dcb1f0, v_h=<unavailable>, holder_ptr=0x00007ffeefbf2ce8, (null)=<unavailable>) at pybind11.h:1295 [opt]
    frame #7: 0x00000001039ae0fd sphgeom.so`pybind11::class_<lsst::sphgeom::ConvexPolygon, std::__1::shared_ptr<lsst::sphgeom::ConvexPolygon>, lsst::sphgeom::Region>::init_instance(inst=0x0000000101dcb1f0, holder_ptr=0x00007ffeefbf2ce8) at pybind11.h:1313 [opt]
    frame #8: 0x0000000103907505 sphgeom.so`pybind11::detail::type_caster_generic::cast(_src=<unavailable>, policy=<unavailable>, parent=<unavailable>, tinfo=<unavailable>, copy_constructor=<unavailable>, move_constructor=<unavailable>, existing_holder=0x00007ffeefbf2ce8)(void const*), void* (*)(void const*), void const*) at cast.h:562:9 [opt]
    frame #9: 0x00000001039851fb sphgeom.so`void pybind11::cpp_function::initialize<pybind11::cpp_function::cpp_function<std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> >, lsst::sphgeom::Pixelization, unsigned long long, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::arg>(std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> > (lsst::sphgeom::Pixelization::*)(unsigned long long) const, pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&, pybind11::arg const&)::'lambda'(lsst::sphgeom::Pixelization const*, unsigned long long), std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> >, lsst::sphgeom::Pixelization const*, unsigned long long, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::arg>(std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> >&&, lsst::sphgeom::Pixelization (*)(unsigned long long), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&, pybind11::arg const&)::'lambda'(pybind11::detail::function_call&)::operator()(pybind11::detail::function_call&) const [inlined] pybind11::detail::type_caster_base<lsst::sphgeom::Region>::cast_holder(src=<unavailable>, holder=<unavailable>) at cast.h:833:16 [opt]
    frame #10: 0x000000010398517b sphgeom.so`void pybind11::cpp_function::initialize<pybind11::cpp_function::cpp_function<std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> >, lsst::sphgeom::Pixelization, unsigned long long, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::arg>(std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> > (lsst::sphgeom::Pixelization::*)(unsigned long long) const, pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&, pybind11::arg const&)::'lambda'(lsst::sphgeom::Pixelization const*, unsigned long long), std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> >, lsst::sphgeom::Pixelization const*, unsigned long long, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::arg>(std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> >&&, lsst::sphgeom::Pixelization (*)(unsigned long long), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&, pybind11::arg const&)::'lambda'(pybind11::detail::function_call&)::operator()(pybind11::detail::function_call&) const [inlined] pybind11::detail::move_only_holder_caster<lsst::sphgeom::Region, std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> > >::cast(src=<unavailable>, (null)=<unavailable>, (null)=<unavailable>) at cast.h:1465 [opt]
    frame #11: 0x0000000103985177 sphgeom.so`void pybind11::cpp_function::initialize<pybind11::cpp_function::cpp_function<std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> >, lsst::sphgeom::Pixelization, unsigned long long, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::arg>(std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> > (lsst::sphgeom::Pixelization::*)(unsigned long long) const, pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&, pybind11::arg const&)::'lambda'(lsst::sphgeom::Pixelization const*, unsigned long long), std::__1::unique_ptr<lsst::sphgeom::Region, std::__1::default_delete<lsst::sphgeom::Region> >, lsst::sphgeom::Pixelization const*, unsigned long long, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::arg>(this=<unavailable>, call=<unavailable>)(unsigned long long), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&, pybind11::arg const&)::'lambda'(pybind11::detail::function_call&)::operator()(pybind11::detail::function_call&) const at pybind11.h:154 [opt]
    frame #12: 0x000000010390f949 sphgeom.so`pybind11::cpp_function::dispatcher(self=<unavailable>, args_in=<unavailable>, kwargs_in=0x0000000000000000) at pybind11.h:627:30 [opt]
    frame #13: 0x000000010002cfdb python`_PyMethodDef_RawFastCallKeywords + 395
    frame #14: 0x000000010002c88a python`_PyObject_FastCallKeywords + 602
    frame #15: 0x0000000100161ed3 python`call_function + 515
    frame #16: 0x00000001001597aa python`_PyEval_EvalFrameDefault + 20618
    frame #17: 0x0000000100153002 python`_PyEval_EvalCodeWithName + 498
    frame #18: 0x00000001001b76dc python`PyRun_InteractiveOneObjectEx + 876
    frame #19: 0x00000001001b409e python`PyRun_InteractiveLoopFlags + 462
    frame #20: 0x00000001001b3e9c python`PyRun_AnyFileExFlags + 60
    frame #21: 0x00000001001de02b python`pymain_run_file + 315
    frame #22: 0x00000001001dd870 python`pymain_run_filename + 128
    frame #23: 0x00000001001dd061 python`pymain_run_python + 145
    frame #24: 0x00000001001dcccb python`pymain_main + 27
    frame #25: 0x000000010000179d python`main + 125
    frame #26: 0x00007fff726c4cc9 libdyld.dylib`start + 1

timj · 2020-07-09T18:02:07Z

Turns out Pim fixed this one in commit 63a0e91 which references pybind/pybind11#1132 which still seems to be open.

All C++ classes are wrapped in the same module, resulting in decreased binary size and import time.

TallJimbo

Code looks good, but I'd prefer for some files to be renamed (especially the new sphgeom.h header) to match existing conventions. It would be preferable to rename the module and the master source file to _sphgeom.cc/_sphgeom.so as well.

python/lsst/sphgeom/SConscript

python/lsst/sphgeom/sphgeom.h

timj · 2020-07-09T18:43:41Z

Thanks. I'll make those changes. Should be quick.

TallJimbo · 2020-07-09T18:44:01Z

Oh, and we may someday need to revert @pschella's workaround for the pybind11 holder issue and replace it with something more intrusive and unpleasant - the problem with this fix is that it prohibits downstream C++ code from using shared_ptr to pass any of these objects through wrapped interfaces. But if this passes Jenkins, it means we're not trying to do that now, and maybe we never will.

smonkewitz and others added 2 commits July 9, 2020 11:15

Wrap sphgeom using a fat-wrapper strategy.

ed92089

All C++ classes are wrapped in the same module, resulting in decreased binary size and import time.

Update yaml loader to import everything from one shared module

be8651f

timj force-pushed the tickets/DM-25877 branch from 505eb29 to be8651f Compare July 9, 2020 18:16

Add a test for Q3cPixelization.pixel

f7aa22b

TallJimbo approved these changes Jul 9, 2020

View reviewed changes

python/lsst/sphgeom/SConscript Show resolved Hide resolved

python/lsst/sphgeom/sphgeom.h Outdated Show resolved Hide resolved

timj added 4 commits July 9, 2020 12:20

Relocate the python interface helper include file to standard location

d7a6f1e

Switch from single to double quotes

2e17fc5

Rename sphgeom.cc to _sphgeom.cc to match modern style

0f9dcf4

Rename all python interface C++ file to use leading underscore

b0bfd1d

timj merged commit d11eeda into master Jul 9, 2020

timj deleted the tickets/DM-25877 branch July 9, 2020 21:13

timj mentioned this pull request Jul 9, 2020

DM-25867: Make package pip installable #25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-25877: Change pybind11 to use a single importable module #26

DM-25877: Change pybind11 to use a single importable module #26

timj commented Jul 9, 2020

timj commented Jul 9, 2020

timj commented Jul 9, 2020

timj commented Jul 9, 2020 •

edited

TallJimbo left a comment

timj commented Jul 9, 2020

TallJimbo commented Jul 9, 2020

DM-25877: Change pybind11 to use a single importable module #26

DM-25877: Change pybind11 to use a single importable module #26

Conversation

timj commented Jul 9, 2020

timj commented Jul 9, 2020

timj commented Jul 9, 2020

timj commented Jul 9, 2020 • edited

TallJimbo left a comment

Choose a reason for hiding this comment

timj commented Jul 9, 2020

TallJimbo commented Jul 9, 2020

timj commented Jul 9, 2020 •

edited