Replace AlignMeasures with ConstrainedReschedule for pulse alignment by nkanazawa1989 · Pull Request #7762 · Qiskit/qiskit

nkanazawa1989 · 2022-03-10T19:55:03Z

Summary

This PR introduces new pass ConstrainedReschedule (name should be discussed) which is a drop-in-replacement of AlignMeasures pass. New pass takes both pulse and acquire alignment constraints thus it guarantees your circuit is executable on recent IBM Quantum backend with non-"1" pulse alignment constraint.

Details and comments

ConstrainedReschedule is implemented as AnalysisPass, which updates node_start_time dictionary in the property set. This is follow-up of #7709 and necessary for #7760 . Note that this pass doesn't regenerate DAG instance, thus performance of scheduling chain will be improved. The pass ordering of preset pass managers has been also updated.

old, at #7709:
scheduling (analysis) -> padding (transform) -> alignment (transform)

new:
scheduling (analysis) -> alignment (analysis) -> padding (transform)

TODO

write release note
update unittests

New pass considers both pulse and acquire alignment constraints, thus it is a drop-in-replacement of the AlignMeasures. Note that ordering of passes in the preset pass managers is updated because new pass is implemented as analysis pass, that only updates `node_start_time` in the property set, thus it should be executed before the padding passes.

ajavadia

looks good. needs some tests

Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>

ajavadia

the PR looks good to me now. but it contains no new tests. is everything here covered by previous tests?

…verlap in rescheduler.

…989/qiskit-terra into upgrade/alignment-to-analysis

…y them to circuit attributes. Schedulign passes are combined in a single ``scheduling`` folder.

coveralls · 2022-03-18T04:29:44Z

Pull Request Test Coverage Report for Build 2024560948

239 of 262 (91.22%) changed or added relevant lines in 21 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 83.53%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/scheduling/alignments/pulse_gate_validation.py	21	22	95.45%
qiskit/transpiler/basepasses.py	2	4	50.0%
qiskit/transpiler/preset_passmanagers/level0.py	18	20	90.0%
qiskit/transpiler/preset_passmanagers/level1.py	18	20	90.0%
qiskit/transpiler/preset_passmanagers/level2.py	18	20	90.0%
qiskit/transpiler/preset_passmanagers/level3.py	18	20	90.0%
qiskit/transpiler/passes/scheduling/alignments/reschedule.py	73	77	94.81%
qiskit/transpiler/passes/scheduling/alignments/check_durations.py	16	24	66.67%

Totals
Change from base Build 2024516747:	0.01%
Covered Lines:	52607
Relevant Lines:	62980

💛 - Coveralls

nkanazawa1989 · 2022-03-18T05:13:59Z

Now unittests are updated and the PR is ready for final review. I noticed reschedule pass also needs classical I/O latency information to compute correct node start/end time of instructions acting on classical registers. These are already covered by existing test case in test.python.transpiler.test_instruction_alignment.

This makes me think adding latencies in the property set is better approach, rather than setting values in the constructor of every pass. Constructor values tend to have poor documentation (because developer just copies value without doc, or sometimes forget to update all docs synchronously, etc...) and to make pass API unstable since we didn't decide spec for these values. Instead, I introduced SetIOLatency analysis pass (we just need to update this pass after we get spec of values in future), that just copies values to property set, and one must call this pass before running scheduling passes (otherwise latencies default to zero). The IO latency information is also copied to output circuit to be used by timeline drawer.

ajavadia

looks good to go basically, just a small question about how SetIOLatency gets called.

ajavadia · 2022-03-22T13:16:22Z

+        pm0.append(
+            InstructionDurationCheck(
+                acquire_alignment=timing_constraints.acquire_alignment,
+                pulse_alignment=timing_constraints.pulse_alignment,
+            )
+        )
+        pm0.append(
+            ConstrainedReschedule(
+                acquire_alignment=timing_constraints.acquire_alignment,
+                pulse_alignment=timing_constraints.pulse_alignment,
+            ),
+            condition=_require_alignment,
+        )
+        pm0.append(
+            ValidatePulseGates(
+                granularity=timing_constraints.granularity,
+                min_length=timing_constraints.min_length,
+            )
+        )


Shouldn't SetIOLatency be part of preset passmanagers at levels 0-3?

It should be but currently the backend data fields are both empty. We don't have any spec for values, i.e. they might be qubit dependent, or a single-value per system, or something else. We can come back to this once we define the format. We can minimize the impact of this unstable API since everything is internal code now.

ajavadia · 2022-03-22T13:17:21Z

+            [
+                SetIOLatency(clbit_write_latency=1000),
+                ASAPSchedule(durations),
+                PadDelay(),
+            ]


Alternatively you could add SetIOLatency to the self.requires of ASAPSchedule and ALAPSchedule which guarantees that the i/o information is available to them.

That sounds like a good plan. I'll keep this in mind.

ajavadia

ok this looks good to me. there is a merge conflict right now but i'm good to merge after that is fixed.

…ignment-to-analysis

nkanazawa1989 · 2022-03-22T19:38:22Z

Sorry seems like I messed up CLA. I think I've fixed the wrong commit. The last commit is to move paddings to own module as what I've done for other two modules, since we have merged #7760 .

…asses In Qiskit#7762 the scheduling passes in the transpiler were completely rewritten to operate in a model where scheduling and alignment happen as metadata on the circuit until a single padding pass is called to adapt the circuit to fit the scheduling metadata. As part of that rework a new pass ConstrainedReschedule was added to perform the alignment for measurement as well as all instructions via this new mechanism. However the way the migration was done was a breaking change and should have been additive so that we can deprecate the old path in 0.21 and allow users time to migrate to the new approach. Additionally as part of Qiskit#7762 the AlignMeasures pass was deprecated and its implementation was replaced with just returning an equivalent ConstraintedReschedule pass. This was problematic for two reasons though, first the two passes were not equivalent and this a breaking change and second it violates the deprecation policy. For the breaking change the AlignMeasures and ConstrainedReschedule while performing the same function do it in a different manner. I assume the deprecation was done this way because AlignMeasures is incompatible with the While the scheduling pass changes were also breaking to do the migration gracefully would require duplicating their functionality and deprecating the old This is also violating the deprecation policy which is defined here: https://qiskit.org/documentation/contributing_to_qiskit.html#deprecation-policy as the alternative wasn't available for a release prior to the start of the deprecation (see 2a in the above link). This commit adds back the breaking classes and renames the new scheduling classes ALAPScheduleAnalysis and ASAPScheduleAnalysis to differentiate them from the previous implementations. This also reverts the deprecation and changes to the AlignMeasures pass until we can do it correctly at the appropriate time. The class is restored to its previous state but instead a PendingDeprecationWarning is now emitted to warn users of the pending deprecation and removal of the class. Additionally, a warning in the release notes will be added as part of Qiskit#7828 to document the incompatibility with the new behavior of the scheduling and alignment passes.

…asses (#7835) * Revert deprecation and breaking changes of scheduling and alignment passes In #7762 the scheduling passes in the transpiler were completely rewritten to operate in a model where scheduling and alignment happen as metadata on the circuit until a single padding pass is called to adapt the circuit to fit the scheduling metadata. As part of that rework a new pass ConstrainedReschedule was added to perform the alignment for measurement as well as all instructions via this new mechanism. However the way the migration was done was a breaking change and should have been additive so that we can deprecate the old path in 0.21 and allow users time to migrate to the new approach. Additionally as part of #7762 the AlignMeasures pass was deprecated and its implementation was replaced with just returning an equivalent ConstraintedReschedule pass. This was problematic for two reasons though, first the two passes were not equivalent and this a breaking change and second it violates the deprecation policy. For the breaking change the AlignMeasures and ConstrainedReschedule while performing the same function do it in a different manner. I assume the deprecation was done this way because AlignMeasures is incompatible with the While the scheduling pass changes were also breaking to do the migration gracefully would require duplicating their functionality and deprecating the old This is also violating the deprecation policy which is defined here: https://qiskit.org/documentation/contributing_to_qiskit.html#deprecation-policy as the alternative wasn't available for a release prior to the start of the deprecation (see 2a in the above link). This commit adds back the breaking classes and renames the new scheduling classes ALAPScheduleAnalysis and ASAPScheduleAnalysis to differentiate them from the previous implementations. This also reverts the deprecation and changes to the AlignMeasures pass until we can do it correctly at the appropriate time. The class is restored to its previous state but instead a PendingDeprecationWarning is now emitted to warn users of the pending deprecation and removal of the class. Additionally, a warning in the release notes will be added as part of #7828 to document the incompatibility with the new behavior of the scheduling and alignment passes. * More release note fixes * Fix lint * Fix docs * Fix doc whitespace * Update dynamical decoupling release note * Copy docstrings from pre-refactor to fix docs build * Add back test coverage for legacy scheduling and alignment passes * Update releasenotes/notes/update-instruction-alignment-passes-ef0f20d4f89f95f3.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Fix release notes * Rename base scheduler class in legacy path This commit renames the BaseScheduler class in the legacy scheduling path to BaseSchedulerTransform to differentiate it from the new path scheduling paths. It also documents the pending deprecation in all the legacy scheduling passes to point people to the new scheduling workflow. * Rename DynamicalDecouplingPadding to PadDynamicalDecoupling Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

nkanazawa1989 requested a review from a team as a code owner March 10, 2022 19:55

ajavadia reviewed Mar 12, 2022

View reviewed changes

wording edits

c0dcd43

Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>

nkanazawa1989 force-pushed the upgrade/alignment-to-analysis branch from 4b6af59 to c0dcd43 Compare March 14, 2022 02:36

nkanazawa1989 added 3 commits March 14, 2022 12:46

split files and separate duration validation from reschedule

2e542ae

lint

a1c5847

add reno

ef8b1c1

ajavadia reviewed Mar 14, 2022

View reviewed changes

Comment thread qiskit/transpiler/passes/scheduling/alignments/check_durations.py Outdated

ajavadia reviewed Mar 14, 2022

View reviewed changes

nkanazawa1989 added 7 commits March 17, 2022 14:07

add reno

2c61295

fix unittests wip

bbba0d7

update property set initialization

70ec595

move latencies to propery set and separately consider qreg and creg o…

a7106a8

…verlap in rescheduler.

Merge branch 'upgrade/alignment-to-analysis' of github.com:nkanazawa1…

ca27872

…989/qiskit-terra into upgrade/alignment-to-analysis

Add set io latency pass to set creg latencies to property set and cop…

48e6398

…y them to circuit attributes. Schedulign passes are combined in a single ``scheduling`` folder.

alignment bug fix

3462b1a

nkanazawa1989 changed the title ~~[WIP] Replace AlignMeasures with ConstrainedReschedule for pulse alignment~~ Replace AlignMeasures with ConstrainedReschedule for pulse alignment Mar 18, 2022

nkanazawa1989 force-pushed the upgrade/alignment-to-analysis branch from 4d99832 to 40900b1 Compare March 18, 2022 05:49

update unittests

6d8dc28

nkanazawa1989 force-pushed the upgrade/alignment-to-analysis branch from 40900b1 to 6d8dc28 Compare March 18, 2022 06:48

ajavadia reviewed Mar 22, 2022

View reviewed changes

ajavadia previously approved these changes Mar 22, 2022

View reviewed changes

nkanazawa1989 dismissed ajavadia’s stale review via d9a5de8 March 22, 2022 17:24

nkanazawa1989 force-pushed the upgrade/alignment-to-analysis branch 2 times, most recently from 5a4a4b6 to 6d8dc28 Compare March 22, 2022 18:33

Qiskit deleted a comment from CLAassistant Mar 22, 2022

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

5dba1e0

…ignment-to-analysis

move padding to own module as well

f3da42f

Merge branch 'main' into upgrade/alignment-to-analysis

5cf21e4

ajavadia approved these changes Mar 23, 2022

View reviewed changes

ajavadia added the automerge label Mar 23, 2022

Merge branch 'main' into upgrade/alignment-to-analysis

0dd89cf

mergify Bot merged commit 3476ef0 into Qiskit:main Mar 23, 2022

mtreinish mentioned this pull request Mar 29, 2022

Revert deprecation and breaking changes of scheduling and alignment passes #7835

Merged

jakelishman added Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog. Changelog: Added Add an "Added" entry in the GitHub Release changelog. Changelog: Changed Add a "Changed" entry in the GitHub Release changelog. labels Mar 30, 2022

mtreinish removed Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog. Changelog: Changed Add a "Changed" entry in the GitHub Release changelog. labels Mar 30, 2022

nkanazawa1989 deleted the upgrade/alignment-to-analysis branch November 25, 2022 02:52

Conversation

nkanazawa1989 commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

ajavadia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajavadia left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2024560948

💛 - Coveralls

Uh oh!

nkanazawa1989 commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajavadia left a comment

Choose a reason for hiding this comment

Uh oh!

ajavadia Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

ajavadia Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

ajavadia left a comment

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nkanazawa1989 commented Mar 10, 2022 •

edited

Loading

coveralls commented Mar 18, 2022 •

edited

Loading

nkanazawa1989 commented Mar 18, 2022 •

edited

Loading