Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-14828

Upgrade pybind11 to 2.2.3

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None

      Description

      Upgrade pybind11 from 2.1.x to 2.2.3, the current release and update our pybind11 wrappers to eliminate compiler warnings from use of deprecated pybind11 features.

        Attachments

          Issue Links

            Activity

            No builds found.
            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Risk Score 0
            rowen Russell Owen made changes -
            Link This issue relates to DM-11695 [ DM-11695 ]
            rowen Russell Owen made changes -
            Watchers Russell Owen [ Russell Owen ] Jim Bosch, Russell Owen [ Jim Bosch, Russell Owen ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-14864 [ DM-14864 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-15126 [ DM-15126 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-15132 [ DM-15132 ]
            rowen Russell Owen made changes -
            Story Points 2 6
            rowen Russell Owen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            rowen Russell Owen made changes -
            Epic Link DM-14447 [ 80385 ]
            rowen Russell Owen made changes -
            Sprint AP F18-2 [ 747 ]
            Team Alert Production [ 10300 ]
            rowen Russell Owen made changes -
            Link This issue mitigates DM-11820 [ DM-11820 ]
            Hide
            rowen Russell Owen added a comment - - edited

            Except as noted. the changes are trivial: merely replace PYBIND11_PLUGIN with the simpler PYBIND11_MODULE (which still works, but produces compiler warnings). If the reviewer is comfortable not looking at all those I'm fine with that. If I missed any, the code will still work but will produce compiler warnings.

            The more interesting packages are as follows:

            afw, meas_algorithms, pex_logging, pex_policy and sphgeom

            These all defined "placement new custom constructors" (({__init__}} methods), which are deprecated .

            That change is mechanical but a bit trickier. Most common errors, such as forgetting to add return, are caught by the compiler. However, forgetting to delete the existing self argument can only be caught by the compiler if the arguments are named (which they are, in most cases). In summary, this change is worth a closer look.

            Also a few packages used a == b to compare pybind11 python objects, which is deprecated in favor of a.is(b) so I updated those.

            In a few packages I also ran clang-format on the wrappers (as a separate commit).

            daf_base

            The pickle support for PropertyList was written as a pybind11 wrapper that called into a Python function. This was rather complicated and caused deprecation warnings when loading the module. Rather than modernize that code, I replaced it with a simple pure Python __reduce__ method on PropertyList, as per Jim Bosch's excellent suggestion. I then added pickle support to PropertySet in the same way, including tests similar to those for PropertyList.

            I also reworked two public functions getstate and setstate that were used in one other package: daf_persistence to persist property containers. The functions had isinstance switches to handle both PropertySet and PropertyList but the code that called those functions always knew the type. Also I felt the names were too ambiguous. So I broke them into 4 functions: getPropertySetState, getPropertyListState, setPropertySetState, and setPropertyListState.

            daf_persistence

            As described just above, this was adjusted to use the new public functions for getting and setting property container state – a very small code change.

            pybind11

            The usual 3rd party package update.

            Show
            rowen Russell Owen added a comment - - edited Except as noted. the changes are trivial: merely replace PYBIND11_PLUGIN with the simpler PYBIND11_MODULE (which still works, but produces compiler warnings). If the reviewer is comfortable not looking at all those I'm fine with that. If I missed any, the code will still work but will produce compiler warnings. The more interesting packages are as follows: afw, meas_algorithms, pex_logging, pex_policy and sphgeom These all defined "placement new custom constructors" (({__init__}} methods), which are deprecated . That change is mechanical but a bit trickier. Most common errors, such as forgetting to add return , are caught by the compiler. However, forgetting to delete the existing self argument can only be caught by the compiler if the arguments are named (which they are, in most cases). In summary, this change is worth a closer look. Also a few packages used a == b to compare pybind11 python objects, which is deprecated in favor of a.is(b) so I updated those. In a few packages I also ran clang-format on the wrappers (as a separate commit). daf_base The pickle support for PropertyList was written as a pybind11 wrapper that called into a Python function. This was rather complicated and caused deprecation warnings when loading the module. Rather than modernize that code, I replaced it with a simple pure Python __reduce__ method on PropertyList , as per Jim Bosch 's excellent suggestion. I then added pickle support to PropertySet in the same way, including tests similar to those for PropertyList . I also reworked two public functions getstate and setstate that were used in one other package: daf_persistence to persist property containers. The functions had isinstance switches to handle both PropertySet and PropertyList but the code that called those functions always knew the type. Also I felt the names were too ambiguous. So I broke them into 4 functions: getPropertySetState , getPropertyListState , setPropertySetState , and setPropertyListState . daf_persistence As described just above, this was adjusted to use the new public functions for getting and setting property container state – a very small code change. pybind11 The usual 3rd party package update.
            rowen Russell Owen made changes -
            Reviewers Nate Lust [ nlust ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            rowen Russell Owen made changes -
            Comment [ A deprecation warning to keep in mind (in afw); seen when running unit tests:
            {code}
            /Users/rowen/UW/LSST/lsstsw3/miniconda/lib/python3.6/importlib/_bootstrap.py:205: FutureWarning: pybind11-bound class 'lsst.pex.policy.policyFile.PolicyFile' is using an old-style placement-new '__init__' which has been deprecated. See the upgrade guide in pybind11's docs. This message is only visible when compiled in debug mode.
            {code} ]
            rowen Russell Owen made changes -
            Comment [ pybind11 2.2 [deprecates placement-new custom constructors |https://github.com/pybind/pybind11/blob/master/docs/upgrade.rst#new-api-for-defining-custom-constructors-and-pickling-functions]. Unfortunately the new syntax is not available in pybindd 2.1 so fixing this is part of the upgrade to pybind11 2.2. Affected packages include afw, meas_algorithms, pex_policy, sphgeom (based on a search for {{\_\_init\_\_}} in .cc files) ]
            rowen Russell Owen made changes -
            Risk Score 0 1
            rowen Russell Owen made changes -
            Comment [ I tried running Jenkins with 2.2.3 and afw failed with the following error:
            {code}
            /home/jenkins-slave/workspace/stack-os-matrix/centos-6.devtoolset-6.py3/lsstsw/stack/Linux64/pybind11/tickets.DM-14828-g441a3cd0df/include/pybind11/pybind11.h:65:9: required from 'pybind11::cpp_function::cpp_function(Func&&, const Extra& ...) [with Func = lsst::afw::detection::pybind11_init()::<lambda(lsst::afw::detection::FootprintSet&, std::shared_ptr<std::vector<std::shared_ptr<lsst::afw::detection::Footprint> > >)>; Extra = {pybind11::name, pybind11::is_method, pybind11::sibling}; <template-parameter-1-3> = void]'
            ::::: [2018-06-19T00:23:43.758666Z] /home/jenkins-slave/workspace/stack-os-matrix/centos-6.devtoolset-6.py3/lsstsw/stack/Linux64/pybind11/tickets.DM-14828-g441a3cd0df/include/pybind11/pybind11.h:1086:73: required from 'pybind11::class_<type_, options>& pybind11::class_<type_, options>::def(const char*, Func&&, const Extra& ...) [with Func = lsst::afw::detection::pybind11_init()::<lambda(lsst::afw::detection::FootprintSet&, std::shared_ptr<std::vector<std::shared_ptr<lsst::afw::detection::Footprint> > >)>; Extra = {}; type_ = lsst::afw::detection::FootprintSet; options = {std::shared_ptr<lsst::afw::detection::FootprintSet>, lsst::daf::base::Citizen}]'
            ::::: [2018-06-19T00:23:43.758681Z] python/lsst/afw/detection/footprintSet.cc:114:30: required from here
            ::::: [2018-06-19T00:23:43.758695Z] /home/jenkins-slave/workspace/stack-os-matrix/centos-6.devtoolset-6.py3/lsstsw/stack/Linux64/pybind11/tickets.DM-14828-g441a3cd0df/include/pybind11/pybind11.h:132:19: error: cannot convert 'pybind11::cpp_function::initialize(Func&&, Return (*)(Args ...), const Extra& ...) [with Func = lsst::afw::detection::pybind11_init()::<lambda(lsst::afw::detection::FootprintSet&, std::shared_ptr<std::vector<std::shared_ptr<lsst::afw::detection::Footprint> > >)>; Return = void; Args = {lsst::afw::detection::FootprintSet&, std::shared_ptr<std::vector<std::shared_ptr<lsst::afw::detection::Footprint>, std::allocator<std::shared_ptr<lsst::afw::detection::Footprint> > > >}; Extra = {pybind11::name, pybind11::is_method, pybind11::sibling}]::<lambda(pybind11::detail::function_call&)>' to 'pybind11::handle (*)(pybind11::detail::function_call&)' in assignment
            ::::: [2018-06-19T00:23:43.758709Z] rec->impl = [](function_call &call) -> handle {
            ::::: [2018-06-19T00:23:43.758723Z]
            {code} ]
            rowen Russell Owen made changes -
            Description Upgrade pybind11 from 2.1.x to 2.2.3, the current release.

            This will result in compiler warnings due to using an older macro in our wrappers.
            Upgrade pybind11 from 2.1.x to 2.2.3, the current release and update code to eliminate compiler warnings.
            rowen Russell Owen made changes -
            Description Upgrade pybind11 from 2.1.x to 2.2.3, the current release and update code to eliminate compiler warnings. Upgrade pybind11 from 2.1.x to 2.2.3, the current release and update our pybind11 wrappers to eliminate compiler warnings from use of deprecated pybind11 features.
            Hide
            nlust Nate Lust added a comment -

            A few things need looked at before this can be merged. There are some comments to address, and some packages need rebased.

            Show
            nlust Nate Lust added a comment - A few things need looked at before this can be merged. There are some comments to address, and some packages need rebased.
            Hide
            rowen Russell Owen added a comment -

            Nate Lust thank you for a really helpful and heroic review effort. I fixed everything except as follows:

            • The optional renaming of a class in a doc string; I left the old name because you allowed it and I didn't want to rock that boat
            • I have not yet figured out what is going on with qserv. My theory is that Jenkins is failing because it is building qserv against pybind11 2.1, which is bound to fail. If that passes then I am inclined to believe that the module/file name mismatch is unpleasant but works – worth fixing on a separate ticket, which I will file. I am closing this before figuring out qserv because that I worked on that package on a "best effort" basis, as a favor to the qserv team.
            Show
            rowen Russell Owen added a comment - Nate Lust thank you for a really helpful and heroic review effort. I fixed everything except as follows: The optional renaming of a class in a doc string; I left the old name because you allowed it and I didn't want to rock that boat I have not yet figured out what is going on with qserv. My theory is that Jenkins is failing because it is building qserv against pybind11 2.1, which is bound to fail. If that passes then I am inclined to believe that the module/file name mismatch is unpleasant but works – worth fixing on a separate ticket, which I will file. I am closing this before figuring out qserv because that I worked on that package on a "best effort" basis, as a favor to the qserv team.
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status In Review [ 10004 ] Done [ 10002 ]
            rowen Russell Owen made changes -
            Link This issue is triggering DM-15183 [ DM-15183 ]
            rowen Russell Owen made changes -
            Link This issue is triggering DM-15187 [ DM-15187 ]
            rowen Russell Owen made changes -
            Link This issue is triggering DM-15151 [ DM-15151 ]
            tjenness Tim Jenness made changes -
            Link This issue duplicates DM-11765 [ DM-11765 ]

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Nate Lust
              Watchers:
              Jim Bosch, Nate Lust, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.