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

Add support for pybind11 to build system

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None
    • Story Points:
      8
    • Sprint:
      DRP F16-1
    • Team:
      Data Release Production

      Description

      Add pybind11 as third party package to the stack. Update sconsUtils to support building with pybind11. Use daf_base DateTime to demonstrate that this works.

        Attachments

          Issue Links

            Activity

            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Ready for review. But note that I cannot yet run CI on it until RFC-196 is adopted and the pybind11 package is available. Also note that this ticket will not be merged to master after review, but will rather be merged onto the branch for the associated epic where all pybind11 commits will live until a change from Swig is approved.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Ready for review. But note that I cannot yet run CI on it until RFC-196 is adopted and the pybind11 package is available. Also note that this ticket will not be merged to master after review, but will rather be merged onto the branch for the associated epic where all pybind11 commits will live until a change from Swig is approved.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Also please note that the proposed pybind11 LSST package currently lives on https://github.com/lsst-dm/pybind11

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Also please note that the proposed pybind11 LSST package currently lives on https://github.com/lsst-dm/pybind11
            Hide
            rowen Russell Owen added a comment - - edited

            daf_base review: overall this looks very nice. I have a few comments on github. My only requested changse of any significance are:

            • Import explicit symbols or the module name instead of using from X import * in exampleLib.py
            • Please add _repr_ to the wrapped classes unless repr(obj) and str(obj) already print something useful
            • Please support keyword arguments in the constructors, especially if they have multiple arguments

            Could you please briefly explain (on this ticket, not as a code comment) what _exampleLib is in this expression: PYBIND11_PLUGIN(_exampleLib). I know it is standard pybind11, but I have not yet figured out what the symbol _exampleLib is or how it is used. It looks like an argument, but it has no type and is not used in the function.

            Show
            rowen Russell Owen added a comment - - edited daf_base review: overall this looks very nice. I have a few comments on github. My only requested changse of any significance are: Import explicit symbols or the module name instead of using from X import * in exampleLib.py Please add _ repr _ to the wrapped classes unless repr(obj) and str(obj) already print something useful Please support keyword arguments in the constructors, especially if they have multiple arguments Could you please briefly explain (on this ticket, not as a code comment) what _exampleLib is in this expression: PYBIND11_PLUGIN(_exampleLib) . I know it is standard pybind11, but I have not yet figured out what the symbol _exampleLib is or how it is used. It looks like an argument, but it has no type and is not used in the function.
            Hide
            rowen Russell Owen added a comment -

            sconsUtils looks fine. I put one minor documentation request on github.

            Show
            rowen Russell Owen added a comment - sconsUtils looks fine. I put one minor documentation request on github.
            Hide
            jbosch Jim Bosch added a comment -

            Could you please briefly explain (on this ticket, not as a code comment) what _exampleLib is in this expression: PYBIND11_PLUGIN(_exampleLib). I know it is standard pybind11, but I have not yet figured out what the symbol _exampleLib is or how it is used. It looks like an argument, but it has no type and is not used in the function.

            This defines an "extern C" free function named init_exampleLib that's called by Python when the module is imported. All compiled Python extension modules must define a function with that naming convention (i.e. function name is the module name, prefixed with "init") and a specific call signature, and they contain boilerplate for initializing modules that pybind11 handles for us.

            Show
            jbosch Jim Bosch added a comment - Could you please briefly explain (on this ticket, not as a code comment) what _exampleLib is in this expression: PYBIND11_PLUGIN(_exampleLib). I know it is standard pybind11, but I have not yet figured out what the symbol _exampleLib is or how it is used. It looks like an argument, but it has no type and is not used in the function. This defines an "extern C" free function named init_exampleLib that's called by Python when the module is imported. All compiled Python extension modules must define a function with that naming convention (i.e. function name is the module name, prefixed with "init") and a specific call signature, and they contain boilerplate for initializing modules that pybind11 handles for us.
            Hide
            tjenness Tim Jenness added a comment -

            Pim Schellart [X] there is no problem adding pybind11 to repos.yaml. You can add any eups packages you want there.

            Show
            tjenness Tim Jenness added a comment - Pim Schellart [X] there is no problem adding pybind11 to repos.yaml . You can add any eups packages you want there.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Merged into ticket branch for epic (DM-6168) instead of master.

            Made most of the requested changes, except adding keyword arguments for all constructors. I chose not to do this for two reasons:

            • It can create ambiguity with overloading.
            • I believe the scope of the trial to be limited to provide equivalent functionality. Making our C++ code behave more Pythonically raises design questions and can introduce complications upstream. I thus believe adding keyword arguments for constructors that don't have them on the C++ side to be out of scope. We should return to this after the (possible) migration.
            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Merged into ticket branch for epic ( DM-6168 ) instead of master. Made most of the requested changes, except adding keyword arguments for all constructors. I chose not to do this for two reasons: It can create ambiguity with overloading. I believe the scope of the trial to be limited to provide equivalent functionality. Making our C++ code behave more Pythonically raises design questions and can introduce complications upstream. I thus believe adding keyword arguments for constructors that don't have them on the C++ side to be out of scope. We should return to this after the (possible) migration.

              People

              Assignee:
              pschella Pim Schellart [X] (Inactive)
              Reporter:
              pschella Pim Schellart [X] (Inactive)
              Reviewers:
              Russell Owen
              Watchers:
              Colin Slater, Jim Bosch, John Swinbank, Pim Schellart [X] (Inactive), Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.