Skip to content

Remove pulse support in QPY in 2.0#13814

Merged
mtreinish merged 20 commits intoQiskit:mainfrom
eliarbel:remove-pulse-qpy
Feb 26, 2025
Merged

Remove pulse support in QPY in 2.0#13814
mtreinish merged 20 commits intoQiskit:mainfrom
eliarbel:remove-pulse-qpy

Conversation

@eliarbel
Copy link
Copy Markdown
Member

@eliarbel eliarbel commented Feb 9, 2025

Summary

This PR removes support for saving ScheduleBlock programs in qpy.dump. It also includes support for loading payloads with ScheduleBlocks or pulse gates while ignoring the actual pulse information, returning partially specified circuits.

Part of #13662

Details and comments

Some details to note and give feedback on:

  1. Loading a ScheduleBlock program via qpy.load results with a QuantumCircuit object containing only the name and possibly metadata of that ScheduleBlock. Loading a QuantumCircuit payload with pulse gates leads to getting circuit object with undefined custom instructions (representing pulse gates without calibrations). In both cases warnings of type QPYLoadingDeprecatedFeatureWarning are being issued. Is this the type of warning we want to use here? Maybe we should add something like QPYLoadingRemovedFeatureWarning and update the Compatibility section in the QPY doc page accordingly?
  2. This PR does not change the QPY format version, i.e. it stays on 13. This means that we still write CALIBRATION header (with 0 cals) so that load continues to work (since it still includes code for reading calibrations, for graceful backward compatibility).
  3. When loading payloads with calibrations, the code will issue a message for each calibration, warning the user that if there exists a gate with this name, it will be undefined. Theoretically, one can add calibrations to a circuit without any pulse gates. We could check this and have the warnings accurately relate to existing pulse gates, but it think it'll just impose extra runtime overhead. I'm open to other opinions though.

Open Tasks:

  • Complete handling of test/qpy_compat

@eliarbel eliarbel added Changelog: Removed Add a "Removed" entry in the GitHub Release changelog. mod: qpy Related to QPY serialization labels Feb 9, 2025
@eliarbel eliarbel added this to the 2.0.0 milestone Feb 9, 2025
@coveralls
Copy link
Copy Markdown

coveralls commented Feb 9, 2025

Pull Request Test Coverage Report for Build 13522069571

Details

  • 19 of 61 (31.15%) changed or added relevant lines in 4 files are covered.
  • 96 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.1%) to 87.831%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/qpy/interface.py 5 7 71.43%
qiskit/qpy/binary_io/circuits.py 3 10 30.0%
qiskit/qpy/binary_io/schedules.py 9 42 21.43%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.23%
qiskit/pulse/builder.py 1 85.31%
qiskit/pulse/instructions/reference.py 1 90.32%
qiskit/qpy/binary_io/circuits.py 2 88.79%
qiskit/qpy/interface.py 2 86.15%
qiskit/qpy/type_keys.py 4 82.16%
crates/qasm2/src/lex.rs 5 92.48%
qiskit/qpy/binary_io/value.py 5 86.26%
qiskit/pulse/library/symbolic_pulses.py 8 93.01%
crates/qasm2/src/parse.rs 18 96.68%
Totals Coverage Status
Change from base Build 13512719904: -0.1%
Covered Lines: 77084
Relevant Lines: 87764

💛 - Coveralls

@1ucian0 1ucian0 changed the title Remove pulse support in QPY 2.0 Remove pulse support in QPY in 2.0 Feb 10, 2025
@eliarbel eliarbel marked this pull request as ready for review February 11, 2025 12:19
@eliarbel eliarbel requested a review from a team as a code owner February 11, 2025 12:19
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

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

Copy link
Copy Markdown
Contributor

@raynelfss raynelfss left a comment

Choose a reason for hiding this comment

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

Just a typo I found :)

Comment thread qiskit/qpy/binary_io/circuits.py Outdated
Comment thread qiskit/qpy/type_keys.py
eliarbel and others added 2 commits February 12, 2025 10:48
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
@eliarbel eliarbel marked this pull request as draft February 13, 2025 17:32
@eliarbel eliarbel marked this pull request as ready for review February 16, 2025 13:43
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

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

@eliarbel eliarbel added the mod: pulse Related to the Pulse module label Feb 17, 2025
@mtreinish mtreinish self-assigned this Feb 18, 2025
@eliarbel eliarbel marked this pull request as draft February 18, 2025 15:12
@eliarbel eliarbel marked this pull request as ready for review February 19, 2025 14:34
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

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

Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Overall this looks good. I left a few comments and suggestions inline, but nothing major.

I was confused at first by the changes in the binary_io/schedules.py flle because I skipped over the module docs. But it's clear now that it's basically a bunch of seek() methods now. It's obvious now they're needed because the size to read for the pulse parts isn't known ahead of time, so this is needed to skip the pulse portion of a pulse gates circuit payload.

Comment thread qiskit/qpy/__init__.py
Comment thread qiskit/qpy/__init__.py Outdated
Comment thread qiskit/qpy/binary_io/circuits.py Outdated
Comment thread qiskit/qpy/binary_io/schedules.py Outdated
Comment thread qiskit/qpy/binary_io/schedules.py Outdated
Comment thread releasenotes/notes/remove-pulse-qpy-07a96673c8f10e38.yaml Outdated
Comment thread test/qpy_compat/test_qpy.py Outdated
Comment thread test/qpy_compat/test_qpy.py Outdated
@eliarbel
Copy link
Copy Markdown
Member Author

Thanks for the review @mtreinish, all comments have been addressed.

Comment thread test/qpy_compat/test_qpy.py Outdated
Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, I had two small inline comments but nothing worth blocking over. Mostly my idle musings and one extra space in a docstring. Thanks for doing this and the quick updates.

Comment thread test/qpy_compat/test_qpy.py
Comment thread qiskit/qpy/__init__.py
Comment thread test/qpy_compat/test_qpy.py
@mtreinish mtreinish added this pull request to the merge queue Feb 25, 2025
Merged via the queue into Qiskit:main with commit f1729db Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Removed Add a "Removed" entry in the GitHub Release changelog. mod: pulse Related to the Pulse module mod: qpy Related to QPY serialization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants