-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Update to PyO3 0.27 #15213
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
Update to PyO3 0.27 #15213
Conversation
|
One or more of the following people are relevant to this code:
|
ad97234 to
9b02348
Compare
Pull Request Test Coverage Report for Build 18885640393Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
7b7f83d to
a61c772
Compare
The new `cast` variants are available on both `Bound` and `Borrowed`, and have slightly different semantics around the returned errors.
The new form of the trait uses `Borrowed`, causing it to need another lifetime parameter, and an explicit `Error` type. Where appropriate / easy, I switched the error types away from the complete `PyErr`, even if most of our handling of those errors does eventually convert them into `PyErr` for now. In places where extraction delegates to other methods, particularly where those methods are logical mirrors to `FromPyObject`, this commit also updates the signatures to use `Borrowed` instead of `&Bound`. In a couple of places, the extraction being in terms of `Borrowed` meant that trait implementations that are only accessible on `Bound` (for example `impl IntoIterator for &Bound<PyDict>`) needed some dereferencing inserted to re-satisfy the bounds.
a61c772 to
d0212b4
Compare
jakelishman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think this should be working now, with the release of rust-numpy 0.27 and pyo3 0.27.1. It was a bit of effort unifying the hashbrown versions between indexmap, rustworkx-core, pyo3 and qiskit, but I think I've got there now.
mtreinish
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this LGTM. As you described in the PR summary this is mostly about adapting to the new FromPyObject trait which for the most part is pretty straightforward. I just had two small questions inline.
| Err(PyTypeError::new_err(format!("invalid input: {ob}"))) | ||
| Err(PyTypeError::new_err(format!( | ||
| "invalid input: {}", | ||
| ob.repr()? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a different error message. I assume this was intentional because the Display return isn't as nice looking here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ob changed from being Bound to Borrowed, and lost its Display impl. Using repr seemed more like what we intended than swapping to Debug, although:
- looking now,
Bound<'_, T>::fmtusedstr - it's maybe a PyO3 oversight that
Borroweddoesn't have the sameDisplayimpl anyway.
I can swap to str if you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess that probably would make more sense here. I guess I was just wondering what would be more natural here. But it's not a huge deal either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to ob.to_owned() in b0db82a, which will cause it to use exactly the same handling as before, since it'll then go down the Bound as Display path.
| .extract(), | ||
| .extract() | ||
| // The extraction is infallible. | ||
| .map_err(|x| match x {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a normal pattern for discarding the error here? I probably have done something like:
Ok(extract().unwrap())or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my way so the unwrap/expect will become a compilation error (unhandled match variants) if we change the error type and forget to update the handling. In nightly there's a Result::into_ok that would be clearer, but no date yet for a release of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a normal pattern for discarding the error here?
Just to be clear, this doesn't discard the error - it completely handles all variants. It's just that the error is uninhabitable. If the error were any type (even ()) then my code would be a compile-time error.
* Update PyO3 dependencies * Switch uses of `Bound::downcast*` to corresponding `cast*` The new `cast` variants are available on both `Bound` and `Borrowed`, and have slightly different semantics around the returned errors. * Switch to new `FromPyObject` signature The new form of the trait uses `Borrowed`, causing it to need another lifetime parameter, and an explicit `Error` type. Where appropriate / easy, I switched the error types away from the complete `PyErr`, even if most of our handling of those errors does eventually convert them into `PyErr` for now. In places where extraction delegates to other methods, particularly where those methods are logical mirrors to `FromPyObject`, this commit also updates the signatures to use `Borrowed` instead of `&Bound`. In a couple of places, the extraction being in terms of `Borrowed` meant that trait implementations that are only accessible on `Bound` (for example `impl IntoIterator for &Bound<PyDict>`) needed some dereferencing inserted to re-satisfy the bounds. * Switch to undeprecated `import_exception` * Revert failure message change
Summary
This small chain of commits follows the steps in the PyO3 migration guide to update from 0.26 to 0.27.
The main change is around the new
FromPyObjectsignature.The new form of the trait uses
Borrowed, causing it to need another lifetime parameter, and an explicitErrortype. Where appropriate / easy, I switched the error types away from the completePyErr, even if most of our handling of those errors does eventually convert them intoPyErrfor now.In places where extraction delegates to other methods, particularly where those methods are logical mirrors to
FromPyObject, this commit also updates the signatures to useBorrowedinstead of&Bound.In a couple of places, the extraction being in terms of
Borrowedmeant that trait implementations that are only accessible onBound(for exampleimpl IntoIterator for &Bound<PyDict>) needed some dereferencing inserted to re-satisfy the bounds.Details and comments
The downgrade of one particular instance of
[email protected]to0.15.5is slightly coincidental - I had to undo some of the changes cargo applied while bumping PyO3, and apparently I was slightly overzealous. It's actually better to have slightly fewer builds of it, though, and we largely need things to be aligned sincehashbrownfrequently leaks out in interfaces between Qiskit, rustworkx and (conversion traits in) PyO3.Blocked on PyO3/rust-numpy#515 for now.
The lint failure is caused by the bug fixed in PyO3/pyo3#5538, which should be backported in PyO3 0.27.1 in a day or so. If we want to merge this sooner, we can workspace-allow
clippy::declare_interior_mutable_constin the rootCargo.toml.See PyO3 0.27's migration guide for more information on some of the changes in here.