-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add QPY support for switch
statement
#9926
Conversation
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:
|
Pull Request Test Coverage Report for Build 4704152491
💛 - Coveralls |
QPY had a somewhat implicit assumption that most classes can be reconstructed by doing `type(obj)(*obj.params)`, which `SwitchCaseOp` very much does not abide by - to do so would compromise more core parts of how it is meant to function. This commit puts in a special case so that instances of `SwitchCaseOp` present their "params" as the information necessary to reconstruct the op in the above form, rather than actually using the parameters. That didn't feel _entirely_ satisfying, but does work fine.
Squashed versions of Qiskitgh-9916, Qiskitgh-9919 and Qiskitgh-9926.
@@ -93,6 +101,8 @@ class Value(TypeKeyBase): | |||
INTEGER = b"i" | |||
FLOAT = b"f" | |||
COMPLEX = b"c" | |||
CASE_DEFAULT = b"d" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Value
group originally defines the keys for Python built-in classes + parameter which both circuit and pulse module may use. If operands for control flow are expected to evolve further, probably you can define new group to avoid future key conflict with other programs. Pulse module already has own:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move it if you think there's something more sensible for me to put it - I wasn't sure. It's unlikely that control flow itself will gain more keys that need adding here, but we will need an entire new subtree to handle extended classical expressions (Qiskit/RFCs#30) at some point in the future (that's completely separate to this). It's likely they'll be able to have an entirely separate namespace, though.
I'm not sure what advantage there is to having these separated enums; there's cross-dependencies between them already, so it's insufficient to just make a key unique within a single enum. In the general case, the keys have to be globally unique. I found that when trying to use an r
key in Value
: it clashed with an r
for the Python builtin range
in Container
. I feel like it might be cleaner to unify these into a global enumeration and enforce complete uniqueness; once we've got the enum, there's no real need for the ASCII characters to have any mnemonic value, and since the allowed keys in any given place in the QPY format are often a mixture of a few enums, they're not offering much type safety at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for separate enum is to make the context clearer, because pulse programs consist of many objects (alignment type, instruction type, waveform, value, channel type, etc...) and they have sort of hierarchy in its representation. It was convenient to have separate enum for them. But I also agree this structure makes management of key conflict harder. We may need a global enum, or we should use namespaced key (at lest two character key). I want to overhaul QPY module at some point, but I don't have really good motivation for now. Since protected QPY serializers are called unexpectedly in other Qiskit packages, its interface must be updated as wel.
Anyways, if you think control flow operand doesn't grow, I think current key assignment is okey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair enough, yeah. I'm not really familiar with the QPY code at all, so I'm happy to put it somewhere else if you'd prefer.
We currently don't have any plans to extend the control-flow handling, though I suppose at some very indeterminate time in the future there might be the concept of a relocatable subroutine call. That's probably a year+ away at least, though, so I wouldn't worry about it.
I already have #9890 which may conflict. Do you want to merge this first? |
I'm happy to merge PRs whichever way round - if yours merges first, I'll fix the merge conflicts here no trouble. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jake. I reviewed the PR and the proposed change looks good to me.
qiskit/qpy/common.py
Outdated
@@ -20,7 +20,7 @@ | |||
|
|||
from qiskit.qpy import formats | |||
|
|||
QPY_VERSION = 6 | |||
QPY_VERSION = 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to bump QPY version since you didn't break any data structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels odd not to, to me - since I defined some type keys that weren't in previous versions, we'd be breaking that property that all things written to the original version 6 spec would be able to load these new programs. That said, the point's moot since your PR absolutely needed a version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is any possibility of a bug being introduced, even if it's not obvious, a version bump would help consumers track it down.
qiskit/qpy/__init__.py
Outdated
Version 7 | ||
========= | ||
|
||
Version 7 is identical to version 6, except two new type keys are added to the INSTRUCTION_PARAM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block can go to
https://github.com/Qiskit/qiskit-terra/blob/912ba0783420463b4adf7f53bee7cba04bd915f8/qiskit/qpy/__init__.py#L905-L919
maybe we need better document structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My problem was that that block is under the "version 1" heading, and the new type keys aren't defined in version 1, so it didn't seem right to put it there. For example, in version 4, we add two type keys to the same INSTRUCTION_PARAMS
struct, but don't put them in version 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we need to rework the documentation. At the time it was trying to build off what each version's structs looked like so you'd incrementally build the formats off each other. I think we probably need to reorganize it so the current format is at the top and then we describe (with full examples what things looked like in older versions). I'll open an issue about this so we can track it as something in a follow up. For this we can probably leave the new addition in just the version 7 section and fix this in that followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this LGTM, it's a pretty straightforward addition to handle the extra edge cases around the switch statement. But I'd like to get a final ack from @nkanazawa1989 before enqueuing for merge to ensure his concerns are addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. My comment above just followed precedent and I also tend to prefer bumping version with new data keys. Reorganizing documentation and adding some policy for version control would be really nice.
QPY had a somewhat implicit assumption that most classes can be reconstructed by doing `type(obj)(*obj.params)`, which `SwitchCaseOp` very much does not abide by - to do so would compromise more core parts of how it is meant to function. This commit puts in a special case so that instances of `SwitchCaseOp` present their "params" as the information necessary to reconstruct the op in the above form, rather than actually using the parameters. That didn't feel _entirely_ satisfying, but does work fine.
Summary
QPY had a somewhat implicit assumption that most classes can be reconstructed by doing
type(obj)(*obj.params)
, whichSwitchCaseOp
very much does not abide by - to do so would compromise more core parts of how it is meant to function. This commit puts in a special case so that instances ofSwitchCaseOp
present their "params" as the information necessary to reconstruct the op in the above form, rather than actually using the parameters. That didn't feel entirely satisfying, but does work fine.Details and comments