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

Add support for user defined exception translators #273

Merged
merged 1 commit into from
Jul 11, 2016

Conversation

pschella
Copy link
Contributor

@pschella pschella commented Jul 8, 2016

Implementation of the feature requested in issue #246.

@wjakob
Copy link
Member

wjakob commented Jul 9, 2016

Hello Pim,

I had a look at your PR and added a few changes:

The first was with regards to your usage of std::function, which comes with a rather stiff penalty (bloated code generation) on most current compilers. An older version of pybind11 (before the first official release) used it to bind all functions via std::function. Switching to a custom solution in cpp_function that exactly reproduces the functionality of std::function reduced the size of pybind11 extension modules by a factor of ~2.5! Thus, my policy is to not use it anywhere within the core library (except in the optional header functional.h).

I think it's reasonable to impose that exception translators should be stateless, which makes things even simpler.

Thus I switched from

std::forward_list<std::function<void(std::exception_ptr)>> registered_exception_translators;

to raw function pointers, e.g.

std::forward_list<void(*)(std::exception_ptr)> registered_exception_translators;

I also removed the exception_translator.h header and rolled it into pybind11.h (no need for a separate header if just one inline function is involved).

A patch with these changes and some documentation edits is here: patch.txt

At that point, I unfortunately hit a snag. On Python 3.5, I get the following error message:

Can we catch a MyException?
Traceback (most recent call last):
  File "example/example19.py", line 10, in <module>
    example.throws1()
SystemError: exception <class 'example.MyException'> not a BaseException subclass

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "example/example19.py", line 11, in <module>
    except example.MyException as e:
TypeError: catching classes that do not inherit from BaseException is not allowed

So it looks like there is no way around creating some kind of py::exception wrapper as disucussed here: https://github.com/lsst-dm/pybind11/pull/3/files.

However, a lean version of this modification would need to be found that doesn't create overheads for normal non-exception objects.

@pschella pschella force-pushed the master branch 2 times, most recently from 7dc65ac to 949b237 Compare July 11, 2016 15:19
@pschella
Copy link
Contributor Author

Hi,

I agree with your changes. State is not important, and it is nice to be lean.

As for the Python 3.5 incompatibility. That is indeed unfortunate. And I would like to add the functionality for inheriting from BaseException. However, I think most users probably won't need this, being able to set a custom exception type with the C-API is enough. Therefore I have edited the example and comments to limit it to using PyErr_SetString with a custom type created with PyErr_NewException.
We could merge this now, while we (by which I mean I) continue to work on the inheritance solution for use with PyErr_SetObject.

Kind regards,

Pim Schellart

@wjakob
Copy link
Member

wjakob commented Jul 11, 2016

What are your thoughts on adding a wrapper py::exception<MyException>(m, "Exception1"); that basically does the raw python API calls you now added? If/once multiple inheritance is available, it could be replaced by a small wrapper around class_<..>.

@wjakob
Copy link
Member

wjakob commented Jul 11, 2016

looks like there is some minor issue with the name of the exception on Python 3.5 (Travis CI build bot)

@pschella pschella force-pushed the master branch 2 times, most recently from 949b237 to c0f8040 Compare July 11, 2016 17:05
@pschella
Copy link
Contributor Author

Yes, that sounds like a good idea. I'll put it in and update the request.
I also noticed the python 3 problem. It now seems to print class instead of type in all cases.
Just replaced it with __class__.__name__ in the printing which should fix it.

PyModule_AddObject(m.ptr(), "MyException", my_exception_type);

// make a new custom exception and use it as a translation target
static py::exception<MyException> ex(m, "MyException");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: change

static py::exception<MyException> ex(m, "MyException");

to

py::exception<MyException> ex(m, "MyException");

together with other change below

@pschella
Copy link
Contributor Author

I'll add the inc_ref, but actually the static is needed because we can't use lambda capture (due to statelessness change) for ex and we need to refer to it within the exception translator.
An alternative would be a macro (yak) which creates a new global (such as PyExc_Exception).

@wjakob
Copy link
Member

wjakob commented Jul 11, 2016

no, this seems reasonable then.

@pschella
Copy link
Contributor Author

Ok, shall I rebase all commits into a single one? Or do you want something else for the merge? Assuming you want the code of course.

@wjakob
Copy link
Member

wjakob commented Jul 11, 2016

Rebase + squash will be perfect!

@pschella
Copy link
Contributor Author

Did a rebase + squash + a minor change to the documentation.

@wjakob
Copy link
Member

wjakob commented Jul 11, 2016

Perfect -- thank you very much for contributing this feature!

@wjakob wjakob merged commit 3c6ada3 into pybind:master Jul 11, 2016
@pschella
Copy link
Contributor Author

Thanks for taking it! And for the productive discussion.

jagerman added a commit to jagerman/pybind11 that referenced this pull request Jul 20, 2016
The custom exception handling added in PR pybind#273 is robust, but is overly
complex for declaring the most common simple C++ -> Python exception
mapping that needs only to copy `what()`.  This add a simple
py::register_exception<CppException>(module, "PythonException");
function that greatly simplifies the basic case of translation of a
simple CppException into a simple PythonException.
jagerman added a commit to jagerman/pybind11 that referenced this pull request Aug 22, 2016
The custom exception handling added in PR pybind#273 is robust, but is overly
complex for declaring the most common simple C++ -> Python exception
mapping that needs only to copy `what()`.  This add a simple
py::register_exception<CppException>(module, "PythonException");
function that greatly simplifies the basic case of translation of a
simple CppException into a simple PythonException.
jagerman added a commit to jagerman/pybind11 that referenced this pull request Aug 23, 2016
The custom exception handling added in PR pybind#273 is robust, but is overly
complex for declaring the most common simple C++ -> Python exception
mapping that needs only to copy `what()`.  This add a simple
py::register_exception<CppException>(module, "PythonException");
function that greatly simplifies the basic case of translation of a
simple CppException into a simple PythonException.
jagerman added a commit to jagerman/pybind11 that referenced this pull request Sep 15, 2016
The custom exception handling added in PR pybind#273 is robust, but is overly
complex for declaring the most common simple C++ -> Python exception
mapping that needs only to copy `what()`.  This add a simpler
`py::register_exception<CppExp>(module, "PyExp");` function that greatly
simplifies the common basic case of translation of a simple CppException
into a simple PythonException, while not removing the more advanced
capabilities of defining custom exception handlers.
wjakob pushed a commit that referenced this pull request Sep 16, 2016
The custom exception handling added in PR #273 is robust, but is overly
complex for declaring the most common simple C++ -> Python exception
mapping that needs only to copy `what()`.  This add a simpler
`py::register_exception<CppExp>(module, "PyExp");` function that greatly
simplifies the common basic case of translation of a simple CppException
into a simple PythonException, while not removing the more advanced
capabilities of defining custom exception handlers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants