Update pulse gate transpiler pass to use target. #9587

nkanazawa1989 · 2023-02-15T09:52:39Z

Summary

PulseGate pass is updated to internally use Target, rather than InstructionScheduleMap. This is to fix a bug that custom gate definition is ignored when circuit is transpiled with v2 backend. In the previous implementation, target is always provided when the backend argument is V2, and target has higher priority than the inst_map in the PulseGate pass. After this update, custom gates in the inst_map are copied into target before transform.

Fix #9489

Details and comments

There are several updates to realize above behavior. First, Target.update_from_instruction_schedule_map has been updated not to raise an error when the method is called without inst_name_map. InstructionProperties._calibration now only takes CalibrationEntry so that we can always get Signature object from there to parse parameter value-object mapping to lower the OpNode with assigned parameters (i.e. CircuitInstruction doesn't preserve parameter name after assignment). CalibrationEntry now provides get_duration method to efficiently compute schedule duration without parsing PulseQobj JSON.

qiskit-bot · 2023-02-15T09:52:42Z

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:

@Qiskit/terra-core
@nkanazawa1989

releasenotes/notes/update-pulse-gate-pass-for-target-ebfb0ec9571f058e.yaml

mtreinish · 2023-02-15T10:41:16Z

qiskit/pulse/calibration_entries.py

@@ -64,6 +64,15 @@ def get_schedule(self, *args, **kwargs) -> Union[Schedule, ScheduleBlock]:
        """
        pass

+    @abstractmethod


Can you do this without breaking compatibility? Wouldn't adding a new required abstract method break any downstream usage of CalibrationEntry?

Done in 33b76b3

I'm not sure this has been fixed (or if it was the fix got undone at some point), because this is still a new abstractmethod which will cause a hard failure for any existing implementations of this interface.

Previously I implemented an abstractmethod to return schedule duration. Note that schedule duration is computed here for added schedule from the inst map, but this requires Qobj parsing that is the biggest performance bottleneck for non-pulse users. So I have implemented a custom (complicated) method that computes the duration without parsing Qobj.
https://github.com/Qiskit/qiskit-terra/blob/bf3bb96cebc141f79ce46b5938724eef30778c8e/qiskit/transpiler/target.py#L449-L457

New abstract method just returns whether it is provided by backend or by end-user. If your schedule is provided from backend, you should know duration of it and we don't need to update duration in the first place. This avoids unnecessary Qobj parsing for duration. If it is user-provided one, usually they don't define schedule with JSON format, and no parsing overhead.

nkanazawa1989 · 2023-02-16T06:47:36Z

f692207 and bbc0a0e are from #9597 so I'll rebase this PR after it is merged.

Currently I am struggling with handling of synthesis and decomposition passes (see failed test). In main branch, these passes are executed to get optimal gate decomposition based on InstructionProperties.error reported by the backend. When we have a custom gate calibration in the inst map, it must change the reference fidelity of these passes. However, it still refers to the backend value. In b5876dc I updated the transpiler to immediately copy the inst map to the target to reflect user provided calibrations to the rest of passes. However, gate error dict is not available there and the gate error becomes None. This crashes calculation of cost function in these passes, i.e TypeError due to BinOp(None, float). Note that, even if I run Target.update_from_instruction_schedule_map locally in the pulse gate pass, it mutates target in all other passes. So the same problem will occur.

Unfortunately, I am not familiar with these optimization passes. What would be the correct behavior of these passes when gate fidelity is not available?

mtreinish · 2023-02-16T15:21:17Z

That looks like a bug in the recently merged #9175 and is probably a similar if not the same issue as #9592. In the internal target data model the values can be None and the updated unitary synthesis isn't handling this correctly. I'll work on a PR shortly

nkanazawa1989 · 2023-02-17T11:00:42Z

Thanks Matthew. As far as I saw, qiskit.transpiler.passes.optimization.optimize_1q_decomposition and qiskit.transpiler.passes.synthesis.unitary_synthesis use similar fidelity based optimization:

https://github.com/Qiskit/qiskit-terra/blob/4362c72ccaa34acc339377f6084cfe48ecbc8413/qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py#L196

The getattr default value only works when instruction property itself is None. This corresponds to the case the gate itself is not defined, though I am not sure if this is intended condition.

(1q gate bug is probably resolved by #9578?)

mtreinish · 2023-02-17T14:04:43Z

For 1q yeah I think #9578 should fix it. For the general question though the pass should be robust enough to handle no error definition or no instruction properties definition for it's scoring heuristics. What the passes are trying to do is after synthesizing circuits in all the target bases which the Target says it has support for (e.g. if the target says it has support for rz, rx, ry, cx then for 1q it will try XZX, XYX, ZXZ, and ZYZ synthesis) it's trying to figure out which of those results is the best performing and then using that. The default heuristic with a Target provided is looking at the error rates and estimating the circuit fidelity based on that, but if the target is missing the error rates for any instruction (which can be treated as a None for target[inst][(qubit,)].error being None, target[inst][(qubit,)] being None, or target[inst] being None or {None: None} depending on the type of instruction) that should be modeled as an ideal implementation of a gate (i.e. error == 0.0). We just need to update the pass code to handle this.

Edit: This internal implementation details of the target are tricky, I've had a PR up to add helper methods to hide these details #9158 (I need to circle back to it for 0.24).

#9617 should fix the issue

nkanazawa1989 · 2023-02-20T08:51:52Z

Rebased. Thanks @mtreinish. I reviewed #9158

coveralls · 2023-03-10T14:48:18Z

Pull Request Test Coverage Report for Build 4562112367

129 of 137 (94.16%) changed or added relevant lines in 6 files are covered.
1273 unchanged lines in 96 files lost coverage.
Overall coverage increased (+0.02%) to 85.388%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/calibration/pulse_gate.py	9	10	90.0%
qiskit/transpiler/target.py	66	69	95.65%
qiskit/pulse/calibration_entries.py	47	51	92.16%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/dense_layout.rs	1	95.3%
qiskit/algorithms/amplitude_amplifiers/amplification_problem.py	1	84.72%
qiskit/algorithms/amplitude_amplifiers/amplitude_amplifier.py	1	94.0%
qiskit/algorithms/eigen_solvers/eigen_solver.py	1	97.78%
qiskit/algorithms/evolvers/real_evolver.py	1	92.31%
qiskit/algorithms/phase_estimators/phase_estimation_result.py	1	96.36%
qiskit/algorithms/time_evolvers/variational/var_qite.py	1	94.74%
qiskit/circuit/add_control.py	1	97.2%
qiskit/circuit/library/arithmetic/adders/vbe_ripple_carry_adder.py	1	98.55%
qiskit/circuit/library/standard_gates/p.py	1	98.9%

Totals
Change from base Build 4416721543:	0.02%
Covered Lines:	67321
Relevant Lines:	78841

💛 - Coveralls

When inst_map is provided, it copies schedules there into target instance. This fixes a bug that custom schedules in the inst_map are ignored when transpiling circuit with V2 backend. To support this behavior, internal machinery of Target is updated so that a target instance can update itself only with inst_map without raising any error. Also InstructionProperties.calibration now only stores CalibrationEntry instances. When Schedule or ScheduleBlock are provided as a calibration, it converts schedule into CalibrationEntry instance.

IBM backend still provide ugate calibrations in CmdDef and they are loaded in the instmap. If we update target with the instmap, these gates are accidentally registered in the target, and they may be used in the following 1q decomposition. To prevent this, update_from_instruction_schedule_map method is updated.

…g-instmap-v2-backend

mtreinish

Thanks for doing this and ensuring the pulse gates path works as expected with a target. I'm looking forward to having this better handling of calibrations and InstructionScheduleMap compatibility in place. I left a few comments inline, but my biggest concern right now is around api compatibility both for CalibrationEntry as a whole (I'm not sure what the downstream upgrade path looks like because I'm pretty sure any downstream users will break with these changes) and also to a lesser degree the behavior around setting InstructionProperties.calibration (I'm a bit worried that this will subtly change things causing the PulseGates pass to run when it doesn't need to.

The other question that I have, which is kind of future facing, is in a world with only a Target what does the replacement for this line look like: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/preset_passmanagers/common.py#L449
We shouldn't change this here, but I expect we'll need another API added to the target for this, which we can do in a follow up PR.

mtreinish · 2023-03-14T14:41:10Z

qiskit/pulse/calibration_entries.py

@@ -34,11 +34,12 @@ class CalibrationEntry(metaclass=ABCMeta):
    """A metaclass of a calibration entry."""

    @abstractmethod
-    def define(self, definition: Any):
+    def define(self, definition: Any, user_provided: bool):


This will be a breaking API change for any downstream usage of CalibrationEntry right? Like if you have an existing CalibrationEntry subclass in 0.23.x then when upgrading to 0.24.0 that class's define method signature will no long match the abstract interface definition.

No, all subclasses provide default value for user_provided to conform to previous default behavior. Currently in 0.23.x, this flag is implicitly determined by type information and only PulseQobjDef is recognized as a backend provided entry. However one may want to provide backend calibration as ScheduleDef, to bypass cumbersome JSON data generation. This could be a separate PR though.

mtreinish · 2023-03-14T14:42:02Z

qiskit/pulse/calibration_entries.py

@@ -64,6 +64,15 @@ def get_schedule(self, *args, **kwargs) -> Union[Schedule, ScheduleBlock]:
        """
        pass

+    @abstractmethod


I'm not sure this has been fixed (or if it was the fix got undone at some point), because this is still a new abstractmethod which will cause a hard failure for any existing implementations of this interface.

mtreinish · 2023-03-14T14:59:57Z

qiskit/transpiler/target.py

@@ -89,7 +92,7 @@ def calibration(self):
    def calibration(self, calibration: Union[Schedule, ScheduleBlock, CalibrationEntry]):
        if isinstance(calibration, (Schedule, ScheduleBlock)):
            new_entry = ScheduleDef()
-            new_entry.define(calibration)
+            new_entry.define(calibration, user_provided=True)


Is this actually going to be the case? Like can we safely assume that no backend is going to set a calibration with the setter? If we need to make this assumption we should at least be sure that it's documented clearly somewhere because this is new behavior. The other option I'm wondering if it would be better is to add a dedicated method like set_custom_calibration which has a kwarg to set this (and defaults to True).

Yes. If a provider defines a backend schedule, one can directly create CalibrationEntry instance with user_provided=False. This code block is invoked only when .calibration setter is called with raw Schedule or ScheduleBlock, which is likely the workflow of the end users.

Then can we document this behavior? I think it's a bit non-obvious without reading the code. I agree the typical backend construction should be populating the calibration via the InstructionProperties constructor but we should make it clear that if a provider were to add calibrations after initial construction via the setter it will be treated as a custom calibration.

Added in f203f59. I think setter docs is not rendered in Sphinx build, so added detailed note section to the property method.

qiskit/transpiler/target.py

releasenotes/notes/update-pulse-gate-pass-for-target-ebfb0ec9571f058e.yaml

…g-instmap-v2-backend

nkanazawa1989 · 2023-03-14T17:42:11Z

Thanks Matthew for the review. Regarding the backward compatibility

CalibrationEntry and downstream users: It only affects users who define own CalibrationEntry subclass. I don't think this is the case, because CalibrationEntry is an internal class to manage calibrations. All built-in subclasses provide own default value to keep backward compatibility, so there should not be breaking change.
InstructionProperties.calibration: This doesn't change any behavior with this PR. Previously any Schedule or ScheduleBlock set to is wrapped by ScheduleDef which is always considered as a custom calibration (because of its type). Having user_provided=True just clarifies this behavior, i.e. when backend adds calibration, one must generate CalibrationEntry subclass from the schedule with user_provided=False.

The other question that I have, which is kind of future facing, is in a world with only a Target

I didn't check these pass factory functions. We should update them to cover the situation that StagedPassManager is directly instantiated outside the transpile. On the other hand, updating these function indicates update_from_instruction_schedule_map is called twice when you use transpile function. Regarding has_custom_gate compatibility in Target, in principle we should scan all InstructionProperties.calibration.user_provided in the target gate map. This looks like a heavy overhead (but currently InstructionScheduleMap is implemented in that way). It's hard to avoid this because target and inst map are both mutable and user can mutate calibration at anytime.

mtreinish

Ah ok, if CalibrationEntry is an internal interface (I was initially confused because of the EXPERIMENT_SERVICE in the publisher enum and assumed something external might be using it) then this mostly LGTM now. Just two small doc things, the first is an inline, but the other is to add something to the class docstring for CalibrationEntry saying it's private without a stable user facing API. I realize now it's in the module docstring but having it explicitly in the class would be good so we make it clear if someone was randomly using it outside of terra.

mtreinish · 2023-03-27T18:40:13Z

qiskit/transpiler/target.py

@@ -89,7 +92,7 @@ def calibration(self):
    def calibration(self, calibration: Union[Schedule, ScheduleBlock, CalibrationEntry]):
        if isinstance(calibration, (Schedule, ScheduleBlock)):
            new_entry = ScheduleDef()
-            new_entry.define(calibration)
+            new_entry.define(calibration, user_provided=True)


Then can we document this behavior? I think it's a bit non-obvious without reading the code. I agree the typical backend construction should be populating the calibration via the InstructionProperties constructor but we should make it clear that if a provider were to add calibrations after initial construction via the setter it will be treated as a custom calibration.

nkanazawa1989 · 2023-03-30T07:32:47Z

Thanks Matthew. The calibration behavior is detailed in f203f59, and use case of CalibrationEntry is added by 2c4b687. Another small change made by 20977ed is to more strictly validate opaque Gate object when any gate mapping is found and it is generated by schedules.

nkanazawa1989 · 2023-03-30T08:18:00Z

Another change in f217d01 is necessary to prevent a bug but it might be slight performance regression with deepcopy. This is only triggered when the instmap is provided and thus no impact for the workflow of standard applications. We should cleanup the handling of transpiler arguments maybe in next release.

mtreinish

This LGTM now. Thanks for the updates and adding the note sections to the docs. I think that makes it much more explicit how this works now.

* Update PulseGate pass to use Target internally. When inst_map is provided, it copies schedules there into target instance. This fixes a bug that custom schedules in the inst_map are ignored when transpiling circuit with V2 backend. To support this behavior, internal machinery of Target is updated so that a target instance can update itself only with inst_map without raising any error. Also InstructionProperties.calibration now only stores CalibrationEntry instances. When Schedule or ScheduleBlock are provided as a calibration, it converts schedule into CalibrationEntry instance. * Remove fix note * Remove get_duration * Update the logic to get instruction object * Update target immediately when inst map is available * Update tests * Fix edge case. IBM backend still provide ugate calibrations in CmdDef and they are loaded in the instmap. If we update target with the instmap, these gates are accidentally registered in the target, and they may be used in the following 1q decomposition. To prevent this, update_from_instruction_schedule_map method is updated. * cleanup release note * Minor review suggestions * More strict gate uniformity check when create from schedules. * Added note for calibration behavior * More documentation for CalibrationEntry * Add logic to prevent unintentional backend mutation with instmap. * fix lint

nkanazawa1989 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Feb 15, 2023

mtreinish reviewed Feb 15, 2023

View reviewed changes

releasenotes/notes/update-pulse-gate-pass-for-target-ebfb0ec9571f058e.yaml Outdated Show resolved Hide resolved

mtreinish reviewed Feb 15, 2023

View reviewed changes

mtreinish added this to the 0.24.0 milestone Feb 15, 2023

mtreinish added the priority: high label Feb 15, 2023

nkanazawa1989 force-pushed the fix/missing-instmap-v2-backend branch from 48e6724 to c78ead8 Compare February 16, 2023 06:37

nkanazawa1989 force-pushed the fix/missing-instmap-v2-backend branch from c78ead8 to 8529043 Compare February 20, 2023 05:20

nkanazawa1989 marked this pull request as ready for review March 10, 2023 14:39

nkanazawa1989 requested review from a team, alexanderivrii, ShellyGarion, eggerdj and jyu00 as code owners March 10, 2023 14:39

This was referenced Mar 10, 2023

Fix gate error handling in unitary synthesis #9774

Merged

Gates with custom pulses fail to transpile with the new provider #9781

Closed

nkanazawa1989 added 8 commits March 14, 2023 01:53

Remove fix note

c7f10c0

Remove get_duration

bfd33d4

Update the logic to get instruction object

a256743

Update target immediately when inst map is available

dd4ad0c

Update tests

5207b47

cleanup release note

d10fcf9

Merge branch 'main' of github.com:Qiskit/qiskit-terra into fix/missin…

066d9d8

…g-instmap-v2-backend

nkanazawa1989 force-pushed the fix/missing-instmap-v2-backend branch from 57ea4e2 to 066d9d8 Compare March 13, 2023 16:55

mtreinish reviewed Mar 14, 2023

View reviewed changes

nkanazawa1989 added 2 commits March 15, 2023 01:20

Minor review suggestions

bdb790a

Merge branch 'main' of github.com:Qiskit/qiskit-terra into fix/missin…

a722e5e

…g-instmap-v2-backend

mtreinish reviewed Mar 27, 2023

View reviewed changes

nkanazawa1989 added 3 commits March 30, 2023 15:34

More strict gate uniformity check when create from schedules.

20977ed

Added note for calibration behavior

f203f59

More documentation for CalibrationEntry

2c4b687

nkanazawa1989 added 2 commits March 30, 2023 16:59

Add logic to prevent unintentional backend mutation with instmap.

f217d01

fix lint

d0b2109

mtreinish approved these changes Mar 30, 2023

View reviewed changes

mtreinish added Changelog: New Feature Include in the "Added" section of the changelog Changelog: API Change Include in the "Changed" section of the changelog and removed Changelog: Bugfix Include in the "Fixed" section of the changelog labels Mar 30, 2023

mtreinish added this pull request to the merge queue Mar 30, 2023

Merged via the queue into Qiskit:main with commit 2f03a6b Mar 30, 2023

nkanazawa1989 deleted the fix/missing-instmap-v2-backend branch December 11, 2023 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pulse gate transpiler pass to use target. #9587

Update pulse gate transpiler pass to use target. #9587

nkanazawa1989 commented Feb 15, 2023 •

edited

Loading

qiskit-bot commented Feb 15, 2023

mtreinish Feb 15, 2023

nkanazawa1989 Feb 16, 2023

mtreinish Mar 14, 2023

nkanazawa1989 Mar 14, 2023 •

edited

Loading

nkanazawa1989 commented Feb 16, 2023 •

edited

Loading

mtreinish commented Feb 16, 2023

nkanazawa1989 commented Feb 17, 2023 •

edited

Loading

mtreinish commented Feb 17, 2023 •

edited

Loading

nkanazawa1989 commented Feb 20, 2023

coveralls commented Mar 10, 2023 •

edited

Loading

mtreinish left a comment

mtreinish Mar 14, 2023

nkanazawa1989 Mar 14, 2023

mtreinish Mar 14, 2023

mtreinish Mar 14, 2023

nkanazawa1989 Mar 14, 2023

mtreinish Mar 27, 2023

nkanazawa1989 Mar 30, 2023

nkanazawa1989 commented Mar 14, 2023

mtreinish left a comment

mtreinish Mar 27, 2023

nkanazawa1989 commented Mar 30, 2023

nkanazawa1989 commented Mar 30, 2023

mtreinish left a comment

Update pulse gate transpiler pass to use target. #9587

Update pulse gate transpiler pass to use target. #9587

Conversation

nkanazawa1989 commented Feb 15, 2023 • edited Loading

Summary

Details and comments

qiskit-bot commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

nkanazawa1989 commented Feb 16, 2023 • edited Loading

mtreinish commented Feb 16, 2023

nkanazawa1989 commented Feb 17, 2023 • edited Loading

mtreinish commented Feb 17, 2023 • edited Loading

nkanazawa1989 commented Feb 20, 2023

coveralls commented Mar 10, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4562112367

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 commented Mar 14, 2023

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 commented Mar 30, 2023

nkanazawa1989 commented Mar 30, 2023

mtreinish left a comment

Choose a reason for hiding this comment

nkanazawa1989 commented Feb 15, 2023 •

edited

Loading

nkanazawa1989 Mar 14, 2023 •

edited

Loading

nkanazawa1989 commented Feb 16, 2023 •

edited

Loading

nkanazawa1989 commented Feb 17, 2023 •

edited

Loading

mtreinish commented Feb 17, 2023 •

edited

Loading

coveralls commented Mar 10, 2023 •

edited

Loading