Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python exception translation #947

Closed
foundrytom opened this issue May 30, 2023 · 5 comments · Fixed by #1190
Closed

Python exception translation #947

foundrytom opened this issue May 30, 2023 · 5 comments · Fixed by #1190
Assignees

Comments

@foundrytom
Copy link
Collaborator

foundrytom commented May 30, 2023

What

Ensure information is not lost from Python exceptions when bridged to C++

Also see #866 (duplicate issue)

Why

We currently lose information about Python exceptions when caught in C++.

When Python code calls a C++ function, and that C++ function raises an exception, there is a built-in hook via pybind11 to translate the C++ exception to a Python exception (see usages of register_exception_translator).

However, when C++ calls a Python function, and that Python function raises an exception, pybind11 will detect the Python exception and throw an (unhelpfully named) error_already_set exception. There is no pybind11 hook available to modify this.

In this case, consumers (hosts or manager plugins) must catch std::exception and interrogate the what() string to disambiguate.

We really need to devise a way to translate Python exceptions to the appropriate C++ exceptions, so that consumers can disambiguate using the exception type (i.e. in catch blocks).

An unfortunate wrinkle is in the special status that pybind11's error_already_set exception has within pybind11 itself. If this exception propagates back into Python, then the original Python exception is restored, complete with stack trace etc.

So ideally we would only conditionally translate an error_already_set exception into a more appropriate C++ exception, by somehow figuring out whether the exception will be caught in C++ or Python code.

ACs

  • Define approach for the above
@foundrytom foundrytom added this to the Core stable milestone May 30, 2023
@feltech
Copy link
Member

feltech commented May 30, 2023

I'd imagine any pybind11 trampoline class (i.e. anywhere we have a Python class that can inherit from C++) needs to have its member functions wrapped in try/catch, plus any other functions where we call out to Python (e.g. createPythonPluginSystemManagerImplementationFactory)

I wonder if multiple inheritance could help us here.

class openassetio::PluginError : std::runtime_error { ... };

class openassetio::PyPluginError : openassetio::PluginError, py::error_already_set { ... };

void rethrowCppException(std::exception_ptr pexc) {
  try {
    std::rethrow_exception(std::move(pexc))
  } catch (py::error_already_set& exc) {
    // ... disambiguate ... then:
   throw PyPluginError{exc};  // Absorb `error_already_set` and construct `PluginError`.
  }
}

void initialize(InfoDictionary managerSettings, const HostSessionPtr& hostSession) override {
  try {
    PYBIND11_OVERRIDE_PURE(void, ManagerInterface, initialize, std::move(managerSettings),
                           hostSession);
  } catch (...) {
    rethrowCppException(std::current_exception());
  }
}

Then C++ consumers can catch PluginError, and Python consumers will get the original Python exception. This might also work with catching in C++ then rethrowing back to Python.

@foundrytom
Copy link
Collaborator Author

Dont know if its relevant, but some prior art here too https://github.com/ImageEngine/cortex/blob/main/include/IECorePython/ExceptionAlgo.h

@feltech
Copy link
Member

feltech commented Jun 13, 2023

This issue duplicates #866. Closed that other issue in favour of this one.

@foundrytom foundrytom changed the title Python exception translation [Investigation] Python exception translation Oct 12, 2023
@foundrytom foundrytom modified the milestones: v1.0.0b1, v1.0.0 Oct 16, 2023
@feltech
Copy link
Member

feltech commented Oct 31, 2023

Looking specifically at the project linked above, ImageEngine's Cortex:

(Boost.Python) error_already_set catches are found around most of the Boost equivalent of pybind11's trampoline methods, translated to either a generic exception or using an exception_ptr stashed on the Python exception object.

@feltech
Copy link
Member

feltech commented Oct 31, 2023

Quick Compiler Explorer mockup of the sketch I did above, seems to be viable.

@feltech feltech self-assigned this Nov 1, 2023
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. In preparation for Python->C++ exception translation, and
to address previously encountered issues where Python and C++ exceptions
were accidentally out of sync, establish a C++ template-driven strategy
for binding and testing Python exceptions from a single list
(`kCppExceptionsAndPyClassNames`), leveraging compile-time assertions to
ensure the list is kept up to date.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Augment pybind11 override macros such that
OpenAssetIO-specific exceptions, thrown in the Python override of a C++
virtual method, will be converted from a Python exception (captured by
pybind11 as  an `error_already_set` exception) to an exception that can
propagate and be caught either in C++, as the corresponding C++
exception type, or back in Python.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Add a section to our coding standards notifying
contributors that they should always use the `OPENASSETIO_`-prefixed
pybind11 override macros.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Move Python->C++ exception translation utils to a shared
private header.

Primarily this was so that the functions can be used in the
openassetio-python-bridge target. However, on inspection there isn't
actually a current use-case for this in the Python bridge.

This change still seems worthwhile for future-proofing, and since it's a
little neater.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. In preparation for Python->C++ exception translation, and
to address previously encountered issues where Python and C++ exceptions
were accidentally out of sync, establish a C++ template-driven strategy
for binding and testing Python exceptions from a single list
(`kCppExceptionsAndPyClassNames`), leveraging compile-time assertions to
ensure the list is kept up to date.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Augment pybind11 override macros such that
OpenAssetIO-specific exceptions, thrown in the Python override of a C++
virtual method, will be converted from a Python exception (captured by
pybind11 as  an `error_already_set` exception) to an exception that can
propagate and be caught either in C++, as the corresponding C++
exception type, or back in Python.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Add a section to our coding standards notifying
contributors that they should always use the `OPENASSETIO_`-prefixed
pybind11 override macros.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Move Python->C++ exception translation utils to a shared
private header.

Primarily this was so that the functions can be used in the
openassetio-python-bridge target. However, on inspection there isn't
actually a current use-case for this in the Python bridge.

This change still seems worthwhile for future-proofing, and since it's a
little neater.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. In preparation for Python->C++ exception translation, and
to address previously encountered issues where Python and C++ exceptions
were accidentally out of sync, establish a C++ template-driven strategy
for binding and testing Python exceptions from a single list
(`kCppExceptionsAndPyClassNames`), leveraging compile-time assertions to
ensure the list is kept up to date.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Augment pybind11 override macros such that
OpenAssetIO-specific exceptions, thrown in the Python override of a C++
virtual method, will be converted from a Python exception (captured by
pybind11 as  an `error_already_set` exception) to an exception that can
propagate and be caught either in C++, as the corresponding C++
exception type, or back in Python.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Add a section to our coding standards notifying
contributors that they should always use the `OPENASSETIO_`-prefixed
pybind11 override macros.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 8, 2023
Part of OpenAssetIO#947. Move Python->C++ exception translation utils to a shared
private header.

Primarily this was so that the functions can be used in the
openassetio-python-bridge target. However, on inspection there isn't
actually a current use-case for this in the Python bridge.

This change still seems worthwhile for future-proofing, and since it's a
little neater.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 9, 2023
Part of OpenAssetIO#947. Augment pybind11 override macros such that
OpenAssetIO-specific exceptions, thrown in the Python override of a C++
virtual method, will be converted from a Python exception (captured by
pybind11 as  an `error_already_set` exception) to an exception that can
propagate and be caught either in C++, as the corresponding C++
exception type, or back in Python.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 9, 2023
Part of OpenAssetIO#947. Add a section to our coding standards notifying
contributors that they should always use the `OPENASSETIO_`-prefixed
pybind11 override macros.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 9, 2023
Part of OpenAssetIO#947. Move Python->C++ exception translation utils to a shared
private header.

Primarily this was so that the functions can be used in the
openassetio-python-bridge target. However, on inspection there isn't
actually a current use-case for this in the Python bridge.

This change still seems worthwhile for future-proofing, and since it's a
little neater.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 15, 2023
Part of OpenAssetIO#947. In order to translate exceptions from Python->C++ (as well
as work around other pybind11 issues), we must always use the
`OPENASSETIO_`-prefixed `PYBIND11_OVERRIDE` macros, which augment
pybind11's implementation with our patches.

So add a simple CI `grep` check to enforce this.

Re-use the existing check for "OAIO" and re-purpose as a generic
"disallowed phrases" check. This may also be useful for OpenAssetIO#1112.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 15, 2023
Part of OpenAssetIO#947. In preparation for Python->C++ exception translation, and
to address previously encountered issues where Python and C++ exceptions
were accidentally out of sync, establish a C++ template-driven strategy
for binding and testing Python exceptions from a single list
(`kCppExceptionsAndPyClassNames`), leveraging compile-time assertions to
ensure the list is kept up to date.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 15, 2023
Part of OpenAssetIO#947. Augment pybind11 override macros such that
OpenAssetIO-specific exceptions, thrown in the Python override of a C++
virtual method, will be converted from a Python exception (captured by
pybind11 as  an `error_already_set` exception) to an exception that can
propagate and be caught either in C++, as the corresponding C++
exception type, or back in Python.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 15, 2023
Part of OpenAssetIO#947. Add a section to our coding standards notifying
contributors that they should always use the `OPENASSETIO_`-prefixed
pybind11 override macros.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 15, 2023
Part of OpenAssetIO#947. Move Python->C++ exception translation utils to a shared
private header.

Primarily this was so that the functions can be used in the
openassetio-python-bridge target. However, on inspection there isn't
actually a current use-case for this in the Python bridge.

This change still seems worthwhile for future-proofing, and since it's a
little neater.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 15, 2023
Part of OpenAssetIO#947. In order to translate exceptions from Python->C++ (as well
as work around other pybind11 issues), we must always use the
`OPENASSETIO_`-prefixed `PYBIND11_OVERRIDE` macros, which augment
pybind11's implementation with our patches.

