QPY 2.4rc2 fixes by gadial · Pull Request #15900 · Qiskit/qiskit

gadial · 2026-03-30T09:35:04Z

Summary

Fixes two bug in the QPY module related to the handling of ParameterExpression.

Details and comments

Substitution bug

The rust code handling the (now obsolete) substitution command was functioning incorrectly when the input circuit had substitution operations involving parameters not present in the final circuit, and when two substitution commands were used one after the other (the bug making them be applied in inverted order). This was fixed and a backwards compatibility test was added.

Removed symbols bug

In an expression like 2 + x*0, the parameter x is not included in the final expression, but is still counted as a parameter of the expression. Until now, the Rust version of from_qpy which recreated the parameter expression from the qpy replay data stored in the QPY file did not take such parameters into account. This was fixed, with parameters that are substituted away being removed from the list of additional symbols to add. A unit was added.

`test_qpy` printing

As it was difficult pinpointing the problems with the current test_qpy output, more informative prints (with errors being clearly marked with QPY Error) were added.

qiskit-bot · 2026-03-30T14:49:01Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core
@mtreinish

raynelfss

I just briefly went through the code, there are some additions I don't understand (mostly due to unfamiliarity with QPY) but I've pointed out some Rust semantics and surface questions about the implementations.

raynelfss · 2026-03-31T11:09:15Z

crates/circuit/src/parameter/parameter_expression.rs

+        let mut symbols = match additional_symbols {
+            None => HashSet::new(),
+            Some(symbol_map) => symbol_map.clone(),
+        };


If you're not using the additional_symbols argument later, you could just call .unwrap_or() or unwrap_or_default() to avoid cloning the hashmap.

Suggested change

let mut symbols = match additional_symbols {

None => HashSet::new(),

Some(symbol_map) => symbol_map.clone(),

};

let mut symbols = additional_symbols.unwrap_or_default();

This one causes me several problems:

There's no default for HashSet.

I did let mut symbols = additional_symbols.unwrap_or(HashSet::new()); which fails since we need &HashSet.

I did

let empty_set = HashSet::new(); let mut symbols = additional_symbols.unwrap_or(&empty_set);

This made result.extend_symbols(symbols); fail because it needs a set of Symbol, not &Symbol. I changed this, but now it needs lifetime parameters and it still clones the symbol at the end. I'm reverting for now.

HashSet does implement Default. But &HashSet does not.

If the additional symbols thing gets consumed during its usage (which seems to be the case), maybe the argument should drop the reference and use additional_symbols: Option<Hashset<_>>. Just let the user handle the ownership in said case.

Yeah, that tracks, I had not seen that you were using a argument by reference, though I think you can get away with it being passed by value.

crates/qpy/src/params.rs

crates/circuit/src/parameter/parameter_expression.rs

raynelfss · 2026-03-31T13:24:49Z

crates/circuit/src/parameter/parameter_expression.rs

