Fully port InverseCancellation to Rust #13013
Merged
Conversation
This file contains 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
mtreinish
added
performance
Rust
mtreinish
requested review from
nonhermitian,
a team,
alexanderivrii and
ShellyGarion
as code owners
|
One or more of the following people are relevant to this code:
|
mtreinish
force-pushed
the
rust-inverse-cancellation
branch
from
98e6159
to
7614689
Compare
Pull Request Test Coverage Report for Build 10745716091Details
💛 - Coveralls |
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Aug 23, 2024
This commit builds off of Qiskit#13013 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for creating `UnitaryGate` instances and `ParameterExpression` for global phase. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different gates so we should be able to increase the throughput of the pass by leveraging multithreading to handle each gate in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Part of Qiskit#12208
mtreinish
force-pushed
the
rust-inverse-cancellation
branch
from
7614689
to
00c7e10
Compare
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Aug 25, 2024
This commit builds off of Qiskit#13013 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for creating `UnitaryGate` instances and `ParameterExpression` for global phase. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different gates so we should be able to increase the throughput of the pass by leveraging multithreading to handle each gate in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Part of Qiskit#12208
This commit builds off of Qiskit#12959 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for handling parameter expressions. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different inverse gates/pairs so we should be able to the throughput of the pass by leveraging multithreading to handle each inverse option in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Fixes Qiskit#12271 Part of Qiskit#12208
mtreinish
force-pushed
the
rust-inverse-cancellation
branch
from
00c7e10
to
170148c
Compare
mtreinish
changed the title
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Aug 30, 2024
This commit builds off of Qiskit#13013 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for creating `UnitaryGate` instances and `ParameterExpression` for global phase. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different gates so we should be able to increase the throughput of the pass by leveraging multithreading to handle each gate in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Part of Qiskit#12208
kevinhartman
reviewed
Sep 4, 2024
| } | ||
| let mut collect_set: HashSet<String> = HashSet::with_capacity(1); | ||
| collect_set.insert(gate.operation.name().to_string()); | ||
| let gate_runs: Vec<Vec<NodeIndex>> = dag.collect_runs(collect_set).unwrap().collect(); |
There was a problem hiding this comment.
It looks like DAGCircuit::collect_runs returns an Option only because it directly returns the result of the Rustworkx core function, which uses None to represent a cycle in the graph (...which is questionable lol).
Perhaps it is beyond the scope of this PR, but it might be nice if you can change DAGCircuit::collect_runs to unwrap that option internally. If it fails, it should be on DAGCircuit for getting itself into an invalid state.
There was a problem hiding this comment.
Yeah, I'm fine with making this change to collect_runs() but I'd say lets save this for a follow-up PR so we can do it in isolation.
| } else { | ||
| panic!("Not an op node") | ||
| }; | ||
| if inst.qubits != next_qargs { |
There was a problem hiding this comment.
Cool! I think this is the first example I've seen of the intern IDs saving us some time :)
| .and_modify(|count| *count += value) | ||
| .or_insert(*value); | ||
| } | ||
| let circuit_to_dag = imports::CIRCUIT_TO_DAG.get_bound(py); |
There was a problem hiding this comment.
Maybe it's worth adding a count_ops method to CircuitData, and calling that directly instead of converting to a DAGCircuit first.
I'm not saying that we should necessarily bother maintaining op counts in CircuitData like we do in a DAGCircuit, but computing on the fly should still be faster than recursively converting everything to a DAGCircuit.
Not necessary for this PR, but I wouldn't be opposed to doing it here either.
There was a problem hiding this comment.
It's funny you mention this, that was added last week: 5fc1635
but the implementation there is governed by the api of the python space method and not how I would implement it for performance in the transpiler.
kevinhartman
previously approved these changes
Sep 6, 2024
kevinhartman
approved these changes
Sep 6, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 9, 2024
* Fully port Split2QUnitaries to rust This commit builds off of #13013 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for creating `UnitaryGate` instances and `ParameterExpression` for global phase. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different gates so we should be able to increase the throughput of the pass by leveraging multithreading to handle each gate in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Part of #12208 * Update pass logic with changes from #13095 Some of the logic inside the Split2QUnitaries pass was updated in a recently merged PR. This commit makes those changes so the rust implementation matches the current state of the previous python version. * Use op_nodes() instead of topological_op_nodes() * Use Fn trait instead of FnMut for callback We don't need the callback to be mutable currently so relax the trait to just be `Fn` instead of `FnMut`. If we have a need for a mutable environment callback in the future we can change this easily enough without any issues. * Avoid extra edge operations in replace_on_incoming_qubits * Rename function
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
This commit builds off of #12959 and the other data model in Rust
infrastructure and migrates the InverseCancellation pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves Rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly for handling parameter
expressions. But otherwise the entirety of the pass
operates in rust now.
This is just a first pass at the migration here, it moves the pass to
use loops in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done for different inverse gates/pairs so we should
be able to the throughput of the pass by leveraging multithreading to
handle each inverse option in parallel. This commit does not attempt
this though, because of the Python dependency and also the data
structures around gates and the dag aren't really setup for
multithreading yet and there likely will need to be some work to
support that.
Details and comments
This PR is based on top of #12959 and as such github shows the entire contents of #12959 in addition to the contents of this PR. To see the contents of this PR you can look at HEAD on this branch, or just look at:rebased00c7e10
Fixes #12271
Part of #12208
TODO: