Closed wyfo closed 11 months ago
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 😄
What do we want that implementation to look like? A pure python package, or something written in rust (either with pyo3 or pyo3-ffi)?
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?
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?
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.
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.
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.
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?
Well said and I think this could be added to the discussion in https://github.com/PyO3/pyo3/pull/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.
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:
SystemExit
??)PanicException
as a user-facing error, e.g. https://pola-rs.github.io/polars/py-polars/html/reference/exceptions.html#exceptionsI 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:
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 #2102)
panic=abort
and still use catch_unwind
, but just abort rather than create the exception.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]
.
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.
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)
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 😂
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:
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 fromBaseException
), 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 uniquePanicException
, 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 https://github.com/PyO3/pyo3/issues/1632#issuecomment-1763536886 for the context of this issue)