Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: None
-
Labels:
-
Story Points:6
-
Epic Link:
-
Sprint:AP F18-2
-
Team:Alert Production
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
- duplicates
-
DM-11765 Update pybind11 wrappers to use PYBIND11_MODULE instead of init
- Invalid
- is blocked by
-
DM-14864 Make afw pybind11 wrappers compatible with pybind11 2.2.3
- Done
-
- Done
-
- Done
- is triggering
-
- Done
-
- Done
-
- Done
- mitigates
-
- Won't Fix
- relates to
-
- Invalid
Activity
A few things need looked at before this can be merged. There are some comments to address, and some packages need rebased.
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.
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.