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 all commits
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
31 changes: 25 additions & 6 deletions qiskit/pulse/calibration_entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"""Internal format of calibration data in target."""
from __future__ import annotations
import inspect
import warnings
from abc import ABCMeta, abstractmethod
from collections.abc import Sequence, Callable
from enum import IntEnum
Expand All @@ -22,6 +23,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 +322,20 @@ 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 as ex:
# When the play waveform data is missing in pulse_lib we cannot build schedule.
# Instead of raising an error, get_schedule should return None.
warnings.warn(
f"Pulse calibration cannot be built and the entry is ignored: {ex.message}.",
UserWarning,
)
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 +351,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 +373,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
12 changes: 12 additions & 0 deletions releasenotes/notes/fix-missing-pulse-lib-c370f5b9393d0df6.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
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 reports only partial calibration data, for
example referencing a waveform pulse in a command definition but not
including that waveform pulse in the pulse library. In this situation, the
Qiskit pulse object cannot be built, resulting in a failure to build the pulse
schedule for the calibration. Now when calibration data is incomplete
the :class:`.Target` treats it as equivalent to no calibration being reported
at all and does not raise an exception.
58 changes: 58 additions & 0 deletions test/python/pulse/test_calibration_entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,30 @@ def test_add_qobj(self):
)
self.assertEqual(schedule_to_test, schedule_ref)

def test_missing_waveform(self):
"""Test incomplete Qobj should raise warning and calibration returns None."""
serialized_program = [
PulseQobjInstruction(
name="waveform_123456",
t0=20,
ch="d0",
),
]
entry = PulseQobjDef(converter=self.converter, name="my_gate")
entry.define(serialized_program)

with self.assertWarns(
UserWarning,
msg=(
"Pulse calibration cannot be built and the entry is ignored: "
"Instruction waveform_123456 on channel d0 is not found in Qiskit namespace. "
"This instruction cannot be deserialized."
),
):
out = entry.get_schedule()

self.assertIsNone(out)

def test_parameterized_qobj(self):
"""Test adding and managing parameterized qobj.

Expand Down Expand Up @@ -434,3 +458,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())