Fix qpy serialization of substitution of type ParameterExpression

[jlapeyre]

When substitution history ParameterExpression._qpy_replay is serialized, there was no branch for the case that the substituted value is of type ParameterExpression.

This commit fixes this oversight.

• I have added the tests to cover my changes.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.

Tests need to be added.

Closes #13879

[coveralls]

Pull Request Test Coverage Report for Build 13609395583

Details

• 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
• 10 unchanged lines in 2 files lost coverage.
• Overall coverage remained the same at 86.96%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	4	92.48%
crates/qasm2/src/parse.rs	6	96.22%

Totals
Change from base Build 13608179063:	0.0%
Covered Lines:	76073
Relevant Lines:	87480

---

💛 - Coveralls

[jlapeyre]

EDIT: Fixed in fa0d254

As it stands, this PR is not an adequate fix because, although, serialization does not error, deserialization of the result does error.

It looks like the current fix is inadequate because the value of a symbol in the replay is assumed to not be a ParameterExpression. In particular, the header written will not correspond to the data written. Both strings and numbers are written. Since the header is nonsense, an attempt is made to read numeric data into a string, which results in an invalid utf8 codec.

[qiskit-bot]

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

• @Qiskit/terra-core
• @mtreinish
• @nkanazawa1989

[jlapeyre]

I ported this to Qiskit v1.3.2 and verified that it works.

On the main branch, I can't test with qiskit-ibm-runtime, because it is incompatible with the qiskit development branch.

The fix applied to the stable release Qiskit v1.3.2 is not worth much, because deserializing on the server end fails of course. It will need to be updated, too.

Because of this, it looks like this will require a version bump in qpy.

[mtreinish]

Overall this LGTM, I left one small comment inside about trying to minimize the diff for backport reasons. The other thing which might be good is adding a backwards compatibilty test to https://github.com/Qiskit/qiskit/blob/main/test/qpy_compat/test_qpy.py to cover this case so that we ensure we can load payloads with these types of expressions from qiskit >= 1.4.1 with QPY 13.

Because of this, it looks like this will require a version bump in qpy.

I'm on the fence about this. From the way the format spec is written this was just an oversight in the implementation so I view it as a straight bug in the v13 implementation that neither serialization or deserialization works with nested substituted parameter expressions. But your also correct that no v13 implementation out there works with this model yet which makes it feel like doing this in v14 is a better choice.

I'm inclined to just leave this as a v13 fix, and not require v14 for it since the format is still parsable in v13, it will just error because of the deserialization bug.

[mtreinish]

Do you mind reverting the name change here? I agree with the change in the naming, but since we're probably going to backport this PR for 1.4.1 I think it's better to minimize the diff to reduce the risk of regression. So we can keep the change here to just the functional component below adding the missing handling of the PARAMETER_EXPRESSION type in the extra expression/symbols.

We can do the renaming in a standalone PR that we don't backport.

[jlapeyre]

Yes, I agree it's better to revert.

I was uncomfortable with the fact that most of the diff is due to this name change. The motivation is that in various places, the substituted items are referred to as "symbols". Now that we know they are not always symbols, this is misleading. The logic is already kind of hard to follow. I wanted to make it a bit easier for the next person. Having said that, the keys (not the substituted values) are symbols. And if I recall correctly, I didn't change variable names to make them more descriptive everywhere.

The bottom line is the renaming was not as well-thought-out as it could be.

As you say, renaming could be done later. Then it could be done more carefully. Or maybe "never" is a good time to make the change. Because, hopefully, this code won't need to be touched much before it is ported to rust.

A side comment: This code could serve as a base for a pure-Python QPY implementation. It's common for people to choose a slower pure Python library over a fast, compiled, one because the former is more lightweight and flexible. Although at the moment, I don't know of any demand.

[jlapeyre]

Done in 41d61c7

[jlapeyre]

@mtreinish : In the same spirit, I think I should revert this:

-            symbol_map.update(_encode_replay_subs(inst, file_obj, version))
+            _encode_replay_subs(inst, file_obj, version)
+            symbol_map.update(inst.binds)

This cleaned up the logic: _encode_replay_subs does not need to return anything. But it is not necessary for this fix.

EDIT: I indeed did revert in 06daa34

Regarding your comment on increasing version number from 13 to 14. I had a similar thought after writing the comment suggesting doing it.

I agree, it's better not to increase the version number: If you try to deserialize what is written with this fix with the unfixed code you get an error either way: you either hit the bug, or fail because of version mismatch. Furthermore, it is really a bug fix. It's not the kind of bug that people have inadvertently come to rely on. Going to v14 incurs complexity.

[jlapeyre]

EDIT: Fixed

• In qpy compat tests, one needs to bind parameters to numbers in order to compare circuits with parameters. So I did this.

• The unexplained behaviors: Most of this taken care of by always running test_qpy.py only once per python session.

CI is failing when running the qpy compat tests.

I have not yet discovered how to control the testing. Here is a record of debugging.

Fixed.

I have added a new test file with one test: test/python/qpy/test_serialize_value_objects.py
This is a very close to minimal test of the bug fix. It will fail if the bug is not fixed. This passes locally and in CI

To test qpy compatibility:
I copied the code from test_serialize_value_objects.py verbatim to test/qpy_compat/test_qpy.py following the pattern.

I added a function:

def generate_replay_with_expression_substitutions():
    """Circuits with parameters that have substituted expressions in the replay"""
    a = Parameter("a")
    b = Parameter("b")
    a1 = a * 2
    a2 = a1.subs({a: 3 * b})
    qc = QuantumCircuit(1)
    qc.rz(a2, 0)

    return [qc]

Then I added at the bottom of the function generate_circuits():

    if version_parts >= (1, 4, 1):
        output_circuits["replay_with_expressions.qpy"] = (
            generate_replay_with_expression_substitutions()
        )

I try running this: python ./test/qpy_compat/test_qpy.py -v 2.0.0 generate
But this fails, apparently importing a version of qiskit without the fix.

Note that I verify that python is resolving to the python in my virtual environment, which in all other things I run, picks up the current state of my tree (provided I have compiled the most recent rust extensions)

I try the same thing after cd'ing to ./test/qpy_compat . No difference. What is going on here?

Trying solutions at random: I run the ipython cli and do this (again, this is within the proper venv.)

In [1]: %run -i ./test/qpy_compat/test_qpy.py generate --version 2.0.0

All the expected qpy files are generated silently.

Now I run:

In [2]: %run -i ./test/qpy_compat/test_qpy.py load --version 2.0.0
Loading qpy file: full.qpy
Loading qpy file: unitary.qpy
Loading qpy file: multiple.qpy
Loading qpy file: string_parameters.qpy
Loading qpy file: register_edge_cases.qpy
Reference Circuit 0: 
[blah blah]
is not equivalent to qpy loaded circuit 0:

Notice that this is failing on a circuit that has nothing to do with the bug or the fix.

Now in CI, the failure does look like it is indeed failing when trying to load the compat test for this bug fix: replay_with_expressions.qpy

I am continuing to debug. But, if anyone has clarifying insights....

EDIT: I verify this:

> python -c "import qiskit; print(qiskit.__version__)"

2.0.0.dev0+539ab62

I confirm again:

> python ./test/qpy_compat/test_qpy.py generate --version 2.0.0
AttributeError: 'ParameterExpression' object has no attribute 'name'. Did you mean: '_names'?

This is the error you get if the bug fix is not in place

EDIT: There is really something state dependent or stochastic going on with the qpy compat testing:

If I call ./test/qpy_compat/test_qpy.py load --version 2.0.0 repeatedly, sometimes exiting. I get different errors.
(This is from the ipython cli, not starting python from the shell)

Occasionally, I am getting the same error seen in CI

[mtreinish]

This LGTM now, thanks for catching this, debugging it, and fixing the issue.