Make PanicException exportable

[wyfo]

It's a common practice for Python libraries to define custom exceptions. PanicException is not really different of other custom exception (except it is derived from BaseException), so it could be made exportable, to be imported in Python code.
As a side-effect, every library compiled with PyO3 will thus have a unique PanicException, but is it an issue? After all, a panic from library A doesn't necessarily means the same than a panic from library B. Also, it could be exported with a different name, more related to the context of the library.

(See #1632 (comment) for the context of this issue)

[davidhewitt]

See https://discuss.python.org/t/c-api-for-coroutines-iterators-generators-etc-with-native-implementations/35944/8

Overall the plan which we came up with in #3073 was to publish pyo3_runtime on PyPI with a PanicException, and use the exception that package. @encukou's additional suggestion was to fall back on generating one from inside PyO3 if the import fails (i.e. the user hasn't installed that package). I'd be happy with that as a way to not suddenly require all PyO3 extension publishers to add pyo3_runtime as a dependency.

I think we're all on board with that plan, so I think all that's waiting is an implementation 😄

[mejrs]

What do we want that implementation to look like? A pure python package, or something written in rust (either with pyo3 or pyo3-ffi)?

[davidhewitt]

I think pyo3_runtime should be pure python for now (the empty package is already in this repo). I think inside pyo3 if the pyo3_runtime import fails then we could build the equivalent from FFI calls?

[wyfo]

Actually, my point for the issue was quite the opposite. I think library authors could just add

m.add("PanicError", py.get_type::<pyo3::panic::PanicException>())?;

to their module if they want it to be catch by their users, and that we don't need a pyo3_runtime package for that. Because I think a panic in a library A should not raise the same exception than a panic in library B.

But it was very late, and I totally forgot that it was possible to export PanicException, so I opened this issue, quite for nothing in fact. Should I close it?

[davidhewitt]

They can, but from experience the reason why users occasionally want to catch panics is because they have some kind of production service which they don't want to have any downtime. I think asking them to add exception catching for N different PanicException for all their N rust dependencies (some which they may not know about) is not a satisfying solution for them. Other than this use case, I don't know a good reason users want to interact with panics, especially in a way where there are N flavours of panic.

Polars actually already does export PanicException btw.

[wyfo]

I think asking them to add exception catching for N different PanicException for all their N rust dependencies (some which they may not know about) is not a satisfying solution for them.

Catching a pyo3_runtime.PanicError without any indication of which library caused it seems to me a little bit the same than catching BaseException directly, so I don't really see the value of it.

Anyway, I've pretty much no experience in pyo3 library (I need coroutine support to build my first one), so my opinion has not so much value here.

[birkenfeld]

Catching a pyo3_runtime.PanicError without any indication of which library caused it seems to me a little bit the same than catching BaseException directly, so I don't really see the value of it.

Catching panics should be a last-ditch effort in an "outer" handler anyway, akin to catching BaseException. So the value is in being able to catch any PyO3 extension panic, since the point where you catch it might not even know about all transitive PyO3 based dependencies.

[wyfo]

since the point where you catch it might not even know about all transitive PyO3 based dependencies.

This argument has already been used, but I don't understand it. What's the interest of catching exceptions from library you don't know, except when you do a global catch like except BaseException? How does a PanicException of a library A have the same meaning that a PanicException of a library B, and how does it differs from another BaseException raised by some C/C++ library?

I think asking them to add exception catching for N different PanicException for all their N rust dependencies (some which they may not know about) is not a satisfying solution for them.

I think providing a PyO3 specific error while ignoring other Rust/C++/C/Fortran/Zig/Nim/etc. bindings doesn't really help, because it's either too specific (only PyO3), or too broad (all PyO3 libraries mixed).
If you have something important like an invariant to maintain, you will already catch BaseException, even if you have PyO3 based dependencies, because, in fact, you don't have only PyO3 based dependencies.

I can't think of any use case where someone will write except pyo3_runtime.PanicError and not except BaseException (because again, all libraries are not PyO3 based).
However, if a particular library is known to have recoverable panic, I can see the point of writing except some_library.PanicError: some_library.reset_global_state().

Also, let's imagine that some library with a PyO3 backend is rewritten in Zig (because why not), you see the consequence...
pyo3_runtime.PanicError only conveys an implementation detail (it's PyO3 based) instead of the reason for which it was raised. How could you even react to it without knowing the library raising it?

[adamreichold]

Well said and I think this could be added to the discussion in #3057 which covers a lot of ground of why users of PyO3 want this. I think w.r.t. your argument, their main point is that PanicException should not have been a BaseException in the first place (they do have a catch-all for exceptions, but not base exceptions) and exposing it gives these users the ability that handle them as normal exception but still let base exceptions pass.

EDIT: So exporting it is a compromise giving control over its interpretation to downstream code instead of e.g. adjusting it via magic environment variables or build time flags.

[davidhewitt]

Agreed, and I think the insight that it's an implementation detail which is leaking out is true. I think this is also a result of differing needs between PyO3 and library authors:

• PyO3 has attempted to model a panic as a faithful representation of the language feature: a fatal error which unwinds and exits by default. (Maybe we should have inherited from SystemExit??)
• Library authors can lean on this exception mechanism to expose PanicException as a user-facing error, e.g. https://pola-rs.github.io/polars/py-polars/html/reference/exceptions.html#exceptions

I think if library authors were instead working to minimise panics the volume of debate on whether to catch PanicException would be much reduced because it'd happen much less frequently. So maybe the other direction to go is to explore how to discourage library authors from doing this:

• For example, maybe by default PyO3 requires panic=abort? A feature panic-unwind-exception could be opt-in to get the current functionality. There would also be a runtime advantage to panic=abort because we wouldn't have to have catch_unwind wrapping all Rust code. (See also Attempts to catch_unwind (on Linux) result in SIGABRT #2102)
  • A reduced form of this is that we don't require panic=abort and still use catch_unwind, but just abort rather than create the exception.
• Maybe we should go the other way and make it harder for library authors to expose PanicException, so they struggle to document it as a user facing error?

Maybe there is also a way where we can let individual PyO3 extensions register their own PanicException (and thus choose what inheritance hierarchy it has), similar to #[global_allocator].

[adamreichold]

still use catch_unwind, but just abort rather than create the exception.

I have not had the time to really form an opinion on all of this, but I think this would even be easier by using our existing PanicTrap, just removing catch_unwind should already have exactly this effect.

[wyfo]

Maybe there is also a way where we can let individual PyO3 extensions register their own PanicException

Why not just add a #[pyo3(panic = MyPanicException)] attribute, and keep the current behavior as default?
Also, because adding an attribute for every exported function is cumbersome, it might have the effect of "discouraging library authors from doing this" as you want 😄
(And PanicException remains exportable if developers still want to expose it)

[davidhewitt]

I appreciate the per-function attribute makes it delightfully awkward for users but I think that might be a step too far in bad ergonomics. We cannot win 😂

[CTimmerman]

I'm having trouble catching it in Python for https://jpegxl.info/test-page/Webkit-logo-P3.jxl

DEBUG: Loading 4/4 test\JPEG XL\Webkit-logo-P3.jxl
DEBUG: Stat: os.stat_result(st_mode=33206, st_ino=1125899906941330, st_dev=676036590, st_nlink=1, st_uid=0, st_gid=0, st_size=10591, st_atime=1712744515, st_mtime=1712744264, st_ctime=1712744263)
thread '<unnamed>' panicked at src\lib.rs:197:18:
Unsupported dtype for decoding
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Exception in Tkinter callback
Traceback (most recent call last):
  File "D:\code\image viewer\tk_image_viewer\main.py", line 347, in im_load
    IMAGE = Image.open(path)
  File "C:\Users\C\AppData\Local\Programs\Python\Python310\lib\site-packages\PIL\Image.py", line 3288, in open
    im = _open_core(fp, filename, prefix, formats)
  File "C:\Users\C\AppData\Local\Programs\Python\Python310\lib\site-packages\PIL\Image.py", line 3274, in _open_core
    im = factory(fp, filename)
  File "C:\Users\C\AppData\Local\Programs\Python\Python310\lib\site-packages\PIL\ImageFile.py", line 137, in __init__
    self._open()
  File "C:\Users\C\AppData\Local\Programs\Python\Python310\lib\site-packages\pillow_jxl\JpegXLImagePlugin.py", line 30, in _open
    self.jpeg, self._jxlinfo, self._data = self._decoder(self.fc)
pyo3_runtime.PanicException: Unsupported dtype for decoding

If i catch BaseException, Pylint (W0718:broad-exception-caught Catching too general exception) and Sonarlint (python:S5754 Catch a more specific exception or reraise the exception) complain. Solved using my current code:

    # pylint: disable=W0718
    except (
        tkinter.TclError,
        IOError,
        MemoryError,  # NOSONAR
        EOFError,  # NOSONAR
        ValueError,  # NOSONAR
        BufferError,  # NOSONAR
        OSError,  # NOSONAR
        BaseException,  # NOSONAR  # https://github.com/PyO3/pyo3/issues/3519
    ) as ex: