Enable avoiding Python operation creation in transpiler #12692

mtreinish · 2024-06-28T22:00:57Z

Summary

Since #12459 accessing node.op in the transpiler eagerly creates a Python object on access. This is because we now are no longer storing a Python object internally and we need to rebuild the object to return the python object as expected by the api. This is causing a significant performance regression because of the extra overhead. The longer term goal is to move as much of the performance critical passes to operate in rust which will eliminate this overhead. But in the meantime we can mitigate the performance overhead by changing the Python access patterns to avoid the operation object creation. This commit adds some new getter methods to DAGOpNode to give access to the inner rust data so that we can avoid the extra overhead. As a proof of concept this updates the unitary synthesis pass in isolation. Doing this fixes the regression caused by #12459 for that pass. We can continue this migration for everything else in follow up PRs. This commit is mostly to establish the pattern and add the python space access methods.

Details and comments

Since Qiskit#12459 accessing `node.op` in the transpiler eagerly creates a Python object on access. This is because we now are no longer storing a Python object internally and we need to rebuild the object to return the python object as expected by the api. This is causing a significant performance regression because of the extra overhead. The longer term goal is to move as much of the performance critical passes to operate in rust which will eliminate this overhead. But in the meantime we can mitigate the performance overhead by changing the Python access patterns to avoid the operation object creation. This commit adds some new getter methods to DAGOpNode to give access to the inner rust data so that we can avoid the extra overhead. As a proof of concept this updates the unitary synthesis pass in isolation. Doing this fixes the regression caused by Qiskit#12459 for that pass. We can continue this migration for everything else in follow up PRs. This commit is mostly to establish the pattern and add the python space access methods.

qiskit-bot · 2024-06-28T22:01:02Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core
@kevinhartman
@mtreinish

coveralls · 2024-06-28T22:25:15Z

Pull Request Test Coverage Report for Build 9718970161

Details

52 of 134 (38.81%) changed or added relevant lines in 6 files are covered.
8 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.08%) to 89.694%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	5	7	71.43%
crates/circuit/src/circuit_instruction.rs	0	33	0.0%
crates/circuit/src/dag_node.rs	22	69	31.88%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.35%
crates/qasm2/src/lex.rs	6	92.37%

Totals
Change from base Build 9718390567:	-0.08%
Covered Lines:	64110
Relevant Lines:	71476

💛 - Coveralls

qiskit/transpiler/passes/synthesis/unitary_synthesis.py

coveralls · 2024-06-29T00:24:48Z

Pull Request Test Coverage Report for Build 9719830598

Details

53 of 135 (39.26%) changed or added relevant lines in 6 files are covered.
16 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.09%) to 89.687%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	5	7	71.43%
crates/circuit/src/circuit_instruction.rs	0	33	0.0%
crates/circuit/src/dag_node.rs	22	69	31.88%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.35%
crates/qasm2/src/parse.rs	6	97.61%
crates/qasm2/src/lex.rs	8	92.37%

Totals
Change from base Build 9718390567:	-0.09%
Covered Lines:	64105
Relevant Lines:	71476

💛 - Coveralls

coveralls · 2024-06-29T15:53:12Z

Pull Request Test Coverage Report for Build 9725168342

Details

127 of 181 (70.17%) changed or added relevant lines in 9 files are covered.
28 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.07%) to 89.707%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/circuit_instruction.rs	26	33	78.79%
crates/circuit/src/dag_node.rs	46	69	66.67%
crates/circuit/src/operations.rs	7	31	22.58%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.35%
crates/qasm2/src/lex.rs	8	91.35%
crates/qasm2/src/parse.rs	18	96.69%

Totals
Change from base Build 9718390567:	-0.07%
Covered Lines:	64147
Relevant Lines:	71507

💛 - Coveralls

This commit updates the commutative cancellation and commutation analysis transpiler pass. It builds off of Qiskit#12692 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression on these passes introduced by the extra conversion cost of Qiskit#12459. As part of this the commutation checker is rewritten in rust since all that requires is gates in rust which we've had a representation of since Qiskit#12459 merged.

ElePT

I think that it's nice to have an intermediate solution until the passes are migrated to rust, but I wonder how do we want to approach the follow-ups. Do we want to try migrating as many python passes to not access node.op first, independently of when we plan to have a rust implementation for them? Or does it make sense to wait and see which passes we don't manage to migrate to rust in the near term and only update those?

ElePT · 2024-07-01T12:20:03Z

crates/circuit/src/dag_node.rs

+            py, op, None, None, params, label, duration, unit, condition,
+        )?;
+        instruction.qubits = qargs.into();
+        instruction.clbits = cargs.into();


I might be missing something here, but why aren't the qubits and clbits directly given to the CircuitInstruction constructor?

It's because of a type mismatch. The constructor for a circuit instruction takes in an impl IntoIterator while we have a PyTuple here which isn't doesn't implement the IntoItertator trait. I thought about collecting the tuples into a Vec so we could pass them to new(), but at the end of the day the CircuitInstruction has a PyTuple in it, so I just elected to do this to avoid needing any conversions or tweaks to the interfaces.

mtreinish · 2024-07-01T13:59:19Z

I think that it's nice to have an intermediate solution until the passes are migrated to rust, but I wonder how do we want to approach the follow-ups. Do we want to try migrating as many python passes to not access node.op first, independently of when we plan to have a rust implementation for them? Or does it make sense to wait and see which passes we don't manage to migrate to rust in the near term and only update those?

Yeah I was thinking of trying to do this first. At this point I'm a bit skeptical we'll get any passes ported before the 1.2 PR proposal freeze deadline in ~2 weeks given the current state of the dagcircuit PR. So I pushed this up to give us a way to mitigate the regression caused by #12459 in the short term for 1.2. But there are probably some passes that we can work with just DAGOpNode/CircuitInstruction from rust like what I did in #12650 and we should try to do that instead of updating the Python code in those cases when we encounter them.

This commit updates the commutative cancellation and commutation analysis transpiler pass. It builds off of Qiskit#12692 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression on these passes introduced by the extra conversion cost of Qiskit#12459.

This commit moves to use rust gates for the ConsolidateBlocks transpiler pass. Instead of generating the unitary matrices for the gates in a 2q block Python side and passing that list to a rust function this commit switches to passing a list of DAGOpNodes to the rust and then generating the matrices inside the rust function directly. This is similar to what was done in Qiskit#12650 for Optimize1qGatesDecomposition. Besides being faster to get the matrix for standard gates, it also reduces the eager construction of Python gate objects which was a significant source of overhead after Qiskit#12459. To that end this builds on the thread of work in the two PRs Qiskit#12692 and Qiskit#12701 which changed the access patterns for other passes to minimize eager gate object construction.

This commit updates the BasisTranslator transpiler pass. It builds off of Qiskit#12692 and Qiskit#12701 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression introduced by the extra conversion cost of Qiskit#12459 on the BasisTranslator.

ElePT

Sounds good. Approving!

This commit updates the commutative cancellation and commutation analysis transpiler pass. It builds off of Qiskit#12692 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression on these passes introduced by the extra conversion cost of Qiskit#12459.

This commit moves to use rust gates for the ConsolidateBlocks transpiler pass. Instead of generating the unitary matrices for the gates in a 2q block Python side and passing that list to a rust function this commit switches to passing a list of DAGOpNodes to the rust and then generating the matrices inside the rust function directly. This is similar to what was done in Qiskit#12650 for Optimize1qGatesDecomposition. Besides being faster to get the matrix for standard gates, it also reduces the eager construction of Python gate objects which was a significant source of overhead after Qiskit#12459. To that end this builds on the thread of work in the two PRs Qiskit#12692 and Qiskit#12701 which changed the access patterns for other passes to minimize eager gate object construction.

* Avoid Python op creation in commutative cancellation This commit updates the commutative cancellation and commutation analysis transpiler pass. It builds off of #12692 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression on these passes introduced by the extra conversion cost of #12459. * Remove stray print * Don't add __array__ to DAGOpNode or CircuitInstruction

This commit moves to use rust gates for the ConsolidateBlocks transpiler pass. Instead of generating the unitary matrices for the gates in a 2q block Python side and passing that list to a rust function this commit switches to passing a list of DAGOpNodes to the rust and then generating the matrices inside the rust function directly. This is similar to what was done in Qiskit#12650 for Optimize1qGatesDecomposition. Besides being faster to get the matrix for standard gates, it also reduces the eager construction of Python gate objects which was a significant source of overhead after Qiskit#12459. To that end this builds on the thread of work in the two PRs Qiskit#12692 and Qiskit#12701 which changed the access patterns for other passes to minimize eager gate object construction.

This commit updates the BasisTranslator transpiler pass. It builds off of Qiskit#12692 and Qiskit#12701 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression introduced by the extra conversion cost of Qiskit#12459 on the BasisTranslator.

This commit moves to use rust gates for the ConsolidateBlocks transpiler pass. Instead of generating the unitary matrices for the gates in a 2q block Python side and passing that list to a rust function this commit switches to passing a list of DAGOpNodes to the rust and then generating the matrices inside the rust function directly. This is similar to what was done in Qiskit#12650 for Optimize1qGatesDecomposition. Besides being faster to get the matrix for standard gates, it also reduces the eager construction of Python gate objects which was a significant source of overhead after Qiskit#12459. To that end this builds on the thread of work in the two PRs Qiskit#12692 and Qiskit#12701 which changed the access patterns for other passes to minimize eager gate object construction.

* Use rust gates for ConsolidateBlocks This commit moves to use rust gates for the ConsolidateBlocks transpiler pass. Instead of generating the unitary matrices for the gates in a 2q block Python side and passing that list to a rust function this commit switches to passing a list of DAGOpNodes to the rust and then generating the matrices inside the rust function directly. This is similar to what was done in #12650 for Optimize1qGatesDecomposition. Besides being faster to get the matrix for standard gates, it also reduces the eager construction of Python gate objects which was a significant source of overhead after #12459. To that end this builds on the thread of work in the two PRs #12692 and #12701 which changed the access patterns for other passes to minimize eager gate object construction. * Add rust filter function for DAGCircuit.collect_2q_runs() * Update crates/accelerate/src/convert_2q_block_matrix.rs --------- Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>

This commit updates the BasisTranslator transpiler pass. It builds off of #12692 and #12701 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression introduced by the extra conversion cost of #12459 on the BasisTranslator.

* Avoid Python operation creation in transpiler Since Qiskit#12459 accessing `node.op` in the transpiler eagerly creates a Python object on access. This is because we now are no longer storing a Python object internally and we need to rebuild the object to return the python object as expected by the api. This is causing a significant performance regression because of the extra overhead. The longer term goal is to move as much of the performance critical passes to operate in rust which will eliminate this overhead. But in the meantime we can mitigate the performance overhead by changing the Python access patterns to avoid the operation object creation. This commit adds some new getter methods to DAGOpNode to give access to the inner rust data so that we can avoid the extra overhead. As a proof of concept this updates the unitary synthesis pass in isolation. Doing this fixes the regression caused by Qiskit#12459 for that pass. We can continue this migration for everything else in follow up PRs. This commit is mostly to establish the pattern and add the python space access methods. * Remove unused import * Add path to avoid StandardGate conversion in circuit_to_dag * Add fast path through dag_to_circuit

* Avoid Python op creation in commutative cancellation This commit updates the commutative cancellation and commutation analysis transpiler pass. It builds off of Qiskit#12692 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression on these passes introduced by the extra conversion cost of Qiskit#12459. * Remove stray print * Don't add __array__ to DAGOpNode or CircuitInstruction

* Use rust gates for ConsolidateBlocks This commit moves to use rust gates for the ConsolidateBlocks transpiler pass. Instead of generating the unitary matrices for the gates in a 2q block Python side and passing that list to a rust function this commit switches to passing a list of DAGOpNodes to the rust and then generating the matrices inside the rust function directly. This is similar to what was done in Qiskit#12650 for Optimize1qGatesDecomposition. Besides being faster to get the matrix for standard gates, it also reduces the eager construction of Python gate objects which was a significant source of overhead after Qiskit#12459. To that end this builds on the thread of work in the two PRs Qiskit#12692 and Qiskit#12701 which changed the access patterns for other passes to minimize eager gate object construction. * Add rust filter function for DAGCircuit.collect_2q_runs() * Update crates/accelerate/src/convert_2q_block_matrix.rs --------- Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>

This commit updates the BasisTranslator transpiler pass. It builds off of Qiskit#12692 and Qiskit#12701 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression introduced by the extra conversion cost of Qiskit#12459 on the BasisTranslator.

mtreinish added performance Changelog: None Do not include in changelog labels Jun 28, 2024

mtreinish added this to the 1.2.0 milestone Jun 28, 2024

mtreinish requested review from alexanderivrii, ShellyGarion and a team as code owners June 28, 2024 22:00

mtreinish changed the title ~~Avoid Python operation creation in transpiler~~ Enable avoiding Python operation creation in transpiler Jun 28, 2024

mtreinish commented Jun 28, 2024

View reviewed changes

qiskit/transpiler/passes/synthesis/unitary_synthesis.py Outdated Show resolved Hide resolved

Remove unused import

d70cebe

mtreinish added Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Jun 29, 2024

mtreinish added 2 commits June 29, 2024 10:27

Add path to avoid StandardGate conversion in circuit_to_dag

48ec3ce

Add fast path through dag_to_circuit

cba43f1

ElePT reviewed Jul 1, 2024

View reviewed changes

mtreinish mentioned this pull request Jul 1, 2024

Avoid Python op creation in commutative cancellation #12701

Merged

mtreinish mentioned this pull request Jul 1, 2024

Use rust gates for ConsolidateBlocks #12704

Merged

3 tasks

mtreinish mentioned this pull request Jul 1, 2024

Avoid Python op creation in BasisTranslator #12705

Merged

3 tasks

ElePT approved these changes Jul 2, 2024

View reviewed changes

ElePT added this pull request to the merge queue Jul 2, 2024

Merged via the queue into Qiskit:main with commit d2ab4df Jul 2, 2024
15 checks passed

mtreinish deleted the python-access-methods-to-rust-data branch July 2, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable avoiding Python operation creation in transpiler #12692

Enable avoiding Python operation creation in transpiler #12692

mtreinish commented Jun 28, 2024

qiskit-bot commented Jun 28, 2024

coveralls commented Jun 28, 2024 •

edited

Loading

coveralls commented Jun 29, 2024 •

edited

Loading

coveralls commented Jun 29, 2024 •

edited

Loading

ElePT left a comment

ElePT Jul 1, 2024

mtreinish Jul 1, 2024

mtreinish commented Jul 1, 2024

ElePT left a comment

Enable avoiding Python operation creation in transpiler #12692

Enable avoiding Python operation creation in transpiler #12692

Conversation

mtreinish commented Jun 28, 2024

Summary

Details and comments

qiskit-bot commented Jun 28, 2024

coveralls commented Jun 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9718970161

Details

💛 - Coveralls

coveralls commented Jun 29, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9719830598

Details

💛 - Coveralls

coveralls commented Jun 29, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9725168342

Details

💛 - Coveralls

ElePT left a comment

Choose a reason for hiding this comment

ElePT Jul 1, 2024

Choose a reason for hiding this comment

mtreinish Jul 1, 2024

Choose a reason for hiding this comment

mtreinish commented Jul 1, 2024

ElePT left a comment

Choose a reason for hiding this comment

coveralls commented Jun 28, 2024 •

edited

Loading

coveralls commented Jun 29, 2024 •

edited

Loading

coveralls commented Jun 29, 2024 •

edited

Loading