Add fast-path construction to `DAGCircuit` methods #10753

jakelishman · 2023-08-31T17:43:43Z

Summary

This optimises the constructor functions
DAGCircuit.apply_operation_back, apply_operation_front and compose by giving callers a way to skip the validity checking when passing known-good values. In most cases when we're operating on the DAG, we are certain that we're already upholding the invariants of the data structure by nature of having a copy_empty_like or similar, and the user should not pay a runtime price for what we should catch during testing.

Details and comments

I ran a benchmark suite and asv didn't pop up anything statistically significant in changes but quite a bit of stuff saw ~5% improvements. I'm just running benchmarks on a laptop in other use, and our benchmark suite is quite outdated now.

In a more targeted microbenchmark with setup

from qiskit.circuit.library import QuantumVolume
from qiskit.converters import circuit_to_dag
qc = QuantumVolume(100, seed=0).decompose()

the call circuit-to_dag(qc, copy_operations=False) went from 34.3(7)ms to 28.6(7)ms, so a ~15% speedup. That's indicative of what's happening when we're doing things like rebuilding the DAG after Sabre or during ApplyLayout.

This optimises the constructor functions `DAGCircuit.apply_operation_back`, `apply_operation_front` and `compose` by giving callers a way to skip the validity checking when passing known-good values. In most cases when we're operating on the DAG, we are certain that we're already upholding the invariants of the data structure by nature of having a `copy_empty_like` or similar, and the user should not pay a runtime price for what we should catch during testing.

qiskit-bot · 2023-08-31T17:43:47Z

One or more of the the following people are requested to review this:

@ElePT
@Qiskit/terra-core
@ikkoham
@woodsp-ibm

jakelishman · 2023-08-31T17:45:10Z

qiskit/dagcircuit/dagcircuit.py

-    def _add_op_node(self, op, qargs, cargs):
-        """Add a new operation node to the graph and assign properties.
-
-        Args:
-            op (qiskit.circuit.Operation): the operation associated with the DAG node
-            qargs (list[Qubit]): list of quantum wires to attach to.
-            cargs (list[Clbit]): list of classical wires to attach to.
-        Returns:
-            int: The integer node index for the new op node on the DAG
-        """
-        # Add a new operation node to the graph
-        new_node = DAGOpNode(op=op, qargs=qargs, cargs=cargs, dag=self)
-        node_index = self._multi_graph.add_node(new_node)
-        new_node._node_id = node_index
-        self._increment_op(op)
-        return node_index
-


This is a fairly simple function used in only two places which already necessarily had a lot of duplication, and throws away information that the outer contexts need (the DAGOpNode - we previously had to retrieve it from _multi_graph again), so I just inlined it into its call sites.

jlapeyre · 2023-09-06T12:48:47Z

It would be useful to design a canonical way to choose whether to do expensive safety checks. The simplest "design" is using the same name for the option to elide checks. For example, we have

qiskit/qiskit/synthesis/discrete_basis/solovay_kitaev.py

Line 93 in 61e5bde

check_input: bool = True,

qiskit/qiskit/circuit/library/generalized_gates/linear_function.py

Lines 78 to 79 in 61e5bde

    
                       validate_input: if True, performs more expensive input validation checks, 
        
                           such as checking that a given n x n matrix is invertible.

I'm concerned about adding more variety to these options, making the barrier to cleaning it up in the future even larger.

Maybe this PR could use the design we want (again, maybe simplest is best and the design is just a naming convention). And a followup PR could make this uniform to the extent possible across qiskit.

jakelishman · 2023-09-06T13:10:31Z

Fwiw we also have validate, require_cp, require_tp in quantum_info, and cycle_check within DAGCircuit, which are all types of input validation as well. I don't mind especially what we go for (I err on the shorter side when the meaning is unambiguous) - which do you prefer so I can update the PR?

jlapeyre · 2023-09-06T17:33:37Z

Short story. I am ok with using check for now. It may not be the best name, but the marginal tech debt of a name that must be changed is small. Trying to figure this out now would derail this PR.

longer:

For the sake of brevity, I didn't mention concerns about: corner cases, internal vs external api, conceptual and motivational differences in eliding checks, interactions between checks.

Of course that all becomes immediately important. I won't suggest unifying require_tp and require_cp into check, as they were already split (#3660) from require_cptp and given different defaults. No reason was given, but I'm sure there was one. I grepped all of the qiskit repos and only find these options in a few tests in qiskit-experiments, and always set to False.

I think standardizing on something like check_some_invariant, and check_all might be a good first step. I.e. in this case check becomes check_all and require_tp becomes check_tp. Some of these currently log a warning others error. That should probably be worked into the naming.

jakelishman · 2023-09-06T20:27:12Z

That sounds like an interesting topic to pursue potentially under the guise of defining a "naming convention" guide for Qiskit. Perhaps we could move that into a separate issue to track?

edit: just pushed a commit to fix the merge conflict.

jlapeyre · 2023-09-07T04:56:10Z

In bits_in_operation, shouldn't this

qiskit/qiskit/dagcircuit/dagcircuit.py

Lines 572 to 574 in 7252db3

    
           if (condition := getattr(operation, "condition", None)) is not None: 
        
               yield from condition_resources(condition).clbits 
        
           if isinstance(operation, SwitchCaseOp):

have elif isinstance(operation, SwitchCaseOp)? without elif it's harder to read if you don't know the code. I mean it's not obvious that these are not mutually exclusive branches.

jlapeyre · 2023-09-07T05:11:51Z

qiskit/dagcircuit/dagcircuit.py

+        return getattr(operation, "condition", None) is not None or isinstance(
+            operation, SwitchCaseOp
+        )
+


This optimization is in addition to the optimization discussed in the opening comment, right? Did you benchmark this optimization? It's clear that set(self._bits_in_operation(op)).union(cargs) will be slower than just cargs even when the generator is empty. But I would not be surprised if this is not measurable in benchmarks.

Yeah, looks like I didn't mention this little bit in the commit message. Removing the _may_have_bits check and always eagerly calling _bits_in_operation caused the microbenchmark at the top to go from 27.7(7)ms with the current PR to 29.3(9)ms when timed just now.

That's worth the trouble although it adds a bit of clutter

jlapeyre · 2023-09-07T05:15:59Z

I think that this

qiskit/qiskit/dagcircuit/dagcircuit.py

Lines 1194 to 1197 in 7252db3

    
           if not propagate_condition: 
        
               node_wire_order += [ 
        
                   bit for bit in self._bits_in_operation(node.op) if bit not in node_cargs 
        
               ]

should use _operation_may_have_bits as well.

jlapeyre · 2023-09-07T05:18:07Z

The questions above are all I have. The main part of the PR, i.e. introducing a check option looks good to me.

jakelishman · 2023-09-07T10:18:15Z

I think I wrote that SwitchCaseOp if/if as a defensive thing - the two branches are mutually exclusive right now, but to me making it if/else felt like making more assumptions than were needed for the function.

jakelishman · 2023-09-07T10:27:35Z

I've done the substitute_node_with_dag thing in 14b215c. In general I didn't look to optimise any of the other DAGCircuit functions to keep the scope of the PR targetted and because DAG reconstruction from SabreSwap is one place that bottlenecks transpile which only uses apply_operation_back at the moment.

jakelishman · 2023-09-07T16:47:12Z

(you need to leave an approving review to enable merge - the merge queue handles bringing the PR up-to-date with main, so that's not necessary, but the code-owner approval is)

jlapeyre · 2023-09-07T17:00:43Z

Huh. I tried to do that. Clicked on the green approve button....
I'll try again.
Maybe the first click only changed input focus ?

jlapeyre

Looks good.

jlapeyre

Looks good.

jakelishman added performance Changelog: New Feature Include in the "Added" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Aug 31, 2023

jakelishman added this to the 0.45.0 milestone Aug 31, 2023

jakelishman requested review from alexanderivrii, ShellyGarion, a team, woodsp-ibm and ElePT as code owners August 31, 2023 17:43

jakelishman commented Aug 31, 2023

View reviewed changes

mtreinish assigned jlapeyre Aug 31, 2023

Merge remote-tracking branch 'ibm/main' into dag-fast-path

51e4a8e

jlapeyre reviewed Sep 7, 2023

View reviewed changes

jakelishman added 2 commits September 7, 2023 11:25

Check for possibility of bits before iterating

14b215c

Merge remote-tracking branch 'ibm/main' into dag-fast-path

20a5407

jlapeyre enabled auto-merge September 7, 2023 16:39

Merge branch 'main' into dag-fast-path

820649a

jlapeyre approved these changes Sep 7, 2023

View reviewed changes

jlapeyre added this pull request to the merge queue Sep 7, 2023

Merged via the queue into Qiskit:main with commit 3059193 Sep 7, 2023
13 checks passed

jakelishman mentioned this pull request Sep 8, 2023

Use physical qubits internally within Sabre #10782

Merged

Raghav-Bell mentioned this pull request Sep 14, 2023

Removed code deprecated in qiskit-terra 0.21, released on June 2022 #10754

Merged

jakelishman deleted the dag-fast-path branch February 7, 2024 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fast-path construction to `DAGCircuit` methods #10753

Add fast-path construction to `DAGCircuit` methods #10753

jakelishman commented Aug 31, 2023

qiskit-bot commented Aug 31, 2023

jakelishman Aug 31, 2023

jlapeyre commented Sep 6, 2023

jakelishman commented Sep 6, 2023

jlapeyre commented Sep 6, 2023

jakelishman commented Sep 6, 2023 •

edited

Loading

jlapeyre commented Sep 7, 2023 •

edited

Loading

jlapeyre Sep 7, 2023

jakelishman Sep 7, 2023

jlapeyre Sep 7, 2023

jlapeyre commented Sep 7, 2023

jlapeyre commented Sep 7, 2023

jakelishman commented Sep 7, 2023

jakelishman commented Sep 7, 2023

jakelishman commented Sep 7, 2023

jlapeyre commented Sep 7, 2023 •

edited

Loading

jlapeyre left a comment

jlapeyre left a comment

Add fast-path construction to DAGCircuit methods #10753

Add fast-path construction to DAGCircuit methods #10753

Conversation

jakelishman commented Aug 31, 2023

Summary

Details and comments

qiskit-bot commented Aug 31, 2023

jakelishman Aug 31, 2023

Choose a reason for hiding this comment

jlapeyre commented Sep 6, 2023

jakelishman commented Sep 6, 2023

jlapeyre commented Sep 6, 2023

jakelishman commented Sep 6, 2023 • edited Loading

jlapeyre commented Sep 7, 2023 • edited Loading

jlapeyre Sep 7, 2023

Choose a reason for hiding this comment

jakelishman Sep 7, 2023

Choose a reason for hiding this comment

jlapeyre Sep 7, 2023

Choose a reason for hiding this comment

jlapeyre commented Sep 7, 2023

jlapeyre commented Sep 7, 2023

jakelishman commented Sep 7, 2023

jakelishman commented Sep 7, 2023

jakelishman commented Sep 7, 2023

jlapeyre commented Sep 7, 2023 • edited Loading

jlapeyre left a comment

Choose a reason for hiding this comment

jlapeyre left a comment

Choose a reason for hiding this comment

Add fast-path construction to `DAGCircuit` methods #10753

Add fast-path construction to `DAGCircuit` methods #10753

jakelishman commented Sep 6, 2023 •

edited

Loading

jlapeyre commented Sep 7, 2023 •

edited

Loading

jlapeyre commented Sep 7, 2023 •

edited

Loading