qiskit icon indicating copy to clipboard operation
qiskit copied to clipboard

Rust implementation of the QPY module

Open gadial opened this issue 9 months ago • 9 comments

Summary

Adds a rust-based implementation of qpy serialization.

Addresses #13131

Details and comments

We use rust structs along with the binrw library to specify and write the byte representation of data (this turns out to be somewhat simpler than how it's currently done in python). There are still many python-dependent elements we need to take into consideration:

  1. Backwards compatibility is currently not supported; whenever an older version is passed, the Python version is used instead. The goal of this PR is to establish a baseline from which it would be relatively easy to add support for previous versions.
  2. File header is still created in Python space; the entrypoint for the rust code is write_circuit and read_circuit (this is being kept until a later PR adds backward compatability)
  3. For every instruction, QPY stores the name of the corresponding python class, e.g. for X gate it stores the string XGate. Currently all the class names are obtained by accessing the __name__ field of the python class; for standard gates and instructions we'll need to switch to a python-independent solution (an ad-hoc dictionary is currently used for the reader component; there may be a better solution).
  4. Since Expr was only recently moved to rust, after most of this PR was complete, we do not have a native Rust serialization yet; rather, we rely on the python serialization code (the _write_expr and _read_expr methods). There are many more places where Rust might be used but Python is used instead.
  5. numpy object serialization is done via python call to numpy's save.

In this PR we make the distinction between packing an object and serializing it:

  • serializing an object means generating a bytes representation for it (a Vec<u8> object, aliased to Bytes in the module.
  • packing an object means extracting its data fields and generating a corresponding rust struct, taken from the formats.rs files. This might involve serializing some of the object's fields, and packing other fields.

Our goal is to minimize serialization as much as possible, allowing us to generate a large packed struct reflecting the structure of the qpy file as much as possible before serializing; the code can be further improved to reflect this, although I think this can wait to a future PR.

gadial avatar Apr 03 '25 09:04 gadial

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

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

qiskit-bot avatar Apr 03 '25 09:04 qiskit-bot

Hi @gadial, this is still in draft mode so I just gave it a brief look. I have a few general questions and comments at this stage:

  1. With the way the code is written now it's geared towards being called from Python, e.g. with the file header written in interface.py, the Python calls in py_write_circuit, pack_circuit_header and more. Maybe it's just an artifact of this PR still being WIP, but with the C-API for circuit construction (this https://github.com/Qiskit/qiskit/pull/14006 and follow-ons) we should consider a C-api equivalent for the dump workflow or some aspects of it.
  2. For the circuit header handling in pack_circuit_header maybe QuantumCircuitData in converters or a similar approach might be useful here instead of all the getattr calls.
  3. Generally speaking about 1 & 2: assuming we'll still need some Python calls, can we make the Rust-logic as Python-free as possible, say by having a layer to extract as much info as possible from the Python objects before passing it to the actual pack functions? I realize this might not be that straightforward, but still worth trying. Eventually we should have enough of the data model Python-free so it makes sense to design for that already.
  4. Is use_rust in dump just a WIP thing for debug or do you plan to keep Python's write_circuit as an option? If so why? Otherwise it would be good to have the formats defined only in one place. Currently formats.rs mirrors formats.py which makes sense for the WIP status but the duplication is not good for the longer term. Just mentioning.

eliarbel avatar May 04 '25 14:05 eliarbel

Thanks @eliarbel

  1. That's mostly at artifact of originally planning a smaller scope for this PR (in order to proceed incrementally instead of one huge PR) - I'm still trying to aim for the MVP, although this can be changed if needed since the header is a relatively easy part.
  2. Thanks, I'll try looking into it once the PR is passing.
  3. It's a little tricky since we deal with many different data types, each with its own Pythonic quirks, and some (e.g. parameters) are already transitioning to Rust. My priorities in this PR is to flush out the QPY structure as much as possible (i.e. by using detailed structs) and keeping the actual data extraction in specific serialize_X functions which can be amended on demand as we move more logic from Python to Rust. But I thing adding new extractors is out of scope for this PR which aims to establish the baseline.
  4. That's most definitely a WIP thing - I use it to test both Rust and Python generate the same output. At the end I believe we should remove all the Python code related to QPY dumping, but we'll have to keep the formats until we handle reading as well.

Hi @gadial, this is still in draft mode so I just gave it a brief look. I have a few general questions and comments at this stage:

  1. With the way the code is written now it's geared towards being called from Python, e.g. with the file header written in interface.py, the Python calls in py_write_circuit, pack_circuit_header and more. Maybe it's just an artifact of this PR still being WIP, but with the C-API for circuit construction (this Add initial C API for circuit construction #14006 and follow-ons) we should consider a C-api equivalent for the dump workflow or some aspects of it.
  2. For the circuit header handling in pack_circuit_header maybe QuantumCircuitData in converters or a similar approach might be useful here instead of all the getattr calls.
  3. Generally speaking about 1 & 2: assuming we'll still need some Python calls, can we make the Rust-logic as Python-free as possible, say by having a layer to extract as much info as possible from the Python objects before passing it to the actual pack functions? I realize this might not be that straightforward, but still worth trying. Eventually we should have enough of the data model Python-free so it makes sense to design for that already.
  4. Is use_rust in dump just a WIP thing for debug or do you plan to keep Python's write_circuit as an option? If so why? Otherwise it would be good to have the formats defined only in one place. Currently formats.rs mirrors formats.py which makes sense for the WIP status but the duplication is not good for the longer term. Just mentioning.

gadial avatar May 04 '25 15:05 gadial

Pull Request Test Coverage Report for Build 20428701070

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.

Details

  • 1282 of 3886 (32.99%) changed or added relevant lines in 26 files are covered.
  • 1009 unchanged lines in 26 files lost coverage.
  • Overall coverage decreased (-2.0%) to 86.349%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/circuit_data.rs 11 14 78.57%
crates/circuit/src/classical/expr/binary.rs 0 3 0.0%
crates/circuit/src/classical/expr/unary.rs 0 3 0.0%
crates/circuit/src/bit.rs 9 16 56.25%
crates/circuit/src/operations.rs 76 93 81.72%
crates/qpy/src/formats.rs 68 85 80.0%
crates/circuit/src/parameter/parameter_expression.rs 5 27 18.52%
crates/qpy/src/consts.rs 25 55 45.45%
crates/qpy/src/annotations.rs 29 106 27.36%
crates/qpy/src/bytes.rs 54 183 29.51%
<!-- Total: 1282 3886
Files with Coverage Reduction New Missed Lines %
crates/cext/src/transpiler/passes/unitary_synthesis.rs 1 89.08%
qiskit/qasm2/export.py 1 98.65%
crates/qasm2/src/lex.rs 2 92.54%
crates/circuit/src/converters.rs 3 96.1%
crates/circuit/src/parameter/parameter_expression.rs 3 80.98%
crates/transpiler/src/transpiler.rs 3 93.39%
crates/qasm2/src/parse.rs 6 97.56%
crates/circuit/src/blocks.rs 8 68.62%
qiskit/providers/basic_provider/basic_simulator.py 10 96.77%
qiskit/qpy/binary_io/value.py 10 72.82%
<!-- Total: 1009
Totals Coverage Status
Change from base Build 20130816757: -2.0%
Covered Lines: 97908
Relevant Lines: 113386

💛 - Coveralls

coveralls avatar May 11 '25 08:05 coveralls

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

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

qiskit-bot avatar May 11 '25 13:05 qiskit-bot

Hey @gadial, thanks for doing all of this.

This isn't a complete review by any means, but I just wanted to give you some comments to work through ahead of your holiday. I'll continue reviewing this coming week.

The general questions I have so far are:

  1. Are there any changes to the current QPY format to support any of this, or is it compatible without introducing a new version?
  2. This PR is quite a bit larger than I was expecting. Are there any more meaningful chunks that you might be able to split this into to make reviewing it easier to understand architecturally? I think when we spoke about it before, you mentioned that testing was easier if both reader and writer were in place. But that leads me to the third point:
  3. Should we have Rust-level unit testing for any of this? Or, is there some amount of stress testing we could do, i.e. generating complex circuits with both the old and new serializers and making sure the outputs are equivalent? I have some doubt that our existing QPY test coverage is sufficient to detect potential differences introduced here (but perhaps I am wrong). How confident are you that the new code is sufficiently exercised by what we've got?
  1. No changes.
  2. There are three conceptual parts for the PR: First is the formats file which defines the structure of the qpy file as much as possible (there are still some Bytes fields e.g. for storing multiple possible values types; this may be further refined in a future PR). Next are the writer and the reader parts. The writer takes the QuantumCircuit object and generates a struct; the reader takes a struct and generates a QuantumCircuit. The reader and the writer obviously depend on the formats, but not on each other. However, not adding all three together would result in some dead code in the PR, which is not preferred.
  3. In the long run we definitely should have Rust-level testing (which if I'm not mistaken is currently missing from most of our Rust code?). I did some stress testing during development (which indeed encountered some problems which were fixed) and will also do it as part of the benchmarking, but I have relatively good confidence in the current tests, due to the large amounts of problems they managed to detect in relatively niche situations.

gadial avatar Aug 02 '25 18:08 gadial

Thanks @gadial!

Just want to note here that I will likely need to come back to this after getting control flow sorted ahead of the Sept. deadline in lieu of recent requirement changes there. Perhaps someone else can pick up where I'd left off here and I can have another supplemental look in a few weeks from now.

kevinhartman avatar Sep 11 '25 15:09 kevinhartman

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

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

qiskit-bot avatar Nov 18 '25 17:11 qiskit-bot

Deferring to 2.4, as discussed offline. We should target merging it early in the 2.4 cycle though to avoid Gadi needing to update this more than necessary and to give enough testing time 🙂

Cryoris avatar Dec 09 '25 17:12 Cryoris