Bump PyO3 and rust-numpy to 0.23.x #13577
Conversation
This commit bumps the version of pyo3 and rust-numpy used by qiskit to the latest release 0.23. The largest change by volume of code is the deprecation of all the `*_bound()` methods. These are just warnings but they would be fatal to our CI so it needs to be updated. THe larger functional change that required updating the code is the change in the traits around converting to Python objects. This actually found a bug in the target where we were not returning a proper instruction type for standard gates. This also opens up the opportunity to update our hashbrown version to 0.15.x, but we can't do that until the next rustworkx-core release.
|
One or more of the following people are relevant to this code:
|
In pyo3 0.23 the behavior of the default conversion for a `SmallVec<u8>` and similar arrays of u8 default to bytes objects when converted to python. This was documented as an api change in the release, however it was cauing the unitary synthesis test to fail because when we were evaluating whether the synthesis was in terms of the natural direction of the 2q gate on the backend it was evaluating `[0, 1] == b"\x00\x11"` which evaluates to `False` and the pass flipped the direction of the 2q gate. This was causing the test failure because all the 2q gates which were correctly directed were getting incorrectly flipped. This commit fixes this by manually creating a pylist so the comparisons work as expected.
| @@ -106,7 +106,7 @@ pub(super) fn compose_transforms<'a>( | |||
| for (node, params) in nodes_to_replace { | |||
| let param_mapping: HashMap<ParameterUuid, Param> = equiv_params | |||
| .iter() | |||
| .map(|x| ParameterUuid::from_parameter(x.to_object(py).bind(py))) | |||
| .map(|x| ParameterUuid::from_parameter(&x.into_pyobject(py).unwrap())) | |||
There was a problem hiding this comment.
For others, the unwrap is needed here because PyO3's IntoPyObject::into_pyobject now returns a Result<T, E> type. The error type is PyErr in this case (dictated by the #[derive(IntoPyObjectRef)] attribute on Param).
There was a problem hiding this comment.
It's not clear to me why the Param needs converting into a Python object at all here. If it isn't a Param::Parameter variant, the whole thing will fail, so it seems like we could have sorted out that typing a while prior.
| @@ -57,30 +58,12 @@ type GateMapState = Vec<(String, Vec<(Option<Qargs>, Option<InstructionPropertie | |||
|
|
|||
| /// Represents a Qiskit `Gate` object or a Variadic instruction. | |||
| /// Keeps a reference to its Python instance for caching purposes. | |||
| #[derive(FromPyObject, Debug, Clone)] | |||
| #[derive(FromPyObject, Debug, Clone, IntoPyObjectRef)] | |||
There was a problem hiding this comment.
Should we also derive IntoPyObject?
There was a problem hiding this comment.
Probably, I probably didn't do it because nothing used it, I'll add this. There might havee been a reason I didn't add it, but I'll find out when I try this (and will report back if it can't be added).
There was a problem hiding this comment.
Ah, so trying this locally adding the definition confuses the return of self._gate_name_map[x].into_py_object() below because it tries to do it with an owned version instead of the ref.
There was a problem hiding this comment.
I imagine that's expected / intended - I'd assume the spelling would need to become (&self._gate_name_map[x]).into_py_object(). I think in general t.into_py() goes to t.into_py_object() and t.to_object() goes to (&t).into_py_object()?
| @@ -206,7 +206,8 @@ mod test { | |||
| fn default_key_exists() { | |||
| let mut interner = Interner::<[u32]>::new(); | |||
| assert_eq!(interner.get_default(), interner.get_default()); | |||
| assert_eq!(interner.get(interner.get_default()), &[]); | |||
| let res: &[u32] = &[]; | |||
| assert_eq!(interner.get(interner.get_default()), res); | |||
There was a problem hiding this comment.
What's going on here?
There was a problem hiding this comment.
TBH I can't remember. It's either the compiler started complaining here and suggested I do this to make it work, or I was debugging something and did this to do println!("{:?}", res) before the assertion and forgot to change it back but removed the print. I'll try reverting this and see what happens.
There was a problem hiding this comment.
After trying it locally I remember now. The compiler was complaining it can't infer the type of &[] anymore so I used the res var to define it as a &[u32].
There was a problem hiding this comment.
This seems weird that the PyO3 update would cause the inference here to suddenly fail. The types of Interner and get_default() shouldn't have changed.
| @@ -211,89 +211,108 @@ pub enum InternalBytecode { | |||
| }, | |||
| } | |||
|
|
|||
| impl IntoPy<Bytecode> for InternalBytecode { | |||
| impl<'py> IntoPyObject<'py> for InternalBytecode { | |||
There was a problem hiding this comment.
Is this not derivable because the enum variants have named fields?
There was a problem hiding this comment.
Yeah when I did the derive approach it didn't work, I don't remember the exact details because I worked on this > 1 month ago. But I had to go this route.
There was a problem hiding this comment.
Using the semantics of the derive macros would substantially change how the Rust/Python boundary layer is handled here. Even if PyO3 didn't error, the types would be entirely different to what Python is expecting.
Co-authored-by: Kevin Hartman <kevin@hart.mn>
There was a problem hiding this comment.
Thanks for doing this. I think there's a fair number of function returns that spiritually ought to swap from Py<PyAny> (or the PyObject spelling) into Bound<PyAny> - I think making that change was one of the driving reasons to make the trait change in the first place. We have a lot of explicit unbinds in places where we're returning the object itself to Python space, not hiding it in a Rust object that needs to be GIL-independent (could be into_bound_py_any in many cases?). No need to go hunting for them, though, for this PR.
(Side note: it's weird to me that into_bound_py_any was chosen, rather than having into_py_any getting called into_unbound_py_any, given the shift of focus away from Py in those conversions.)
I feel like there's at least a couple of places I've written that does the same thing as IntoPyObjectExt::into_pyobject_or_pyerr, but I don't really remember them off the top of my head.
It's most likely lack of experience, but I've got no intuition for what the IntoPyObject derive macro will do to enums with compound variants. Personally, I'm not sure I like the derive macros for record structs much at all - turning stuff into dict feels icky - but that's just a knee-jerk reaction right now and I'll see what I think with more experience.
I didn't manage to 100% convince myself that all the Target changes produce visibly equivalent code, so I'm pretty much banking on the tests there.
| @@ -109,11 +110,13 @@ impl OneQubitGateSequence { | |||
|
|
|||
| fn __getitem__(&self, py: Python, idx: PySequenceIndex) -> PyResult<PyObject> { | |||
| match idx.with_len(self.gates.len())? { | |||
| SequenceIndex::Int(idx) => Ok(self.gates[idx].to_object(py)), | |||
| indices => Ok(PyList::new_bound( | |||
| SequenceIndex::Int(idx) => Ok(self.gates[idx].clone().into_py_any(py)?), | |||
There was a problem hiding this comment.
The clone and move seems weird here. Does it work with (&self.gates[idx]).into_py_any(py) to do it implicitly (and potentially make it easier to elide the clone/move)?
There was a problem hiding this comment.
I solved this by implementing IntoPyObject for &T for all our local T: PyClass + Clone (since I can't do the blanket impl before of coherence rules). I might ask over on PyO3 if it's something that should have a blanket impl, be part of the pyclass generation, or something else.
|
I'm sorting out the merge conflicts and looking into fixing up a couple of the comments I left in the last review, then I'll merge. |
This makes it more ergonomic to use the blanket implementations of `IntoPyObject` on ad-hoc structures like `(&T1, &T2)` when one of the contained types is a `StandardGate`.
There was a problem hiding this comment.
I'm happy with this now, though somebody else ought to sign off on my commit cd99d9f.
|
That looks good to me, so I went ahead and added it to merge queue. |
* Bump PyO3 and rust-numpy to 0.23.x This commit bumps the version of pyo3 and rust-numpy used by qiskit to the latest release 0.23. The largest change by volume of code is the deprecation of all the `*_bound()` methods. These are just warnings but they would be fatal to our CI so it needs to be updated. THe larger functional change that required updating the code is the change in the traits around converting to Python objects. This actually found a bug in the target where we were not returning a proper instruction type for standard gates. This also opens up the opportunity to update our hashbrown version to 0.15.x, but we can't do that until the next rustworkx-core release. * Fix cache pygates build * Fix cargo test * Fix impl of IntoPyObject for target's operation * Fix impl of IntoPyObject for equivalence library circuit * Remove commented out code * Fix commutation library conversion * Fix conversion of qasm3 py register type * Use into_py_any() * Use borrowed instead of cloning py bound for NormalOperation * Fix unitary synthesis failure In pyo3 0.23 the behavior of the default conversion for a `SmallVec<u8>` and similar arrays of u8 default to bytes objects when converted to python. This was documented as an api change in the release, however it was cauing the unitary synthesis test to fail because when we were evaluating whether the synthesis was in terms of the natural direction of the 2q gate on the backend it was evaluating `[0, 1] == b"\x00\x11"` which evaluates to `False` and the pass flipped the direction of the 2q gate. This was causing the test failure because all the 2q gates which were correctly directed were getting incorrectly flipped. This commit fixes this by manually creating a pylist so the comparisons work as expected. * Fix cargo fmt * Apply suggestions from code review Co-authored-by: Kevin Hartman <kevin@hart.mn> * Fix compilation error * Use into_py_any in missed spot * Fix lint * Fix cargo fmt * Remove overeager trait derives * Pull in latest pyo3 bugfix release * Fix error handling * Fix lifetime for _to_matrix() * Implement `IntoPyObject` for `&StandardGate` This makes it more ergonomic to use the blanket implementations of `IntoPyObject` on ad-hoc structures like `(&T1, &T2)` when one of the contained types is a `StandardGate`. * Implement `IntoPyObject` for all `&T` where `T: PyClass + Copy` --------- Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Summary
This commit bumps the version of pyo3 and rust-numpy used by qiskit to the latest release 0.23. The largest change by volume of code is the deprecation of all the
*_bound()methods. These are just warnings but they would be fatal to our CI so it needs to be updated. The larger functional change that required updating the code is the change in the traits around converting to Python objects. This actually found a bug in the target where we were not returning a proper instruction type for standard gates. This also opens up the opportunity to update our hashbrown version to 0.15.x, but we can't do that until the next rustworkx-core release.Details and comments