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

PyErr does not implement std::error::Error #682

Closed
nagisa opened this issue Dec 4, 2019 · 13 comments · Fixed by #1115
Closed

PyErr does not implement std::error::Error #682

nagisa opened this issue Dec 4, 2019 · 13 comments · Fixed by #1115
Milestone

Comments

@nagisa
Copy link
Contributor

nagisa commented Dec 4, 2019

PyErr is a very odd structure. It pretends to emulate the 3-tuple returned by PyErr_Fetch, but then its pvalue is a 3-variant that also allows lazy construction of exception objects.

As thus it fails to implement a couple of traits typically expected of the error type (std::error::Error, std::fmt::Display, Sync and Send) and does not integrate very well with the error handling story in Python.

There are a couple of ways to go forward here, but one of the simpler things that could be done I think is:

  1. Add a method along the lines of fn(Python, PyErr) -> Py<BaseException>;
    • I don’t believe this can be done easily right now without unsafe code.
  2. Implement the typical error traits for Py<ExceptionT> types.
    • Specifically for Display and friends I think it is fine to just do a Python::acquire_gil() inside if that is necessary to format the data.

Willing to contribute the necessary code to implement this. Please let me know what you think about this. Perhaps other approaches seem better?

@kngwyu
Copy link
Member

kngwyu commented Dec 5, 2019

Thank you for the proposal.
Error handling is indeed a major pain around PyO3 and thus should be enhanced.

Perhaps other approaches seem better?

I'm not sure they are better, but possible alternatives are
3. Get error string when fetching error (difficult, but I don't think it's impossible)
4. Hold some 'intermediate' error message (see #652)

Implement the typical error traits for Py types.

PyErr has already the same memory-management semantics as Py<T> so I don't think Py<ExceptionT> is appropriate.
But now it is really unclear how PyErr and exceptions are related and I think we need better semantics.

@nagisa
Copy link
Contributor Author

nagisa commented Dec 5, 2019

PyErr has already the same memory-management semantics as Py so I don't think Py is appropriate.

To make PyErr Send + Sync it would require a breaking change to the lazy-initialized variants of the pvalue field. If those variants did not exist, this problem would be significantly easier and consist of basically making a Display implementation, something I already have.

FWIW I feel it is fine to have multiple ways to do the same thing in pyo3. It is a necessity, in fact, due to the fact that some conversions are not easily possible (such as going from PyErr to BaseException…)


Unless I misunderstand them, options 3&4 are insufficient as they lose information: the type, traceback and causation chain (raise exc from otherexc) are not preserved. BaseException and derived objects store all that information in the object (type(), __traceback__, etc.)

@kngwyu
Copy link
Member

kngwyu commented Dec 5, 2019

So how about separating Rust-originated errors into another struct?
Then we would have(roughly)

struct PyErrRust(Box<dyn IntoPyErrorTrait>);
struct PyErr {
    ptype: Py<PyType>,
    pvalue: PyObject,
    ptracebacj: Option<PyObject>,
}

@nagisa
Copy link
Contributor Author

nagisa commented Dec 5, 2019

Yeah, that’s definitely something that can work, the suggested PyErr is almost exactly what I have used for my PoC.

I do think that it is a waste of space to store ptype and ptraceback, given that they can also be stored within pvalue, but I can live with that.

@flaper87
Copy link

I don't have a better proposal right now but I wanted to voice my concern about the current error handling in pyo3 and my interest in this issue. Right now, I can't seem to be able to fetch the actual python error string in a nice way that I can integrate with my application.

Thanks for working on this!

P.S: Is there an example that shows how to deal with errors right now? How should python tracebacks be extracted, etc?

@nagisa
Copy link
Contributor Author

nagisa commented Dec 21, 2019

P.S: Is there an example that shows how to deal with errors right now? How should python tracebacks be extracted, etc?

There isn’t an example right now, but if you consider that you need to essentially write the python to extract the information there might be some relevant python tutorials.

Extracting tracebacks will never integrate well, sadly, but we could at least add a method to obtain it and implement nice printing.

@flaper87
Copy link

P.S: Is there an example that shows how to deal with errors right now? How should python tracebacks be extracted, etc?

There isn’t an example right now, but if you consider that you need to essentially write the python to extract the information there might be some relevant python tutorials.

Extracting tracebacks will never integrate well, sadly, but we could at least add a method to obtain it and implement nice printing.

I've written some python to extract the traceback but it doesn't seem to work. Basically, call the traceback module when an error happens. I might be doing something wrong, tho. Thanks for the tip

@davidhewitt
Copy link
Member

I have a PR for this almost ready which I'd love get done for 0.12.

@vitorenesduarte
Copy link

Hi. I'm currently using the pytry macro below in order to get meaningful errors like ModuleNotFoundError: No module named 'matplotli'. Ignoring the fact that it's not very ergonomic, I found this approach to work really well for me.

Is the plan to allow these nice errors but only with PyModule::import(py, "matplotli")? instead of something like pytry!(py, PyModule::import(py, "matplotli"))?

use pyo3::prelude::*;

#[macro_export]
macro_rules! pytry {
    ($py:expr, $e:expr) => {{
        match $e {
            Ok(v) => v,
            Err(e) => color_eyre::eyre::bail!("{:?}", e.print($py)),
        }
    }};
}

fn main() -> Result<(), color_eyre::Report> {
    let gil = Python::acquire_gil();
    let py = gil.python();

    let _ = pytry!(py, PyModule::import(py, "matplotli"));
    Ok(())
}

My first attempt at this macro was pytry0 below, but the output was something like PyErr { type: Py(0x7fc111eeb780, PhantomData) }. Is still possible to get the nice output of pytry without having access to Python? Thanks.

use pyo3::prelude::*;

#[macro_export]
macro_rules! pytry0 {
    ($e:expr) => {{
        match $e {
            Ok(v) => v,
            Err(e) => color_eyre::eyre::bail!("{:?}", e),
        }
    }};
}

fn main() -> Result<(), color_eyre::Report> {
    let gil = Python::acquire_gil();
    let py = gil.python();

    let _ = pytry0!(PyModule::import(py, "matplotli"));
    Ok(())
}

@davidhewitt
Copy link
Member

Having access to Python is equivalent to having the Python GIL, which is necessary to access the exception information safely.

I've wondered about the idea of PyErr automatically acquiring the GIL internally during formatting. It doesn't feel great to have formatting potentially block for a long time while waiting for the GIL. So I think it's unlikely that the nice messages will be available without Python...

@vitorenesduarte
Copy link

Thanks @davidhewitt. I took a quick look at PyModule::import code. Would it be possible/make sense to extract these nice messages when creating the PyErr here? At least here there's no overhead in acquiring the GIL since we already given a Python.

While digging through the code I've also learned about self.py(), which is really handy. With it, I was able to move the pytry business inside my Matplotlib wrapper: https://github.com/vitorenesduarte/fantoch/pull/106/files. Currently, the non-ergonomic code is contained inside this wrapper, which is not so bad.

@davidhewitt
Copy link
Member

Glad to hear you've found a reasonable compromise here. All feedback like this is very useful.

@davidhewitt
Copy link
Member

Would it be possible/make sense to extract these nice messages when creating the PyErr here?

Perhaps. At the moment I believe PyErr is designed as is so that it provides as little overhead as possible. Doing nice string formatting automatically would probably add quite a cost that a lot of users may not need.

I do agree though that some improvements to the pyo3 API could definitely be made in this space. We'll figure out the right design eventually!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants