Implemented writing and reading of instruction duration and unit in qpy #10751

Abhiraj-Shrotriya · 2023-08-31T14:22:43Z

Summary

Implemented a way of writing and reading duration and unit data of instructions in qpy.
closes #10662

Details and comments

Backward compatible

qiskit-bot · 2023-08-31T14:22:46Z

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 the following people are requested to review this:

@Qiskit/terra-core
@mtreinish
@nkanazawa1989

coveralls · 2023-08-31T15:05:35Z

Pull Request Test Coverage Report for Build 6038628513

25 of 26 (96.15%) changed or added relevant lines in 4 files are covered.
25 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.02%) to 87.273%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/qpy/binary_io/circuits.py	17	18	94.44%

Files with Coverage Reduction	New Missed Lines	%
qiskit/qpy/binary_io/circuits.py	1	92.93%
crates/qasm2/src/lex.rs	6	90.63%
crates/qasm2/src/parse.rs	18	96.67%

Totals
Change from base Build 6037904724:	-0.02%
Covered Lines:	74407
Relevant Lines:	85258

💛 - Coveralls

…d794f3dbd07e90.yaml

…skit-terra into fixissue10662branch

…d794f3dbd07e90.yaml

Abhiraj-Shrotriya · 2023-09-01T15:38:32Z

@mtreinish @jakelishman
Hi,
I am having problems making this qpy version backwards compatible since the qpy_version is now increased to Integer 10 (2-digit) up from Integer 9 ( 1-digit).

The qpy FILE_HEADER_PACK needs to be changed from "!6sBBBBQ"(Allowing only 1-digit int) to "!6sHBBBQ"(Allowing 2-digit int, hence allowing to have qpy_version values upto integer 99).

Changing this would break the qpy files created in past versions because then it will have a different structure.

Suggestion-

Create a FILE_HEADER_V2?

But then still the problem will persist.

Please help.

Thank you

jakelishman · 2023-09-01T16:35:28Z

There's no need to encode an integer as its ASCII character representation - the current one-byte integer encoding of the QISKIT_VERSION can hold versions up to 256 without issue.

jakelishman · 2023-09-01T16:37:31Z

I haven't looked through the PR completely, but on a very quick scan, it looks like you're converting a lot of things to strings that shouldn't be strings; QPY is a binary format, and we should prefer to use denser type-code / numeric encodings wherever possible. For the most part, only things that were already strings should be written as strings, and not even all of those - for example, the unit of Instruction is actually enum-like, and we could encode the possible values in a single-byte type code.

Abhiraj-Shrotriya · 2023-09-01T17:21:02Z

@jakelishman Thank you so much for clearing my confusion. Also can you please elaborate a little on the idea concerning unit of instruction .

…skit-terra into fixissue10662branch

…d794f3dbd07e90.yaml

Abhiraj-Shrotriya · 2023-09-07T13:55:25Z

@mtreinish I think this pull request is now ready for a review. Thank you

mtreinish

Overall this looks good, this is the basic framework of what's needed thanks for doing this. I left a couple inline comments on some improvements we can make to the actual struct format used to encode the duration and unit so that it's a bit more compact.

Besides those comments the only thing missing are tests and format documentation. It'd be good to have tests for the use of unit and duration in https://github.com/Qiskit/qiskit/blob/main/test/python/qpy/test_circuit_load_from_qpy.py to verify we can round trip serialize and deserialize these circuits (to ensure we don't regress). It'll also be good to add backwards compatibility tests to https://github.com/Qiskit/qiskit/blob/main/test/qpy_compat/test_qpy.py to ensure that moving forward we're able to load qpy files from old versions of qiskit that have unit and duration set.

Besides that the format documentation it would just be adding something to: https://github.com/Qiskit/qiskit/blob/main/qiskit/qpy/__init__.py#L148 to describe the new struct format and how it's read.

mtreinish · 2023-10-11T18:38:46Z

qiskit/qpy/binary_io/circuits.py

+        if duration == 0:
+            duration = None
+        # Load Unit
+        unit = file_obj.read(instruction.unit_size).decode(common.ENCODE)


Instead of using a string like this we can probably get by with a char. The only supported units can all be represented by single characters:

"f": -15, "p": -12, "n": -9, "u": -6, "µ": -6, "m": -3, "k": 3, "M": 6, "G": 9, "T": 12, "P": 15,

Then we can just assign a special character for None probably just use bytes(0) and d for "dt". This will save us some space as we can tightly pack the unit and also it will be faster.

@mtreinish Can you please elaborate on why the above map that you have given contains the integers as values since you are saying that these units can be stored as characters. Correct me if I am wrong but did you want to store the integer ( making the qpy file numerically dense) and read these numbers to retrieve character units? If yes then the struct pack should not have a 'c' at the end.

Thank you

Sorry for the delay this fell through the cracks at the end of last year and I missed replying here until now. What I meant is that right now you've got an arbitrary width encoding for the unit field. But instead of doing this you can do a fixed single character encoding of the unit in the actual QPY payload. So you would replace unit_size and the string decoding by putting a single character at the end of the struct packing. This will be the most efficient representation because it's just a single byte and then it's just about mapping that to the units that go in the instruction object's unit field.

mtreinish · 2023-10-11T18:39:24Z

qiskit/qpy/binary_io/circuits.py

+    else:
+        pass


This is unnecessary as it's implicit if you don't have an else statement:

Suggested change

else:

pass

mtreinish · 2023-10-11T18:40:27Z

qiskit/qpy/formats.py

+        "unit_size",
+    ],
+)
+CIRCUIT_INSTRUCTION_V3_PACK = "!HHHIIBHqIIII"


Isn't duration normally a float, especially if it has an SI unit? If so I would make this:

Suggested change

CIRCUIT_INSTRUCTION_V3_PACK = "!HHHIIBHqIIII"

CIRCUIT_INSTRUCTION_V3_PACK = "!HHHIIBHqIIdI"

to have duration be a double.

mtreinish · 2023-10-11T18:41:23Z

qiskit/qpy/formats.py

+        "unit_size",
+    ],
+)
+CIRCUIT_INSTRUCTION_V3_PACK = "!HHHIIBHqIIII"


Per my comment in circuits.py I'd probably change this to:

Suggested change

"unit_size",

],

)

CIRCUIT_INSTRUCTION_V3_PACK = "!HHHIIBHqIIII"

"unit",

],

)

CIRCUIT_INSTRUCTION_V3_PACK = "!HHHIIBHqIIIc"

and then handle the static mapping from single characters to value in the reader and writer.

vtomole · 2023-11-02T19:34:18Z

Thanks for working on this @Abhiraj-Shrotriya. Are you going to respond to the requested changes soon or is it okay if I take over this PR?

Abhiraj-Shrotriya · 2023-11-02T20:11:29Z

Thanks for working on this @Abhiraj-Shrotriya. Are you going to respond to the requested changes soon or is it okay if I take over this PR?

@vtomole
Hello,

Yes do intend to fix this issue. Thank you for your interest in helping. If I do however require help, I will contact you here :)

I intend to fix this soon.

Thank you again

vtomole · 2024-02-23T15:59:15Z

Hey @Abhiraj-Shrotriya , how's this going?

Abhiraj-Shrotriya · 2024-02-24T00:49:31Z

@vtomole I asked a question to @mtreinish but did not get a reply in time. So I was not able to know what is expected of the code. That is why this PR is delayed. I am not blaming anyone since it is no one's fault.

If you know what is required of the code for this PR, please feel free to take over it.

I am sorry for the inconvenience and delay.

vtomole · 2024-02-26T23:59:25Z

@Abhiraj-Shrotriya Ah, i see. No worries then! Let's wait for @mtreinish. Thanks.

Abhiraj-Shrotriya · 2024-03-04T00:52:36Z

@vtomole as I said earlier, if you know what is needed of this PR, please go ahead. @mtreinish maybe busy and is not getting time to respond to the queries raised, which is fine. It is of course not a good thing to keep this PR delayed for long. It would be best to resolve it as soon as possible no matter who does it. That is the spirit of open source development and I would like to to keep that spirit alive.

I am quite confident that you will be able to fix it :)

If, however, you are busy but do understand the work that needs to be done here, you can contact me with the guidance and I will work on it.

I would recommend you to just check the latest comments on my code as I think that will help you to have a clear understanding of my situation here.

Looking forward to progress :)

Thank you

implemented writing and reading of instruction duration and unit in qpy

cd03310

Abhiraj-Shrotriya requested a review from a team as a code owner August 31, 2023 14:22

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 31, 2023

Abhiraj-Shrotriya changed the title ~~implemented writing and reading of instruction duration and unit in qpy~~ Implemented writing and reading of instruction duration and unit in qpy Aug 31, 2023

Abhiraj-Shrotriya added 2 commits August 31, 2023 21:07

Merge branch 'main' into fixissue10662branch

2c7f540

Update add-writing-reading-of-instruction-unit-and-duration-in-qpy-0c…

5fc3e9e

…d794f3dbd07e90.yaml

mtreinish self-assigned this Aug 31, 2023

mtreinish added Changelog: New Feature Include in the "Added" section of the changelog Changelog: API Change Include in the "Changed" section of the changelog mod: qpy Related to QPY serialization labels Aug 31, 2023

mtreinish added this to the 0.45.0 milestone Aug 31, 2023

Abhiraj-Shrotriya and others added 6 commits September 1, 2023 13:32

Refined code for reading/loading of duration in qpy

e11d9da

Merge branch 'main' of github.com:Qiskit/qiskit into fixissue10662branch

e54e027

Merge branch 'fixissue10662branch' of github.com:Abhiraj-Shrotriya/qi…

3149d05

…skit-terra into fixissue10662branch

Fixed cyclic import and unsupported type problem

62988fa

Fixed backward compatiblity

c5e5cf7

Update add-writing-reading-of-instruction-unit-and-duration-in-qpy-0c…

209677f

…d794f3dbd07e90.yaml

Abhiraj-Shrotriya and others added 7 commits September 4, 2023 04:02

Merge branch 'main' into fixissue10662branch

595d756

Merge branch 'main' into fixissue10662branch

f4c321f

Implemented suggested changes

6ba80b5

Merge branch 'main' of github.com:Qiskit/qiskit into fixissue10662branch

5cd1742

Merge branch 'fixissue10662branch' of github.com:Abhiraj-Shrotriya/qi…

5ca752b

…skit-terra into fixissue10662branch

Refined unit of instruction reading

d9565c0

Update add-writing-reading-of-instruction-unit-and-duration-in-qpy-0c…

dd88dd7

…d794f3dbd07e90.yaml

mtreinish reviewed Oct 11, 2023

View reviewed changes

mtreinish modified the milestones: 0.45.0, 1.0.0 Oct 18, 2023

mtreinish modified the milestones: 1.0.0, 1.1.0 Jan 23, 2024

	CIRCUIT_INSTRUCTION_V3_PACK = "!HHHIIBHqIIII"
	CIRCUIT_INSTRUCTION_V3_PACK = "!HHHIIBHqIIdI"

Implemented writing and reading of instruction duration and unit in qpy #10751

Are you sure you want to change the base?

Implemented writing and reading of instruction duration and unit in qpy #10751

Uh oh!

Conversation

Abhiraj-Shrotriya commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Aug 31, 2023

Uh oh!

coveralls commented Aug 31, 2023

Pull Request Test Coverage Report for Build 6038628513

💛 - Coveralls

Uh oh!

Abhiraj-Shrotriya commented Sep 1, 2023

Suggestion-

Uh oh!

jakelishman commented Sep 1, 2023

Uh oh!

jakelishman commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Abhiraj-Shrotriya commented Sep 1, 2023

Uh oh!

Abhiraj-Shrotriya commented Sep 7, 2023

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

mtreinish Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

Abhiraj-Shrotriya Dec 3, 2023

Choose a reason for hiding this comment

Uh oh!

mtreinish Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

mtreinish Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

mtreinish Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

mtreinish Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

vtomole commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Abhiraj-Shrotriya commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vtomole commented Feb 23, 2024

Uh oh!

Abhiraj-Shrotriya commented Feb 24, 2024

Uh oh!

vtomole commented Feb 26, 2024

Uh oh!

Abhiraj-Shrotriya commented Mar 4, 2024

Uh oh!

Uh oh!

Abhiraj-Shrotriya commented Aug 31, 2023 •

edited

Loading

jakelishman commented Sep 1, 2023 •

edited

Loading

vtomole commented Nov 2, 2023 •

edited

Loading

Abhiraj-Shrotriya commented Nov 2, 2023 •

edited

Loading