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

Wrap pex_exceptions with pybind11

    Details

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

      Description

      While wrapping these packages, pay particular attention to exception translation (see Jim's bullet point 3: https://jira.lsstcorp.org/browse/RFC-182?focusedCommentId=48644&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-48644).

        Attachments

          Issue Links

            Activity

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

            DM-6591 design complete and reviewed. Can now use this design to try to wrap pex_exceptions.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - DM-6591 design complete and reviewed. Can now use this design to try to wrap pex_exceptions.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Now passes all pex_exception unit tests, except for those that depend on deriving from Python Exception. So this has to be looked at next.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Now passes all pex_exception unit tests, except for those that depend on deriving from Python Exception. So this has to be looked at next.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Reduced scope to be just pex_exceptions for easier review.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Reduced scope to be just pex_exceptions for easier review.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Now passes all unit tests with just pybind11 wrappers.
            Uses a similar workaround as with Swig to have a pure Python wrapper take care of deriving from Python Exception.
            Includes a new convenience function python::declare_exception to register custom exception types in other packages (similar in functionality to what we provided with the Swig %declareException macro).
            This all works in concert with the new exception translator support added to Python as part of DM-6783.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Now passes all unit tests with just pybind11 wrappers. Uses a similar workaround as with Swig to have a pure Python wrapper take care of deriving from Python Exception. Includes a new convenience function python::declare_exception to register custom exception types in other packages (similar in functionality to what we provided with the Swig %declareException macro). This all works in concert with the new exception translator support added to Python as part of DM-6783 .
            Hide
            salnikov Andy Salnikov added a comment -

            Pim, I finished my review, all comments are on PR. This is the first time I had to look at pex_exceptions, and I was a bit confused by multi-level wrapping. I'm still not sure I understand everything so some of my comments may be nonsense.

            Show
            salnikov Andy Salnikov added a comment - Pim, I finished my review, all comments are on PR. This is the first time I had to look at pex_exceptions , and I was a bit confused by multi-level wrapping. I'm still not sure I understand everything so some of my comments may be nonsense.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment - - edited

            Thanks for all your helpful suggestions. Definitely not nonsense I will try to clean things up a bit. I ported over too much of the direct Python API stuff from the Swig solution. This can be done much nicer with pybind11 RAII indeed.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - - edited Thanks for all your helpful suggestions. Definitely not nonsense I will try to clean things up a bit. I ported over too much of the direct Python API stuff from the Swig solution. This can be done much nicer with pybind11 RAII indeed.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment - - edited

            Merged (onto epic branch instead of master) after implementing comments.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - - edited Merged (onto epic branch instead of master) after implementing comments.

              People

              • Assignee:
                pschella Pim Schellart [X] (Inactive)
                Reporter:
                swinbank John Swinbank
                Reviewers:
                Andy Salnikov, Jim Bosch
                Watchers:
                Andy Salnikov, Jim Bosch, John Swinbank, Pim Schellart [X] (Inactive)
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel