-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[Oxidize BasisTranslator]: Move the rest of the BasisTranslator
to Rust.
#13237
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 11614982733Details
💛 - Coveralls |
Here's what the benchmarks look like across the board. I'd like to have better ones to show so if you'd like to suggest one I should run please leave it in a comment: basis_translator
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. utility scale
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. randomized benchmarking
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
Fixes Port `BasisTranslator` to Rust Qiskit#12246 This is the final act of the efforts to move the `BasisTranslator` transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.) Methodology: The way this works is by keeping the original `BasisTranslator` python class, and have it store the rust-space counterpart (now called `CoreBasisTranslator`) which will perform all of the operations leveraging the existent Rust API's available for the `Target` (Qiskit#12292), `EquivalenceLibrary`(Qiskit#12585) and the `BasisTranslator` methods `basis_search` (Qiskit#12811) and `compose_transforms`(Qiskit#13137). All of the inner methods will have private visibility and will not be accessible to `Python` as they're intended to be internal by design. By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made. Changes: - Add the pyo3 class/struct `BasisTranslator` that will contain allof the main data used by the transpiler pass to perform its operation. - Convert the `target_basis` into a set manually from python before sending it into the Rust space. - Remove the exposure of `basis_search` and `compose_transforms` to python. - Change `basis_search` so that it accepts references to `HashSet` instances instead of accepting a `HashSet<&str>` instance. - Change inner method's visibility for `basis_search` and `compose_transform` modules in rust. - Expose the exception imports from `Target` to the `accelerate` crate. - Expose `DAGCircuit::copy_empty_like` to the rest of the crates. - Remove all of the unused imports in the Python-side `BasisTranslator`. Blockers: - [ ] Qiskit#12811
14efd38
to
3c00fdd
Compare
- Remove extra copies of `target`, `target_basis`, `equiv_lib`, and `min_qubits`. - Remove unnecessary mutability in `apply_transforms` and `replace_node`.
- Using this method avoids the creation of a datastructure in rust and the overhead of deserializing rust structures which can be overly slow due to multiple cloning. With this update, since the `BasisTranslator` never mutates, it is better to not store anything in Rust.
The latest updates remove the intermediate struct which also resolves the overhead of deserialization when performing multithreaded operations, here are some benchmarks after the update vs the latest main: All benchmarks that improved (that I could find):
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
…dag. - Add function signature in `base_run`.
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.
Thanks a lot Ray! I think that the PR looks good, I just left a couple of very high-level comments and a reminder that a merge conflict has come up :)
|
||
#[allow(clippy::too_many_arguments)] | ||
#[pyfunction(name = "base_run", signature = (dag, equiv_lib, qargs_with_non_global_operation, min_qubits, target_basis=None, target=None, non_global_operations=None))] | ||
fn run( |
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.
Have you considered moving these functions to a new basis_translator.rs
file? I know it's also the way the target code is structured but I personally find looking for source code in mod.rs
super counter-intuitive.
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 could do that, but I wanted to keep the main functions as part of the module of basis_translator
. Since we have more functions that have their own directories within the module, it made sense for me to keep the main functionality in the mod.rs
file. If it's better to have it all be part of a basis_translator.rs
file and to ditch the module file structure, I could do that. But I'd like to keep at least the basis
folder for all basis-related transpiler passes that end up in Rust.
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 would for sure advocate for keeping the basis
folder, and a module structure, it's really just the source code being in mod.rs
that bugs me. This might be my problem because I mentally associate mod.rs
to __init__.py
, and it's anyway a minor suggestion that could be applied in a follow-up.
Ok((out_dag, is_updated)) | ||
} | ||
|
||
fn replace_node( |
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.
Do you think it would make sense to eventually expose this functionality as a DAGCircuit
method?
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'm not quite sure, especially because of the instr_map argument (Which could easily be changed to work with another DAGCircuit
and Param
collection). One thing I'm certain of is that if we do add it, we need to change the name as it is a little misleading because we're not replacing any nodes but rather copying over nodes.
Another note: it might be nice to add a release note pointing out the performance improvements in the transpiler pass! I know we don't do this consistently, but given that you have run a lot of benchmarks and the improvement is great, I think it would be a good addition. |
Co-authored-by: Elena Peña Tapia <[email protected]>
- Use `QuantumCircuit._has_calibration_for()` when trying to obtain calibrations from a `QuantumCircuit` due to the deprecation of the `Pulse` package.
Latest benchmark run (after #13141 merged)
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
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.
Alright, I think this looks good to merge. We can leave the release note suggestion for the release preparation PR if you'd like.
* Initial: Change version to 3.0 - Move loose release notes. * Update cargo dependencies in `Cargo.lock` * 1st round (to be rebased) * Apply suggestions from code review Co-authored-by: Rebecca Dimock <[email protected]> * Apply suggestions from code review Co-authored-by: Julien Gacon <[email protected]> * Check for typos in any release notes. Co-authored-by: Rebecca Dimock <[email protected]> Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> * Add missing release note for #13237 * Move new missing release note to 1.3 folder * Apply suggestions from code review Co-authored-by: Rebecca Dimock <[email protected]> * Apply code suggestions from code review Co-authored-by: Rebecca Dimock <[email protected]> Co-authored-by: Julien Gacon <[email protected]> * More code review comments Co-authored-by: Rebecca Dimock <[email protected]> * Add prelude note * Fix: Incorrent indentation of prelude release note. * Fix: Spelling errors and broken links. * Move release note to 1.3 folder * Update prepare-1.3.0-7c45598775fc2bbf.yaml Co-authored-by: Elena Peña Tapia <[email protected]> * Move release notes from 1.1, 1.2 into their respective folders. * Apply suggestions from code review Co-authored-by: Matthew Treinish <[email protected]> * Apply suggestions from code review Co-authored-by: Matthew Treinish <[email protected]> * More review comments Co-authored-by: Matthew Treinish <[email protected]> * Move latest release note * Fix a couple of sphinx formatting errors * Change wording on prelude * Update releasenotes/notes/1.3/deprecate-basic-simulator-configuration-9d782925196993e9.yaml * Update releasenotes/notes/1.3/pauli-evo-plugins-612850146c3f7d49.yaml Co-authored-by: Julien Gacon <[email protected]> * More corrections on release notes Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: Alexander Ivrii <[email protected]> * Apply suggestions to prelude note Co-authored-by: Matthew Treinish <[email protected]> * Consolidate transpiler pass port related release notes * Move in new release note * Modify new release note * Add missing transpiler passes to the Rust epic - Restored some deleted release notes. - Fix formatting issues. * Remove stray release notes * Fix: Use plot directive effectively * Remove all 1.2 release notes and build from 1.3.0b1 Due to how we end up branching 1.2.0 without a tag this is confusing reno's version detection because on the stable/1.3 branch there is no tag before 1.3.0b1 associated with any of the release notes for 1.2.0. This commit just removes all the release notes to fix this. Additionally, this corrects the earliest-version field in the sphinx directive for reno so we include the notes associated with 1.3.0b1. * Update transpiler pass porting note * Copy edit most release notes * Fix typos * Slight reword in oxidize-transppiler-passes release note --------- Co-authored-by: Rebecca Dimock <[email protected]> Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: Alexander Ivrii <[email protected]>
Fixes #12246
Summary
This is the final act of the efforts to move the
BasisTranslator
transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.)Details and comments
The way this works is by keeping the original
BasisTranslator
python class, and have it call a rust-space counterpart of therun
method (now calledbase_run
) which will perform all of the operations leveraging the existent Rust API's available for theTarget
(#12292),EquivalenceLibrary
(#12585) and theBasisTranslator
methodsbasis_search
(#12811) andcompose_transforms
(#13137).All of the inner methods will have private visibility and will not be accessible to
Python
as they're intended to be internal by design.By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made.
Changes:
Add the pyo3 class/structAmmended by 81bdec4 in favor of aBasisTranslator
that will contain all of the main data used by the transpiler pass to perform its operation.pyfunction
.target_basis
into a set manually from python before sending it into the Rust space.basis_search
andcompose_transforms
to python.basis_search
so that it accepts references toHashSet
instances instead of accepting aHashSet<&str>
instance.basis_search
andcompose_transform
modules in rust.Target
to theaccelerate
crate.DAGCircuit::copy_empty_like
to the rest of the crates.BasisTranslator
.Blockers:
Once these changes have merged this branch will be properly rebased.
basis_search
andBasisSearchVisitor
to rust. #12811