+    {
+        for symbol in symbols {
+            let name = symbol.repr(false);
+            self.name_map.entry(name).or_insert(symbol);


Is this supposed to replace the name map entries that already exist? If so, this is not doing that. It will only insert the ones that are not present. If you wanted to replace the ones that exist as well you should add a call to Entry::and_modify(|_| foo()) before the or_insert call.

No, we don't need to replace anything. The goal here is to add symbols which are not present in the reply but are present in the ParameterExpression (this happens in the Rust based implementation when we have expressions like x*0+2). In theory, all the symbols that we encounter in the replay should already be present here so we can replace, but we don't need it.

I'm worried that we're layering hack on top of hack here. The blast radius of ParameterExpression::from_qpy not being defined in terms of native substitution is getting huge here.

I get that the trouble is coming in because OPReplay was defined in such a way that it can't represent the subs operation, but the cleanest solution (which would avoid all of these extra rewrites of the replay stream) would be to make the OpReplay struct able to represent them properly, and then just call that.

Note that this part of the code is not related to the subs operation; we must add the symbols that were left out of the QPY replay and were not present in the QPY file except in the symbol table. The subs operations merely tell us which of them we don't need to keep in the final count.

crates/qpy/src/params.rs

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

jakelishman

I'm sorry this is a bit late, and that it's not complete (no great excuse other than I've had a bad headache today).

I think I would like to take an extra day here, and let me try and offer an alternative PR later tomorrow. I think fundamentally the problem is that OPReplay was just defined wrong way back when ParameterExpression moved to Rust, and we're layering more and more fragile code and long-range assumptions on top of each other to deal with this now we're trying to use it with QPY. I would like to do major re-work to that that makes it hew far closer to the original intent of the replay, and obviates the need for pre-processing it so heavily in Rust. It won't make Rust's use of it in serialisation any less efficient, it'll just mean that it can also natively represent what we need for the backwards compatibility to pre-Rust ParameterExpression.

I know we're already inside the rc period, but I don't think what I'm proposing could ever be suitable for a backport; we need the rc testing of it. I'm not at all confident that the combination of subs_expression and additional_symbols handles every possible weird combination we might have from pre-Rust ParameterExpression, especially because it's hard to write/run the automated tests of those (because of the multiple-version requirement), and that if we encounter more problems, we're not going to be able to fix them in-branch.

edit: You can ignore my comments in the actual review for now - I left them in when I pressed submit, but they don't need responding to or acting on.

jakelishman · 2026-03-31T17:06:30Z

crates/qpy/src/params.rs

+    if let Ok(symbol) = exp.try_to_symbol() {
+        if exp.num_of_symbols() == 1 {
+            Some(symbol)
+        } else {
+            None
+        }
+    } else {
+        None
+    }


No need to change this, but if it helps in the future, the "one-liner" version of this is something like

exp.try_to_symbol().ok().filter(|_| exp.num_of_symbols() == 1)

Nice! Done in 775b496

jakelishman · 2026-03-31T17:07:18Z

crates/circuit/src/parameter/parameter_expression.rs

        replay
    }
+    #[inline]
+    pub fn num_of_symbols(&self) -> usize {


Minor style nitpick: we don't usually use "of" - num_symbols is typical (compare QuantumCircuit.num_qubits, etc).

Done in 775b496

jakelishman · 2026-03-31T17:08:50Z

crates/circuit/src/parameter/parameter_expression.rs

    pub fn from_qpy(
        replay: &[OPReplay],
        subs_operations: Option<Vec<(usize, HashMap<Symbol, ParameterExpression>)>>,
+        additional_symbols: Option<&HashSet<Symbol>>,
    ) -> Result<Self, ParameterError> {


Can you pull this function into the QPY crate as part of this patch, so we can apply the "no panicking" lint rule to it (and fix the subsequent failures)? I'm still very worried about the amount of panics here.

I don't know how - moving it to the QPY crate creates circular dependency since ParameterExpression::__setstate__ requires from_qpy. However, I tried removing the panics from the function nevertheless in 775b496.

jakelishman · 2026-03-31T17:33:58Z

crates/circuit/src/parameter/parameter_expression.rs

+    {
+        for symbol in symbols {
+            let name = symbol.repr(false);
+            self.name_map.entry(name).or_insert(symbol);


I'm worried that we're layering hack on top of hack here. The blast radius of ParameterExpression::from_qpy not being defined in terms of native substitution is getting huge here.

I get that the trouble is coming in because OPReplay was defined in such a way that it can't represent the subs operation, but the cleanest solution (which would avoid all of these extra rewrites of the replay stream) would be to make the OpReplay struct able to represent them properly, and then just call that.

jakelishman

Ok, so I added the test_degenerate_* tests into the test_roundtrip handling to stress-test it more, like this:

diff --git a/test/python/qpy/test_roundtrip.py b/test/python/qpy/test_roundtrip.py
index d33e9259fd..7394971b78 100644
--- a/test/python/qpy/test_roundtrip.py
+++ b/test/python/qpy/test_roundtrip.py
@@ -78,6 +78,7 @@ class TestQPYRoundtrip(QiskitTestCase):
         )
         self.assertEqual(circuit, new_circuit)
         self.assertEqual(circuit.layout, new_circuit.layout)
+        self.assertEqual(circuit.parameters, new_circuit.parameters)
 
     @all_qpy_combinations(QPY_RUST_READ_MIN_VERSION)
     def test_simple(self, version, write_with, read_with):
@@ -223,6 +224,20 @@ class TestQPYRoundtrip(QiskitTestCase):
         qc.ry(exp, 0)
         self.assert_roundtrip_equal(qc, version=version, read_with=read_with, write_with=write_with)
 
+    @all_qpy_combinations(QPY_RUST_READ_MIN_VERSION)
+    def test_degenerate_parameter_expression(self, version, write_with, read_with):
+        """Test a circuit with a parameter expression that simplifies to 0."""
+        x = Parameter("x")
+        y_vec = ParameterVector("y", 2)
+        z = Parameter("z")
+        cases = [0 * x, 0 * x + 2, 0 * x + z, x - x, 0 * y_vec[0], 0 * (x + y_vec[1])]
+        for case in cases:
+            qc = QuantumCircuit(1)
+            qc.rz(case, 0)
+            self.assert_roundtrip_equal(
+                qc, version=version, write_with=write_with, read_with=read_with
+            )
+
     @all_qpy_combinations(QPY_RUST_READ_MIN_VERSION)
     def test_random_circuits(self, version, write_with, read_with):
         """Test loading a random circuit works"""

and I find that this PR still has some problems:

test.python.qpy.test_roundtrip.TestQPYRoundtrip.test_degenerate_parameter_expression_10__17___Python____Rust__
--------------------------------------------------------------------------------------------------------------

Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):

      File "/Users/jake/code/qiskit/venv313/lib/python3.13/site-packages/ddt.py", line 221, in wrapper
    return func(self, *args, **kwargs)

      File "/Users/jake/code/qiskit/terra/test/python/qpy/test_roundtrip.py", line 237, in test_degenerate_parameter_expression
    self.assert_roundtrip_equal(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        qc, version=version, write_with=write_with, read_with=read_with
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^

      File "/Users/jake/code/qiskit/terra/test/python/qpy/test_roundtrip.py", line 73, in assert_roundtrip_equal
    new_circuit = read_circuit(
        qpy_file,
    ...<2 lines>...
        use_rust=use_rust_for_read,
    )

      File "/Users/jake/code/qiskit/terra/qiskit/qpy/binary_io/circuits.py", line 1690, in read_circuit
    return _qpy.read_circuit(
           ~~~~~~~~~~~~~~~~~^
        file_obj, version, metadata_deserializer, use_symengine, annotation_factories
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^

    qiskit.qpy.exceptions.QpyError: 'type conversion failed: Failure while loading parameter expression'


test.python.qpy.test_roundtrip.TestQPYRoundtrip.test_degenerate_parameter_expression_11__17___Rust____Python__
--------------------------------------------------------------------------------------------------------------

Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):

      File "/Users/jake/code/qiskit/venv313/lib/python3.13/site-packages/ddt.py", line 221, in wrapper
    return func(self, *args, **kwargs)

      File "/Users/jake/code/qiskit/terra/test/python/qpy/test_roundtrip.py", line 237, in test_degenerate_parameter_expression
    self.assert_roundtrip_equal(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        qc, version=version, write_with=write_with, read_with=read_with
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^

      File "/Users/jake/code/qiskit/terra/test/python/qpy/test_roundtrip.py", line 81, in assert_roundtrip_equal
    self.assertEqual(circuit.parameters, new_circuit.parameters)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

      File "/Users/jake/.local/share/uv/python/cpython-3.13.9-macos-aarch64-none/lib/python3.13/unittest/case.py", line 907, in assertEqual
    assertion_func(first, second, msg=msg)
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^

      File "/Users/jake/.local/share/uv/python/cpython-3.13.9-macos-aarch64-none/lib/python3.13/unittest/case.py", line 900, in _baseAssertEqual
    raise self.failureException(msg)

    AssertionError: ParameterView([Parameter(x)]) != ParameterView([])

it's ^ this kind of stuff I'm worried about with the layering-on approach we're taking here.

jakelishman · 2026-04-01T17:54:21Z

I've opened #15934 as an admittedly more invasive patch, especially at this point in the cycle, but one that should be more compatible with other versions of Qiskit (both QPY and ParameterExpression) -the deserialisation logic is just single-stage and conceptually hewing to what the replay format was supposed to be, which makes it more resilient to variations in how it's generated.

gadial added 6 commits March 30, 2026 12:34

Empty commit to trigger tests

c5ce8c9

Add a failing backwards compatibility test

fc566e2

Partial fix for the new test case, and better error printing

17011cd

Bufgix in how multiple subs for the same replay op are performed

238e5c2

Test + fix to the empty replay bug

6bbdd14

Remove symbols that were substituted away

d30189e

gadial marked this pull request as ready for review March 30, 2026 14:48

gadial requested a review from a team as a code owner March 30, 2026 14:48

This was referenced Mar 31, 2026

[DO NOT MERGE] Add backwards compatibility test for real amplitude circuits #15898

Closed

QPY: Correctly load ParameterExpression with empty replay #15876

Closed

raynelfss reviewed Mar 31, 2026

View reviewed changes

gadial and others added 2 commits March 31, 2026 17:18

Apply suggestions from code review

a811a4e

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

Revert one change

4ca2dae

jakelishman reviewed Mar 31, 2026

View reviewed changes

Fixes according to PR review

775b496

jakelishman reviewed Apr 1, 2026

View reviewed changes

jakelishman mentioned this pull request Apr 1, 2026

Rewrite QPY ParameterExpression handling in pure Polish form #15934

Open

jakelishman added this to the 2.4.0rc3 milestone Apr 1, 2026

Conversation

gadial commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Substitution bug

Removed symbols bug

test_qpy printing

Uh oh!

qiskit-bot commented Mar 30, 2026

Uh oh!

raynelfss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakelishman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

jakelishman commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gadial commented Mar 30, 2026 •

edited

Loading

`test_qpy` printing

jakelishman left a comment •

edited

Loading