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

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

Merged
merged 13 commits into from
Mar 31, 2022

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Mar 29, 2022

Summary

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 new code path, but it is a breaking change
and should have been avoided.

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 classes from before the breaking changes 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.

Details and comments

@mtreinish mtreinish added priority: high Changelog: None Do not include in changelog labels Mar 29, 2022
@mtreinish mtreinish added this to the 0.20 milestone Mar 29, 2022
@mtreinish mtreinish requested a review from a team as a code owner March 29, 2022 21:13
@mtreinish mtreinish force-pushed the fix-deprecation-align-measures branch from 9ed6b1f to b86ceb5 Compare March 29, 2022 21:20
Comment on lines +15 to +16
from .asap import ASAPScheduleAnalysis
from .alap import ALAPScheduleAnalysis
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from .asap import ASAPScheduleAnalysis
from .alap import ALAPScheduleAnalysis
from .asap import ASAPSchedule, ASAPScheduleAnalysis
from .alap import ALAPSchedule, ALAPScheduleAnalysis

Should we preserve the existing exports of A{S,L}APSchedule?

Copy link
Member Author

Choose a reason for hiding this comment

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

They should be preserved in the parent directory of this onr. The directory structure is a bit weird now with qiskit/transpiler/passes/scheduling/scheduling containing just the new things, and qiskit/transpiler/passes/scheduling containing the old legacy classesm

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't have good naming for scheduling sub-group. Indeed the workflow now is scheduling -> alignment -> padding. Do you have any suggestion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with the new naming structure, except I might rename scheduling -> scheduling_analysis to make it clear that it's only setting the property set. I was more commenting that this gets a bit weird when add back the old path because we have two paths now scheduling and scheduling.scheduling that contains the new scheduling analysis passes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps timing_analysis then? scheduling analysis sounds bit weird to me. This makes the difference clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can change it, timing_analysis sounds good to me. But I'm also fine leaving it as scheduling. It's just a bit confusing for the next few releases while we deprecate and remove the legacy ALAPSchedule and ASAPSchedule

@coveralls
Copy link

coveralls commented Mar 29, 2022

Pull Request Test Coverage Report for Build 2072180035

  • 253 of 355 (71.27%) changed or added relevant lines in 16 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 83.696%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/scheduling/alap.py 58 60 96.67%
qiskit/transpiler/passes/scheduling/asap.py 62 64 96.88%
qiskit/transpiler/passes/scheduling/alignments/align_measures.py 68 72 94.44%
qiskit/transpiler/passes/scheduling/base_scheduler.py 26 31 83.87%
qiskit/transpiler/passes/scheduling/dynamical_decoupling.py 17 106 16.04%
Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 89.36%
Totals Coverage Status
Change from base Build 2072177507: -0.08%
Covered Lines: 53970
Relevant Lines: 64483

💛 - Coveralls

…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.
@mtreinish mtreinish force-pushed the fix-deprecation-align-measures branch from 9ceee85 to 1b0f0ba Compare March 30, 2022 13:08
from .scheduling import ALAPSchedule
from .scheduling import ASAPSchedule
from .scheduling import DynamicalDecouplingPadding
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be careful for this because some people are trying to use upgraded pass for their workshop. This change could break upgraded code so we need kind announce in community slack channel about how to use DD pass with recent systems and how they can migrate (I assumed there is no subclass of them and keep API consistent so that this doesn't become breaking "API" change).

If I understand correctly, DynamicalDecouplingPadding pass will be deprecated again once it replaces DynamicalDecoupling. However, this pass is not integrated into preset pass manager and user should integrate DynamicalDecouplingPadding into their experimental code. This could bother experimentalists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well we don't necessarily need to deprecate and rename DynamicalDecouplingPadding back to DynamicalDecoupling. We can just deprecate and remove DynamicalDecoupling and make DynamicalDecouplingPadding the new canonical name moving forward. I do think it's a bit uglier but it is a bit more descriptive too.

TBH, I wasn't thinking too much about the longer term plan here more that we should just avoid a breaking change here because anyone that was using DynamicalDecoupling today successfully (which there are even with the new alignment constraints on IBM backends it works elsewhere) would be broken by changing the underlying implementation to be a padding pass

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 Mar 30, 2022

Choose a reason for hiding this comment

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

Then the name should be PadDynamicalDecoupling or rename PadDelay to DelayPadding? I think both should be okey. Anyways I am not an active user of this pass, so I'd like to hear @ajavadia 's thought.

because anyone that was using DynamicalDecoupling today successfully

No one can use the pass successfully today. Anyways I agree if someone has custom DD pass this change will break their code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like PadDynamicalDecoupling that fits the new naming and is clearer to me than what I used. I'll rename it

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in: ecfa245

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm almost good with this PR. Just holding my approval until we hear some user's voice about the class name and future deprecation plan.

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think ASAPScheduleAnalysis and ALAPScheduleAnalysis and PadDynamicalDecoupling are fine, they are explicit about what they do.

qiskit/transpiler/passes/scheduling/base_scheduler.py Outdated Show resolved Hide resolved
Comment on lines +15 to +16
from .asap import ASAPScheduleAnalysis
from .alap import ALAPScheduleAnalysis
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't have good naming for scheduling sub-group. Indeed the workflow now is scheduling -> alignment -> padding. Do you have any suggestion?

mtreinish and others added 4 commits March 30, 2022 10:10
…4f89f95f3.yaml

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
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.
@1ucian0
Copy link
Member

1ucian0 commented Mar 31, 2022

Should #7762 be retagged as Changelog: None or is @qiskit-bot smart enough to handle reverts?

@mtreinish
Copy link
Member Author

mtreinish commented Mar 31, 2022

Qiskit bot isn't smart enough to do that, it just looks at the PRs for merged commits included in a release and checks the labels on the pr to know what to write in the changelog. That being said, this isn't a full revert #7762 it still adds a new feature this pr just undoes the deprecation and breaking changes. So we want the PRs tagged as new feature still.

@mergify mergify bot merged commit 859e3a1 into Qiskit:main Mar 31, 2022
@mtreinish mtreinish deleted the fix-deprecation-align-measures branch March 31, 2022 17:18
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Apr 4, 2022
In Qiskit#7835 we renamed the passes used in the new scheduler pass workflow
to differentiate them from the existing legacy scheduling pass workflow
(which we restored pending a future deprecation in that PR). However,
while we updated the docstring for the legacy path to update them to
point to the new passes, we neglected to update the docstrings for the
renamed classes to reflect the new names. The examples in the docstrings
still worked because of the old passes, but it wasn't showing an example
of how to use the new workflow anymore. This commit corrects this
oversight so that they actually explain how to use them.
mergify bot added a commit that referenced this pull request Apr 19, 2022
* Update docstrings for renamed scheduling passes

In #7835 we renamed the passes used in the new scheduler pass workflow
to differentiate them from the existing legacy scheduling pass workflow
(which we restored pending a future deprecation in that PR). However,
while we updated the docstring for the legacy path to update them to
point to the new passes, we neglected to update the docstrings for the
renamed classes to reflect the new names. The examples in the docstrings
still worked because of the old passes, but it wasn't showing an example
of how to use the new workflow anymore. This commit corrects this
oversight so that they actually explain how to use them.

* Add scheduling section to top level transpiler doc

* Update reference

* Fix lint

* Fix typos

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* Apply suggestions from code review

Co-authored-by: Kevin Hartman <kevin@hart.mn>

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: Kevin Hartman <kevin@hart.mn>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot pushed a commit that referenced this pull request Apr 19, 2022
* Update docstrings for renamed scheduling passes

In #7835 we renamed the passes used in the new scheduler pass workflow
to differentiate them from the existing legacy scheduling pass workflow
(which we restored pending a future deprecation in that PR). However,
while we updated the docstring for the legacy path to update them to
point to the new passes, we neglected to update the docstrings for the
renamed classes to reflect the new names. The examples in the docstrings
still worked because of the old passes, but it wasn't showing an example
of how to use the new workflow anymore. This commit corrects this
oversight so that they actually explain how to use them.

* Add scheduling section to top level transpiler doc

* Update reference

* Fix lint

* Fix typos

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* Apply suggestions from code review

Co-authored-by: Kevin Hartman <kevin@hart.mn>

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: Kevin Hartman <kevin@hart.mn>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit a4edc11)
mergify bot added a commit that referenced this pull request Apr 19, 2022
* Update docstrings for renamed scheduling passes

In #7835 we renamed the passes used in the new scheduler pass workflow
to differentiate them from the existing legacy scheduling pass workflow
(which we restored pending a future deprecation in that PR). However,
while we updated the docstring for the legacy path to update them to
point to the new passes, we neglected to update the docstrings for the
renamed classes to reflect the new names. The examples in the docstrings
still worked because of the old passes, but it wasn't showing an example
of how to use the new workflow anymore. This commit corrects this
oversight so that they actually explain how to use them.

* Add scheduling section to top level transpiler doc

* Update reference

* Fix lint

* Fix typos

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* Apply suggestions from code review

Co-authored-by: Kevin Hartman <kevin@hart.mn>

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: Kevin Hartman <kevin@hart.mn>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit a4edc11)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants