Fix QPY loading delay integers durations incorrectly (#16076)

* Fix QPY loading delay integers durations incorrectly

This commit fixes an issue with parsing QPY payloads which have circuits
that contain delays with duration of units dt. Durations of dt are
integers and that is preserved in the QPY data. However our rust data
model doesn't have support for an integer in a Rust PackedInstruction's
parameters (this is an inconsistency which we should arguably fix but
that is separate from this bugfix). To workaround this the QPY reader in
rust was sticking the integer into a ParameterExpression as a constant
without any symbols. This resulted in storing the integer in Rust but
treating the object as a ParameterExpression and not an int, which in
Qiskit's rust data model is mapped to a Param::Obj (indicating a Python
object parameter). This mismatch in types was not really noticeable to
Python because the ParameterExpression with the constant integer was
coerced to an integer when it's passed to Python. However, this would
break underlying assumptions for Rust code that is interacting with the
delay. For example, the experimental rust qasm3 exporter would encounter
the ParameterExpression on the delay and error because it can't handle
parameter expressions yet. However fundamentally it could because this
is just an integer. The reproducer for this failure is:

```python
import io
from qiskit import qpy, qasm3, QuantumCircuit

qc = QuantumCircuit(1)
qc.delay(1, 0)

qasm3.dumps_experimental(qc)

with io.BytesIO() as fptr:
    qpy.dump(qc, fptr)
    fptr.seek(0)
    qc2 = qpy.load(fptr)[0]

qasm3.dumps_experimental(qc2)
```

The OQ3 experimental exporter is wrong not to handle
`ParameterExpression::try_as_value` returning an `int`, but it's _more_
wrong that QPY is producing a `ParameterExpression` on deserialisation
in the first place.

To fix this issue this commit removes the conversion of the `int` in the
qpy payload into a ParameterExpression and just retains an integer until
we write out the Delay's PackedInstruction where we convert that rust
int into a python int for the parameter. Doing this unraveled a deeper
issue in how endianess is handled in QPY. In general everything in QPY
is supposed to be encoded using network byte order (i.e. big endian).
However, in the case of instructions' parameters there was a mistake
made in QPY where the integer and float values for an instruction's
parameters were encoded in little endian. All other uses of floats or
ints are correctly big endian. When the raw int was returned to Python
it was incorrectly assuming all integers were a big endian bytes value.
To fix this an endian arg is added to the function which is converting
the bytes arrays into a `GenericValue` enum for floats and ints. Then
the callers of this function in circuit_reader are updated to explicitly
assert what endianess the data in the circuit payload is if there are
any floats or ints. This is `Endian::Little` for any instruction params
that are in the parameters list explicitly and `Endian::Big` everywhere
else. At the same time the handling of flipping the endianess of value
in several places was fixed because this was no longer necessary as the
data was read now using the correct byte order.

This fundamentally stems from all the base values being stored in a
single binrw generic value pack which is trying to encode all the primitive
types in a single place. Ideally we should be handling the primitive types
explicitly for each data pack field. But this was not
changed to keep the diff minimal for backport

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* Fix typo in qpy compat test

* Fix tests again

* Fix delay stretch expr test typo

* Add more round-trip tests

* Remove dead control-flow switching logic

All the entries in this list either take zero parameters, or would have
caused program control flow to enter `unpack_control_flow` and _not_
`unpack_py_instruction`, so this switching logic is dead.

* Remove endianness switching from `BigUint` parsing

The `BigUint` keys are only valid in contexts where they are guaranteed
to be in network order.  The `pack_biguint` function knew this, but
`unpack_biguint` mistakenly gained an `endian` argument, which was
required to always be `Big`.

* Reword release note

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

Author: mtreinish

crates/qpy/src/circuit_reader.rs

@@ -19,7 +19,7 @@
 // Ideally, serialization is done by packing in a binrw-enhanced struct and using the
 // `write` method into a `Cursor` buffer, but there might be exceptions.
 
-use binrw::Endian::Little;
+use binrw::Endian;
 use hashbrown::HashMap;
 use num_bigint::BigUint;
 use num_complex::Complex64;
@@ -48,7 +48,6 @@ use qiskit_circuit::operations::{
 };
 use qiskit_circuit::packed_instruction::{PackedInstruction, PackedOperation};
 use qiskit_circuit::parameter::parameter_expression::ParameterExpression;
-use qiskit_circuit::parameter::symbol_expr;
 use qiskit_circuit::var_stretch_container::{StretchType, VarType};
 use qiskit_circuit::{Block, classical, imports};
 use qiskit_circuit::{Clbit, Qubit};
@@ -153,7 +152,7 @@ fn unpack_condition(
     match &condition_pack.data {
         ConditionData::None => Ok(None),
         ConditionData::Expression(exp_pack) => {
-            let exp_value = unpack_generic_value(exp_pack, qpy_data)?;
+            let exp_value = unpack_generic_value(exp_pack, qpy_data, Endian::Big)?;
             match exp_value {
                 GenericValue::Expression(exp) => Ok(Some(Condition::Expr(exp.clone()))),
                 _ => Err(QpyError::InvalidExpression(
@@ -243,13 +242,16 @@ fn get_instruction_bits(
 fn get_instruction_values(
     instruction: &formats::CircuitInstructionV2Pack,
     qpy_data: &mut QPYReadData,
+    endian: Endian,
 ) -> Result<Vec<GenericValue>, QpyError> {
     // note that numbers are not read correctly - they are read in big endian, but for instruction parameters, due to historical reasons,
     // they are stored in little endian
     let inst_params: Vec<GenericValue> = instruction
         .params
         .iter()
-        .map(|packed_param: &formats::GenericDataPack| unpack_generic_value(packed_param, qpy_data))
+        .map(|packed_param: &formats::GenericDataPack| {
+            unpack_generic_value(packed_param, qpy_data, endian)
+        })
         .collect::<Result<_, QpyError>>()?;
     Ok(inst_params)
 }
@@ -287,18 +289,10 @@ pub fn instruction_values_to_params(
     } else {
         // params
         let inst_params: Vec<Param> = values
-            .iter()
+            .into_iter()
             .map(|value| -> Result<_, QpyError> {
-                match value.as_le() {
-                    // TODO: is the "as_le" here enough to solve the "params are little endian" problem?
+                match value {
                     GenericValue::Float64(float) => Ok(Param::Float(float)),
-                    GenericValue::Int64(int) => {
-                        let value_expression =
-                            symbol_expr::SymbolExpr::Value(symbol_expr::Value::Int(int));
-                        Ok(Param::ParameterExpression(Arc::new(
-                            ParameterExpression::from_symbol_expr(value_expression),
-                        )))
-                    }
                     GenericValue::ParameterExpression(exp) => Ok(Param::ParameterExpression(exp)),
                     GenericValue::ParameterExpressionSymbol(symbol) => {
                         Ok(Param::ParameterExpression(Arc::new(
@@ -310,7 +304,7 @@ pub fn instruction_values_to_params(
                             ParameterExpression::from_symbol(symbol),
                         )))
                     }
-                    _ => Ok(Param::Obj(py_convert_from_generic_value(value)?)),
+                    _ => Ok(Param::Obj(py_convert_from_generic_value(&value)?)),
                 }
             })
             .collect::<Result<_, QpyError>>()?;
@@ -403,7 +397,7 @@ fn unpack_standard_gate(
             instruction.gate_class_name
         )));
     };
-    let param_values = get_instruction_values(instruction, qpy_data)?;
+    let param_values = get_instruction_values(instruction, qpy_data, Endian::Little)?;
     Ok((op, param_values))
 }
 
@@ -420,7 +414,7 @@ fn unpack_standard_instruction(
             instruction.gate_class_name
         )));
     };
-    let param_values = get_instruction_values(instruction, qpy_data)?;
+    let param_values = get_instruction_values(instruction, qpy_data, Endian::Little)?;
     Ok((op, param_values))
 }
 
@@ -433,21 +427,21 @@ fn unpack_pauli_product_measurement(
             "Pauli Product Measurement should have exactly 3 parameters".to_string(),
         ));
     }
-    let z = unpack_generic_value(&instruction.params[0], qpy_data)?
+    let z = unpack_generic_value(&instruction.params[0], qpy_data, Endian::Little)?
         .as_typed::<Vec<bool>>()
         .ok_or_else(|| {
             QpyError::InvalidParameter(
                 "Pauli product measurement z parameter should be a boolean vector".to_string(),
             )
         })?;
-    let x = unpack_generic_value(&instruction.params[1], qpy_data)?
+    let x = unpack_generic_value(&instruction.params[1], qpy_data, Endian::Little)?
         .as_typed::<Vec<bool>>()
         .ok_or_else(|| {
             QpyError::InvalidParameter(
                 "Pauli product measurement x parameter should be a boolean vector".to_string(),
             )
         })?;
-    let neg = unpack_generic_value(&instruction.params[2], qpy_data)?
+    let neg = unpack_generic_value(&instruction.params[2], qpy_data, Endian::Little)?
         .as_typed::<bool>()
         .ok_or_else(|| {
             QpyError::InvalidParameter(
@@ -470,22 +464,22 @@ fn unpack_pauli_product_rotation(
             "No matrix for unitary op".to_string(),
         ));
     }
-    let z = unpack_generic_value(&instruction.params[0], qpy_data)?
+    let z = unpack_generic_value(&instruction.params[0], qpy_data, Endian::Little)?
         .as_typed::<Vec<bool>>()
         .ok_or_else(|| {
             QpyError::InvalidParameter(
                 "Pauli product measurement z parameter should be a boolean vector".to_string(),
             )
         })?;
-    let x = unpack_generic_value(&instruction.params[1], qpy_data)?
+    let x = unpack_generic_value(&instruction.params[1], qpy_data, Endian::Little)?
         .as_typed::<Vec<bool>>()
         .ok_or_else(|| {
             QpyError::InvalidParameter(
                 "Pauli product measurement x parameter should be a boolean vector".to_string(),
             )
         })?;
-    let angle_value = unpack_generic_value(&instruction.params[2], qpy_data)?;
-    let angle = generic_value_to_param(&angle_value, Little)?;
+    let angle_value = unpack_generic_value(&instruction.params[2], qpy_data, Endian::Little)?;
+    let angle = generic_value_to_param(&angle_value)?;
     let rotation = PauliProductRotation { z, x, angle };
     let pbc = Box::new(PauliBased::PauliProductRotation(rotation));
     let op = PackedOperation::from_pauli_based(pbc);
@@ -498,7 +492,7 @@ fn unpack_unitary(
     qpy_data: &mut QPYReadData,
 ) -> Result<(PackedOperation, Vec<GenericValue>), QpyError> {
     let GenericValue::NumpyObject(py_matrix) =
-        unpack_generic_value(&instruction.params[0], qpy_data)?
+        unpack_generic_value(&instruction.params[0], qpy_data, Endian::Little)?
     else {
         return Err(QpyError::InvalidParameter(
             "No matrix for unitary op".to_string(),
@@ -543,7 +537,7 @@ fn unpack_control_flow(
                 .params
                 .iter()
                 .skip(1)
-                .map(|param| unpack_generic_value(param, qpy_data))
+                .map(|param| unpack_generic_value(param, qpy_data, Endian::Little))
                 .collect::<Result<_, QpyError>>()?;
             let duration_value = if let Some(duration_pack) = instruction.params.first() {
                 unpack_duration_value(duration_pack, qpy_data)?
@@ -566,7 +560,8 @@ fn unpack_control_flow(
         ControlFlowType::BreakLoop => ControlFlow::BreakLoop,
         ControlFlowType::ContinueLoop => ControlFlow::ContinueLoop,
         ControlFlowType::ForLoop => {
-            let mut instruction_values = get_instruction_values(instruction, qpy_data)?;
+            let mut instruction_values =
+                get_instruction_values(instruction, qpy_data, Endian::Big)?;
             param_values = instruction_values.split_off(2);
             let mut iter = instruction_values.into_iter();
             let (mut collection_value_pack, loop_param_value_pack) =
@@ -590,17 +585,18 @@ fn unpack_control_flow(
         ControlFlowType::IfElse => {
             let condition = unpack_condition(&instruction.condition, qpy_data)?
                 .ok_or_else(|| QpyError::MissingData("if else condition is missing".to_string()))?;
-            param_values = get_instruction_values(instruction, qpy_data)?;
+            param_values = get_instruction_values(instruction, qpy_data, Endian::Big)?;
             ControlFlow::IfElse { condition }
         }
         ControlFlowType::WhileLoop => {
             let condition = unpack_condition(&instruction.condition, qpy_data)?
                 .ok_or_else(|| QpyError::MissingData("if else condition is missing".to_string()))?;
-            param_values = get_instruction_values(instruction, qpy_data)?;
+            param_values = get_instruction_values(instruction, qpy_data, Endian::Big)?;
             ControlFlow::While { condition }
         }
         ControlFlowType::SwitchCase => {
-            let mut instruction_values = get_instruction_values(instruction, qpy_data)?;
+            let mut instruction_values =
+                get_instruction_values(instruction, qpy_data, Endian::Big)?;
             let (target_value, case_label_list) = if instruction_values.len() < 3 {
                 // we follow the python way of storing switch params
                 // the first param is the target, the next param is the cases specifier
@@ -708,12 +704,12 @@ fn unpack_py_instruction(
     qpy_data: &mut QPYReadData,
 ) -> Result<(PackedOperation, Vec<GenericValue>), QpyError> {
     let name = instruction.gate_class_name.clone();
-    let mut instruction_values = get_instruction_values(instruction, qpy_data)?;
+    let mut instruction_values = get_instruction_values(instruction, qpy_data, Endian::Little)?;
     Python::attach(|py| -> Result<_, QpyError> {
         let mut py_params: Vec<Bound<PyAny>> = instruction_values
             .iter()
             .map(|value| -> Result<_, QpyError> {
-                generic_value_to_param(value, binrw::Endian::Little)?
+                generic_value_to_param(value)?
                     .into_pyobject(py)
                     .map_err(QpyError::from)
             })
@@ -872,12 +868,12 @@ fn unpack_custom_instruction(
     let custom_instruction = custom_instructions_map.get(&name).ok_or_else(|| {
         QpyError::MissingData("Custom instruction data not found for {name}".to_string())
     })?;
-    let instruction_values = get_instruction_values(instruction, qpy_data)?;
+    let instruction_values = get_instruction_values(instruction, qpy_data, Endian::Little)?;
     Python::attach(|py| -> Result<_, QpyError> {
         let py_params: Vec<Bound<PyAny>> = instruction_values
             .iter()
             .map(|value| -> Result<_, QpyError> {
-                generic_value_to_param(value, binrw::Endian::Little)?
+                generic_value_to_param(value)?
                     .into_pyobject(py)
                     .map_err(QpyError::from)
             })
@@ -1171,8 +1167,12 @@ fn deserialize_pauli_evolution_gate(
                 sparse_pauli_op_pack,
             )) => {
                 // formats::PauliDataPack::SparsePauliOp(sparse_pauli_op_pack) => {
-                let data =
-                    load_value(ValueType::NumpyObject, &sparse_pauli_op_pack.data, qpy_data)?;
+                let data = load_value(
+                    ValueType::NumpyObject,
+                    &sparse_pauli_op_pack.data,
+                    qpy_data,
+                    Endian::Big,
+                )?;
                 if let GenericValue::NumpyObject(op_raw_data) = data {
                     Ok(imports::SPARSE_PAULI_OP
                         .get_bound(py)
@@ -1193,7 +1193,12 @@ fn deserialize_pauli_evolution_gate(
     };
     // time is of type ParameterValueType = Union[ParameterExpression, float]
     // we don't have a rust PauliEvolutionGate so we'll convert the time to python
-    let time = load_value(packed_data.time_type, &packed_data.time_data, qpy_data)?;
+    let time = load_value(
+        packed_data.time_type,
+        &packed_data.time_data,
+        qpy_data,
+        Endian::Big,
+    )?;
     let py_time: Py<PyAny> = match time {
         GenericValue::Float64(value) => value.into_py_any(py)?,
         GenericValue::ParameterExpression(exp) => exp.as_ref().clone().into_py_any(py)?,
@@ -1499,14 +1504,12 @@ pub(crate) fn unpack_circuit(
             .annotation_handler
             .load_deserializers(annotation_deserializers_data)?;
     }
-    let global_phase = generic_value_to_param(
-        &load_value(
-            packed_circuit.header.global_phase_type,
-            &packed_circuit.header.global_phase_data,
-            &mut qpy_data,
-        )?,
-        binrw::Endian::Big,
-    )?;
+    let global_phase = generic_value_to_param(&load_value(
+        packed_circuit.header.global_phase_type,
+        &packed_circuit.header.global_phase_data,
+        &mut qpy_data,
+        Endian::Big,
+    )?)?;
     qpy_data.circuit_data.set_global_phase_param(global_phase)?;
     add_standalone_vars(packed_circuit, &mut qpy_data)?;
     add_registers_and_bits(packed_circuit, &mut qpy_data)?;

crates/qpy/src/params.rs

@@ -451,6 +451,7 @@ pub(crate) fn unpack_parameter_expression(
                             item.item_type,
                             &item.item_bytes,
                             qpy_data,
+                            Endian::Big,
                         )?)?;
                         Ok((sym, replacement))
                     })
@@ -628,14 +629,7 @@ pub(crate) fn pack_param_obj(
     })
 }
 
-pub(crate) fn generic_value_to_param(
-    value: &GenericValue,
-    endian: Endian,
-) -> Result<Param, QpyError> {
-    let value = match endian {
-        Endian::Big => value,
-        Endian::Little => &value.as_le(),
-    };
+pub(crate) fn generic_value_to_param(value: &GenericValue) -> Result<Param, QpyError> {
     match value {
         GenericValue::Float64(float_val) => Ok(Param::Float(*float_val)),
         GenericValue::ParameterExpressionSymbol(symbol) => {

crates/qpy/src/value.rs

@@ -405,10 +405,25 @@ impl<T: FromGenericValue> FromGenericValue for Vec<T> {
     }
 }
 
+/// Load a bytes array value of a specified type
+///
+/// # Args
+///
+/// * `type_key` - The type of the data
+/// * `bytes` - The raw data as a u8 array
+/// * `qpy_data` - QPY reader metadata
+/// * `endian` - The endianess of the data used only for reading
+///   `ValueType::Integer` and `ValueType::Float` primitive types
+///   (this applies recursively for `ValueType::Tuple` too). All other
+///   data is big endian per the QPY format documentation. The use of
+///   little endian data for floats and integers in instruction parameter
+///   contexts only is an oversight/mistake in the QPY implementation for
+///   format versions <=17. All data is supposed to be network byte order.
 pub(crate) fn load_value(
     type_key: ValueType,
     bytes: &Bytes,
     qpy_data: &mut QPYReadData,
+    endian: Endian,
 ) -> Result<GenericValue, QpyError> {
     match type_key {
         ValueType::Bool => {
@@ -417,15 +432,21 @@ pub(crate) fn load_value(
         }
         ValueType::Integer => {
             // a little tricky since this can be either i64 or biguint
-            let result = bytes.try_into();
-            if let Ok(value) = result {
-                Ok(GenericValue::Int64(value))
+            if bytes.len() <= 8 {
+                let mut bytes_array: [u8; 8] = [0; 8];
+                for (idx, byte) in bytes.iter().enumerate() {
+                    bytes_array[idx] = *byte;
+                }
+                match endian {
+                    Endian::Little => Ok(GenericValue::Int64(i64::from_le_bytes(bytes_array))),
+                    Endian::Big => Ok(GenericValue::Int64(i64::from_be_bytes(bytes_array))),
+                }
             } else {
                 load_biguint_value(bytes)
             }
         }
         ValueType::Float => {
-            let value: f64 = bytes.try_into()?;
+            let value: f64 = bytes.try_to_f64(endian)?;
             Ok(GenericValue::Float64(value))
         }
         ValueType::Complex => {
@@ -465,7 +486,7 @@ pub(crate) fn load_value(
         }
         ValueType::Tuple => {
             let (elements_pack, _) = deserialize::<GenericDataSequencePack>(bytes)?;
-            let values = unpack_generic_value_sequence(elements_pack, qpy_data)?;
+            let values = unpack_generic_value_sequence(elements_pack, qpy_data, endian)?;
             Ok(GenericValue::Tuple(values))
         }
         ValueType::NumpyObject => {
@@ -619,8 +640,9 @@ pub(crate) fn pack_generic_value(
 pub(crate) fn unpack_generic_value(
     value_pack: &GenericDataPack,
     qpy_data: &mut QPYReadData,
+    endian: Endian,
 ) -> Result<GenericValue, QpyError> {
-    let result = load_value(value_pack.type_key, &value_pack.data, qpy_data)?;
+    let result = load_value(value_pack.type_key, &value_pack.data, qpy_data, endian)?;
     Ok(result)
 }
 
@@ -636,7 +658,7 @@ pub(crate) fn unpack_duration_value(
             let duration = unpack_duration(deserialize::<DurationPack>(&value_pack.data)?.0);
             Ok(GenericValue::Duration(duration))
         }
-        _ => unpack_generic_value(value_pack, qpy_data), // fallback (duration can also be expression)
+        _ => unpack_generic_value(value_pack, qpy_data, Endian::Little), // fallback (duration can also be expression)
     }
 }
 
@@ -690,11 +712,12 @@ pub(crate) fn pack_generic_value_sequence(
 pub(crate) fn unpack_generic_value_sequence(
     value_seqeunce_pack: GenericDataSequencePack,
     qpy_data: &mut QPYReadData,
+    endian: Endian,
 ) -> Result<Vec<GenericValue>, QpyError> {
     value_seqeunce_pack
         .elements
         .iter()
-        .map(|data_pack| unpack_generic_value(data_pack, qpy_data))
+        .map(|data_pack| unpack_generic_value(data_pack, qpy_data, endian))
         .collect()
 }