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

Implement exception translators in upstream pybind11

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • None
    • None
    • 4
    • DRP F16-1, DRP F16-2
    • 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

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

            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.

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

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

            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 jbosch if you have time, please have a look and tell me what you think.

            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 jbosch if you have time, please have a look and tell me what you think.
            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.

            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.

            pschella – 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! )

            swinbank John Swinbank added a comment - pschella – 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! )

            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

            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

            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.

            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.

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

            Merged in upstream.

            pschella Pim Schellart [X] (Inactive) added a comment - Merged in upstream.

            People

              pschella Pim Schellart [X] (Inactive)
              pschella Pim Schellart [X] (Inactive)
              Jim Bosch
              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.