0.6 release notes and deprecation policy #1385
Conversation
CONTRIBUTING.md
Outdated
| Qiskit Experiments's deprecation policy is based on Qiskit's [Deprecation | ||
| Policy](https://github.com/Qiskit/qiskit/blob/45e42525224e9e1857300e7e5529273fe619c9e4/DEPRECATION.md) | ||
| prior to its 1.0 release. Public-facing changes must come with a deprecation warning | ||
| for at least three months or two version cycles before the old feature is removed. | ||
| Deprecations can only happen on minor releases and not on patch releases. | ||
|
|
||
| The timeline for deprecating an existing feature is as follows: | ||
|
|
||
| * Minor release 1: An alternative path is provided. A `PendingDeprecationWarning` should be issued | ||
| when the old path is used, indicating to users how to switch to the new path and the release | ||
| in which the old path will no longer be available. | ||
| * Minor release 2: The `PendingDeprecationWarning` becomes a `DeprecationWarning`. The release note | ||
| should indicate the feature has been deprecated and how to switch to the new path. | ||
| * Minor release 3: The old feature is removed. The release note should indicate that the feature has | ||
| been removed and how to switch to the new path. |
There was a problem hiding this comment.
@wshanks I'm linking to the commit where DEPRECATION.md was added to Qiskit. Not sure if there's a better link.
What do you think about this timeline? It's faster than the Qiskit policy, which says the old feature cannot be removed in the minor release immediately after the warnings, but that would be far too long for us. The other thing is we don't use PendingDeprecationWarning that much right now, and it creates more code churn than just starting with a DeprecationWarning and keeping that for two cycles.
There was a problem hiding this comment.
That link seems okay. A cleaner link might be the rc1 tag: https://github.com/Qiskit/qiskit/blob/1.0.0rc1/DEPRECATION.md That's a little misleading if the file gets changed before 1.0.0, but the file isn't in the 0.46 release branch. It could probably be backported.
Three cycles seems like too much for this package. I like the pending deprecation warnings, but they may be too much overhead for us to manage right now. Perhaps we could use deprecation helper functions that check the current version and automatically determine the warning type by comparing to the since argument. The goal with pending is to give downstream libraries a chance to test for and fix pending deprecations before they become deprecations since those print out to users. I don't know that we have many downstream consumers that are not end users. It makes most sense for library consumers in between the package emitting the warning and end users.
We have also discussed reducing the deprecation cycle to a single version for features outside of the framework.
There was a problem hiding this comment.
Thanks, updated the link. I broke the changes into three types in 553f502 and made specifying pending warnings optional.
| This leverages a custom JSON encoder and decoder to serialize | ||
| the entire calibration data including user provided schedule templates. | ||
| Output JSON data is formatted into the standard data model which is intentionally | ||
| agnostic to the calibration data structure, and this allows community | ||
| agnostic to the calibration data structure, which this allows community | ||
| developer to reuse the calibration data in their platform. | ||
| See :mod:`qiskit_experiments.calibration_management.save_utils` for data models. |
There was a problem hiding this comment.
@nkanazawa1989 Should calibration_management.save_utils be exposed in the API docs? It seems to be internal-only. If so, we should remove the mention here.
There was a problem hiding this comment.
It says at the top of the file that it is not intended to be a stable user-facing API. It has to be stable to whatever commitment we make to loading old data though. The main question is should we document the format at all or expect users who care to look into the Python code themselves any way.
I would say just remove the reference here, but the previous sentence doesn't make sense if the model is not documented. I think I would just remove the previous sentence as well because we don't need to commit to supporting a calibration storage format. Others can still use it if they want to. What do you think @nkanazawa1989?
There was a problem hiding this comment.
I'm fine with removing the sentence.
There was a problem hiding this comment.
Done in b57deb0.
There was a problem hiding this comment.
I would remove part or all of the previous sentence as well. Since the format is not documented, we don't really support users using it outside of the package (though they can try if they want).
0.3.1 release notes were added retroactively, so they must be added to an existing note file created during the correct release
…eriments into 0.6-release-notes
CONTRIBUTING.md
Outdated
| Qiskit Experiments's deprecation policy is based on Qiskit's [Deprecation | ||
| Policy](https://github.com/Qiskit/qiskit/blob/45e42525224e9e1857300e7e5529273fe619c9e4/DEPRECATION.md) | ||
| prior to its 1.0 release. Public-facing changes must come with a deprecation warning | ||
| for at least three months or two version cycles before the old feature is removed. | ||
| Deprecations can only happen on minor releases and not on patch releases. | ||
|
|
||
| The timeline for deprecating an existing feature is as follows: | ||
|
|
||
| * Minor release 1: An alternative path is provided. A `PendingDeprecationWarning` should be issued | ||
| when the old path is used, indicating to users how to switch to the new path and the release | ||
| in which the old path will no longer be available. | ||
| * Minor release 2: The `PendingDeprecationWarning` becomes a `DeprecationWarning`. The release note | ||
| should indicate the feature has been deprecated and how to switch to the new path. | ||
| * Minor release 3: The old feature is removed. The release note should indicate that the feature has | ||
| been removed and how to switch to the new path. |
There was a problem hiding this comment.
That link seems okay. A cleaner link might be the rc1 tag: https://github.com/Qiskit/qiskit/blob/1.0.0rc1/DEPRECATION.md That's a little misleading if the file gets changed before 1.0.0, but the file isn't in the 0.46 release branch. It could probably be backported.
Three cycles seems like too much for this package. I like the pending deprecation warnings, but they may be too much overhead for us to manage right now. Perhaps we could use deprecation helper functions that check the current version and automatically determine the warning type by comparing to the since argument. The goal with pending is to give downstream libraries a chance to test for and fix pending deprecations before they become deprecations since those print out to users. I don't know that we have many downstream consumers that are not end users. It makes most sense for library consumers in between the package emitting the warning and end users.
We have also discussed reducing the deprecation cycle to a single version for features outside of the framework.
releasenotes/notes/0.6/feature-support-calibrations-roundtrip-47f09bd9ff803479.yaml
Outdated
Show resolved
Hide resolved
| This leverages a custom JSON encoder and decoder to serialize | ||
| the entire calibration data including user provided schedule templates. | ||
| Output JSON data is formatted into the standard data model which is intentionally | ||
| agnostic to the calibration data structure, and this allows community | ||
| agnostic to the calibration data structure, which this allows community | ||
| developer to reuse the calibration data in their platform. | ||
| See :mod:`qiskit_experiments.calibration_management.save_utils` for data models. |
There was a problem hiding this comment.
It says at the top of the file that it is not intended to be a stable user-facing API. It has to be stable to whatever commitment we make to loading old data though. The main question is should we document the format at all or expect users who care to look into the Python code themselves any way.
I would say just remove the reference here, but the previous sentence doesn't make sense if the model is not documented. I think I would just remove the previous sentence as well because we don't need to commit to supporting a calibration storage format. Others can still use it if they want to. What do you think @nkanazawa1989?
Co-authored-by: Will Shanks <wshaos@posteo.net>
There was a problem hiding this comment.
These look good to me.
Some minor comments from reading through the rendered notes:
- We could link to qiskit-ibm-runtime and qiskit-ibm-provider when mentioning them in the prelude.
- There are a couple classes not referenced with Sphinx like ParallelExperiment and BatchExperiment.
- It would be nice to group the notes so that the experiment data changes, the plotting changes, etc are all together but I think that is hard to do. It would also be nice if the changes highlighted in the prelude were near the top of the notes.
|
@wshanks Added subsections and pointed provider mentions to their module's intersphinx entries. Subsections aren't in a reno stable release yet so I pinned reno to the latest git commit. We could arguably have a composite experiments section separate from the base experiment class, but maybe there are already a lot of sections. I also had to add a new features bullet point just so that the |
| - [features_expupdate, Experiment Updates, 2] | ||
| - [features_expclass, Experiment Class Features, 2] | ||
| - [features_analysis, Analysis Class Features, 2] | ||
| - [features_expdata, Experiment Data Features, 2] |
There was a problem hiding this comment.
I tested with #1342 merged in. Just a note that its release note needs to be updated to go under a section after it is merged (assuming it merges before this PR).
There was a problem hiding this comment.
Will do. I have to move the note to the 0.6/ folder too so this PR should be the last one merged before release.
| - [features_expdata, Experiment Data Features, 2] | ||
| - [features_curvefit, Curve Fit Features, 2] | ||
| - [features_calibration, Calibration Features, 2] | ||
| - [features_visualization, Visualization Features, 2] |
There was a problem hiding this comment.
I like the sections. Do you think it is too much to add some for upgrade and fixes? When the list gets longer than a page, I find it gets overwhelming to read through.
I think the categories you used for features also work but there could be one more category for packaging upgrades (Qiskit 1.0, providers, Python versions). Maybe those could go under "other"? The deprecated methods and options list might be split out into different sections if we had them (analysis, experiments, experiment class). If we did all those, maybe we would group deprecation notes as well though that list is fairly short.
There was a problem hiding this comment.
Added in 5df6090 along with a Package Upgrades subsection. I ended up putting the removal notes in that subsection and keeping deprecation notes in its own section since I like them grouped rather than separated into sections, but I don't feel that strongly about it.
| Any change to the existing package code that affects how the user interacts with the package | ||
| should give the user clear instructions and advanced warning if the change is nontrivial. | ||
| Qiskit Experiments's deprecation policy is based on [Qiskit's | ||
| policy](https://github.com/Qiskit/qiskit/blob/1.0.0rc1/DEPRECATION.md) prior to its 1.0 release, but | ||
| we impose less stringent requirements such that developers can iterate more quickly. | ||
| Deprecations and feature removals can only happen on minor releases and not on patch releases. | ||
|
|
||
| The deprecation policy depends on the significance of the user-facing change, which we have divided into | ||
| three categories: | ||
|
|
||
| A **core feature change** is one that affects how the framework functions, for example a | ||
| change to `BaseExperiment`. The timeline for deprecating an existing core feature is as follows: | ||
|
|
||
| * Minor release 1: An alternative path is provided. A `PendingDeprecationWarning` | ||
| should be issued when the old path is used, indicating to users how to switch to | ||
| the new path and the release in which the old path will no longer be available. The | ||
| developer may choose to directly deprecate the feature and issue a `DeprecationWarning` instead, | ||
| in which case the release note should indicate the feature has been deprecated and how to switch | ||
| to the new path. | ||
| * Minor release 2: The `PendingDeprecationWarning` becomes a `DeprecationWarning`, or the | ||
| `DeprecationWarning` remains in place. The release note should indicate the feature has | ||
| been deprecated and how to switch to the new path. | ||
| * Minor release 3: The old feature is removed. The release note should indicate that the feature has | ||
| been removed and how to switch to the new path. | ||
|
|
||
| If the three-release cycle takes fewer than three months, the feature removal must wait for more | ||
| releases until three months has elapsed since the first issuing of the `PendingDeprecationWarning` | ||
| or `DeprecationWarning`. | ||
|
|
||
| A **non-core feature change** may be a change to a specific experiment class or modules such as the | ||
| plotter. The timeline is shortened for such a change: | ||
|
|
||
| * Minor release 1: An alternative path is provided. A `DeprecationWarning` should be issued | ||
| when the old path is used, indicating to users how to switch to the new path and the release | ||
| in which the old path will no longer be available. | ||
| * Minor release 2: The old feature is removed. The release note should indicate that the feature has | ||
| been removed and how to switch to the new path. | ||
|
|
||
| Lastly, a **minor, non-core change** could be a cosmetic change such as output file names or a | ||
| change to helper functions that isn't directly used in the package codebase. These can be made in | ||
| one release without a deprecation process as long as the change is clearly described in the | ||
| release notes. |
There was a problem hiding this comment.
Is this documented in the user facing documentation? Personally I think ensuring the deprecation and stability policy are clear for end users too is important as it establishes user expectations and how to deal with potential api changes. For qiskit we put this here: https://github.com/Qiskit/documentation/blob/main/docs/start/install.mdx#L218-L290 which is the user facing documentation of the stability policy (which I still need to update qiskit's in-repo contributing guide to reflect the qiskit 1.0 changes)
There was a problem hiding this comment.
Good point, added in 8edbd46. Might be time to split the Installation section out from Getting Started given its length.
| Any change to the existing package code that affects how the user interacts with the package | ||
| should give the user clear instructions and advanced warning if the change is nontrivial. | ||
| Qiskit Experiments's deprecation policy is based on [Qiskit's | ||
| policy](https://github.com/Qiskit/qiskit/blob/1.0.0rc1/DEPRECATION.md) prior to its 1.0 release, but |
There was a problem hiding this comment.
One of the reasons to still link to the Qiskit policy despite laying a mostly complete policy below is that the Qiskit policy has some good suggestions on how to implement deprecations. We might mention that here.
There was a problem hiding this comment.
Yes, it's a bit awkward that we have our own usage examples but the Qiskit policy is more thorough. I pared down what we have currently to the QE specific deprecate_func() example and added a link to the relevant Qiskit policy section.
coruscating
force-pushed
the
0.6-release-notes
branch
from
a391d46 to
e1ed9fb
Compare
Summary
Update the release notes and deprecation policy prior to release.