Skip to content

QPY 18: give the register/clbit payload a tag byte instead of a "\x00"-prefixed string - #16765

Merged
gadial merged 11 commits into
Qiskit:mainfrom
Mohamedma96:qpy-18-tagged-register
Aug 19, 2026
Merged

QPY 18: give the register/clbit payload a tag byte instead of a "\x00"-prefixed string#16765
gadial merged 11 commits into
Qiskit:mainfrom
Mohamedma96:qpy-18-tagged-register

Conversation

@Mohamedma96

@Mohamedma96 Mohamedma96 commented Aug 13, 2026

Copy link
Copy Markdown

QPY values of type 'R' can represent either a ClassicalRegister or a single Clbit.

QPY ≤17: both use the existing string-based encoding, including the \x00 prefix hack for clbits.
QPY 18+: introduce an explicit tagged binary representation: Clbit uses a tag + uint32 index, while ClassicalRegister uses a tag + name.
Added a binrw enum in Rust and matching Python serialization/deserialization.
Added round-trip tests for single-clbit conditions across QPY versions and Rust/Python readers/writers.
Legacy QPY ≤17 encoding remains unchanged.

built on top of this PR:
#16707
which already made the changes needed for pumping up the qpy version.

Relevant commit to review:
(Last commit) - 7dae5fe

AI/LLM disclosure

  • I didn't use LLM tooling, or only used it privately.
  • I used the following tool to help write this PR description:
    Claude Code (Anthropic), version
  • I used the following tool to generate or modify code:

@Mohamedma96
Mohamedma96 requested a review from a team as a code owner August 13, 2026 15:32
@Mohamedma96
Mohamedma96 requested a review from gadial August 13, 2026 15:32
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 13, 2026
@qiskit-bot

Copy link
Copy Markdown
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core
  • @mtreinish

@Cryoris

Cryoris commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

@Mohamedma96 something went wrong in your commit history here, it looks like merge gone wrong. Could you clean up the PR history to only show your commits?

eladven and others added 7 commits August 16, 2026 12:44
Pulse gate calibrations were removed in Qiskit 2.0. Since then the
CalibrationsPack field in the QPY circuit payload has always been written
as an empty placeholder (num_cals=0). Version 18 removes it entirely.

- Bump QPY_VERSION to 18 in qiskit/qpy/common.py
- Gate CalibrationsPack field in QPYCircuit on version < 18 (binrw
  Option + if attribute covers both reader and writer)
- Guard Python writer and reader paths with version < 18 / 5 <= version < 18
- Add regression test asserting v18 output is 2 bytes smaller than v17
- Add reno release note and Version 18 docstring section

Signed-off-by: Elad Venezian <eladv@il.ibm.com>
QPY format versions 1–17 mistakenly serialised integer and float
INSTRUCTION_PARAM values in little-endian byte order. Version 18
corrects this to big-endian, consistent with the rest of the format.

The fix is fully backward-compatible: files written at version ≤ 17
are still read correctly via the LittleForV17AndBelow path. The Python
read/write path (circuits.py) only handles versions ≤ 16 and is
unchanged — the little-endian encoding there remains correct for those
versions.

Key implementation points:
- New ValueEndian enum (Big / Little / LittleForV17AndBelow) replaces
  binrw::Endian at every QPY value call site; ValueEndian::resolve(version)
  is the single place where the version-to-endian mapping lives.
- GenericValue::as_little_for_v17_and_below(&self, version) consolidates
  the remaining as_le() call sites that work directly with GenericValue.
- Regression tests in test/python/qpy/test_v18_big_endian_params.py verify
  that v17 and v18 bytes differ for float params, for-loop integer lists,
  and switch-case labels, and that both versions round-trip correctly.

Signed-off-by: Elad Venezian <eladv@il.ibm.com>
- Inline endian.resolve() at use sites in load_value instead of
  extracting an upfront variable
- Update Version 18 docstring: remove backward-compat boilerplate,
  add mention of the endianness fix
- Consolidate v18 tests into a single test_v18.py with one class
  (TestV17VsV18); drop round-trips already covered by test_roundtrip.py

Signed-off-by: Elad Venezian <eladv@il.ibm.com>
- cargo fmt: reformat circuit_reader.rs, circuit_writer.rs, params.rs,
  py_methods.rs, value.rs (line-length wrapping)
- black: remove extra blank line in test_v18.py
- clippy: add #[allow(dead_code)] on ValueEndian::Little to silence the
  "variant never constructed" warning while keeping the variant for
  completeness

Signed-off-by: Elad Venezian <eladv@il.ibm.com>
Signed-off-by: Elad Venezian <eladv@il.ibm.com>
The CI round-trip tests for QPY v18 were failing because the test
framework was generating Python writer/reader combinations for v17+,
which are intentionally unsupported — Rust is the only supported
codec for those versions.

Root cause: all_qpy_combinations() in test_roundtrip.py generated
(v18, Python, Rust) and (v18, Rust, Python) test cases by patching
QPY_RUST_WRITE_MIN_VERSION. The Python codec was never updated to
handle the v18 big-endian encoding, so round-trips silently produced
wrong values (e.g. integer 1 read back as 72057594037927936).

Fix:
- Filter out Python writer/reader combinations for versions >=
  QPY_RUST_WRITE_MIN_VERSION in all_qpy_combinations(), keeping Python
  only for v13-16 where it is the only option.
- Add explicit QpyError assertions in write_circuit() and read_circuit()
  in binary_io/circuits.py: if the Python path is entered for a version
  that requires Rust, raise immediately with a clear message rather than
  silently producing corrupt output. This makes the version boundary
  self-documenting and prevents future regressions.

Also fix test/python/qpy/test_v18.py license header (http -> https).

Signed-off-by: Elad Venezian <eladv@il.ibm.com>
Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
@Mohamedma96
Mohamedma96 force-pushed the qpy-18-tagged-register branch from a8432fb to d922727 Compare August 16, 2026 09:46
@ShellyGarion ShellyGarion removed the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 16, 2026
@ShellyGarion ShellyGarion added this to the 2.6.0 milestone Aug 16, 2026
@ShellyGarion ShellyGarion added the mod: qpy Related to QPY serialization label Aug 16, 2026
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
@Mohamedma96
Mohamedma96 force-pushed the qpy-18-tagged-register branch from d922727 to 7dae5fe Compare August 16, 2026 12:52
gadial and others added 2 commits August 18, 2026 12:04
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>

@gadial gadial left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; I added some minor comments.

Comment thread crates/qpy/src/value.rs Outdated
}

/// Position of `clbit` in the circuit being written, which is how both encodings identify it.
fn find_clbit_index(clbit: &ShareableClbit, qpy_data: &QPYWriteData) -> Result<Clbit, QpyError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all good; but let's check whether (a) there are already functions doing this, which I may have missed in the first place and (b) whether the QPY code has other places with this functionality which can also use these functions.

If you already looked into it - great!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, there is fn CircuitData::clbit_index and fn CircuitDatA::cregs_data(&self) that can help avoid going over the cregs with iterator and duplicate code, I saw another code duplication of getting the clbit index or the creg name and updated the code in expr.rs

Both spellings live here even though ``qpy.load`` currently dispatches every payload from
version 13 on to the Rust reader: this is the reference implementation of the format, it is what
``use_rust=False`` selects, and ``test/python/qpy/test_roundtrip.py`` runs it against the Rust
one at every version. A Python implementation that stopped at version 17 would read the newest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Python implementation should not even try to open a QPY18 file (maybe we need to verify this is the case). I'm not sure we need this code at all, although since you already wrote it I don't object to keeping it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but can't the user force using python by setting use_rust = false?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing the circuit read and write functions in #16701, we should have no user-facing use_rust parameter. Basically, whenever the user runs qpy it should always use rust unless the read or write version is too small, and then it falls back to Python.

Comment thread qiskit/qpy/__init__.py Outdated
The ``type`` field can be ``'i'``, ``'f'``, ``'p'``, ``'e'``, ``'s'``, ``'c'``,
``'R'`` or ``'n'`` which dictate the format. ``'R'`` is a `REGISTER_PARAM` payload,
identifying a :class:`.ClassicalRegister` or a single :class:`.Clbit` in the encoding described in
:ref:`version 18 <qpy_version_18>`. For ``'i'`` it's an integer, ``'f'`` it's

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a description of QPY17; we should not mention any QPY18 format here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted, done

Comment thread test/python/qpy/test_roundtrip.py Outdated
qc.if_else(condition, body, false_body, [qc.qubits[0]], [])
self.assert_roundtrip_equal(qc, version=version, read_with=read_with, write_with=write_with)

@all_qpy_combinations(QPY_RUST_READ_MIN_VERSION)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roundtrip tests should not cover QPY18 features which are not supposed to be handled in Python. We do need to test the new features; the #16707 PR added ‎test/python/qpy/test_v18.py‎ for that purpose.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved the tests to test_v18.py and removed the tests from round_trip.py

Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
@gadial
gadial enabled auto-merge August 19, 2026 07:32
@gadial
gadial self-requested a review August 19, 2026 07:33

@gadial gadial left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after the changes.

@gadial
gadial added this pull request to the merge queue Aug 19, 2026
Merged via the queue into Qiskit:main with commit 2268a17 Aug 19, 2026
31 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to Done in Qiskit 2.6 Aug 19, 2026
@ShellyGarion ShellyGarion added the Changelog: Changed Add a "Changed" entry in the GitHub Release changelog. label Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Changed Add a "Changed" entry in the GitHub Release changelog. mod: qpy Related to QPY serialization

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants