Fix parameter deserialization bugs in `QPY` #13727

ElePT · 2025-01-23T13:41:18Z

Summary

This PR fixes a series of reported issues in QPY:

The common denominator for these issues is the assumption that parameter names are unique within a circuit. This may not be true in cases where parameter re-assignment has taken place and both the old and new parameters share a name (but have different uuids). In these cases, the substitution operation is recorded in the qpy_replay field and should be taken into account during serialization. The bug materialized in two flavors:

name clash when deserializing the ParameterVector component of a ParameterExpression (for which a new from_expr flag has been added to read_parameter_vec). This came up with NLocal implementations (including subclasses).
name clash when deserializing the ParameterExpression directly. This use case is tested in test_duplicated_param_name, and fix by tweaking the encoded mapping to use uuids as keys instead of names.

This fix tweaks the way ParameterExpressions are stored, so it probably needs a QPY version bump.

Details and comments

coveralls · 2025-01-23T14:11:09Z

Pull Request Test Coverage Report for Build 15437163816

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

24 of 24 (100.0%) changed or added relevant lines in 3 files are covered.
240 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.03%) to 87.911%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	1	92.48%
crates/synthesis/src/two_qubit_decompose.rs	1	91.63%
qiskit/synthesis/multi_controlled/mcx_synthesis.py	4	97.7%
crates/qasm2/src/parse.rs	6	97.61%
qiskit/visualization/circuit/matplotlib.py	228	48.79%

Totals
Change from base Build 15415448813:	0.03%
Covered Lines:	81413
Relevant Lines:	92608

💛 - Coveralls

test/python/circuit/test_circuit_load_from_qpy.py

Fix black

qiskit-bot · 2025-01-28T13:45:28Z

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

@Qiskit/terra-core
@mtreinish
@nkanazawa1989

releasenotes/notes/fix-qpy-fauli-feature-map-33292033755879ee.yaml

Cryoris · 2025-01-28T15:45:45Z

It seems like this should also be reproducible outside of the NLocal, or do you think this only happens in that context? 🤔

…yaml Co-authored-by: Julien Gacon <gaconju@gmail.com>

ElePT · 2025-01-28T16:24:36Z

I know what you mean, the parameter replay sequence should be reproducible, but I have not been able to reproduce it outside of NLocal. I did stop trying after a while so it's something I guess could be retried in the future.

Cryoris · 2025-02-14T10:23:06Z

I don't fully understand how this bug is triggered and why we weren't able to reproduce it outside of the PauliFeatureMap, which makes me a bit worried that we don't 100% understand what's going on and that we might fix a symptom but not the full bug. E.g. could this also happen for Parameters instead of ParameterVectors, if we used these in PauliFeatureMap? I think I'd slightly prefer holding off a merge until we really understood this since it's touching a quite fundamental bit, but I'm happy to be overvoted if people think otherwise 🙂

…blueprint

…fails for legacy ZZFeatureMap)

ElePT · 2025-05-12T13:02:49Z

To summarize the changes in this PR:

It modifies the parameter serialization format to use uuid instead of name to avoid overwriting data in the case where two parameters with the same name have been used in the same circuit (it is not possible to have 2 parameters with the same name simultaneously, but any parameter used in the circuit is encoded in the parameter expression replay and it is possible to have two different parameters with the same name at different points in time, for example, if a parameter is substituted by one of the same name)
Because the change in (1) is a breaking change (QPY 14 would not be able to load a circuit generated with this change, the opposite is still possible), it bumps the QPY format to 15
The change in (1) meant that now "intermediate" parameter vectors (not necessarily used in the present circuit) can raise a partial initialization user warning. To avoid this, I introduced the initialize_full_vec flag that counts the full parameter vector as initialized if the parameter vector is part of a parameter expression. This avoided the warning for new n_local circuit library components (zz_feature_map, pauli_feature_map...) but did not prevent the warning for the blueprint circuit counterparts (ZZFeatureMap, PauliFeatureMap), because they do this partial initialization of standalone parameter vectors that are not detected as part of any parameter expression. I believe that this is indistinguishable from the use case that we are testing where a parameter vector is used directly and we want the user warning to be raised.
The only solution I could find for bypassing this warning was adding a manual exception for the 3 known classes: ZZFeatureMap, ZFeatureMap, PauliFeatureMap. This bypass actually also affects the new circuit library counterparts, because they share the same name, but I figured that we should keep the more general solution applied in (3) in case we stop supporting loading blueprint circuits and we want to get rid of this exceptional handling by gate name.

qiskit/qpy/binary_io/circuits.py

t-imamichi · 2025-05-13T14:42:27Z

Thank you for fixing the issue. I added a minor comment.

Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>

t-imamichi · 2025-05-27T13:23:55Z

LGTM. I would like someone of qpy team to review it.

Cryoris

Sorry it took so long to review -- this looks like a good solution to me (it should maybe be noted I'm a QPY padawan still tho). I have one main concern/question about the hardcoded exceptions below 🙂

qiskit/qpy/__init__.py

qiskit/qpy/binary_io/value.py

qiskit/qpy/binary_io/circuits.py

test/python/circuit/test_circuit_load_from_qpy.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

…e element uuids from root uuid

Cryoris

LGTM, thank you!

* Don't assume parameter vector names are unique * Add from_expr flag * Add reno * Specify issue number Fix black * Update releasenotes/notes/fix-qpy-fauli-feature-map-33292033755879ee.yaml Co-authored-by: Julien Gacon <gaconju@gmail.com> * Update fix, add tests * Update reno * Update comment * Update test_circuit_load_from_qpy.py Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com> * Bump qiskit version, update docs * Avoid new warning * Assert user warning is not raised during regression tests (currently fails for legacy ZZFeatureMap) * Rename flag and add comments * Remove unnecessary logic * Document new option * Document new option * Apply suggestions from Imamichi-san's code review Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com> * Update qiskit/qpy/__init__.py Co-authored-by: Julien Gacon <gaconju@gmail.com> * Cleaner implementation: use parameter vector ROOT uuid as key, resolve element uuids from root uuid * Improve docs and comments * Fix lint * Add parameter equality tests --------- Co-authored-by: Julien Gacon <gaconju@gmail.com> Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>

Don't assume parameter vector names are unique

457068f

ElePT force-pushed the fix-qpy-blueprint branch from 0de998b to 457068f Compare January 24, 2025 10:44

Add from_expr flag

c4820b8

ElePT mentioned this pull request Jan 27, 2025

Reduce parallelism for qpy tests #13735

Merged

ElePT changed the title ~~Fix bug in QPY with PauliFeatureMap class~~ Fix bug in qpy with PauliFeatureMap class Jan 27, 2025

ElePT added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jan 28, 2025

Add reno

01645a3

ElePT commented Jan 28, 2025

View reviewed changes

test/python/circuit/test_circuit_load_from_qpy.py Outdated Show resolved Hide resolved

Specify issue number

36835f7

Fix black

ElePT force-pushed the fix-qpy-blueprint branch from 934fa78 to 36835f7 Compare January 28, 2025 13:45

ElePT added this to the 2.0.0 milestone Jan 28, 2025

ElePT marked this pull request as ready for review January 28, 2025 13:45

ElePT requested a review from a team as a code owner January 28, 2025 13:45

ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jan 28, 2025

Cryoris reviewed Jan 28, 2025

View reviewed changes

releasenotes/notes/fix-qpy-fauli-feature-map-33292033755879ee.yaml Outdated Show resolved Hide resolved

Update releasenotes/notes/fix-qpy-fauli-feature-map-33292033755879ee.…

3ed897c

…yaml Co-authored-by: Julien Gacon <gaconju@gmail.com>

1ucian0 assigned Cryoris and ElePT Feb 13, 2025

mtreinish modified the milestones: 2.0.0, 1.4.1 Mar 3, 2025

1ucian0 modified the milestones: 1.4.1, 1.4.2 Mar 4, 2025

ElePT mentioned this pull request Mar 25, 2025

QAOA circuit dumped to QPY 13 raises an error when loading #14089

Closed

ElePT added 2 commits March 25, 2025 14:54

Merge branch 'main' of https://github.com/Qiskit/qiskit into fix-qpy-…

0c8fc49

…blueprint

Update fix, add tests

8020674

This was referenced Apr 8, 2025

QAOA circuits cause Error 3211 with qiskit-ibm-runtime 0.37 Qiskit/qiskit-ibm-runtime#2193

Closed

ZZFeatureMap dumped to QPY 13 raises an error when loading #14088

Closed

eliarbel added the mod: qpy Related to QPY serialization label Apr 15, 2025

Avoid new warning

4b8942a

ElePT force-pushed the fix-qpy-blueprint branch from 4f30d29 to 4b8942a Compare May 12, 2025 11:55

ElePT added 3 commits May 12, 2025 14:03

Assert user warning is not raised during regression tests (currently …

8f047bb

…fails for legacy ZZFeatureMap)

Rename flag and add comments

3227ff5

Remove unnecessary logic

2ca4f08

ElePT requested a review from t-imamichi May 12, 2025 13:23

ElePT added 2 commits May 12, 2025 16:22

Document new option

82858fc

Document new option

7bac395

t-imamichi reviewed May 13, 2025

View reviewed changes

qiskit/qpy/binary_io/circuits.py Outdated Show resolved Hide resolved

Apply suggestions from Imamichi-san's code review

9b64ea4

Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>

Cryoris reviewed Jun 2, 2025

View reviewed changes

qiskit/qpy/__init__.py Outdated Show resolved Hide resolved

qiskit/qpy/binary_io/value.py Outdated Show resolved Hide resolved

qiskit/qpy/binary_io/circuits.py Outdated Show resolved Hide resolved

test/python/circuit/test_circuit_load_from_qpy.py Show resolved Hide resolved

ElePT and others added 6 commits June 3, 2025 15:05

Update qiskit/qpy/__init__.py

6df285c

Co-authored-by: Julien Gacon <gaconju@gmail.com>

Merge branch 'main' into fix-qpy-blueprint

97b70bc

Cleaner implementation: use parameter vector ROOT uuid as key, resolv…

87ae03e

…e element uuids from root uuid

Improve docs and comments

5f5d4cd

Fix lint

73cb441

Add parameter equality tests

273eec6

ElePT removed the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jun 4, 2025

Cryoris approved these changes Jun 4, 2025

View reviewed changes

Cryoris enabled auto-merge June 4, 2025 08:21

Cryoris added this pull request to the merge queue Jun 4, 2025

Merged via the queue into Qiskit:main with commit 1dd1983 Jun 4, 2025
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix parameter deserialization bugs in `QPY` #13727

Fix parameter deserialization bugs in `QPY` #13727

Uh oh!

ElePT commented Jan 23, 2025 •

edited

Loading

Uh oh!

coveralls commented Jan 23, 2025 •

edited

Loading

Uh oh!

Uh oh!

qiskit-bot commented Jan 28, 2025

Uh oh!

Uh oh!

Cryoris commented Jan 28, 2025

Uh oh!

ElePT commented Jan 28, 2025

Uh oh!

Cryoris commented Feb 14, 2025

Uh oh!

ElePT commented May 12, 2025 •

edited

Loading

Uh oh!

Uh oh!

t-imamichi commented May 13, 2025

Uh oh!

t-imamichi commented May 27, 2025

Uh oh!

Cryoris left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cryoris left a comment

Uh oh!

Uh oh!

Uh oh!

Fix parameter deserialization bugs in QPY #13727

Fix parameter deserialization bugs in QPY #13727

Uh oh!

Conversation

ElePT commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

coveralls commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15437163816

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Uh oh!

qiskit-bot commented Jan 28, 2025

Uh oh!

Uh oh!

Cryoris commented Jan 28, 2025

Uh oh!

ElePT commented Jan 28, 2025

Uh oh!

Cryoris commented Feb 14, 2025

Uh oh!

ElePT commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

t-imamichi commented May 13, 2025

Uh oh!

t-imamichi commented May 27, 2025

Uh oh!

Cryoris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cryoris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fix parameter deserialization bugs in `QPY` #13727

Fix parameter deserialization bugs in `QPY` #13727

ElePT commented Jan 23, 2025 •

edited

Loading

coveralls commented Jan 23, 2025 •

edited

Loading

ElePT commented May 12, 2025 •

edited

Loading