So add a simple CI `grep` check to enforce this.

Re-use the existing check for "OAIO" and re-purpose as a generic
"disallowed phrases" check. This may also be useful for OpenAssetIO#1112.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 15, 2023
Part of OpenAssetIO#947. In order to translate exceptions from Python->C++ (as well
as work around other pybind11 issues), we must always use the
`OPENASSETIO_`-prefixed `PYBIND11_OVERRIDE` macros, which augment
pybind11's implementation with our patches.

So add a simple CI `grep` check to enforce this.

Re-use the existing check for "OAIO" and re-purpose as a generic
"disallowed phrases" check. This may also be useful for OpenAssetIO#1112.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 15, 2023
Part of OpenAssetIO#947. In order to translate exceptions from Python->C++ (as well
as work around other pybind11 issues), we must always use the
`OPENASSETIO_`-prefixed `PYBIND11_OVERRIDE` macros, which augment
pybind11's implementation with our patches.

So add a simple CI `grep` check to enforce this.

Re-use the existing check for "OAIO" and re-purpose as a generic
"disallowed phrases" check. This may also be useful for OpenAssetIO#1112.

Signed-off-by: David Feltell <david.feltell@foundry.com>
@foundrytom foundrytom changed the title [Investigation] Python exception translation Python exception translation Nov 16, 2023
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 16, 2023
Part of OpenAssetIO#947. In preparation for Python->C++ exception translation, and
to address previously encountered issues where Python and C++ exceptions
were accidentally out of sync, establish a C++ template-driven strategy
for binding and testing Python exceptions from a single list
(`kCppExceptionsAndPyClassNames`), leveraging compile-time assertions to
ensure the list is kept up to date.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 16, 2023
Part of OpenAssetIO#947. Augment pybind11 override macros such that
OpenAssetIO-specific exceptions, thrown in the Python override of a C++
virtual method, will be converted from a Python exception (captured by
pybind11 as  an `error_already_set` exception) to an exception that can
propagate and be caught either in C++, as the corresponding C++
exception type, or back in Python.

Move macros under their own header, since there's a fair chunk of code
now, and since not every Python binding needs them.

Move Python->C++ exception translation utils to a shared private header.
Primarily this was so that the functions can be used in the
openassetio-python-bridge target. However, on inspection there isn't
actually a current use-case for this in the Python bridge. This change
still seems worthwhile for future-proofing, and since it's a little
neater.

Add a section to our coding standards notifying contributors that they
should always use the `OPENASSETIO_`-prefixed pybind11 override macros.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 16, 2023
Part of OpenAssetIO#947. In order to translate exceptions from Python->C++ (as well
as work around other pybind11 issues), we must always use the
`OPENASSETIO_`-prefixed `PYBIND11_OVERRIDE` macros, which augment
pybind11's implementation with our patches.

So add a simple CI `grep` check to enforce this.

Re-use the existing check for "OAIO" and re-purpose as a generic
"disallowed phrases" check. This may also be useful for OpenAssetIO#1112.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 17, 2023
Part of OpenAssetIO#947. In preparation for Python->C++ exception translation, and
to address previously encountered issues where Python and C++ exceptions
were accidentally out of sync, establish a C++ template-driven strategy
for binding and testing Python exceptions from a single list
(`kCppExceptionsAndPyClassNames`), leveraging compile-time assertions to
ensure the list is kept up to date.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 17, 2023
Part of OpenAssetIO#947. Augment pybind11 override macros such that
OpenAssetIO-specific exceptions, thrown in the Python override of a C++
virtual method, will be converted from a Python exception (captured by
pybind11 as  an `error_already_set` exception) to an exception that can
propagate and be caught either in C++, as the corresponding C++
exception type, or back in Python.

Move macros under their own header, since there's a fair chunk of code
now, and since not every Python binding needs them.

Move Python->C++ exception translation utils to a shared private header.
Primarily this was so that the functions can be used in the
openassetio-python-bridge target. However, on inspection there isn't
actually a current use-case for this in the Python bridge. This change
still seems worthwhile for future-proofing, and since it's a little
neater.

Add a section to our coding standards notifying contributors that they
should always use the `OPENASSETIO_`-prefixed pybind11 override macros.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO that referenced this issue Nov 17, 2023
Part of OpenAssetIO#947. In order to translate exceptions from Python->C++ (as well
as work around other pybind11 issues), we must always use the
`OPENASSETIO_`-prefixed `PYBIND11_OVERRIDE` macros, which augment
pybind11's implementation with our patches.

So add a simple CI `grep` check to enforce this.

Re-use the existing check for "OAIO" and re-purpose as a generic
"disallowed phrases" check. This may also be useful for OpenAssetIO#1112.

Signed-off-by: David Feltell <david.feltell@foundry.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants