Feature/local exception translator #2650

jesse-sony · 2020-11-10T20:38:03Z

Description

Allow exception translators to be optionally registered local to a module instead of applying globally across all pybind11 modules.

This is useful in a situation where there are more than one compiled modules (e.g., clients for different micro-services) that share a lot of the same core code on the c++ side, including exceptions. Shared classes can be handled via the py::module_local() tag on the classes, but there is currently no way to deal with exception remapping.

So in code like:

    py::register_exception_translator([](std::exception_ptr p) {
      try {
        if (p) {
          std::rethrow_exception(p);
        }
      }
      catch (const ::vasg::exceptions::VasgException& e) {
        auto err = py::cast(e);
        auto errType = err.get_type().ptr();
        PyErr_SetObject(errType, err.ptr());
      }

The exception translator is global, so which module's exception you get back is determined by import order.

As an example, if you did something like

import tempo # compiled lib 

tempo.resolveKey(badPath)

you get back an exception from the tempo module.

However, if you were to do

import rhapsody # compiled lib
import tempo # compiled lib

tempo.resolveKey(badPath)

you'd get back an exception from the rhapsody module. Since those are different types in python (but the same type in c++), it means catch statements which are expecting the exception to come from the same module as the call are failing.

Suggested changelog entry:

Allow exception translators to be optionally registered local to a module instead of applying globally across all pybind11 modules. Use ``register_local_exception_translator(ExceptionTranslator&& translator)`` instead of  ``register_exception_translator(ExceptionTranslator&& translator)`` to keep your exception remapping code local to the module.

Why does this not require an Internals Version bump?

The internals struct in pybind11 is shared between all the modules. Any changes to that definitely requires a lot of careful thought about whether backwards compatibility is being broken. The local_internals that are added here should, by design, be segregated to a single module. Whether a module has local internals or not should not impact any other modules, because the only things accessing the local internals is the module that contains them.

rhaschke

I think this is an important feature and the implementation looks reasonable to me.

YannickJadoul

I general, I'm happy with this (but I was also involved in the Gitter discussion, so I'm not entirely well-placed to judge this).
This is also one of the things @wjakob should probably have a look at? Does it make sense to always check module-local exception translators before the global, shared ones?

We're still lacking changes to the actual documentation, by the way. A note about the existence of module-local exception/exception translators, and stating the order in which they are resolved is important.

Also, what about the auxiliary function register_exception? Should it get an extra argument, or somehow use the py::module_local() tag, or an extra template argument, or a sibling function register_module_local_exception, or ... ?

include/pybind11/detail/internals.h

YannickJadoul · 2020-11-16T23:42:01Z

include/pybind11/detail/internals.h

 /// Works like `internals.registered_types_cpp`, but for module-local registered types:
 inline type_map<type_info *> &registered_local_types_cpp() {
-    static type_map<type_info *> locals{};
-    return locals;
+    return get_module_internals().registered_local_types_cpp;


Not sure how other parts of pybind11 do it, but you do have a duplicate use of registered_local_types_cpp here (field as well as the function name). Can you check what conventions other parts of internals.h follow?

Well, I overcame my laziness and looked up myself. Can we go for get_registered_local_types_cpp as function name, for example? I don't think this should break ABI things.

would you prefer me to rename this function or just replace usages of it with get_module_internals().registered_local_types_cpp; so it works more like get_internals?

or would that break ABI? I don't think this function should be ever used in user code

Matching get_internals() closer might be nice, indeed, yes, if you ask me :-)
And no, this shouldn't break user code (since it's in the detail namespace), and ... yeah, no, the whole point is that this isn't shared between modules, so there's no shared ABI :-)

I have updated this to be called get_local_internals(), deleted get_registered_local_types_cpp() completely, and changed the usage to be more like get_internals(). Do we want to leave the double local in the names or match the member names to the internals member names?
currently:

get_local_internals().registered_local_types_cpp;

proposed change to remove the local in the member name

get_local_internals().registered_types_cpp;

YannickJadoul · 2020-11-16T23:43:46Z

include/pybind11/detail/internals.h

@@ -311,12 +311,23 @@ PYBIND11_NOINLINE inline internals &get_internals() {
    return **internals_pp;
 }

+
+struct module_internals {


Can/should we make this module_local_internals?

(same for get_module_local_internals, then, if we do)

I'd do either local_internals or module_internals to contrast with the normal internals. I don't see a reason to give it all the names. but if that is what people want, I'll do it

I was thinking of "module local" being a concept, but it's true it also wasn't done for registered_local_types_cpp. Maybe matching local is more important than module, though?

Let's see what others say.

currently went with local_internals and get_local_internals. obviously easy to change depending on what everyone wants

include/pybind11/pybind11.h

jesse-sony · 2020-11-17T17:16:53Z

Documentation has also been updated

YannickJadoul

Looks good to me. If this gets one or two more reviews, I think it can be merged :-)

docs/advanced/exceptions.rst

YannickJadoul · 2020-11-18T22:58:02Z

include/pybind11/cast.h

@@ -171,7 +171,7 @@ PYBIND11_NOINLINE inline detail::type_info* get_type_info(PyTypeObject *type) {
 }

 inline detail::type_info *get_local_type_info(const std::type_index &tp) {
-    auto &locals = registered_local_types_cpp();
+    auto &locals = get_local_internals().registered_local_types_cpp;


I'm fine with this, given it's 1) readable, and 2) matching the non-module-local internals. The suggested rename of registered_local_types_cpp to registered_types_cpp would even make sense to me, to avoid having two times local.

Let's check with other reviewers, though.

okay. My preference is to remove the second local too. Hopefully other people agree

@rwgk any thoughts on the naming in the local_internals struct?

struct local_internals { type_map<type_info *> registered_local_types_cpp; std::forward_list<void (*) (std::exception_ptr)> registered_local_exception_translators; };

vs

struct local_internals { type_map<type_info *> registered_types_cpp; std::forward_list<void (*) (std::exception_ptr)> registered_exception_translators; };

include/pybind11/detail/internals.h

YannickJadoul · 2020-11-18T22:59:03Z

include/pybind11/pybind11.h

+            auto & registered_local_exception_translators = get_local_internals().registered_local_exception_translators;
+            for (auto& translator : registered_local_exception_translators) {
+                try {
+                    translator(last_exception);
+                } catch (...) {
+                    last_exception = std::current_exception();
+                    continue;
+                }
+                return nullptr;
+            }


This is slightly repetitive, but fixing it might be more annoying than keeping the code duplication.

The sort of thing that chain from python is perfect for but doesn't seem to be std lib version of it

I think this is too much duplication, and I assume it's easy to move this to a helper function. Could you please try that? (Even if the number of lines of code is similar in the end, it's worth it IMO. Duplication easily leads to accidents when working on maintenance changes.)

this duplication has been removed and made into a helper. Let me know if you don't like something about my implementation.

YannickJadoul · 2020-11-18T23:01:57Z

include/pybind11/pybind11.h

+ * exception object and translator directly.
+ */
+template <typename CppException>
+exception<CppException> &register_local_exception(handle scope,


Other reviewers: should this be merged with register_exception? Answer: probably.

How do we best do this? An auxiliary helper with an extra runtime or template argument, and keep the current API with 2 functions? Or do we also want to merge these two functions in the API (in a backwards-compatible way, with a default argument at the end of the argument list or template arguments) ?

Yes, absolutely, this is too much duplication.
I think having two functions (register_exception, register_local_exception) is fine, but supported by detail::register_exception_impl or similar.
I'd try hard if necessary. This kind of duplication isn't healthy.

tests/test_exceptions.cpp

rwgk · 2021-07-15T14:20:45Z

Thanks @Skylion007 for pointing out this PR, I missed it before. I didn't look at all details, but I see others have already. My thinking: @jesse-sony if you rebase and resolve merge conflicts, I'll run the refreshed version through the Google-global testing (includes testing a large number of open source packages) asap, if that checks out I'll merge.

Since we now have two things that are going to be module local, it felt correct to add a struct to manage them.

These are added via the register_local_exception_translator function and are then applied before the global translators

Rename it to get_registered_local_types_cpp() to disambiguate from the new member of module_internals

jesse-sony · 2021-07-16T18:52:12Z

@rwgk Branch has been rebased and pushed. Should be in a good state to merge

rwgk · 2021-07-16T19:15:12Z

Thanks! My testing will take a day or so.

rwgk

The Google-global testing came back green, feature-wise this PR looks great to me, but the code duplication is a concern. I think it's important to go the extra mile for best practices straightaway. Usually such things don't get fixed later but rather tend to get duplicated even more (slippery slope).

rwgk · 2021-07-17T16:09:04Z

docs/advanced/exceptions.rst

+Local vs Global Exception Translators
+=====================================
+
+Similar to how the ``py::module_local`` flag allows uesrs to limit a bound class


This first sentence seems misplaced and the grammar incomplete. I think it reads better if you simply remove it here. The section title provides sufficient context.

rwgk · 2021-07-17T16:12:14Z

docs/advanced/exceptions.rst

+consistent error handling behavior is needed, then local translators should be
+used.
+
+Changing the previous example to use register_local_exception_translator would


double backquotes (code)

rwgk · 2021-07-17T16:13:11Z

docs/advanced/exceptions.rst

+mean that when invalid_argument is thrown in the module2 code, the module2
+translator will always handle it, while in module1, the module1 translator will
+do the same.
+>>>>>>> 854aa95a (Update documentation for new local exception feature)


Accident resolving merge conflict?

rwgk · 2021-07-17T16:24:21Z

include/pybind11/detail/internals.h

+
+/// Works like `get_internals`, but for things which are locally registered.
+inline local_internals &get_local_internals() {
+  static local_internals locals;


Did you think through the consequences of mixing extensions built with e.g. pybind11 2.7.0 and 2.7.1 (assuming it includes this change)?
At first sight it seems fine to me, but this is the kind of thing with devils in the details. Ideally, could you please add the rationale to the PR description (bottom) why it is safe to not bump the PYBIND11_INTERNALS_VERSION even though the locals type here changes, to document that we thought about it carefully?

My thought process on this (and I'm definitely not an expert in all the intricacies of pybind11 ABI compatibility) is that the internals struct in pybind11 is shared between all the modules. Any changes to that definitely requires a lot of careful thought about whether backwards compatibility is being broken. The local_internals that I added here should, by design, be segregated to a single module. Whether a module has local internals or not should not impact any other modules, because the only things accessing the local internals is the module that contains them.
If this makes sense to you as well, I'll happily add a comment to the end of this PR explaining that thought process

Make total sense to me, but I don't consider myself an authority on this, too. Adding this to the PR description is to expose it, in case someone is able to poke a hole into the rationale, we want to know asap.

rwgk · 2021-07-17T16:35:31Z

include/pybind11/pybind11.h

+            auto & registered_local_exception_translators = get_local_internals().registered_local_exception_translators;
+            for (auto& translator : registered_local_exception_translators) {
+                try {
+                    translator(last_exception);
+                } catch (...) {
+                    last_exception = std::current_exception();
+                    continue;
+                }
+                return nullptr;
+            }


I think this is too much duplication, and I assume it's easy to move this to a helper function. Could you please try that? (Even if the number of lines of code is similar in the end, it's worth it IMO. Duplication easily leads to accidents when working on maintenance changes.)

rwgk · 2021-07-17T16:36:36Z

include/pybind11/pybind11.h

+/**
+  * Add a new module-local exception translator. Locally registered functions
+  * will be tried before any globally registered exception translators, which
+  * will only be invoked if the moduke-local handlers do not deal with


typo (strange that codespell didn't get this one!?)

rwgk · 2021-07-17T16:47:31Z

include/pybind11/pybind11.h

+  * the exception.
+  */
+template <typename ExceptionTranslator>
+void register_local_exception_translator(ExceptionTranslator&& translator) {


I believe, unfortunately this duplicates something that's not great in the existing code: there is zero wiggle room for what ExceptionTranslator can be: it has to be void (*) (std::exception_ptr) exactly. I actually tried it out recently (https://github.com/pybind/pybind11/pull/3095/files) but didn't apply it to master. However, now that we get into it here, I think it's best to fold in the simplification, and to change the existing function and this new function to be simple (not template) inline functions with void (*translator) (std::exception_ptr) as the argument. Could you please give that a try? But please don't hesitate to push back if you run into trouble or if you think I'm overlooking something.

Okay, I did this. I did not change any of the type information, because I'm pretty sure that would be a PYBIND11_INTERNALS_VERSION ABI breaking change. internals still has

std::forward_list<ExceptionTranslator> registered_exception_translators;

not

std::forward_list<const ExceptionTranslator> registered_exception_translators;

as it did in your PR.

But I removed all the templating from the register functions. I also turned ExceptionTranslator into a using alias, which makes the typing a little bit nicer to read.

Yes, perfect, thanks. I did not mean to suggest adding the const. My PR was just to record that I thought about it carefully and then discarded the idea because I think it's too disruptive for too little gain.

rwgk · 2021-07-17T16:48:21Z

include/pybind11/pybind11.h

@@ -2085,6 +2114,32 @@ exception<CppException> &register_exception(handle scope,
    return ex;
 }

+/**
+ * Registers a Python exception in `m` of the given `name` and installs an exception translator to
+ * translate the C++ exception to the created Python exception using the exceptions what() method.


delete exceptions (to not repeat that word too much in one sentence)

rwgk · 2021-07-17T16:55:00Z

include/pybind11/pybind11.h

+ * exception object and translator directly.
+ */
+template <typename CppException>
+exception<CppException> &register_local_exception(handle scope,


Yes, absolutely, this is too much duplication.
I think having two functions (register_exception, register_local_exception) is fine, but supported by detail::register_exception_impl or similar.
I'd try hard if necessary. This kind of duplication isn't healthy.

Separated out the exception processing into a standalone function in the details namespace. Clean-up some comments as per PR notes as well

But I added a using declaration to alias the type.

rwgk · 2021-07-20T04:28:55Z

Hi @jesse-sony, today I only got a chance to glance through your comments really quickly, not even the code. The comments I saw look good, thanks!. I'll come back here asap.

rwgk

Looks awesome, thanks!

I'm still struggling a bit navigating this review system. If I overlooked a question you asked, please tag me again. (I tried to tie all my responses to the Review.)

I'm holding off merging until you make your final decision about the local_internals naming. Just let me know.

rwgk · 2021-07-20T15:06:07Z

include/pybind11/detail/internals.h

+
+/// Works like `get_internals`, but for things which are locally registered.
+inline local_internals &get_local_internals() {
+  static local_internals locals;


Make total sense to me, but I don't consider myself an authority on this, too. Adding this to the PR description is to expose it, in case someone is able to poke a hole into the rationale, we want to know asap.

rwgk · 2021-07-20T15:08:27Z

include/pybind11/pybind11.h

+  * the exception.
+  */
+template <typename ExceptionTranslator>
+void register_local_exception_translator(ExceptionTranslator&& translator) {


Yes, perfect, thanks. I did not mean to suggest adding the const. My PR was just to record that I thought about it carefully and then discarded the idea because I think it's too disruptive for too little gain.

rwgk · 2021-07-20T15:10:53Z

include/pybind11/detail/internals.h

-    return locals;
+
+struct local_internals {
+  type_map<type_info *> registered_local_types_cpp;


Answering the question you asked in a different comment:
I'd prefer the 2nd alternative you pointed out there, with local not repeated inside the struct.
But I'd approve either.

Names have been changed, since everyone seems to prefer them without the local

jesse-sony · 2021-07-21T00:40:52Z

@rwgk Cleaned up the naming, updated the PR description with a note about why it does not break ABI compatability with the internals struct, and also add some notes in the internals.h file about the same ABI stuff. Happy to make other changes if there is more to be done, but I think I've hit everything mentioned so far.

rwgk · 2021-07-21T12:19:53Z

The one failing CI job is a flake:

Run jwlawson/[email protected]
Error: getaddrinfo ENOTFOUND api.github.com

Safe to ignore.

rwgk · 2021-07-21T12:22:09Z

Thanks @jesse-sony for the change, @rhaschke, @YannickJadoul for the reviews, and @Skylion007 for pointing out this PR as one we should look at again. I'll merge this now.

rhaschke approved these changes Nov 11, 2020

View reviewed changes

YannickJadoul reviewed Nov 16, 2020

View reviewed changes

YannickJadoul added this to the v2.7 milestone Nov 17, 2020

YannickJadoul approved these changes Nov 18, 2020

View reviewed changes

jesse-sony force-pushed the feature/localExceptionTranslator branch from 5498fe8 to eec4e1b Compare January 15, 2021 14:50

jesse-sony force-pushed the feature/localExceptionTranslator branch from eec4e1b to 8e4d62d Compare January 22, 2021 18:17

jesse-sony force-pushed the feature/localExceptionTranslator branch from 8e4d62d to 9f07a85 Compare March 9, 2021 22:09

jesse-sony force-pushed the feature/localExceptionTranslator branch 2 times, most recently from 59b35a7 to c0cfc66 Compare April 26, 2021 15:51

jesse-sony added 12 commits July 16, 2021 13:38

Create a module_internals struct

33744b0

Since we now have two things that are going to be module local, it felt correct to add a struct to manage them.

Add local exception translators

a0af4c3

These are added via the register_local_exception_translator function and are then applied before the global translators

Add unit tests to show the local exception translator works

55c1023

Fix a bug in the unit test with the string value of KeyError

f44ce2e

Fix a formatting issue

3818f36

Rename registered_local_types_cpp()

7099f26

Rename it to get_registered_local_types_cpp() to disambiguate from the new member of module_internals

Add additional comments to new local exception code path

08e9977

Add a register_local_exception function

5545b1f

Add additional unit tests for register_local_exception

ae9cec5

Use get_local_internals like get_internals

696ece0

Update documentation for new local exception feature

bef0832

Add back a missing space

2f2bd18

jesse-sony force-pushed the feature/localExceptionTranslator branch from c0cfc66 to 2f2bd18 Compare July 16, 2021 18:36

henryiii modified the milestones: v2.7, v2.8 Jul 16, 2021

rwgk requested changes Jul 17, 2021

View reviewed changes

Clean-up some issues in the docs

dfdfe66

jesse-sony added 4 commits July 19, 2021 10:51

Remove the code duplication when translating exceptions

f9d0f6f

Separated out the exception processing into a standalone function in the details namespace. Clean-up some comments as per PR notes as well

Remove the code duplication in register_exception

e0b6dcd

Cleanup some formatting things caught by clang-format

d39b6e3

Remove the templates from exception translators

0e6a331

But I added a using declaration to alias the type.

rwgk approved these changes Jul 20, 2021

View reviewed changes

jesse-sony added 2 commits July 20, 2021 20:33

Remove the extra local from local_internals variable names

6bf85a5

Add an extra explanatory comment to local_internals

33ccf64

Fix a typo in the code

63ab723

rwgk merged commit d65edfb into pybind:master Jul 21, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 21, 2021

jesse-sony deleted the feature/localExceptionTranslator branch July 26, 2021 22:51

Skylion007 mentioned this pull request Sep 1, 2021

make_iterator throws exception after re-initializing interpreter #2101

Closed

henryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2650

Closed

Feature/local exception translator #2650

Feature/local exception translator #2650

Conversation

jesse-sony commented Nov 10, 2020 • edited by henryiii Loading

Description

Suggested changelog entry:

Why does this not require an Internals Version bump?

rhaschke left a comment

Choose a reason for hiding this comment

YannickJadoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesse-sony commented Nov 17, 2020

YannickJadoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Jul 15, 2021

jesse-sony commented Jul 16, 2021

rwgk commented Jul 16, 2021

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesse-sony Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Jul 20, 2021

rwgk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesse-sony commented Jul 21, 2021

rwgk commented Jul 21, 2021

rwgk commented Jul 21, 2021

jesse-sony commented Nov 10, 2020 •

edited by henryiii

Loading

jesse-sony Jul 19, 2021 •

edited

Loading

rwgk left a comment •

edited

Loading