Update the deprecation decorator towards SemVer#11296
Conversation
|
One or more of the the following people are requested to review this:
|
1ucian0
added
the
type: qa
| pending: bool = False, | ||
| package_name: str = "qiskit", | ||
| removal_timeline: str = "no earlier than 3 months after the release date", | ||
| removal_timeline: str = "in the next major release", |
There was a problem hiding this comment.
This works fine, but we should have sufficient info to compute the next major version from the since field. We could set this to None and then do something like:
if removal_timeline is None:
removal_major = int(since.split(",")[0]) + 1
removal_timeline = f"in the next major release {removal_major}"
Or if you're worried about deprecated features passing a major version boundary (e.g. we deprecate something in 1.0 and don't remove it until 3.0) we can replace since with the package's __version__.
There was a problem hiding this comment.
good one! Done in cd47576
However, replacing since with __version__ might be tricky. If a features was deprecated in 1.1.0, we want to keep that since after the release of 1.2.0. Or am I missing something?
--------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Pull Request Test Coverage Report for Build 13702148612Details
💛 - Coveralls |
1ucian0
requested review from
ElePT,
ShellyGarion,
alexanderivrii,
eggerdj,
ikkoham,
jyu00,
nonhermitian,
t-imamichi and
wshanks
as code owners
|
@Mergifyio dequeue |
☑️ The pull request is not queued |
Done in 97a2740 |
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
This is arguably a violation of our deprecation policy because we didn't warn about the change in behavior in 1.x at all. Ideally we would have had a commit in 1.x that raised a FutureWarning when removal_timeline was None to tell people the default message would change in 2.0 but still return the 3 months message.
That being said we've already made similar mistakes in a couple of far more consequential places in the Qiskit API for 2.0 that I'm inclined to let this slip through with just a release note as a behavior change in 2.0. I left a small comment inline but otherwise I'm fine moving forward here.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Would it help if I do that in 1.4.1? |
|
Given how close we are to the rc1 tag and that this is still failing CI I'm going to defer this (to 3.0 I guess). We can prepare a deprecation in 2.1 though which will be good. |
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
I know I said that I was ok with this before. But as I was going to hit approve on this I had a crisis of conscience and just can't bring myself to hit approve; I'm really sorry. I'm still just very concerned about this as an api change. This PR is the exact thing the Qiskit deprecation policy says we're not supposed to do. Even though this is would go in a major version release where we're allowed to introducing breaking API changes, we're supposed to document these changes are coming and give a migration path for users so they can prepare. While I feel like this API should not be part of our public API, for better or worse it is and I really think we should follow our own rules here even if it's feels like a super minor part of the API and a very small change.
Why I"m having such a hard time on this PR is because it will silently change the behavior of the decorator to change the message from "no sooner than 6 months" to "in the next major release". While this makes sense given qiskit's stability policy, versioning, and release process we can't make that assumption about our downstream users' packeages. All the usage I could find of it on public github was qiskit-experiments, qiskit-nature, qiskit-qec, etc. and those do not have major versions and afaik don't plan on having them anytime soon/ever. While we can update those individual projects easily we can't know all of the users out there and this behavior change really might not fit with their current usage of the decorator. I'm worried that this change would not get noticed until too late because there is no warning about it coming (which is why we have deprecations)
What I would say we do here is that we should make this a new private function in 2.1 and deprecate all these public decorators for removal in 3.0. This will only delay the usability change you desire by a week or two (since we can migrate internal usage asap on main) and then we've achieved your goals here.
These functions are planned to be removed from the public Qiskit API in a future release (see Qiskit/qiskit#11296 (review)).
This change prepares for Qiskit 2.0 by guarding or replacing references to objects removed in Qiskit 2.0. ## Detailed list of changes: ### Compatibility changes: * Guard references to `BackendV1`. `BackendV1` was removed in Qiskit 2.0. There were only a few remaining references to `BackendV1` after the previous removal of pulse support, so it was not hard to keep the remaining support for now but it should be removed when Qiskit 1 is no longer supported. * Update usage of `qiskit.result.Result.header` to expect a dict (Qiskit 2) as well as a class (Qiskit 1). `ExperimentData` gets the circuit metadata from the header (by convention the metadata is copied into the result and `ExperimentData` relies on this). * Add classes `MeasLevel` and `MeasReturnType` to enumerate the expected measurement level (kerneled or classified) and return type (averaged or not). Previously, private classes of Qiskit were inadvertently used for these purposes but they were removed in Qiskit 2.0. * Remove null default `inst_map` transpile options from experiments. These should have been removed along with the rest of pulse support. * Update tests to use `GenericBackendV2` or a custom `BackendV2` in place of removed `FakeOpenPulse2Q` and `Fake5QV1` backends from Qiskit. * Remove suppression of Qiskit deprecation warnings from the tests. * Remove stray references to `ProviderV1` in tests. * Update the saved `QuantumVolume` results for Qiskit 2.0's change in the quantum volume algorithm. * Vendor deprecation decorators to prepare for their removal from Qiskit's public API (see [here](Qiskit/qiskit#11296 (review))). At first it appeared that these methods would be changed late in the Qiskit 2.0 cycle but the change was pushed back in time. Still the plan is to make the functions private, so we need to stop using them. ### Other changes * Provide a more specific warning in `LocalReadouMitigator` if it is used with a backend without a ``properties()`` method or with a backend that does not the required readout error properties. * Fix a performance issue in `QuantumVolume`. When Qiskit Aer is installed, `QuantumVolume` uses `AerSimulator` to simulate the outcomes of the quantum volume circuits to determine the heavy output probabilities. In its default instantiation, `AerSimulator` regenerates its `Target` every time it is accessed and this generation takes an appreciable amount of time. Because the target was being accessed separately for every circuit, this overhead could accumulate to over a minute in the standard 100 circuit experiment. This overhead was reduced by processing all the circuits in one pass. * Switch `FineFrequency` tests to use `T2HahnBackend` in place `MockIQBackend` since it provides a more realistic backend emulation. * Fix the `stacklevel` on the deprecation warning for `RestlessNode`. --------- Co-authored-by: Eli Arbel <arbel@il.ibm.com>
raynelfss
added
the
Changelog: None
|
I'll close based on Matt's comments above. |
🟠 Waiting for conditions to matchDetails
|
☑️ The pull request is not queued |
8 participants
The current
deprecatedecorators extend the docstring with the following:I suggest the following to be aligned with Qiskit/documentation#366
Other consequences:
since=parameter should be a string. Sometimes, we accidentally use floats for it which is not correct in the generic case. So, it is good to raise in those cases.