Skip to content
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

Fix overhead of DAGCircuit.remove_op_node #11677

Closed
mtreinish opened this issue Jan 30, 2024 · 3 comments · Fixed by #12756
Closed

Fix overhead of DAGCircuit.remove_op_node #11677

mtreinish opened this issue Jan 30, 2024 · 3 comments · Fixed by #12756
Labels
Milestone

Comments

@mtreinish
Copy link
Member

What should we add?

Right now the DAGCircuit,remove_op_node function has quadratic overhead as a function of the number of bits/dag edges are on an operation. This is do to how the underlying rustworkx.PyDiGraph.remove_node_retain_edges method is used by remove_op_node. Normally this doesn't matter because in most cases this is only ever called on operations with 1 or 2 qubits. However, since the introduction of #10323 this is now getting called on a barrier of n qubits for the the width of the target backend. This can become a large performance bottleneck, for example when targeting a 60k qubit backend calling remove_op_node took 47 min while it was processing the list of qubits.

To fix this optimally we'll need to likely add a new interface to rustworkx to handle this more efficiently.

@mtreinish mtreinish added this to the 1.1.0 milestone Jan 30, 2024
@jakelishman
Copy link
Member

Fwiw I have a proof of concept patch to rustworkx and Qiskit locally that solves this, but the issue is to remind us to sort it out properly after the 1.0 release.

@jakelishman
Copy link
Member

See Qiskit/rustworkx#1083 for the Rustworkx side of this, which is the non-trivial part. If that (or something like it) merges there, the changes on the Qiskit side are pretty trivial - we just need to swap DAGCircuit.remove_op_node to use remove_node_retain_edges_by_{id,key}, where we can use by_id if we've got rid of the equality properties of Bit, and by_key if we haven't (or if we have other things in the wires).

@jakelishman
Copy link
Member

Ha, I forgot we had the issue and opened #12156 to remind myself about it as well. Oh well.

@mtreinish mtreinish modified the milestones: 1.1.0, 1.2.0 May 1, 2024
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Jul 11, 2024
This commit updates the minimum rustworkx version to 0.15.0 to pull in
the new PyDiGraph.remove_node_retain_edges_by_id() method introduced
in that release. This new function is used for the
DAGCircuit.remove_op_node() method instead of the
PyDiGraph.remove_node_retain_edges() function. This new method has much
better scaling characteristics and should improve the performance
characteristics of removing very wide operations from a DAGCircuit.

Fixes Qiskit#11677
Part of Qiskit#12156
github-merge-queue bot pushed a commit that referenced this issue Jul 11, 2024
This commit updates the minimum rustworkx version to 0.15.0 to pull in
the new PyDiGraph.remove_node_retain_edges_by_id() method introduced
in that release. This new function is used for the
DAGCircuit.remove_op_node() method instead of the
PyDiGraph.remove_node_retain_edges() function. This new method has much
better scaling characteristics and should improve the performance
characteristics of removing very wide operations from a DAGCircuit.

Fixes #11677
Part of #12156
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this issue Aug 1, 2024
)

This commit updates the minimum rustworkx version to 0.15.0 to pull in
the new PyDiGraph.remove_node_retain_edges_by_id() method introduced
in that release. This new function is used for the
DAGCircuit.remove_op_node() method instead of the
PyDiGraph.remove_node_retain_edges() function. This new method has much
better scaling characteristics and should improve the performance
characteristics of removing very wide operations from a DAGCircuit.

Fixes Qiskit#11677
Part of Qiskit#12156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants