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 anyhow-integration feature which implements From<anyhow::Error> for PyErr #1822

Merged
merged 10 commits into from
Oct 17, 2021

Conversation

chris-laplante
Copy link
Contributor

This makes it possible to use anyhow with pyo3. Includes documentation for feature in guide.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Hmmm.

I agree anyhow is a very useful library, and I understand why this is desirable.

I don't much like how this makes anyhow::Error always map to RuntimeError, which misses out on the richness of Python's range of exception types.

I'd be interested in hearing other maintainer's opinions on whether they think this is a useful feature to add to the library. I could go either way, however my stance at the moment is that I'd like to be persuaded this is necessary.

Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 39 to 41
### `anyhow-integration`

This feature makes it possible to return [`anyhow::Result<T>`](https://docs.rs/anyhow/1.0.43/anyhow/type.Result.html) from functions and methods exposed to Python. It does so by adding `impl From<anyhow::Error> for PyErr`. Currently, the conversion simply stringifies the `anyhow::Error` and shoves it into a `PyRuntimeError`. As a consequence, there is no way to convert a `PyErr` back to the original `anyhow::Error`. It is mostly intended as a developer convenience.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make the section with num_bigint etc into an Optional Dependencies section and add anyhow there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying I should rename the 'Advanced features' section in features.md to 'Optional features'?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more like #1831 - so I went ahead and made that PR 😄

If you put this anyhow feature into that new "Optional Dependencies" section (once merged), that seems most appropriate to me.

@messense
Copy link
Member

IMO it's fine to use RuntimeError since using anyhow usually means the developer don't care about the exact error type.

@birkenfeld
Copy link
Member

birkenfeld commented Aug 24, 2021

I tend to agree with @messense, this is a nice convenience feature for application code where the error type is mostly irrelevant.

For library code, many Python libraries make their own exception class or hierarchy and convert upstream exceptions to those. This is obviously not possible to offer from PyO3 and needs some converter functions in the respective library.

I'm not sure about the formatting, though: is {:?} the correct one? (anyhow::Error also can have context information; this should not be discarded if possible.)

@birkenfeld
Copy link
Member

Of course, it's also possible to provide, under the same feature, a method on PyErr for converting anyhow::Error into arbitrary exception classes if wanted.

@mejrs
Copy link
Member

mejrs commented Aug 24, 2021

Thanks!

Would you be willing to add an entry and doc example here?

@mejrs
Copy link
Member

mejrs commented Aug 24, 2021

my stance at the moment is that I'd like to be persuaded this is necessary.

I think this is also how I feel.

I also think it is inconsistent that eyre has this conversion there and that we'd have anyhows here. But that isn't necessarily a problem to me.

@chris-laplante
Copy link
Contributor Author

I'm not sure about the formatting, though: is {:?} the correct one? (anyhow::Error also can have context information; this should not be discarded if possible.)

From my testing, the context is correctly preserved :)

@chris-laplante
Copy link
Contributor Author

Thanks!

Would you be willing to add an entry and doc example here?

I assume this was directed at me :) in which case will do!

@chris-laplante
Copy link
Contributor Author

my stance at the moment is that I'd like to be persuaded this is necessary.

I think this is also how I feel.

I also think it is inconsistent that eyre has this conversion there and that we'd have anyhows here. But that isn't necessarily a problem to me.

I did think about submitting this change to anyhow instead. But, there are only a handful of popular error handling crates. Contrast that to the bazillion possible crates you may want to use in an application. I feel like it would be asking too much to add support for all those crates into the error handler ones.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍 it sounds like we're generally in favour of adding this, so let's proceed with doing so!

src/err/mod.rs Outdated
Comment on lines 562 to 567
#[cfg(feature = "anyhow")]
impl From<anyhow::Error> for PyErr {
fn from(err: anyhow::Error) -> Self {
crate::exceptions::PyRuntimeError::new_err(format!("{:?}", err))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

From a code organisation perspective I'd quite like this to go into a new file src/conversions/anyhow.rs, and it would be nice to also add tests for it!

…Err impl

This makes it possible to use anyhow::Result<T> as the return type for
methods and functions exposed to Python.

The current implementation just stringifies the anyhow::Error before
shoving it into a PyRuntimeError. Conversion back to the anyhow::Error
is not possible, but it is better than nothing.

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
@chris-laplante
Copy link
Contributor Author

I'm getting a really cryptic macro error when I try to run the WIP test locally. Could someone please take a look? Here is the output. I'm using the nightly toolchain.

   Compiling pyo3 v0.14.3 (/home/laplante/repos/pyo3)
error[E0277]: the trait bound `pyo3::derive_utils::PyFunctionArguments<'_>: From<python::Python<'_>>` is not satisfied
   --> src/lib.rs:361:36
    |
361 |         &|py| pyo3::paste::expr! { [<__pyo3_get_function_ $function_name>] }(py)
    |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<python::Python<'_>>` is not implemented for `pyo3::derive_utils::PyFunctionArguments<'_>`
    |
   ::: src/conversions/anyhow.rs:30:24
    |
30  |             let func = wrap_py
```function!(produce_ok_result)(py).unwrap();
    |                        ----------------------------------- in this macro invocation
    |
    = help: the following implementations were found:
              <pyo3::derive_utils::PyFunctionArguments<'a> as From<&'a pyo3::prelude::PyModule>>
              <pyo3::derive_utils::PyFunctionArguments<'a> as From<pyo3::Python<'a>>>
    = note: required because of the requirements on the impl of `Into<pyo3::derive_utils::PyFunctionArguments<'_>>` for `python::Python<'_>`
note: required by a bound in `__pyo3_get_function_produce_ok_result`
   --> src/conversions/anyhow.rs:17:5
    |
17  |     #[pyfunction]
    |     ^^^^^^^^^^^^^ required by this bound in `__pyo3_get_function_produce_ok_result`
18  |     fn produce_ok_result() -> anyhow::Result<String> {
    |        ----------------- required by a bound in this
    = note: this error originates in the macro `proc_macro_call` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: the method `to_object` exists for reference `&pyo3::types::PyCFunction`, but its trait bounds were not satisfied
   --> src/lib.rs:478:44
    |
478 |           let d = [$((stringify!($val), $val.to_object($py)),)+].into_py_dict($py);
    |                                              ^^^^^^^^^ method cannot be called on `&pyo3::types::PyCFunction` due to unsatisfied trait bounds
    |
   ::: /home/laplante/repos/pyo3/src/types/function.rs:11:1
    |
11  |   pub struct PyCFunction(PyAny);
    |   ------------------------------ doesn't satisfy `pyo3::types::PyCFunction: conversion::ToPyObject`
    |
   ::: src/conversions/anyhow.rs:32:13
    |
32  | /             py_run!(
33  | |                 py,
34  | |                 func,
35  | |                 r#"
36  | |             func()
37  | |             "#
38  | |             );
    | |______________- in this macro invocation
    |
    = note: the following trait bounds were not satisfied:
            `pyo3::types::PyCFunction: conversion::ToPyObject`
            which is required by `&pyo3::types::PyCFunction: conversion::ToPyObject`
    = help: items from traits can only be used if the trait is in scope
    = note: the following trait is implemented but not in scope; perhaps add a `use` for it:
            `use pyo3::ToPyObject;`
    = note: this error originates in the macro `$crate::py_run_impl` (in Nightly builds, run with -Z macro-backtrace for more info)

@davidhewitt
Copy link
Member

davidhewitt commented Aug 27, 2021

Looks like wrap_pyfunction! probably assumes the prelude is imported. IMO that's a bug.

EDIT: opened #1837 to fix that

Comment on lines 17 to 25
#[pyfunction]
fn produce_ok_result() -> anyhow::Result<String> {
Ok(String::from("OK buddy"))
}

#[pyfunction]
fn produce_err_result() -> anyhow::Result<String> {
anyhow::bail!("error time")
}
Copy link
Member

Choose a reason for hiding this comment

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

It's actually not ideal to use #[pyfunction] within unit tests of pyo3's lib, because it relies on pyo3 being available as in the global namespace. We happen to have a self-dev-dependency on pyo3 but it causes unexpected breakages - see #1817 for more detail.

The point here is that it's probably better to just test that let x: PyErr = produce_err_result().unwrap_err().into() works ok, and could check the string contents of the error are as expected.

(If you really want to check the #[pyfunction] case, that's good too, but it'll need to go in a separate file in tests/.)

@mejrs
Copy link
Member

mejrs commented Oct 8, 2021

Hey @chris-laplante - this looks done or almost done 😃 Are you able to finish it up?

If not - I'd like to finish it up. I've been working on a similar eyre feature and I want to push out it and this at a similar time.

@mejrs mejrs mentioned this pull request Oct 13, 2021
@mejrs mejrs added this to the 0.15 milestone Oct 14, 2021
@davidhewitt davidhewitt mentioned this pull request Oct 14, 2021
2 tasks
@chris-laplante
Copy link
Contributor Author

Hey @chris-laplante - this looks done or almost done 😃 Are you able to finish it up?

If not - I'd like to finish it up. I've been working on a similar eyre feature and I want to push out it and this at a similar time.

Hey @mejrs sorry for the lack of reply - I have been busy with other things unfortunately. If you would like to take over I'd appreciate it :)

@mejrs
Copy link
Member

mejrs commented Oct 16, 2021

I think that is everything.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for finishing this off! Just a couple of final comments around documentation.

CHANGELOG.md Outdated
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add `PyAny::py` as a convenience for `PyNativeType::py`. [#1751](https://github.com/PyO3/pyo3/pull/1751)
- Add implementation of `std::ops::Index<usize>` for `PyList`, `PyTuple` and `PySequence`. [#1825](https://github.com/PyO3/pyo3/pull/1825)
- Add range indexing implementations of `std::ops::Index` for `PyList`, `PyTuple` and `PySequence`. [#1829](https://github.com/PyO3/pyo3/pull/1829)
- Add `anyhow` feature which provides `impl From<anyhow::Error> for PyErr`. [#1822](https://github.com/PyO3/pyo3/pull/1822)
Copy link
Member

Choose a reason for hiding this comment

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

Let's put this in the Packaging section too and make sure this and the eyre entry are written the same way?

Comment on lines 86 to 88
### `anyhow`

This feature makes it possible to return [`anyhow::Result<T>`](https://docs.rs/anyhow/1.0.43/anyhow/type.Result.html) from functions and methods exposed to Python. It does so by adding `impl From<anyhow::Error> for PyErr`. Currently, the conversion simply stringifies the `anyhow::Error` and shoves it into a `PyRuntimeError`. As a consequence, there is no way to convert a `PyErr` back to the original `anyhow::Error`. It is mostly intended as a developer convenience.
Copy link
Member

Choose a reason for hiding this comment

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

Similarly I think it'd be nice for this paragraph and the eyre one to read pretty much identically, given that the features are almost equivalent. I don't mind whether it's this one or the eyre one that changes; depends how much detail you think is appropriate.

@@ -0,0 +1,154 @@
#![cfg(feature = "anyhow")]

//! A conversion from [anyhow]’s [`Error`] type to [`PyErr`].
Copy link
Member

Choose a reason for hiding this comment

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

Was it intentional to have this documentation in the test?

Copy link
Member

Choose a reason for hiding this comment

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

I was slightly tipsy and I guess I wrote it twice 😊

Copy link
Member

Choose a reason for hiding this comment

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

Haha whoopsie! 😂

@davidhewitt davidhewitt merged commit 3b94f4b into PyO3:main Oct 17, 2021
@chris-laplante
Copy link
Contributor Author

Thank you @mejrs and @davidhewitt so much for finishing this up :). I appreciate it!

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.

None yet

5 participants