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

Upgrade pybind11 to 2.2.3

    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

            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.
            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.

              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: