From 1606ca35447dfc5a70845dc534b7e351b175e580 Mon Sep 17 00:00:00 2001 From: Jake Lishman Date: Mon, 4 Sep 2023 16:15:44 +0100 Subject: [PATCH] Fail gracefully on bad `SabreDAG` construction (#10724) * Fail gracefully on bad `SabreDAG` construction This is a private, internal Rust type, but it doesn't cost us anything (meaningful) to bounds-check the accessors and ensure we fail gracefully rather than panicking if we ever make a mistake in Python space. This more faithfully fulfills the return value of `SabreDAG::new`, which previously was an effectively infallible `Result`. * Run `cargo fmt` --- .azure/test-linux.yml | 12 ++++-- Cargo.toml | 4 +- crates/accelerate/Cargo.toml | 5 +++ crates/accelerate/src/sabre_layout.rs | 3 +- crates/accelerate/src/sabre_swap/sabre_dag.rs | 43 ++++++++++++++++--- crates/qasm2/Cargo.toml | 5 +++ 6 files changed, 60 insertions(+), 12 deletions(-) diff --git a/.azure/test-linux.yml b/.azure/test-linux.yml index 2f8a12c8b451..c97d4f693975 100644 --- a/.azure/test-linux.yml +++ b/.azure/test-linux.yml @@ -34,6 +34,7 @@ jobs: - task: UsePythonVersion@0 inputs: versionSpec: '${{ parameters.pythonVersion }}' + name: 'usePython' displayName: 'Use Python ${{ parameters.pythonVersion }}' - task: Cache@2 @@ -47,9 +48,14 @@ jobs: displayName: "Cache stestr" - ${{ if eq(parameters.testRust, true) }}: - - bash: | - set -e - cargo test + # We need to avoid linking our crates into full Python extension libraries during Rust-only + # testing because Rust/PyO3 can't handle finding a static CPython interpreter. + - bash: cargo test --no-default-features + env: + # On Linux we link against `libpython` dynamically, but it isn't written into the rpath + # of the test executable (I'm not 100% sure why ---Jake). It's easiest just to forcibly + # include the correct place in the `dlopen` search path. + LD_LIBRARY_PATH: '$(usePython.pythonLocation)/lib:$LD_LIBRARY_PATH' displayName: "Run Rust tests" - bash: | diff --git a/Cargo.toml b/Cargo.toml index d7793938dbac..1f3b4f7c344f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,9 @@ rust-version = "1.64" # Keep in sync with README.md and rust-toolchain.toml. license = "Apache-2.0" [workspace.dependencies] -pyo3 = { version = "0.19.2", features = ["extension-module", "abi3-py38"] } +# This doesn't set `extension-module` as a shared feature because we need to be able to disable it +# during Rust-only testing (see # https://github.com/PyO3/pyo3/issues/340). +pyo3 = { version = "0.19.2", features = ["abi3-py38"] } [profile.release] lto = 'fat' diff --git a/crates/accelerate/Cargo.toml b/crates/accelerate/Cargo.toml index f2e57cf89647..a0b59e6460de 100644 --- a/crates/accelerate/Cargo.toml +++ b/crates/accelerate/Cargo.toml @@ -9,6 +9,11 @@ license.workspace = true name = "qiskit_accelerate" crate-type = ["cdylib"] +[features] +# This is a test-only shim removable feature. See the root `Cargo.toml`. +default = ["extension-module"] +extension-module = ["pyo3/extension-module"] + [dependencies] rayon = "1.7" numpy = "0.19.0" diff --git a/crates/accelerate/src/sabre_layout.rs b/crates/accelerate/src/sabre_layout.rs index c347c2ae4980..4db215e3ed70 100644 --- a/crates/accelerate/src/sabre_layout.rs +++ b/crates/accelerate/src/sabre_layout.rs @@ -144,7 +144,8 @@ fn layout_trial( dag_no_control_forward.num_clbits, dag_no_control_forward.nodes.iter().rev().cloned().collect(), dag_no_control_forward.node_blocks.clone(), - ); + ) + .unwrap(); for _iter in 0..max_iterations { for dag in [&dag_no_control_forward, &dag_no_control_reverse] { diff --git a/crates/accelerate/src/sabre_swap/sabre_dag.rs b/crates/accelerate/src/sabre_swap/sabre_dag.rs index 945ddfd7778a..f6ee63e7eba9 100644 --- a/crates/accelerate/src/sabre_swap/sabre_dag.rs +++ b/crates/accelerate/src/sabre_swap/sabre_dag.rs @@ -12,6 +12,7 @@ use hashbrown::HashMap; use hashbrown::HashSet; +use pyo3::exceptions::PyIndexError; use pyo3::prelude::*; use rustworkx_core::petgraph::prelude::*; @@ -51,7 +52,7 @@ impl SabreDAG { num_clbits: usize, nodes: Vec<(usize, Vec, HashSet)>, node_blocks: HashMap>, - ) -> Self { + ) -> PyResult { let mut qubit_pos: Vec> = vec![None; num_qubits]; let mut clbit_pos: Vec> = vec![None; num_clbits]; let mut dag = DiGraph::with_capacity(nodes.len(), 2 * nodes.len()); @@ -65,30 +66,58 @@ impl SabreDAG { }); let mut is_front = true; for x in qargs { - if let Some(predecessor) = qubit_pos[*x] { + let pos = qubit_pos.get_mut(*x).ok_or_else(|| { + PyIndexError::new_err(format!( + "qubit index {} is out of range for {} qubits", + *x, num_qubits + )) + })?; + if let Some(predecessor) = *pos { is_front = false; dag.add_edge(predecessor, gate_index, ()); } - qubit_pos[*x] = Some(gate_index); + *pos = Some(gate_index); } for x in cargs { - if let Some(predecessor) = clbit_pos[*x] { + let pos = clbit_pos.get_mut(*x).ok_or_else(|| { + PyIndexError::new_err(format!( + "clbit index {} is out of range for {} clbits", + *x, num_qubits + )) + })?; + if let Some(predecessor) = *pos { is_front = false; dag.add_edge(predecessor, gate_index, ()); } - clbit_pos[*x] = Some(gate_index); + *pos = Some(gate_index); } if is_front { first_layer.push(gate_index); } } - SabreDAG { + Ok(SabreDAG { num_qubits, num_clbits, dag, first_layer, nodes, node_blocks, - } + }) + } +} + +#[cfg(test)] +mod test { + use super::SabreDAG; + use hashbrown::{HashMap, HashSet}; + + #[test] + fn no_panic_on_bad_qubits() { + assert!(SabreDAG::new(2, 0, vec![(0, vec![0, 2], HashSet::new())], HashMap::new()).is_err()) + } + + #[test] + fn no_panic_on_bad_clbits() { + assert!(SabreDAG::new(2, 1, vec![(0, vec![0, 1], [0, 1].into())], HashMap::new()).is_err()) } } diff --git a/crates/qasm2/Cargo.toml b/crates/qasm2/Cargo.toml index 3d89bb1b129a..197f70bcf5c8 100644 --- a/crates/qasm2/Cargo.toml +++ b/crates/qasm2/Cargo.toml @@ -9,6 +9,11 @@ license.workspace = true name = "qiskit_qasm2" crate-type = ["cdylib"] +[features] +# This is a test-only shim removable feature. See the root `Cargo.toml`. +default = ["extension-module"] +extension-module = ["pyo3/extension-module"] + [dependencies] hashbrown = "0.14.0" pyo3.workspace = true