Remove sort_key attribute from DAGNode (#13736)

* Remove sort_key attribute from DAGNode

Prior to having the DAGCircuit in rust the `sort_key` attribute was added
as a cache to speed up the lexicographical topological sort. Prior to
having this attribute the topological sort method would compute the sort
key as a string on the fly which ended up being a large bottleneck in
transpilation (see: #4040 for more details). However, since migrating
the DAGCircuit implementation to Rust this sort_key attribute isn't
needed anymore because we call the rustworkx-core function with a tuple
of bit objects instead of a string. The `sort_key` atribute was left on
in place for backwards compatibility (it shouldn't have been public,
this was a mistake in #4040) and when we create a python DAGNode object
that will be returned to Python the creation of the sort key is
unnecessary overhead (it takes roughly 1% of profile time to format the
sort_key string during transpilation). Since nothing in DAGCircuit is
actually using it this commit removes it to save the CPU cycles
creating the string on each dag creation. We will need to add a
deprecation to 1.4.0 to mark this removal for 2.0.0 since this was
missed in 1.3.0.

* Simplify star pre-routing sort_key logic

Author: mtreinish

crates/accelerate/src/gate_direction.rs

@@ -17,7 +17,6 @@ use hashbrown::HashSet;
 use pyo3::intern;
 use pyo3::prelude::*;
 use pyo3::types::PyTuple;
-use pyo3::IntoPyObjectExt;
 use qiskit_circuit::operations::OperationRef;
 use qiskit_circuit::packed_instruction::PackedOperation;
 use qiskit_circuit::{
@@ -399,7 +398,6 @@ fn has_calibration_for_op_node(
                     #[cfg(feature = "cache_pygates")]
                     py_op: packed_inst.py_op.clone(),
                 },
-                sort_key: "".into_py_any(py)?,
             },
             DAGNode { node: None },
         ),

crates/circuit/src/dag_circuit.rs

@@ -2674,9 +2674,8 @@ def _format(operand):
     ///
     /// Args:
     ///     key (Callable): A callable which will take a DAGNode object and
-    ///         return a string sort key. If not specified the
-    ///         :attr:`~qiskit.dagcircuit.DAGNode.sort_key` attribute will be
-    ///         used as the sort key for each node.
+    ///         return a string sort key. If not specified the bit qargs and
+    ///         cargs of a node will be used for sorting.
     ///
     /// Returns:
     ///     generator(DAGOpNode, DAGInNode, or DAGOutNode): node in topological order
@@ -2710,9 +2709,8 @@ def _format(operand):
     ///
     /// Args:
     ///     key (Callable): A callable which will take a DAGNode object and
-    ///         return a string sort key. If not specified the
-    ///         :attr:`~qiskit.dagcircuit.DAGNode.sort_key` attribute will be
-    ///         used as the sort key for each node.
+    ///         return a string sort key. If not specified the qargs and
+    ///         cargs of a node will be used for sorting.
     ///
     /// Returns:
     ///     generator(DAGOpNode): op node in topological order
@@ -5612,22 +5610,22 @@ impl DAGCircuit {
         let dag_node = match weight {
             NodeType::QubitIn(qubit) => Py::new(
                 py,
-                DAGInNode::new(py, id, self.qubits.get(*qubit).unwrap().clone_ref(py)),
+                DAGInNode::new(id, self.qubits.get(*qubit).unwrap().clone_ref(py)),
             )?
             .into_any(),
             NodeType::QubitOut(qubit) => Py::new(
                 py,
-                DAGOutNode::new(py, id, self.qubits.get(*qubit).unwrap().clone_ref(py)),
+                DAGOutNode::new(id, self.qubits.get(*qubit).unwrap().clone_ref(py)),
             )?
             .into_any(),
             NodeType::ClbitIn(clbit) => Py::new(
                 py,
-                DAGInNode::new(py, id, self.clbits.get(*clbit).unwrap().clone_ref(py)),
+                DAGInNode::new(id, self.clbits.get(*clbit).unwrap().clone_ref(py)),
             )?
             .into_any(),
             NodeType::ClbitOut(clbit) => Py::new(
                 py,
-                DAGOutNode::new(py, id, self.clbits.get(*clbit).unwrap().clone_ref(py)),
+                DAGOutNode::new(id, self.clbits.get(*clbit).unwrap().clone_ref(py)),
             )?
             .into_any(),
             NodeType::Operation(packed) => {
@@ -5646,7 +5644,6 @@ impl DAGCircuit {
                                 #[cfg(feature = "cache_pygates")]
                                 py_op: packed.py_op.clone(),
                             },
-                            sort_key: format!("{:?}", self.sort_key(id)).into_py_any(py)?,
                         },
                         DAGNode { node: Some(id) },
                     ),
@@ -5655,12 +5652,12 @@ impl DAGCircuit {
             }
             NodeType::VarIn(var) => Py::new(
                 py,
-                DAGInNode::new(py, id, self.vars.get(*var).unwrap().clone_ref(py)),
+                DAGInNode::new(id, self.vars.get(*var).unwrap().clone_ref(py)),
             )?
             .into_any(),
             NodeType::VarOut(var) => Py::new(
                 py,
-                DAGOutNode::new(py, id, self.vars.get(*var).unwrap().clone_ref(py)),
+                DAGOutNode::new(id, self.vars.get(*var).unwrap().clone_ref(py)),
             )?
             .into_any(),
         };

crates/circuit/src/dag_node.rs

@@ -114,8 +114,6 @@ impl DAGNode {
 #[pyclass(module = "qiskit._accelerate.circuit", extends=DAGNode)]
 pub struct DAGOpNode {
     pub instruction: CircuitInstruction,
-    #[pyo3(get)]
-    pub sort_key: PyObject,
 }
 
 #[pymethods]
@@ -131,7 +129,6 @@ impl DAGOpNode {
     ) -> PyResult<Py<Self>> {
         let py_op = op.extract::<OperationFromPython>()?;
         let qargs = qargs.map_or_else(|| PyTuple::empty(py), |q| q.value);
-        let sort_key = qargs.str().unwrap().into();
         let cargs = cargs.map_or_else(|| PyTuple::empty(py), |c| c.value);
         let instruction = CircuitInstruction {
             operation: py_op.operation,
@@ -143,16 +140,7 @@ impl DAGOpNode {
             py_op: op.unbind().into(),
         };
 
-        Py::new(
-            py,
-            (
-                DAGOpNode {
-                    instruction,
-                    sort_key,
-                },
-                DAGNode { node: None },
-            ),
-        )
+        Py::new(py, (DAGOpNode { instruction }, DAGNode { node: None }))
     }
 
     fn __hash__(slf: PyRef<'_, Self>) -> PyResult<u64> {
@@ -239,7 +227,6 @@ impl DAGOpNode {
         mut instruction: CircuitInstruction,
         deepcopy: bool,
     ) -> PyResult<PyObject> {
-        let sort_key = instruction.qubits.bind(py).str().unwrap().into();
         if deepcopy {
             instruction.operation = instruction.operation.py_deepcopy(py, None)?;
             #[cfg(feature = "cache_pygates")]
@@ -248,15 +235,12 @@ impl DAGOpNode {
             }
         }
         let base = PyClassInitializer::from(DAGNode { node: None });
-        let sub = base.add_subclass(DAGOpNode {
-            instruction,
-            sort_key,
-        });
+        let sub = base.add_subclass(DAGOpNode { instruction });
         Py::new(py, sub)?.into_py_any(py)
     }
 
     fn __reduce__(slf: PyRef<Self>, py: Python) -> PyResult<PyObject> {
-        let state = (slf.as_ref().node.map(|node| node.index()), &slf.sort_key);
+        let state = slf.as_ref().node.map(|node| node.index());
         let temp = (
             slf.instruction.get_operation(py)?,
             &slf.instruction.qubits,
@@ -266,9 +250,8 @@ impl DAGOpNode {
     }
 
     fn __setstate__(mut slf: PyRefMut<Self>, state: &Bound<PyAny>) -> PyResult<()> {
-        let (index, sort_key): (Option<usize>, PyObject) = state.extract()?;
+        let index: Option<usize> = state.extract()?;
         slf.as_mut().node = index.map(NodeIndex::new);
-        slf.sort_key = sort_key;
         Ok(())
     }
 
@@ -461,44 +444,29 @@ impl DAGOpNode {
 pub struct DAGInNode {
     #[pyo3(get)]
     pub wire: PyObject,
-    #[pyo3(get)]
-    sort_key: PyObject,
 }
 
 impl DAGInNode {
-    pub fn new(py: Python, node: NodeIndex, wire: PyObject) -> (Self, DAGNode) {
-        (
-            DAGInNode {
-                wire,
-                sort_key: intern!(py, "[]").clone().into(),
-            },
-            DAGNode { node: Some(node) },
-        )
+    pub fn new(node: NodeIndex, wire: PyObject) -> (Self, DAGNode) {
+        (DAGInNode { wire }, DAGNode { node: Some(node) })
     }
 }
 
 #[pymethods]
 impl DAGInNode {
     #[new]
-    fn py_new(py: Python, wire: PyObject) -> PyResult<(Self, DAGNode)> {
-        Ok((
-            DAGInNode {
-                wire,
-                sort_key: intern!(py, "[]").clone().into(),
-            },
-            DAGNode { node: None },
-        ))
+    fn py_new(wire: PyObject) -> PyResult<(Self, DAGNode)> {
+        Ok((DAGInNode { wire }, DAGNode { node: None }))
     }
 
     fn __reduce__<'py>(slf: PyRef<'py, Self>, py: Python<'py>) -> PyResult<Bound<'py, PyTuple>> {
-        let state = (slf.as_ref().node.map(|node| node.index()), &slf.sort_key);
+        let state = slf.as_ref().node.map(|node| node.index());
         (py.get_type::<Self>(), (&slf.wire,), state).into_pyobject(py)
     }
 
     fn __setstate__(mut slf: PyRefMut<Self>, state: &Bound<PyAny>) -> PyResult<()> {
-        let (index, sort_key): (Option<usize>, PyObject) = state.extract()?;
+        let index: Option<usize> = state.extract()?;
         slf.as_mut().node = index.map(NodeIndex::new);
-        slf.sort_key = sort_key;
         Ok(())
     }
 
@@ -534,44 +502,29 @@ impl DAGInNode {
 pub struct DAGOutNode {
     #[pyo3(get)]
     pub wire: PyObject,
-    #[pyo3(get)]
-    sort_key: PyObject,
 }
 
 impl DAGOutNode {
-    pub fn new(py: Python, node: NodeIndex, wire: PyObject) -> (Self, DAGNode) {
-        (
-            DAGOutNode {
-                wire,
-                sort_key: intern!(py, "[]").clone().into(),
-            },
-            DAGNode { node: Some(node) },
-        )
+    pub fn new(node: NodeIndex, wire: PyObject) -> (Self, DAGNode) {
+        (DAGOutNode { wire }, DAGNode { node: Some(node) })
     }
 }
 
 #[pymethods]
 impl DAGOutNode {
     #[new]
-    fn py_new(py: Python, wire: PyObject) -> PyResult<(Self, DAGNode)> {
-        Ok((
-            DAGOutNode {
-                wire,
-                sort_key: intern!(py, "[]").clone().into(),
-            },
-            DAGNode { node: None },
-        ))
+    fn py_new(wire: PyObject) -> PyResult<(Self, DAGNode)> {
+        Ok((DAGOutNode { wire }, DAGNode { node: None }))
     }
 
     fn __reduce__(slf: PyRef<Self>, py: Python) -> PyResult<PyObject> {
-        let state = (slf.as_ref().node.map(|node| node.index()), &slf.sort_key);
+        let state = slf.as_ref().node.map(|node| node.index());
         (py.get_type::<Self>(), (&slf.wire,), state).into_py_any(py)
     }
 
     fn __setstate__(mut slf: PyRefMut<Self>, state: &Bound<PyAny>) -> PyResult<()> {
-        let (index, sort_key): (Option<usize>, PyObject) = state.extract()?;
+        let index: Option<usize> = state.extract()?;
         slf.as_mut().node = index.map(NodeIndex::new);
-        slf.sort_key = sort_key;
         Ok(())
     }
 

qiskit/dagcircuit/dagdependency_v2.py

@@ -13,6 +13,7 @@
 """_DAGDependencyV2 class for representing non-commutativity in a circuit.
 """
 
+import itertools
 import math
 from collections import OrderedDict, defaultdict, namedtuple
 from typing import Dict, List, Generator, Any
@@ -459,7 +460,9 @@ def topological_nodes(self, key=None) -> Generator[DAGOpNode, Any, Any]:
         """
 
         def _key(x):
-            return x.sort_key
+            return ",".join(
+                f"{self.find_bit(q).index:04d}" for q in itertools.chain(x.qargs, x.cargs)
+            )
 
         if key is None:
             key = _key

qiskit/transpiler/passes/routing/star_prerouting.py

@@ -11,12 +11,20 @@
 # that they have been altered from the originals.
 
 """Search for star connectivity patterns and replace them with."""
+import itertools
 from typing import Iterable, Union, Optional, List, Tuple
 from math import floor, log10
 
 from qiskit.circuit import SwitchCaseOp, Clbit, ClassicalRegister, Barrier
 from qiskit.circuit.controlflow import condition_resources, node_resources
-from qiskit.dagcircuit import DAGOpNode, DAGDepNode, DAGDependency, DAGCircuit
+from qiskit.dagcircuit import (
+    DAGOpNode,
+    DAGDepNode,
+    DAGDependency,
+    DAGCircuit,
+    DAGOutNode,
+    DAGInNode,
+)
 from qiskit.transpiler.basepasses import TransformationPass
 from qiskit.transpiler.layout import Layout
 from qiskit.transpiler.passes.routing.sabre_swap import _build_sabre_dag, _apply_sabre_result
@@ -331,7 +339,14 @@ def star_preroute(self, dag, blocks, processing_order):
         }
 
         def tie_breaker_key(node):
-            return processing_order_index_map.get(node, node.sort_key)
+            processing_order = processing_order_index_map.get(node, None)
+            if processing_order is not None:
+                return processing_order
+            if isinstance(node, (DAGInNode, DAGOutNode)):
+                return str(node.wire)
+            return ",".join(
+                f"{dag.find_bit(q).index:04d}" for q in itertools.chain(node.qargs, node.cargs)
+            )
 
         rust_processing_order = _extract_nodes(dag.topological_op_nodes(key=tie_breaker_key), dag)
 

releasenotes/notes/remove-deprecated-sort-key-8921c52db826c8ba.yaml

@@ -0,0 +1,15 @@
+---
+upgrade_transpiler:
+  - |
+    Removed the deprecated ``DAGNode.sort_key`` attribute. This attribute was deprecated
+    in the Qiskit 1.4.0 release. As the lexicographical topological sorting is done internally
+    and rust and the sort key attribute was unused this was removed to remove the overhead
+    from DAG node creation. If you were relying on the sort key you can reproduce it from
+    a given node using something like::
+
+        def get_sort_key(node: DAGNode):
+            if isinstance(node, (DAGInNode, DAGOutNode)):
+                return str(node.wire)
+            return ",".join(
+                f"{dag.find_bit(q).index:04d}" for q in itertools.chain(node.qargs, node.cargs)
+            )