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

Conversation

nkanazawa1989
Copy link
Contributor

Summary

When a user tries to get calibration data from Target, it may fail because of partly missing calibration data, e.g.

backend.target["some_gate"][(0, 1)].calibration

This may occur when the backend doesn't support waveform payload, while Qiskit doesn't have definition of the corresponding parametric pulse that the backend uses.

Details and comments

This PR adds exception handling for this case. Namely, None value is returned instead of raising an error.

@nkanazawa1989 nkanazawa1989 added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: pulse Related to the Pulse module labels Dec 11, 2023
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core
  • @nkanazawa1989

@mtreinish mtreinish added this to the 0.45.2 milestone Dec 11, 2023
@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Dec 11, 2023
@coveralls
Copy link

coveralls commented Dec 11, 2023

Pull Request Test Coverage Report for Build 7532191522

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.8%) to 89.354%

Totals Coverage Status
Change from base Build 7463210826: 1.8%
Covered Lines: 59498
Relevant Lines: 66587

💛 - Coveralls

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.

The code changes look okay here, but I would a little more explanation of the issue. I don't know all the details of the qobj code, but I am guessing only a few people at most know it better than me, so if I am having trouble understanding what the issue is here probably most other people will as well.

From the release note, I understand that the problem occurs when the backend does not support a waveform payload while Qiskit does not have a definition for the parametric pulse shape reported. So is the situation that the defaults json includes a parametric_pulse entry with a name that is not in the ParametricPulseShapes Enum? So when the converter tries to convert a parametric pulse, it hits an AttributeError here:

pulse = ParametricPulseShapes.to_type(instruction.pulse_shape)(**params, name=pulse_name)

because to_type uses getattr:

def to_type(cls, name: str) -> library.SymbolicPulse:
"""Get symbolic pulse class from the name.
Args:
name: Qobj name of the pulse.
Returns:
Corresponding class.
"""
return getattr(library, cls[name].value)

and that causes the converter to fall back to _convert_generic here:

try:
method = getattr(self, f"_convert_{instruction.name}")
except AttributeError:
method = self._convert_generic

If that is the case, I don't think the part about supporting a waveform payload matters? You wouldn't expect a pulse to be in the waveform pulse library if it was reported as a parametric pulse.

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.

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.

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

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!

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Approving to unblock the merge, thanks a lot!

@ElePT ElePT added this pull request to the merge queue Jan 16, 2024
Merged via the queue into Qiskit:main with commit a9e9e95 Jan 16, 2024
13 checks passed
mergify bot pushed a commit that referenced this pull request Jan 16, 2024
* Fix missing pulse lib bug

* Update releasenotes/notes/fix-missing-pulse-lib-c370f5b9393d0df6.yaml

Co-authored-by: Will Shanks <wshaos@posteo.net>

* Add user warning

---------

Co-authored-by: Will Shanks <wshaos@posteo.net>
(cherry picked from commit a9e9e95)

# Conflicts:
#	qiskit/pulse/calibration_entries.py
github-merge-queue bot pushed a commit that referenced this pull request Jan 17, 2024
#11397) (#11573)

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

* Fix missing pulse lib bug

* Update releasenotes/notes/fix-missing-pulse-lib-c370f5b9393d0df6.yaml

Co-authored-by: Will Shanks <wshaos@posteo.net>

* Add user warning

---------

Co-authored-by: Will Shanks <wshaos@posteo.net>
(cherry picked from commit a9e9e95)

# Conflicts:
#	qiskit/pulse/calibration_entries.py

* Fix merge conflict

---------

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: pulse Related to the Pulse module stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants