Add `since` argument to deprecation functions #9616

Eric-Arellano · 2023-02-17T20:06:09Z

Summary

Prework for #9611 so that we can add deprecation information to our generated Sphinx docs. The Sphinx .. deprecated:: directive expects a version argument, so we need structured metadata for this value.

Builds off of #8600 by @1ucian0

Details and comments

In a future change, I'd like to improve deprecated.py to automatically generate the deprecation message in a standardized way, like Nature does it. That would use this since= argument as part of the message.

The argument is optional to avoid breaking third-party users of deprecated.py. In a followup, we may want to deprecate leaving off since.

We may want to instead use the argument name version rather than since. But I personally find since more intuitive because version is unclear if it means the "removal" version vs. "start" version. We could use start_version or since_version for even more clarity?

qiskit-bot · 2023-02-17T20:06:12Z

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:

coveralls · 2023-02-17T20:36:00Z

Pull Request Test Coverage Report for Build 4255330145

20 of 20 (100.0%) changed or added relevant lines in 9 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.0009%) to 85.337%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	94.93%

Totals
Change from base Build 4252830169:	-0.0009%
Covered Lines:	67511
Relevant Lines:	79111

💛 - Coveralls

qiskit/utils/deprecation.py

levbishop · 2023-02-17T21:09:27Z

since seems intuitive enough. Is it worth having a corresponding optional argument to represent the expected removal revision in case its something other than the default by the deprecation policy? (occasionally we've committed to keeping deprecated functionality around a bit longer than the minimum).

woodsp-ibm · 2023-02-17T21:31:34Z

qiskit/algorithms/amplitude_amplifiers/grover.py

@@ -196,6 +196,7 @@ def __init__(
        "This property will be deprecated in a future release and subsequently "
        "removed after that.",
        category=PendingDeprecationWarning,


Not that it matters too much but all the algorithms were marked pending in 0.22. With the PR to change algorithms over from pending to fully deprecated, for the next release, I imagine this since would change as well (to 0.24.0) to reflect the change to the category. and be a base for that, rather than the version where things commenced with pending.

Ah, as in, I can set these pending ones to be since="0.24.0"? I like that!

It's better than my plan before to have the documentation plugin render the version as 0.23.0_pending. I think 0.24.0 is more clear than 0.23.0_pending. And it avoids the deprecation author from needing to change since=0.23.0 to since=0.24.0 once they switch from pending to deprecated.

I'm referring to #9532 of course. That will change things to deprecated such that its deprecated since 0.24.0. No docs should be published off any of this in the interim - so as long as the deprecated PR makes it in - have no reason to believe it won't though - things would be fine. It does make the change a kind of half-way house though - PendingDeprecation but with the version that the Deprecation is intended to commence at!

That makes sense. This makes me realize these semantics are better for pending deprecations:

If the deprecation is pending, set the version to when the deprecation will officially start.

That makes the .. deprecated :: directive have a more sensible version, e.g. we will show 0.24.0 rather than 0.23.0_pending. And less work for authors when switching Pending because the since version won't need to change at all.

I'm not sure that this is the best strategy - it could just be the responsibility of #9532 to update the version once it's certain. The deprecations of these algorithms were already pushed back one minor version, and while there's no particular indication it'll change again, I think we probably ought to build the policy around "since" referring to the warning that's actually being emitted, not one that we expect to emit in the future.

Eric-Arellano

Is it worth having a corresponding optional argument to represent the expected removal revision in case its something other than the default by the deprecation policy?

Possibly! That was really helpful for the Pantsbuild project's deprecations. But I don't think we need it for this PR. It would be useful for a followup to automatically generate standardized deprecation messages, which is outside the scope of this PR

Eric-Arellano · 2023-02-20T14:22:07Z

qiskit/algorithms/amplitude_amplifiers/grover.py

@@ -196,6 +196,7 @@ def __init__(
        "This property will be deprecated in a future release and subsequently "
        "removed after that.",
        category=PendingDeprecationWarning,


That makes sense. This makes me realize these semantics are better for pending deprecations:

If the deprecation is pending, set the version to when the deprecation will officially start.

That makes the .. deprecated :: directive have a more sensible version, e.g. we will show 0.24.0 rather than 0.23.0_pending. And less work for authors when switching Pending because the since version won't need to change at all.

jakelishman

Aside from the one comment about the version we're reporting in the algorithms, I'm fine with this.

Just to expand: there's a proposal somewhere in this chain to also include a "removal" field for the date set for that (I'm not 100% convinced that we can/should attempt that, but that discussion's not relevant here). If we wanted to include extra metadata on pending deprecations, I'd think it's probably best to do that with its own separate argument too, like having both "pending" and "since" - then we're not in the position of guessing when we'll actually move to fully deprecated and we can keep some complete history, assuming that's something that's useful to people. If the history of the pending component isn't useful, I'd personally still have "since" refer to when the current warning began being emitted, and just update it when the deprecation is promoted to full.

qiskit/visualization/state_visualization.py

jakelishman · 2023-02-20T16:44:04Z

qiskit/algorithms/amplitude_amplifiers/grover.py

@@ -196,6 +196,7 @@ def __init__(
        "This property will be deprecated in a future release and subsequently "
        "removed after that.",
        category=PendingDeprecationWarning,


I'm not sure that this is the best strategy - it could just be the responsibility of #9532 to update the version once it's certain. The deprecations of these algorithms were already pushed back one minor version, and while there's no particular indication it'll change again, I think we probably ought to build the policy around "since" referring to the warning that's actually being emitted, not one that we expect to emit in the future.

Eric-Arellano · 2023-02-20T17:16:07Z

like having both "pending" and "since" - then we're not in the position of guessing when we'll actually move to fully deprecated and we can keep some complete history, assuming that's something that's useful to people.

After something became deprecated, I don't think knowing when something was first pending is very useful. It's not going to influence whether end users migrate or not -- what matters the most to them is "when will this be removed, i.e. how much time do I have to procrastinate?"

If the history of the pending component isn't useful, I'd personally still have "since" refer to when the current warning began being emitted, and just update it when the deprecation is promoted to full.

Okay, that is how I originally had it. I will go back to that. Thanks for the feedback!

Eric-Arellano · 2023-02-20T20:40:55Z

While this is ready for approval, please do not set automerge until I have finished getting the Sphinx extension working. I want to reduce the risk of having to revert or change this PR.

Eric-Arellano · 2023-02-21T22:24:01Z

Alright, @1ucian0 and I have confirmed that the Sphinx extension will indeed work! This is ready to be merged.

Co-authored-by: Luciano Bello <lbello@gmail.com>

…official

… become official" This reverts commit dc30bba.

jakelishman

Thanks, Eric. Let's get this merged now, especially now that we've got the confirmation that the extension-based approach works in principle.

woodsp-ibm · 2023-02-24T14:41:26Z

This created a bunch of conflicts in #9532 where the replacement function done here has also lost the urls to the migration guide

jakelishman · 2023-02-24T15:15:27Z

That's unfortunate sorry. There were always going to be conflicts when we're improving something that's actively used - I imagine that PR isn't the only one. That PR is going beyond its scope in trying to new functionality as part of a simple change to a deprecation category, though, so some of the complexity could have been avoided if it weren't, and I think those changes need to be rolled back.

Eric and the rest of the docs team have been proposing such a change to centralising the message construction, and they'll lead that effort when it comes to it, including being the ones responsible for setting the template text and updating all uses across Terra, since they're effectively our communications department.

woodsp-ibm · 2023-02-24T15:20:41Z

No worries, Eric already offered to fix up the conflicts. It will be really good to have deprecation text come out automatically in the API ref.

* Add `since` argument to deprecation functions Co-authored-by: Luciano Bello <lbello@gmail.com> * Pending deprecations set version to when the deprecation will become official * Review feedback to `del` since argument * Revert "Pending deprecations set version to when the deprecation will become official" This reverts commit dc30bba. * Clarify how to handle pending deprecations --------- Co-authored-by: Luciano Bello <lbello@gmail.com>

* Add `since` argument to deprecation functions Co-authored-by: Luciano Bello <lbello@gmail.com> * Pending deprecations set version to when the deprecation will become official * Review feedback to `del` since argument * Revert "Pending deprecations set version to when the deprecation will become official" This reverts commit dc30bba99b36c5ef8ae1310936eb485a9f240d32. * Clarify how to handle pending deprecations --------- Co-authored-by: Luciano Bello <lbello@gmail.com>

Eric-Arellano marked this pull request as ready for review February 17, 2023 20:37

Eric-Arellano requested review from a team, ikkoham, t-imamichi, nonhermitian, eggerdj, wshanks, alexanderivrii, ShellyGarion, manoelmarques, woodsp-ibm and ElePT as code owners February 17, 2023 20:37

Eric-Arellano requested a review from 1ucian0 February 17, 2023 20:37

Eric-Arellano added the Changelog: None Do not include in changelog label Feb 17, 2023

levbishop reviewed Feb 17, 2023

View reviewed changes

qiskit/utils/deprecation.py Outdated Show resolved Hide resolved

qiskit/utils/deprecation.py Show resolved Hide resolved

woodsp-ibm reviewed Feb 17, 2023

View reviewed changes

Eric-Arellano commented Feb 20, 2023

View reviewed changes

jakelishman reviewed Feb 20, 2023

View reviewed changes

Eric-Arellano force-pushed the since-deprecation branch from f4deaa5 to 9636d6b Compare February 23, 2023 17:40

Eric-Arellano requested a review from jyu00 as a code owner February 23, 2023 17:40

Eric-Arellano and others added 4 commits February 23, 2023 11:40

Add since argument to deprecation functions

2c333f1

Co-authored-by: Luciano Bello <lbello@gmail.com>

Pending deprecations set version to when the deprecation will become …

d762da1

…official

Review feedback to del since argument

97b9e70

Revert "Pending deprecations set version to when the deprecation will…

a43e4a1

… become official" This reverts commit dc30bba.

Clarify how to handle pending deprecations

08a07a7

Eric-Arellano force-pushed the since-deprecation branch from 9636d6b to 08a07a7 Compare February 23, 2023 17:40

jakelishman approved these changes Feb 24, 2023

View reviewed changes

jakelishman added the automerge label Feb 24, 2023

mergify bot merged commit d3b1bd2 into Qiskit:main Feb 24, 2023

jakelishman mentioned this pull request Feb 24, 2023

[WAIT TO MERGE] Store deprecation metadata on functions for docs generation #9611

Closed

Eric-Arellano deleted the since-deprecation branch February 24, 2023 14:10

woodsp-ibm mentioned this pull request Feb 24, 2023

Change algorithms from pending deprecate to deprecate #9532

Closed

1ucian0 mentioned this pull request Feb 27, 2023

Deprecate warning box in docs #8600

Closed

This was referenced May 3, 2023

Update QPY to match Terra 0.24 Qiskit/qiskit-ibm-runtime#821

Merged

Update QPY to match Terra 0.24 Qiskit/qiskit-ibm-provider#606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `since` argument to deprecation functions #9616

Add `since` argument to deprecation functions #9616

Eric-Arellano commented Feb 17, 2023 •

edited

qiskit-bot commented Feb 17, 2023

coveralls commented Feb 17, 2023 •

edited

levbishop commented Feb 17, 2023

woodsp-ibm Feb 17, 2023

Eric-Arellano Feb 17, 2023 •

edited

woodsp-ibm Feb 17, 2023

Eric-Arellano Feb 20, 2023

jakelishman Feb 20, 2023

Eric-Arellano left a comment

Eric-Arellano Feb 20, 2023

jakelishman left a comment

jakelishman Feb 20, 2023

Eric-Arellano commented Feb 20, 2023

Eric-Arellano commented Feb 20, 2023

Eric-Arellano commented Feb 21, 2023

jakelishman left a comment

woodsp-ibm commented Feb 24, 2023

jakelishman commented Feb 24, 2023

woodsp-ibm commented Feb 24, 2023

Add since argument to deprecation functions #9616

Add since argument to deprecation functions #9616

Conversation

Eric-Arellano commented Feb 17, 2023 • edited

Summary

Details and comments

qiskit-bot commented Feb 17, 2023

coveralls commented Feb 17, 2023 • edited

Pull Request Test Coverage Report for Build 4255330145

💛 - Coveralls

levbishop commented Feb 17, 2023

woodsp-ibm Feb 17, 2023

Choose a reason for hiding this comment

Eric-Arellano Feb 17, 2023 • edited

Choose a reason for hiding this comment

woodsp-ibm Feb 17, 2023

Choose a reason for hiding this comment

Eric-Arellano Feb 20, 2023

Choose a reason for hiding this comment

jakelishman Feb 20, 2023

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Feb 20, 2023

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Feb 20, 2023

Choose a reason for hiding this comment

Eric-Arellano commented Feb 20, 2023

Eric-Arellano commented Feb 20, 2023

Eric-Arellano commented Feb 21, 2023

jakelishman left a comment

Choose a reason for hiding this comment

woodsp-ibm commented Feb 24, 2023

jakelishman commented Feb 24, 2023

woodsp-ibm commented Feb 24, 2023

Add `since` argument to deprecation functions #9616

Add `since` argument to deprecation functions #9616

Eric-Arellano commented Feb 17, 2023 •

edited

coveralls commented Feb 17, 2023 •

edited

Eric-Arellano Feb 17, 2023 •

edited