-
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
Fully QPY compatible pulse builder for V2 and runtime backend #8949
Fully QPY compatible pulse builder for V2 and runtime backend #8949
Conversation
…tly converted into ScheduleBlock with AreaBarrier instructions.
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:
|
0f02d6a
to
6a90761
Compare
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 a lot. Overall, this looks fine. I have some questions that need clarification. See the comments.
block = ScheduleBlock() | ||
block.append(AreaBarrier(120, DriveChannel(0))) | ||
block.append(Play(Constant(10, 0.1), DriveChannel(0))) |
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 feels like we are mixing representations. How does this work with parametric duration? What about
block = ScheduleBlock()
block.append(AreaBarrier(120, DriveChannel(0)))
block.append(Play(Constant(Parameter("duration"), 0.1), DriveChannel(0)))
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.
As I type hinted, AreaBarrier
doesn't accept parameter. This is internally used to convert schedule into block, in which schedule doesn't take parameterized duration.
releasenotes/notes/upgrade-pulse-builder-and-rzx-builder-033ac8ad8ad2a192.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/upgrade-pulse-builder-and-rzx-builder-033ac8ad8ad2a192.yaml
Show resolved
Hide resolved
Co-authored-by: Daniel J. Egger <[email protected]>
Pull Request Test Coverage Report for Build 3528859271
💛 - Coveralls |
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.
A few more comments and a suggestion for the directive name.
"""Pulse ``AreaBarrier`` directive. | ||
|
||
This instruction is intended to be used internally within the pulse builder, | ||
to naively convert :class:`.Schedule` into :class:`.ScheduleBlock`. |
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 this is used for a naive
conversion then what would a non-naive conversion look like?
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.
In the current mechanism I "give up" to infer the original alignment context. For example,
with align_sequential():
with align_left():
play(cr45p, ControlChannel(1))
play(cr45p_d0, DriveChannel(0))
play(x, DriveChannel(1))
with align_left():
play(cr45m, ControlChannel(1))
play(cr45m_d0, DriveChannel(0))
play(x, DriveChannel(1))
this would be the context of the ECR schedule, but current mechanism will generate
with align_left():
play(cr45p, ControlChannel(1))
play(cr45p_d0, DriveChannel(0))
time_block(xxx, DriveChannel(1))
play(x, DriveChannel(1))
...
Note that we cannot modify any pulse duration (otherwise you must also modify time block duration), but this should be okey because we are consuming "already scheduled" sequence. To recover original alignment context, we may need to generate DAG to find parallel context. Such logic may become heavy and still has a rick of some edge case. So I just avoid doing over engineering and choose to say "this is naive version of alignment context recovery".
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.
Added bit more detail in 7ab2710
Co-authored-by: Daniel J. Egger <[email protected]>
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.
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
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 I just had a couple of small questions inline
@@ -731,17 +737,6 @@ def append_instruction(self, instruction: instructions.Instruction): | |||
""" | |||
self._context_stack[-1].append(instruction) | |||
|
|||
@_compile_lazy_circuit_before | |||
def append_block(self, context_block: ScheduleBlock): |
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.
Are there any backwards compatibility implications with this removal. The documentation referred to this method previously so was it user facing or is this all internal?
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 should be okey because builder itself is a protected class.
https://github.com/Qiskit/qiskit-terra/blob/8bfb058b9d94353936d7e2aea61015095487b873/qiskit/pulse/builder.py#L520-L521
releasenotes/notes/upgrade-pulse-builder-and-rzx-builder-033ac8ad8ad2a192.yaml
Outdated
Show resolved
Hide resolved
- render class docs of the directive instructions - update qpy format documentation - add separate reno for time blockade
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 I'm happy with the state of the code now, thanks for making the updates. I just have a few small inline comments. The only one I think is really required is adding a unit test for RZXCalibration* with QPY, other than that I'm happy to approve this.
releasenotes/notes/upgrade-pulse-builder-and-rzx-builder-033ac8ad8ad2a192.yaml
Outdated
Show resolved
Hide resolved
@@ -187,6 +194,20 @@ def test_nested_blocks(self): | |||
builder.delay(200, DriveChannel(1)) | |||
self.assert_roundtrip_equal(test_sched) | |||
|
|||
def test_called_schedule(self): |
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.
One thing I think would be good is to maybe have a qpy test with the rzx calibration passes. Since a big part of this PR is fixing the QPY support with the pass output having a test to verify the pass output can be round tripped through qpy is simple way to ensure we don't regress on this moving forward.
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 Matthew. I added new tests for QPY. I also refactored the RZX builder test in 8562d98 since the output schedule has been never really tested. This is why the alignment error has been existed before this PR (this PR unintentionally fixed it).
…es more robust to the rounding error.
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 for the updates it lgtm now.
…#8949) * Eliminate use of Schedule in the builder context. Schedule is implicitly converted into ScheduleBlock with AreaBarrier instructions. * Support AreaBarrier instruction in QPY * Update RZX calibration builder to generate ScheduleBlock * Add release note * Support python3.7 * Fix hadamard schedule in the RZX builder. This should not use sequential context. * Review comments Co-authored-by: Daniel J. Egger <[email protected]> * Unified append_block and append_subroutine * Update instruction name AreaBarrier -> TimeBlockade * Documentation updates Co-authored-by: Daniel J. Egger <[email protected]> * Remove old name * lint fix * add release note for bugfix * review comments - render class docs of the directive instructions - update qpy format documentation - add separate reno for time blockade * more detailed upgrade notes * add new unittests. rescale_cr_inst method is updated so that it becomes more robust to the rounding error. Co-authored-by: Daniel J. Egger <[email protected]>
Summary
At present, Qiskit Pulse supports both
Schedule
andScheduleBlock
representation, but QPY only supportsScheduleBlock
representation. This causes an issue in V2 IBM provider and runtime provider, since they assume QPY payload, and the pulse builder still acceptsSchedule
. For example,this execution will fail with V2 instance because
x_gate_sched
is still provided in legacy schedule format (because pulse defaults data is still PulseQobj format in IBM provider, regardless of backend version). Same issue happens in RZX calibration builder passes, which is in high demand for pulse level optimization of application circuits.This PR allows V2 provider and runtime provider to execute such a program, and circuit optimized with RZX calibration builder passes.
Details and comments
This PR consists of several commits.
805a5b2
This commit removes usage of
Schedule
andCall
from the pulse builder. PreviouslySchedule
was wrapped byCall
instruction and added to the schedule block as an instruction. Note thatCall
instruction is not QPY compatible because it internally keepsSchedule
as-is (just a wrapper).With this commit, schedule is internally converted into schedule block representation to guarantee the QPY compatibility. Because schedule block is not aware of absolute time interval of instruction, a helper compiler directive
AreaBarrier
is introduced to support instruction scheduling. This instruction must be removed before execution because this is not necessary supported by the hardware.a02bcb5
This commit update QPY to support
AreaBarrier
instruction. Now builder-generated program is fully QPY compatible.f2bc607
This commit updates the RZX calibration builder passes to use the updated pulse builder to construct stretched pulse schedules. Thus QPY compatibility is guaranteed.
(note)
AreaBarrier
is renamed toTimeBlocked
according to the review comment.Alternative
Alternatively, we could add
Schedule
support to QPY to fix this problem. However, I hesitate this approach because QPY supports indicates we cannot drop this representation from the code stack permanently, due to the QPY backward compatibility. Schedule tends to be legacy, because we cannot support frame properly on top of it (because of its poor time slot representation).Fix #8933