Migrate to PyO3 0.21's Bounds API #12121
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 8541853371Warning: 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 |
jakelishman
reviewed
Apr 2, 2024
There was a problem hiding this comment.
Thanks for doing this! I'm really excited to see what the benchmarks show. I think the work you did in quantum_circuit::pyext might mean we see a bit less of an improvement than we otherwise might have, but there's the possibility of some speed-ups in qasm3 still. I'm also interested in making a lot more use of the PyO3 bindings in qasm2 to build the circuit rather more directly, if this all works out, but let's see!
Could we configure the build to disable the gil-refs feature by default, so we don't accidentally re-introduce any?
Comment on lines
598
to
604
| fn __reduce__(&self, py: Python) -> Py<PyAny> { | ||
| (py.get_type::<Self>(), (PyString::new(py, self.as_str()),)).into_py(py) | ||
| ( | ||
| py.get_type_bound::<Self>(), | ||
| (PyString::new_bound(py, self.as_str()),), | ||
| ) | ||
| .into_py(py) | ||
| } |
There was a problem hiding this comment.
Looking again at this (I only just wrote it recently), this actually ought to be a __getnewargs__ impl rather than a __reduce__, but that's not important for this PR.
|
I ran some quick asv benchmarks with this PR because I was curious: |
|
Great, thanks @jakelishman for the review, and @mtreinish for running the benchmarks! Should be good to go. |
jakelishman
approved these changes
Apr 3, 2024
There was a problem hiding this comment.
Nice, this looks good to me now, thanks! I realised after I'd written it up top that PyO3 defaults to showing deprecation warnings around the GIL refs, so my previous review comment isn't so important.
This is a fairly mechanical PR, so I think I'll just merge it now, so we can all start making better use of the new PyO3 interfaces.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Update PyO3 to version 0.21, which includes the new PyO3 Bounds API.
Details and comments
The PyO3 Bound API migration guide may be useful for reviewers.
We probably ought to run a few benchmarks (e.g. circuit building) to see how this upgrade impacts performance and memory use (it should be a positive change, given that PyO3's memory management has been improved).