Fix delay padding to respect target's constraints #10007

itoko · 2023-04-21T04:16:47Z

Summary

Fixes delay padding in transpile() function and tranpiler passes to respect target's constraints.

Details and comments

PadDelay and PadDynamicalDecoupling are fixed so that they do not pad any idle time of qubits for which the target does not support Delay instruction. For that, the parent BasePadding as well as PadDelay is updated so that it optionally takes target argument. Legacy scheduling passes ASAPSchedule and ALAPSchedule, which pad delays internally, are also fixed in the same way. In addition, transpile() is fixed to call PadDelay with a target object so that it works correctly when called with scheduling_method option.

Fixes #9993

qiskit-bot · 2023-04-21T04:16:51Z

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

itoko

I think this is an urgent PR, so please feel free to add commits directly if necessary. @nkanazawa1989 @mtreinish

itoko · 2023-04-21T04:19:14Z

qiskit/transpiler/passes/scheduling/alap.py

@@ -138,7 +138,8 @@ def run(self, dag):
            for bit in node.qargs:
                delta = t0 - idle_before[bit]
                if delta > 0:
-                    new_dag.apply_operation_front(Delay(delta, time_unit), [bit], [])
+                    if self.target is None or (bit_indices[bit],) in self.target.get("delay", []):


Do we have a better way to check if an instruction is supported in a target? @mtreinish

I think you can use this method.
https://github.com/Qiskit/qiskit-terra/blob/908adb34323682cf48a8bbc711d01f651dd26932/qiskit/transpiler/target.py#L568-L573

Umm, it may not work for a target that does not support only the delay instruction.

There is the instruction_supported method you can use to do this lookup: https://qiskit.org/documentation/stubs/qiskit.transpiler.Target.instruction_supported.html#qiskit.transpiler.Target.instruction_supported it should handle all the edge cases around doing the lookup correctly

coveralls · 2023-04-21T04:42:53Z

Pull Request Test Coverage Report for Build 4772053053

75 of 75 (100.0%) changed or added relevant lines in 8 files are covered.
18 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.08%) to 85.999%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	95.37%
crates/qasm2/src/lex.rs	5	90.89%
crates/qasm2/src/parse.rs	12	97.11%

Totals
Change from base Build 4759571327:	0.08%
Covered Lines:	71117
Relevant Lines:	82695

💛 - Coveralls

mtreinish

Overall this looks great, thanks for fixing this! The approach you're doing here looks to be the best way to handle this. I left 2 comments inline, neither is really a blocker, just questions more than anything.

As a procedural point, this technically is adding a new argument to the PadDelay, PadDynamicalDecoupling and BasePadding classes which normally would not be eligible for backporting for a stable release. But for 0.24.0 #9993 was identified as an issue where the transpiler is producing incorrect output circuits with real hardware when using the scheduling_method and we need to pass the Target to the padding passes to fix this. So since we're still in the rc period I think it's ok to backport this for the 0.24.0 release as an exception to fix the high priority bug.

mtreinish · 2023-04-21T13:05:21Z

qiskit/transpiler/passes/scheduling/padding/base_padding.py

+                        if self.target is None or self.target.instruction_supported(
+                            "delay", qargs=(bit_indices[bit],)
+                        ):


When I started to look it this after opening #9993 I was thinking here we'd need to also handle the gates in DD sequences here besides just delays. But I think as a first step and fixing what we support in transpile() this enough. Maybe for the DD padding pass we do the additional validation of the extra sequence instructions inside the pass (as a second follow-ups PR)?

mtreinish · 2023-04-21T13:06:48Z

qiskit/transpiler/passes/scheduling/padding/base_padding.py

+                        t_end=circuit_duration,
+                        next_node=node,
+                        prev_node=prev_node,
+                    )


Do you think we should log a message to indicate to users why we're not putting a delay on the the qubit (maybe DEBUG level)? I'm not sure it's needed but I'm wondering if it'll be confusing for users of the scheduling passes if they ask for scheduling and there are qubits without delays?

mtreinish

