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

Fix a bug of missing pulse library entry in PulseQobj parsing #11397

Merged
merged 3 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions qiskit/pulse/calibration_entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
from qiskit.pulse.schedule import Schedule, ScheduleBlock
from qiskit.qobj.converters import QobjToInstructionConverter
from qiskit.qobj.pulse_qobj import PulseQobjInstruction
from qiskit.exceptions import QiskitError


IncompletePulseQobj = object()
"""A None-like constant that represents the PulseQobj is incomplete."""


class CalibrationPublisher(IntEnum):
Expand Down Expand Up @@ -316,11 +321,16 @@ def __init__(
def _build_schedule(self):
"""Build pulse schedule from cmd-def sequence."""
schedule = Schedule(name=self._name)
for qobj_inst in self._source:
for qiskit_inst in self._converter._get_sequences(qobj_inst):
schedule.insert(qobj_inst.t0, qiskit_inst, inplace=True)
self._definition = schedule
self._parse_argument()
try:
for qobj_inst in self._source:
for qiskit_inst in self._converter._get_sequences(qobj_inst):
schedule.insert(qobj_inst.t0, qiskit_inst, inplace=True)
self._definition = schedule
self._parse_argument()
except QiskitError:
# When the play waveform data is missing in pulse_lib we cannot build schedule.
# Instead of raising an error, get_schedule should return None.
self._definition = IncompletePulseQobj
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we emit a warning (UserWarning?) here with the message from the QiskitError? Here we are treating a calibration loading error as equivalent to no calibration being provided and the user might want to know when there is a loading error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Added an error and test in bf723eb


def define(
self,
Expand All @@ -336,9 +346,11 @@ def get_signature(self) -> inspect.Signature:
self._build_schedule()
return super().get_signature()

def get_schedule(self, *args, **kwargs) -> Schedule | ScheduleBlock:
def get_schedule(self, *args, **kwargs) -> Schedule | ScheduleBlock | None:
if self._definition is None:
self._build_schedule()
if self._definition is IncompletePulseQobj:
return None
return super().get_schedule(*args, **kwargs)

def __eq__(self, other):
Expand All @@ -356,4 +368,6 @@ def __str__(self):
if self._definition is None:
# Avoid parsing schedule for pretty print.
return "PulseQobj"
if self._definition is IncompletePulseQobj:
return "None"
return super().__str__()
6 changes: 5 additions & 1 deletion qiskit/qobj/converters/pulse_instruction.py
Original file line number Diff line number Diff line change
Expand Up @@ -959,8 +959,12 @@ def _convert_generic(

yield instructions.Play(waveform, channel)
else:
if qubits := getattr(instruction, "qubits", None):
msg = f"qubits {qubits}"
else:
msg = f"channel {instruction.ch}"
Copy link
Contributor

Choose a reason for hiding this comment

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

ch does not have to be set as an attribute of PulseQobjInstruction either. Should we just hope that either qubits or ch is set? Every instruction should have a ch set except acquire which should have qubits set so I think for a valid qobj that is reasonable (it is weird that this block assumed qubits would always be defined here rather than ch).

Copy link
Contributor Author

@nkanazawa1989 nkanazawa1989 Jan 15, 2024

Choose a reason for hiding this comment

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

Yeah both ch and qubits are optional as you say. For now having either ch and qubits is valid assumption.

raise QiskitError(
f"Instruction {instruction.name} on qubit {instruction.qubits} is not found "
f"Instruction {instruction.name} on {msg} is not found "
"in Qiskit namespace. This instruction cannot be deserialized."
)

Expand Down
11 changes: 11 additions & 0 deletions releasenotes/notes/fix-missing-pulse-lib-c370f5b9393d0df6.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
fixes:
- |
Fixed a bug that results in an error when a user tries to load .calibration
data of a gate in :class:`.Target` in a particular situation.
This occurs when the backend doesn't support pulse waveform payload, while
Qiskit doesn't have corresponding parametric pulse definition.
In this situation, Qiskit pulse object cannot be built, resulting in the failure in
parsing PulseQobj and the user encounters a meaningless error.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the situation where the error is meaningless? This changes the code so that the case where a calibration schedule was unloadable behaves like no calibration was reported by the backend rather than raising an exception. If the user wanted to use the calibration, the user's code will still have a problem. Maybe there is some code that eagerly handles calibrations and can work okay when they are not available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Qobj is parsed when they are called for the first time (i.e. when a user call .calibration of target instruction) and the meaningless error raised when the user called calibration in their program (non-pulse user will never see this). It tries to raise a QiskitError but the error message required .qubits that the play instruction payload cannot provide, and this resulted in AttributeError instead. It was very hard to understand what was happening.

I expect the user implements the code that checks if calibration is available on their side.

This error was suppressed, and the .calibration now returns None value instead,
which implies the calibration data is not properly supplied.
nkanazawa1989 marked this conversation as resolved.
Show resolved Hide resolved
34 changes: 34 additions & 0 deletions test/python/pulse/test_calibration_entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -434,3 +434,37 @@ def test_equality_with_schedule(self):
entry2.define(program)

self.assertEqual(entry1, entry2)

def test_calibration_missing_waveform(self):
"""Test that calibration with missing waveform should become None.

When a hardware doesn't support waveform payload and Qiskit doesn't have
the corresponding parametric pulse definition, CmdDef with missing waveform
might be input to the QobjConverter. This fails in loading the calibration data
because necessary pulse object cannot be built.

In this situation, parsed calibration data must become None,
instead of raising an error.
"""
serialized_program = [
PulseQobjInstruction(
name="SomeMissingPulse",
t0=0,
ch="d0",
)
]
entry = PulseQobjDef(name="qobj_entry")
entry.define(serialized_program)

# This is pulse qobj before parsing it
self.assertEqual(str(entry), "PulseQobj")

# Actual calibration value is None
parsed_output = entry.get_schedule()
self.assertIsNone(parsed_output)

# Repr becomes None-like after it finds calibration is incomplete
self.assertEqual(str(entry), "None")

# Signature is also None
self.assertIsNone(entry.get_signature())