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

Implement exception translators in upstream pybind11

    XMLWordPrintable

    Details

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

      Description

      Pybind11 does not currently support translation of custom exceptions. This ticket tracks work done on upstream pybind11 (internal fork https://github.com/lsst-dm/pybind11-1) to implement this functionality. It should support functionality equivalent to (but not necessarily with the same API) as Boost Python exception translators (http://www.boost.org/doc/libs/1_61_0/libs/python/doc/html/reference/high_level_components/boost_python_exception_translato.html).

        Attachments

          Issue Links

            Activity

            No builds found.
            pschella Pim Schellart [X] (Inactive) created issue -
            pschella Pim Schellart [X] (Inactive) made changes -
            Field Original Value New Value
            Link This issue is child task of DM-6302 [ DM-6302 ]
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Initial attempt ready for review at https://github.com/lsst-dm/pybind11-1/pull/1. I expect we will need to iterate over the design a bit, since we probably cannot change it easily once (if) it is accepted upstream.

            Current design is quite bare bones, but offers a great degree of flexibility. We may want to reduce boilerplate a bit by adding some convenience functions. But we also may choose not to.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Initial attempt ready for review at https://github.com/lsst-dm/pybind11-1/pull/1 . I expect we will need to iterate over the design a bit, since we probably cannot change it easily once (if) it is accepted upstream. Current design is quite bare bones, but offers a great degree of flexibility. We may want to reduce boilerplate a bit by adding some convenience functions. But we also may choose not to.
            pschella Pim Schellart [X] (Inactive) made changes -
            Reviewers Jim Bosch [ jbosch ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            One thing that the current design doesn't support (but I don't know if Swig does this too) is deriving from Python standard Exception. Thus one cannot catch custom exception types with except Exception as e:.
            This looks to be tricky (if even possible) to implement.

            Also note that in the current example the exception will leak. Need to fix this, but am hoping for more general design / use case coverage study for this round of review.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - One thing that the current design doesn't support (but I don't know if Swig does this too) is deriving from Python standard Exception . Thus one cannot catch custom exception types with except Exception as e: . This looks to be tricky (if even possible) to implement. Also note that in the current example the exception will leak. Need to fix this, but am hoping for more general design / use case coverage study for this round of review.
            Hide
            jbosch Jim Bosch added a comment -

            One thing that the current design doesn't support (but I don't know if Swig does this too) is deriving from Python standard Exception. Thus one cannot catch custom exception types with except Exception as e:.
            This looks to be tricky (if even possible) to implement.

            Swig does suffer from the exact same limitation, and I've worked around it with some pure-Python code and Swig macros that generate a new pure-Python class that inherits from Exception for every new C++ class; the Swig-wrapped C++ exception is then held by the pure-Python class, and we do _getattr_ forwarding to make them look like the same class.

            The same approach should work with Pybind11, and I propose that we go with that for now. I have some ideas for how we might clean things up in the future, either a little (by replacing the pure-Python class with a dynamically-created compiled Python class) or a lot (by adding support for inheriting from Python built-ins to Pybind11 itself.

            Show
            jbosch Jim Bosch added a comment - One thing that the current design doesn't support (but I don't know if Swig does this too) is deriving from Python standard Exception. Thus one cannot catch custom exception types with except Exception as e: . This looks to be tricky (if even possible) to implement. Swig does suffer from the exact same limitation, and I've worked around it with some pure-Python code and Swig macros that generate a new pure-Python class that inherits from Exception for every new C++ class; the Swig-wrapped C++ exception is then held by the pure-Python class, and we do _ getattr _ forwarding to make them look like the same class. The same approach should work with Pybind11, and I propose that we go with that for now. I have some ideas for how we might clean things up in the future, either a little (by replacing the pure-Python class with a dynamically-created compiled Python class) or a lot (by adding support for inheriting from Python built-ins to Pybind11 itself.
            Hide
            jbosch Jim Bosch added a comment -

            I don't think I have anything to add beyond what I've commented on in the PR, and even that's pretty minor. Feel free to take this upstream whenever you're ready.

            Show
            jbosch Jim Bosch added a comment - I don't think I have anything to add beyond what I've commented on in the PR, and even that's pretty minor. Feel free to take this upstream whenever you're ready.
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            I am not entirely happy with the current design. In particular I don't like the fact that it requires user registered exception translators to always catch all exceptions. If they forget, it breaks the exception translation chain and goes immediately to the default translator. This can in particular be a problem when exception translators are registered by multiple different (potentially external) packages. I have now added an alternative design on a separate branch (tickets/DM-6591-alternative).

            This design makes the default handing into just another exception translator which is always tried last. With this design user registered translators are no longer required to always catch all exceptions (which they may forget to do, breaking the exception translation chain). Instead they should only catch those exceptions they actually translate, leaving the rest to the next registered translator.

            A downside of this design is that the exception handling code in pybind11 itself is perhaps less readable.

            Although the current code is already marked as reviewed, I still think it is important to think about it a bit more. Especially since it will be hard to change once it is upstream. So Jim Bosch if you have time, please have a look and tell me what you think.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - I am not entirely happy with the current design. In particular I don't like the fact that it requires user registered exception translators to always catch all exceptions. If they forget, it breaks the exception translation chain and goes immediately to the default translator. This can in particular be a problem when exception translators are registered by multiple different (potentially external) packages. I have now added an alternative design on a separate branch (tickets/ DM-6591 -alternative). This design makes the default handing into just another exception translator which is always tried last. With this design user registered translators are no longer required to always catch all exceptions (which they may forget to do, breaking the exception translation chain). Instead they should only catch those exceptions they actually translate, leaving the rest to the next registered translator. A downside of this design is that the exception handling code in pybind11 itself is perhaps less readable. Although the current code is already marked as reviewed, I still think it is important to think about it a bit more. Especially since it will be hard to change once it is upstream. So Jim Bosch if you have time, please have a look and tell me what you think.
            Hide
            jbosch Jim Bosch added a comment -

            I definitely agree with the new direction; I hadn't fully recognized that big flaw in the old design. I've left some comments on the PR.

            Show
            jbosch Jim Bosch added a comment - I definitely agree with the new direction; I hadn't fully recognized that big flaw in the old design. I've left some comments on the PR.
            swinbank John Swinbank made changes -
            Epic Link DM-6168 [ 24680 ]
            swinbank John Swinbank made changes -
            Sprint DRP F16-1 [ 225 ]
            Story Points 4
            Team Data Release Production [ 10301 ]
            Hide
            swinbank John Swinbank added a comment -

            Pim Schellart [X] – I've added an entirely speculative SP count to this story. Please update it to something you think is more reasonable. (Bonus points if you remember to put in SP estimates before starting work in future! )

            Show
            swinbank John Swinbank added a comment - Pim Schellart [X] – I've added an entirely speculative SP count to this story. Please update it to something you think is more reasonable. (Bonus points if you remember to put in SP estimates before starting work in future! )
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            SP look fine (if anything a bit on the high end). I will! Although I think I remember telling you I was going to put this in as a 0 point story and assume the SP were contained by its parent

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - SP look fine (if anything a bit on the high end). I will! Although I think I remember telling you I was going to put this in as a 0 point story and assume the SP were contained by its parent
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment - - edited

            Updated design:

            When an exception is caught, give each registered exception translator a chance to translate it to a Python exception in reverse order of registration.

            A translator may choose to do one of the following:

            • catch the exception and call PyErr_SetString or PyErr_SetObject to set a standard (or custom) Python exception, or
            • do nothing and let the exception fall through to the next translator, or
            • delegate translation to the next translator by throwing a new type of exception.

            The last point is a little tricky, and breaks the ability to mentally view the set of translators as a single block of catch clauses. But it is what Boost does (or seems to do) and may be necessary
            to avoid code duplication.

            Have sent a link to this version to the pybind11 developers. Will now wait for their feedback.
            Assuming the design holds up I intend to use it for DM-6302 and formally it upstream if this suffices.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - - edited Updated design: When an exception is caught, give each registered exception translator a chance to translate it to a Python exception in reverse order of registration. A translator may choose to do one of the following: catch the exception and call PyErr_SetString or PyErr_SetObject to set a standard (or custom) Python exception, or do nothing and let the exception fall through to the next translator, or delegate translation to the next translator by throwing a new type of exception. The last point is a little tricky, and breaks the ability to mentally view the set of translators as a single block of catch clauses. But it is what Boost does (or seems to do) and may be necessary to avoid code duplication. Have sent a link to this version to the pybind11 developers. Will now wait for their feedback. Assuming the design holds up I intend to use it for DM-6302 and formally it upstream if this suffices.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Received very positive feedback on the latest design from the lead pybind11 developer.
            He is keen to accept the code.
            I propose we will attempt DM-6302 with this solution and close this ticket when the design works and is accepted upstream.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Received very positive feedback on the latest design from the lead pybind11 developer. He is keen to accept the code. I propose we will attempt DM-6302 with this solution and close this ticket when the design works and is accepted upstream.
            pschella Pim Schellart [X] (Inactive) made changes -
            Watchers Jim Bosch, John Swinbank, Pim Schellart [ Jim Bosch, John Swinbank, Pim Schellart ] Andy Salnikov, Jim Bosch, John Swinbank, Pim Schellart [ Andy Salnikov, Jim Bosch, John Swinbank, Pim Schellart ]
            swinbank John Swinbank made changes -
            Sprint DRP F16-1 [ 225 ] TCAMS 2016_03, DRP F16-1 [ 209, 225 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            swinbank John Swinbank made changes -
            Sprint TCAMS 2016_03, DRP F16-1 [ 209, 225 ] DRP F16-1, DRP F16-2 [ 225, 231 ]
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Merged in upstream.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Merged in upstream.
            pschella Pim Schellart [X] (Inactive) made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              Assignee:
              pschella Pim Schellart [X] (Inactive)
              Reporter:
              pschella Pim Schellart [X] (Inactive)
              Reviewers:
              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:

                  Jenkins

                  No builds found.