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

Add support for deriving from Python exception types to 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

      DM-6302 adds support for custom exception translators to pybind11. However exceptions mapped do not inherit from Python BaseException or higher. This prevents exceptions from being raised and caught with except Exception as e in Python. This behaviour also occurs with Boost Python and Swig (we hack around it with a pure Python wrapper).

      This ticket aims to solve the problem by adding support for inheritance from Python exception types to pybind11.

        Attachments

          Issue Links

            Activity

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

            After a bit of hacking I have managed to add support for types derived from Python Exception to pybind11.
            So one can easily say (in C++ only, without requiring a Python wrapper) this exception type should inherit from x (pick a favourite Python exception type) and
            it all works. You can raise these exceptions in Python and catch them with except Exception as e as well as except mymodule.MyException as e.
            All that is needed is to add py::base<py::Exception> to your py::class_ line in the interface file.

            However, the inheritance tree can only be specified on one side of the fence.
            When types inherit both in C++ and Python (say a custom tree intersecting with the standard Python exception tree) things break down (i.e. segfault).
            From a first look it seems that fixing this would require somehow adding support for multiple inheritance to pybind11 as well.
            This seems like quite a big task that we might want to avoid if we don't absolutely need it.

            One way to do that is to only derive from Exception or BaseException such that raise and except Exception as e works on the Python side but forego on mapping some types to say ValueError and such;

            Is this an option? Or do we really need multiple inheritance?

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - After a bit of hacking I have managed to add support for types derived from Python Exception to pybind11. So one can easily say (in C++ only, without requiring a Python wrapper) this exception type should inherit from x (pick a favourite Python exception type) and it all works. You can raise these exceptions in Python and catch them with except Exception as e as well as except mymodule.MyException as e . All that is needed is to add py::base<py::Exception> to your py::class_ line in the interface file. However, the inheritance tree can only be specified on one side of the fence. When types inherit both in C++ and Python (say a custom tree intersecting with the standard Python exception tree) things break down (i.e. segfault). From a first look it seems that fixing this would require somehow adding support for multiple inheritance to pybind11 as well. This seems like quite a big task that we might want to avoid if we don't absolutely need it. One way to do that is to only derive from Exception or BaseException such that raise and except Exception as e works on the Python side but forego on mapping some types to say ValueError and such; Is this an option? Or do we really need multiple inheritance?
            Hide
            jbosch Jim Bosch added a comment -

            I think I'd prefer to just use the same workaround we did with Swig for now, and think about multiple inheritance and other options later. I'm very worried about having this and the ndarray upgrades take up the time that we could be spending converting afw, which will be a much more useful test (from the rest of DM's perspective) for whether we want to continue with the switch to pybind11. But I'd also like to continue with the principle of not making any interface changes we can avoid while doing the initial conversion, and instead undertake these later on.

            Show
            jbosch Jim Bosch added a comment - I think I'd prefer to just use the same workaround we did with Swig for now, and think about multiple inheritance and other options later. I'm very worried about having this and the ndarray upgrades take up the time that we could be spending converting afw, which will be a much more useful test (from the rest of DM's perspective) for whether we want to continue with the switch to pybind11. But I'd also like to continue with the principle of not making any interface changes we can avoid while doing the initial conversion, and instead undertake these later on.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            That might be true. If we are sure we won't require multiple inheritance anywhere else. I took this route because I thought it was both easier (less development time) and cleaner. But with multiple inheritance throwing in a wrench I might be wrong. The workaround will however also require changes to pybind11 I'm afraid. Unless I'm mistaken.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - That might be true. If we are sure we won't require multiple inheritance anywhere else. I took this route because I thought it was both easier (less development time) and cleaner. But with multiple inheritance throwing in a wrench I might be wrong. The workaround will however also require changes to pybind11 I'm afraid. Unless I'm mistaken.
            Hide
            jbosch Jim Bosch added a comment -

            We may need multiple inheritance elsewhere, but nothing jumps out at me, and I would prefer to avoid implementing it until we need it.

            I'd be curious to hear why you think the pure-Python workaround will also require changes to pybind11.

            Show
            jbosch Jim Bosch added a comment - We may need multiple inheritance elsewhere, but nothing jumps out at me, and I would prefer to avoid implementing it until we need it. I'd be curious to hear why you think the pure-Python workaround will also require changes to pybind11.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Since we decided to put this path on hold for now and instead use the pure Python workaround to deal with multiple inheritance, I moved the two remaining story points back to DM-6302 and call this done.

            We should still review it and think about sending it upstream for consideration, since we might want to build on this later. For now (I think) it makes pybind11 the only wrapper package with this capability

            To preempt review comments, the current solution does grow every objects holder by three pointers. This is unfortunate. I tried working around it with enable_if but that doesn't work because instance_essentials is assumed to be equal to instance_essentials<void> throughout pybind11 and cast to it without having access to the type. And Python requires those PyObject pointers to be at that position in the struct or else you get lots of seemingly random errors when it tries to interpret it as an Exception subclass. We can fix this (by storing some additional information the associated type_info record and switching on that) but given that we have chosen a different path I will not invest time in it now.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Since we decided to put this path on hold for now and instead use the pure Python workaround to deal with multiple inheritance, I moved the two remaining story points back to DM-6302 and call this done. We should still review it and think about sending it upstream for consideration, since we might want to build on this later. For now (I think) it makes pybind11 the only wrapper package with this capability To preempt review comments, the current solution does grow every objects holder by three pointers. This is unfortunate. I tried working around it with enable_if but that doesn't work because instance_essentials is assumed to be equal to instance_essentials<void> throughout pybind11 and cast to it without having access to the type. And Python requires those PyObject pointers to be at that position in the struct or else you get lots of seemingly random errors when it tries to interpret it as an Exception subclass. We can fix this (by storing some additional information the associated type_info record and switching on that) but given that we have chosen a different path I will not invest time in it now.
            Hide
            jbosch Jim Bosch added a comment -

            I don't remember how the upstream negotations impacted this ticket; is this still worth reviewing, or is it just Won't Fix?

            Show
            jbosch Jim Bosch added a comment - I don't remember how the upstream negotations impacted this ticket; is this still worth reviewing, or is it just Won't Fix?
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment - - edited

            Probably not worth a review at this point. But we might pick it up later.
            What went into upstream is a placeholder for creating custom exception types (a wrapper around PyErr_NewException) without allowing deriving from BaseException from C++.
            This allows users to define their own exception types, which was sufficient for upstream and Python 3, but not subclassing or additional methods / data (e.g. using the type as a pybind11::base).
            For that we would still need something like the work on this ticket.
            But we can probably indefinitely postpone this if we don't need it ourselves.
            If indefinite postponing is equivalent to Won't Fix feel free to mark it as such (with the caveat that we may have to pick it up later if we decide to clean-up exception handling once pybind11 supports multiple inheritance).

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - - edited Probably not worth a review at this point. But we might pick it up later. What went into upstream is a placeholder for creating custom exception types (a wrapper around PyErr_NewException ) without allowing deriving from BaseException from C++. This allows users to define their own exception types, which was sufficient for upstream and Python 3, but not subclassing or additional methods / data (e.g. using the type as a pybind11::base ). For that we would still need something like the work on this ticket. But we can probably indefinitely postpone this if we don't need it ourselves. If indefinite postponing is equivalent to Won't Fix feel free to mark it as such (with the caveat that we may have to pick it up later if we decide to clean-up exception handling once pybind11 supports multiple inheritance).
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Following discussion at this weeks standup I am marking this ticket as Done.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Following discussion at this weeks standup I am marking this ticket as Done.

              People

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

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.