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.

Sign up for GitHub

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

Jump to bottom

Remove ParametrizedSchedule #6949

Merged
merged 9 commits into from
Jan 18, 2022
Merged

Remove ParametrizedSchedule #6949

merged 9 commits into from
Jan 18, 2022

Conversation

nkanazawa1989
Copy link
Contributor

Summary

This PR removes deprecated ParametrizedSchedule. This has been deprecated in 0.17.0 with #5358 and seems no official tutorial has dependency on it.

Details and comments

@jakelishman
Copy link
Member

I see it being used in Ignis in qiskit/ignis/characterization/calibrations/ibmq_utils.py - probably we can't remove it until Ignis is officially gone, or its behaviour is changed. See qiskit-community/qiskit-ignis#292 (though there's not really much comment on the implementation).

@nkanazawa1989
Copy link
Contributor Author

Thanks for finding it. Totally had forgotten. I'll update ignis accordingly.

@nkanazawa1989 nkanazawa1989 mentioned this pull request Aug 26, 2021
Merged
@nkanazawa1989
Merge branch 'main' of github.com:Qiskit/qiskit-terra into remove_par…
d392d95 
…ametrized_schedule

# Conflicts:
#	qiskit/pulse/instruction_schedule_map.py
@kdk kdk added the on hold Can not fix yet label Aug 27, 2021
mtreinish pushed a commit to qiskit-community/qiskit-ignis that referenced this pull request Sep 26, 2021
@nkanazawa1989
Remove ParametrizedSchedule dependency (#592)
9cd8ad6 
Remove dependency on ParametrizedSchedule as per Qiskit/qiskit#6949

* remove ParametrizedSchedule dependency

* add reno

* lint

* bump terra version
@nkanazawa1989
Copy link
Contributor Author

The parametrized schedule dependency has been removed from ignis. This is ready to go in (given ignis release is faster than terra).

@jakelishman
Copy link
Member

We're still on hold in Terra until a new version of Ignis is actually released, but once that happens, we'll be good to go here. We should be able to align things so that this happens either before, or in conjunction with, Terra 0.19.

Just for linking purposes: downstream use was removed in qiskit-community/qiskit-ignis#592.

@jakelishman jakelishman added this to the 0.19 milestone Sep 27, 2021
jakelishman
jakelishman previously approved these changes Sep 27, 2021
View reviewed changes
@jakelishman jakelishman added API affects user-facing API change Changelog: API Change Include in the "Changed" section of the changelog and removed API affects user-facing API change labels Sep 27, 2021
@mtreinish mtreinish added Changelog: Removal Include in the Removed section of the changelog and removed Changelog: API Change Include in the "Changed" section of the changelog labels Sep 27, 2021
@mtreinish
Copy link
Member

Just a heads up the ignis release is blocked on terra 0.19.0 because ignis 0.6.0 will start emitting deprecation warnings (which we can't do until #6867 merges and is released).

@mtreinish mtreinish modified the milestones: 0.19, 0.20 Sep 27, 2021
@jakelishman
Copy link
Member

@mtreinish: can we align to get Terra 0.19 and Ignis 0.6 released at the same time?

@mtreinish
Copy link
Member

@mtreinish: can we align to get Terra 0.19 and Ignis 0.6 released at the same time?

They'll be at the same time. That metapackage release will mark it as deprecated (and also remove aqua) at the same time we bump terra. But there is the tutorials CI issue (where this is failing). We install ignis from pypi in the tutorials job to validate backwards compatibility with existing releases (so there is always an n -> n + 1 upgrade path for users) this will block us until ignis releases which we can't do until terra 0.19.0 releases.

If this is important to remove for 0.19.0 when the release is close we can manually change ci here to install ignis from git to unblock it from merging right before the release date (to minimize our exposure to threading the needle and accidentally introducing an unintentional breaking change) and then merge this for inclusion in 0.19.0. Then after the 0.19.0 release we change it back to using the pypi version. We've used this pattern a couple of times in the past where there were things that got stuck and there was no way to maintain the upgrade path (mostly with aqua which was using internals of terra)

@jakelishman
Copy link
Member

I don't think there's any rush to get this in for 0.19 - it's just a removal of deprecated functionality, after all. That's fair - I hadn't considered the tutorials' CI in managing the warnings before release.

@nkanazawa1989 nkanazawa1989 reopened this Jan 18, 2022
@nkanazawa1989 nkanazawa1989 removed the on hold Can not fix yet label Jan 18, 2022
@nkanazawa1989
Copy link
Contributor Author

I think this PR can be merged now.

@coveralls
Copy link

coveralls commented Jan 18, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 1712165941

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.002%) to 83.141%
Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 89.36%
Totals Coverage Status
Change from base Build 1700054446: -0.002%
Covered Lines: 51982
Relevant Lines: 62523
💛 - Coveralls

@mergify mergify bot merged commit 076fc36 into Qiskit:main Jan 18, 2022
@nkanazawa1989 nkanazawa1989 deleted the remove_parametrized_schedule branch November 25, 2022 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman approved these changes

@DanPuzzuoli DanPuzzuoli Awaiting requested review from DanPuzzuoli

@eggerdj eggerdj Awaiting requested review from eggerdj eggerdj is a code owner

@lcapelluto lcapelluto Awaiting requested review from lcapelluto

@taalexander taalexander Awaiting requested review from taalexander

Assignees
No one assigned
Labels
Changelog: Removal Include in the Removed section of the changelog
Projects
None yet
Milestone
0.20
Development

Successfully merging this pull request may close these issues.
5 participants
@nkanazawa1989 @jakelishman @mtreinish @coveralls @kdk