Skip to content
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

Fixes #7078: Change prefix of MemorySlot channel in Qasm Instructions #8166

Merged
merged 15 commits into from
Jul 27, 2022

Conversation

pollyshaw
Copy link
Contributor

@pollyshaw pollyshaw commented Jun 12, 2022

Summary

Fixes #7078.

  • Add new ClassicalIOChannel class as a subclass of pulse.Channel and a superclass of MemorySlot, RegisterSlot and SnapshotChannel.
  • In qiskit.pulse.transforms.pad, do not apply a delay to instances of ClassicalIOChannels.
  • In the constructors of ShiftFrequency, SetFrequency, SetPhase and ShiftPhase in pulse.instructions, enforce the existing type restriction of the channel parameter to PulseChannel by raising a PulseError if the channel is not a PulseChannel.

Details and comments

This fixes a bug whereby MeasureSlots and MemorySlots are indistinguishable after being serialized into Qobj instructions because they both use the same prefix.

Documentation is necessary due to the change in behaviour of pad and the introduction of the new type. The release note remains to be written.

@qiskit-bot
Copy link
Collaborator

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:

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @pollyshaw this seems to be a reasonable suggestion though I have two concerns:

  • We may need deprecation warning before updating the prefix because some quantum backend compiler may depend on it. I'm not sure if there is any reasonable approach for this. Perhaps introducing a new class with prefix="s" and then adding a deprecation warning to the MemorySlot? However, if you schedule a quantum circuit with scheduler, the result will be converted into new class with prefix="s", and your qobj will have memory slot with s*. If your backend (not limited to IBM hardware here) doesn't support new prefix, then your job will fail. Actually, we should raise the warning on the backend compiler (rather than in Qiskit) so that developer there can notice, but I'm not sure if this is technically possible. Any thoughts? @mtreinish
  • I'm not sure how IBM Quantum backend treats a qobj with memory slot (after migration to qasm3 there will be similar data as a def cal). Since the main user of this pulse qobj is IBM hardware backends (IIRC dynamics and aer pulse simulator receive raw schedule object rather than qobj -- @DanPuzzuoli ), above deprecation issue will be no problem if the backends can accept new format. @taalexander @wshanks

@wshanks
Copy link
Contributor

wshanks commented Jun 13, 2022

I can look into the backend implementation to see if there is a problem, but @taalexander might know how to do that more quickly. Has this change been tested on an IBM backend to see if anything breaks?

@pollyshaw
Copy link
Contributor Author

@wshanks No, I haven't run it on an IBM backend - I'm not set up with it. The only tests I've run are:

  • the tests run in make
  • the tests that happen in CI (possibly not a wider set than make)
  • a casual running of the script quoted in the issue, to make sure that the output changed to what was expected.

@nkanazawa1989 Maybe we can lean on the fact that the prefix of MemorySlot clashes with MeasureChannel to conclude that it isn't in widespread usage after serialisation with Qobj? It's not impossible that it would be, though: if a client never used MeasureChannels and used something other than pulse_instruction.get_channel() to deserialize then they could be using the serialization of MemorySlot as it is. As a newcomer I don't know how likely it is that a client would use this library in this way.

If we do have to be very cautious then I would be happy to implement the new class for MemorySlot - should it be called MemorySlot2 for example?

@nkanazawa1989
Copy link
Contributor

I'm not familiar with how backend compiler works, but I guess they (at least IBM one) don't use qiskit qobj disassembler. I think they can distinguish memory slots from measure channels because memory slots only appear in the operands of acquire instruction, i.e. there is no instruction directly taking memory slots. So I think the prefix of memory slots is less important (but maybe the backend parser does text based matching including the prefix). I agree it would be great if we can give different prefix to the memory slots.

@HuangJunye
Copy link
Collaborator

@pollyshaw Thanks for the contribution. Can you please add "fixes #7078" in the description / body of the PR instead of the title so that the issue can be linked with the PR? Thank you.

@taalexander
Copy link
Contributor

There is no serialization for memory slots as a string in the Qobj as they are not channels but rather a poorly specified form of memory. Where/what are these being used for to yield a clash in Qiskit? A delay on a memory slot is semantically not supported and is an abuse of the pulse object model as far as I can tell as described in #7078. If I were to change anything I would change the prefix of the memory slot, not the measurement channel.

@nkanazawa1989
Copy link
Contributor

Thanks @taalexander for confirmation. Then this PR should be good to go with some cleanup.

@@ -464,6 +464,8 @@ def get_channel(self, channel: str) -> channels.PulseChannel:
return channels.MeasureChannel(index)
elif prefix == channels.ControlChannel.prefix:
return channels.ControlChannel(index)
elif prefix == channels.MemorySlot.prefix:
Copy link
Contributor

Choose a reason for hiding this comment

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

As Thomas mentioned, MemorySlot is just an operand of Acquire instruction taking AcquireChannel so this code should be never reached, i.e. memory slot is implicitly scheduled as an acquire instruction. Just curious, are you developing a custom backend model that schedules memory slot as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'm not building custom backend model - I just picked this up because I wanted to get involved!

Just drawing from the issue and writing the following for documentation, as you're very likely to know this, at the moment it can be reached because if you call

from qiskit.pulse import Schedule, Play, Acquire, MemorySlot, AcquireChannel, DriveChannel
from qiskit.pulse.library.parametric_pulses import Constant
from qiskit.pulse.transforms import pad
from qiskit.qobj.converters.pulse_instruction import QobjToInstructionConverter
from qiskit import assemble

sched=Schedule()
sched+=Play(Constant(4, 0.1), DriveChannel(0))
sched+=Acquire(3, AcquireChannel(0), MemorySlot(0))
pad(sched, inplace=False)

the pad() line does this:

   until = until or schedule.duration
    channels = channels or schedule.channels

    for channel in channels:
        if channel not in schedule.channels:
            schedule |= instructions.Delay(until, channel)
            continue

which creates a Delay on the MemorySlot 'channel'.

However, as @wshanks has said in #7078 (comment) this would be fixed if the Aquire.channels() did not return MemorySlots so it sounds like that's what makes sense, and doesn't require a new prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you added branching for memory slots in pad function, this is no longer necessary?

@@ -354,6 +354,17 @@ def test_delay(self):
self.assertEqual(converted_instruction.duration, instruction.duration)
self.assertEqual(converted_instruction.instructions[0][-1], instruction)

def test_delay_memory_slot(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. This test looks bit weird to me, but it should be okey if you have this use case in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - this was just a way of fabricating directly what happens when you call pad() on an Acquire with a MemorySlot. So I will remove this as it isn't an appropriate use of the library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now this test can be converted into test_pad_no_delay_on_memory_slot?

@HuangJunye HuangJunye added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 14, 2022
@pollyshaw pollyshaw changed the title Fixes #7078: Change prefix of MemorySlot channel in Qasm Instructions [WIP] Fixes #7078: Change prefix of MemorySlot channel in Qasm Instructions Jun 19, 2022
@pollyshaw pollyshaw force-pushed the #7078-change-prefix-for-MemorySlot branch 3 times, most recently from cebb269 to e9969a1 Compare June 19, 2022 13:15
@HuangJunye HuangJunye self-assigned this Jun 29, 2022
@pollyshaw pollyshaw force-pushed the #7078-change-prefix-for-MemorySlot branch from c5105fb to dc47600 Compare June 30, 2022 20:54
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks for continuing this PR @pollyshaw ! For now, what this PR will update are

  • Update memory slot prefix from "m" to "s"
  • Add channel type check to instructions, i.e. we don't expect to have delays on classical IO
  • pad function doesn't insert delays on classical IO channels

I think these are reasonable direction. As @taalexander commented here, perhaps we need to allow Delay to take classical IO channels to implement control flow. Since Schedule adopts absolute timing model, assuming no delays on classical IO should be fine. However, in ScheduleBlock, we might want to write in future

with pulse.build() as measure:
    with pulse.align_left():
        pulse.Acquire(1000, AcquireChannel(0))
        pulse.Delay(10, MemorySlot(0))
        pulse.Load(990, MemorySlot(0))  # this doesn't exist now.

to represent data acquisition from acquisition trigger (in current model triggering acquire immediately store the data in memory slots). So allowing delays to take classical IO channel kind of make sense.

Regarding the pad function, because the input is Schedule which is already schedule in absolute fashion, we don't need to explicitly input delays. So the update in this PR makes sense.

So what we should do in this PR would be:

  • Update memory slot prefix from "m" to "s"
  • Add channel type check to instructions except for delay, e.g. Play doesn't take classical IO
  • pad function doesn't insert delays on classical IO channels

@@ -140,6 +140,11 @@ def name(self) -> str:
"""Return the shorthand alias for this channel, which is based on its type and index."""
return f"{self.__class__.prefix}{self._index}"

@property
def is_schedulable(self) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than having this attribute, I would define ClassicalIOChannel class which is a counterpart of PulseChannel, and turn MemorySlot, RegisterSlot, and SnapshotChannel into its subclass. Note that we schedule instructions rather than channels.

"""
super().__init__(operands=(duration, channel), name=name)
if not channel.is_schedulable:
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be written if isinstance(channel, ClassicalIOChannel) by introducing dedicated class. This would make it clearer what is prohibited. It would be great if you could insert this check to other instructions too.

Perhaps, as @taalexander commented, we might need to consider delays on classical IO channels in relative scheduling models, i.e. ScheduleBlock. So we can drop channel check from Delay instruction, and add this check to other instructions such as Play, ShiftPhase, SetPhase, ShiftFrequency, and SetFrequency, which will never take classical IO channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be great if you could insert this check to other instructions too.

I'm not totally sure which other instructions it should apply to. I've gone through the ones in this namespace and have thrown this error for instructions which have channels that aren't already restricted to a subtype of Channel but that has broken some tests e.g. RelativeBarrier has a test which takes a MemorySlot as one of the channels in its constructor.

@@ -475,6 +475,8 @@ def pad(
channels = channels or schedule.channels

for channel in channels:
if not channel.is_schedulable:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I like this mechanism 💯

@@ -464,6 +464,8 @@ def get_channel(self, channel: str) -> channels.PulseChannel:
return channels.MeasureChannel(index)
elif prefix == channels.ControlChannel.prefix:
return channels.ControlChannel(index)
elif prefix == channels.MemorySlot.prefix:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you added branching for memory slots in pad function, this is no longer necessary?

@@ -354,6 +354,17 @@ def test_delay(self):
self.assertEqual(converted_instruction.duration, instruction.duration)
self.assertEqual(converted_instruction.instructions[0][-1], instruction)

def test_delay_memory_slot(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Now this test can be converted into test_pad_no_delay_on_memory_slot?

@pollyshaw
Copy link
Contributor Author

Thanks for your feedback @nkanazawa1989! I will try implementing the ClassicalIOChannel which I agree is better than bolting on another property, especially as is_schedulable is used elsewhere in the library to mean something different, and also may be misnamed.

@pollyshaw pollyshaw force-pushed the #7078-change-prefix-for-MemorySlot branch from bbba545 to 7e04cda Compare July 9, 2022 11:17
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks for continuously working with this PR. This looks almost good to go. Could you please add several updates?

  • Can you add the same channel type check to other instructions too? Specifically, Play, SetFrequency, ShiftFrequency, SetPhase and ShiftPhase.
  • Please write release note to communicate with users? In this PR, ClassicalIOChannel class has been added as a base class of MemorySlot, RegisterSlot, and SnapshotChannel. Channel type validation has been introduced to pulse instructions.

Do you still want to update memory slot prefix? I think updating the prefix should be fine. Then, please write about this update in the release note.

"""
for channel in channels:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think compiler directives can be applied to classical IO as well.

@pollyshaw
Copy link
Contributor Author

pollyshaw commented Jul 13, 2022

@nkanazawa1989

  • Can you add the same channel type check to other instructions too? Specifically, Play, SetFrequency, ShiftFrequency, SetPhase and ShiftPhase.
  • Play already throws an exception if the type is not PulseChannel - so I think this is covered
  • SetFrequency, SetPhase and ShiftPhase specify PulseChannel as a type for the channel constructor parameter, but don't check the type. Should I add a check but check that it's PulseChannel, rather than specifically checking that it's not ClassicalIOChannel?

@pollyshaw pollyshaw force-pushed the #7078-change-prefix-for-MemorySlot branch from c78b9ed to 7e6c647 Compare July 18, 2022 07:50
ControlChannel,
DriveChannel,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this list in alphabetical order.

@pollyshaw pollyshaw changed the title [WIP] Fixes #7078: Change prefix of MemorySlot channel in Qasm Instructions Fixes #7078: Change prefix of MemorySlot channel in Qasm Instructions Jul 18, 2022
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks. This looks good to me now. Could you please fix linter error so that I can approve?

@pollyshaw pollyshaw force-pushed the #7078-change-prefix-for-MemorySlot branch from 21e7d53 to 7abc2ca Compare July 19, 2022 22:09
@pollyshaw
Copy link
Contributor Author

Thanks. This looks good to me now. Could you please fix linter error so that I can approve?

@nkanazawa1989 Have done - hope I get a green build this time.

Just to make sure, I ran the test that was in the issue again to see if sched2 and sched were the same in the following:

from qiskit.pulse import Schedule, Play, Acquire, MemorySlot, AcquireChannel, DriveChannel
from qiskit.pulse.library.parametric_pulses import Constant
from qiskit.pulse.transforms import pad
from qiskit.qobj.converters.pulse_instruction import QobjToInstructionConverter
from qiskit import assemble

sched=Schedule()
sched+=Play(Constant(4, 0.1), DriveChannel(0))
sched+=Acquire(3, AcquireChannel(0), MemorySlot(0))
sched=pad(sched, inplace=False)

qobj=assemble(sched, qubit_lo_freq=[4.5e9], meas_lo_freq=[5e9], parametric_pulses=['constant'])

converter = QobjToInstructionConverter([])
sched2=Schedule(*[converter(inst) for inst in qobj.experiments[0].instructions])

However, I now get an error on the last line:

Traceback (most recent call last):
  File "/*****/qiskit-terra/qiskit/qobj/converters/pulse_instruction.py", line 78, in get_bound_method
    return self._bound_instructions[bound]
KeyError: '1192ae7dadef42e8e4ff9b67ccad8016bec78136bb36dcda2b2d5d9835435108'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "/*****/qiskit-terra/venv/lib/python3.9/site-packages/IPython/core/interactiveshell.py", line 3437, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-12-56b1883ee859>", line 1, in <module>
    sched2=Schedule(*[converter(inst) for inst in qobj.experiments[0].instructions])
  File "<ipython-input-12-56b1883ee859>", line 1, in <listcomp>
    sched2=Schedule(*[converter(inst) for inst in qobj.experiments[0].instructions])
  File "/*****/qiskit-terra/qiskit/qobj/converters/pulse_instruction.py", line 442, in __call__
    method = self.bind_name.get_bound_method(instruction.name)
  File "/*****qiskit-terra/qiskit/qobj/converters/pulse_instruction.py", line 80, in get_bound_method
    raise QiskitError(f"Bound method for {bound} is not found.") from ex
qiskit.exceptions.QiskitError: 'Bound method for 1192ae7dadef42e8e4ff9b67ccad8016bec78136bb36dcda2b2d5d9835435108 is not found.'

I got this on main as well as far as I can tell. Not to worry if the change is still good, but I can't tell 100% if it would have fixed the issue, and anyway we are slightly reinterpreting the issue as I'm not sure whether we really would be expecting sched2 to equal sched.

@nkanazawa1989
Copy link
Contributor

Thanks for reporting. This is kind of a bug which is caused by the recent upgrade for pulse library #7821 . This replaces ParametricPulse with SymbolicPulse (and thus we implicitly assume user doesn't intentionally use old ParametricPulse), but current qobj converter is only aware of SymbolicPulse.
https://github.com/Qiskit/qiskit-terra/blob/f2c2fe263eedc0dd4cc2f954921bc57e2e89d076/qiskit/qobj/converters/pulse_instruction.py#L36-L46

So if you intentionally use old class with full import path from qiskit.pulse.library.parametric_pulses import *, the converter converts the parametric pulse instance into waveform samples by calling the get_waveform and create a pulse library dictionary in qobj, and thus you cannot disassemble instructions without the pulse library.

I think this will be fixed with #8278 because new logic can handle both ParametricPulse and SymbolicPulse. Meanwhile you can update the import path in your code, i.e. from qiskit.pulse.library import Constant.

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @pollyshaw. This looks good to me.

@pollyshaw
Copy link
Contributor Author

from qiskit.pulse.library import Constant

Thanks @nkanazawa1989: this did fix the error. Comparing sched and sched2 I now get

Schedule((0, Acquire(3, AcquireChannel(0), MemorySlot(0))), (0, Play(Constant(duration=4, amp=(0.1+0j)), DriveChannel(0))), (3, Delay(1, AcquireChannel(0))), name="sched0")
Schedule((0, Acquire(3, AcquireChannel(0), MemorySlot(0))), (0, Play(Constant(duration=4, amp=(0.1+0j), name='constant_e3a2'), DriveChannel(0), name='constant_e3a2')), name="sched3")

so the delay has been lost. Is this as expected?

@nkanazawa1989
Copy link
Contributor

Yes, that is expected. In assembler we implicitly call remove_directives transform which removes all compiler directives. Since the Schedule is absolute time representation of instructions, we don't need to intentionally write delays.

@nkanazawa1989
Copy link
Contributor

Copy link
Contributor

@wshanks wshanks left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Since I took the time to look at it, here are some minor questions/comments I had:

  • It seems a bit weird to me to guard input types in Python, but maybe those checks will flag some issues in existing code.
  • I didn't follow the reasoning for excluding classical IO channels from pad. Isn't the fact that the input is a Schedule a justification for not adding delays to any channel, not just for classical IO channels? What is the purpose of the delays added by pad?
  • Since we decided that delays on classical IO channels are valid, if a delay is added to a classical IO channel by another means, will the problem described in unexpected behaviour of pad/MemorySlot has same prefix as MeasureChannel #7078 with the channel type getting confused still occur?

@nkanazawa1989
Copy link
Contributor

Thanks @wshanks ! New design is based on this comment from Thomas.

A delay on a memory slot is semantically not supported and is an abuse of the pulse object model as far as I can tell as described in #7078.

Actually there is no reason to call the pad function with Schedule. This was a main UX of pulse programming before the builder syntax/ScheduleBlock. When we coded with its syntactic sugar, sometime padding was useful to prevent injection of instructions with .append (i.e. implicit asap). However, I think we are no longer using this programming model. The problem is we are still calling the pad function when the assembler dumps a pulse qobj. This is due to convention, but we can cleanup these transforms when we have a chance of refactoring. In principle, padding is not needed as long as we code with the builder syntax, because the Schedule is absolute time model and appending ScheduleBlock to another block doesn't schedule instructions across blocks.

This prevents the "m" channel prefix from confusing a qobj to disassemble, since no memory slots will appear because of the padding.

@nkanazawa1989 nkanazawa1989 added automerge mod: pulse Related to the Pulse module labels Jul 27, 2022
@mergify mergify bot merged commit 28802cf into Qiskit:main Jul 27, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2741084950

  • 18 of 18 (100.0%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.0001%) to 83.991%

Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/squ.py 2 80.9%
Totals Coverage Status
Change from base Build 2739025125: 0.0001%
Covered Lines: 55896
Relevant Lines: 66550

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: pulse Related to the Pulse module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

unexpected behaviour of pad/MemorySlot has same prefix as MeasureChannel
8 participants