LGTM, thanks for the quick updates. It'll be really good to have this fixed for 0.24.0

* Add and update tests * Fix padding passes to respect target's constraints * Fix transpile with scheduling to respect target's constraints * Add release note * fix reno * use target.instruction_supported * simplify * Add check if all DD gates are supported on each qubit * Add logging * Update DD tests * Make DD gates check target-aware * Fix legacy DD pass (cherry picked from commit 117d188)

* Add and update tests * Fix padding passes to respect target's constraints * Fix transpile with scheduling to respect target's constraints * Add release note * fix reno * use target.instruction_supported * simplify * Add check if all DD gates are supported on each qubit * Add logging * Update DD tests * Make DD gates check target-aware * Fix legacy DD pass (cherry picked from commit 117d188) Co-authored-by: Toshinari Itoko <[email protected]>

* Add and update tests * Fix padding passes to respect target's constraints * Fix transpile with scheduling to respect target's constraints * Add release note * fix reno * use target.instruction_supported * simplify * Add check if all DD gates are supported on each qubit * Add logging * Update DD tests * Make DD gates check target-aware * Fix legacy DD pass

itoko added 3 commits April 21, 2023 12:28

Add and update tests

6d032e0

Fix padding passes to respect target's constraints

da38b13

Fix transpile with scheduling to respect target's constraints

fb1daba

itoko requested a review from a team as a code owner April 21, 2023 04:16

itoko commented Apr 21, 2023

View reviewed changes

Add release note

fbc7851

mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels Apr 21, 2023

mtreinish added this to the 0.24.0 milestone Apr 21, 2023

itoko added 2 commits April 21, 2023 19:58

fix reno

b3e4cb4

use target.instruction_supported

1c3f585

mtreinish reviewed Apr 21, 2023

View reviewed changes

itoko added 6 commits April 22, 2023 00:14

simplify

eee5bae

Add check if all DD gates are supported on each qubit

df82312

Add logging

d8f7de0

Update DD tests

d864b1d

Make DD gates check target-aware

ae972fd

Fix legacy DD pass

deb9117

mtreinish approved these changes Apr 25, 2023

View reviewed changes

mtreinish added this pull request to the merge queue Apr 25, 2023

Merged via the queue into Qiskit:main with commit 117d188 Apr 25, 2023

mergify bot mentioned this pull request Apr 25, 2023

Fix delay padding to respect target's constraints (backport #10007) #10038

Merged

alexanderivrii mentioned this pull request Jun 27, 2023

ConstrainedReschedule raises an error with reset #10354

Closed

mtreinish mentioned this pull request Jul 27, 2023

Prepare 0.25.0 release #10497

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix delay padding to respect target's constraints #10007

Fix delay padding to respect target's constraints #10007

itoko commented Apr 21, 2023 •

edited

Loading

qiskit-bot commented Apr 21, 2023

itoko left a comment

itoko Apr 21, 2023

nkanazawa1989 Apr 21, 2023

itoko Apr 21, 2023

mtreinish Apr 21, 2023

coveralls commented Apr 21, 2023 •

edited

Loading

mtreinish left a comment

mtreinish Apr 21, 2023

mtreinish Apr 21, 2023

mtreinish left a comment

Fix delay padding to respect target's constraints #10007

Fix delay padding to respect target's constraints #10007

Conversation

itoko commented Apr 21, 2023 • edited Loading

Summary

Details and comments

qiskit-bot commented Apr 21, 2023

itoko left a comment

Choose a reason for hiding this comment

itoko Apr 21, 2023

Choose a reason for hiding this comment

nkanazawa1989 Apr 21, 2023

Choose a reason for hiding this comment

itoko Apr 21, 2023

Choose a reason for hiding this comment

mtreinish Apr 21, 2023

Choose a reason for hiding this comment

coveralls commented Apr 21, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4772053053

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Apr 21, 2023

Choose a reason for hiding this comment

mtreinish Apr 21, 2023

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

itoko commented Apr 21, 2023 •

edited

Loading

coveralls commented Apr 21, 2023 •

edited

Loading