search

Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
4.82k stars 2.29k forks source link

Enable avoiding Python operation creation in transpiler #12692

Open mtreinish opened 2 days ago

mtreinish commented 2 days ago

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

qiskit-bot commented 2 days ago

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

coveralls commented 2 days ago

Pull Request Test Coverage Report for Build 9718970161

Details

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%
<!-- Total: 52 134 38.81% -->
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%
<!-- Total: 8 -->
Totals Coverage Status
Change from base Build 9718390567: -0.08%
Covered Lines: 64110
Relevant Lines: 71476
💛 - Coveralls
coveralls commented 2 days ago

Pull Request Test Coverage Report for Build 9719830598

Details

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%
<!-- Total: 53 135 39.26% -->
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%
<!-- Total: 16 -->
Totals Coverage Status
Change from base Build 9718390567: -0.09%
Covered Lines: 64105
Relevant Lines: 71476
💛 - Coveralls
coveralls commented 2 days ago

Pull Request Test Coverage Report for Build 9725168342

Details

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%
<!-- Total: 127 181 70.17% -->
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%
<!-- Total: 28 -->
Totals Coverage Status
Change from base Build 9718390567: -0.07%
Covered Lines: 64147
Relevant Lines: 71507
💛 - Coveralls
mtreinish commented 4 hours ago

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.