Update deprecation policy document for Qiskit 1.0 #11775
Conversation
This updates `DEPRECATION.md` to bring it inline with the documentation hosted on https://docs.quantum.ibm.com. The policy described in this commit is intended to match what we already agreed on and posted publicly, but this particular file got slightly out-of-sync during the initial deployment of the new hosted documentation and the split of certain documentation content from this repository. Most notably, this commit makes the exact extent of the public interface much clearer. We no longer use the "implicit based on object name" form, but only commit to publicly documented objects being part of the public interface.
jakelishman
added
documentation
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7874959295Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| 1. The module `qiskit.circuit.measure` is not publicly documented, so is not part of the public interface. | ||
| 2. The [`Measure` object is documented as being in `qiskit.circuit.library`](https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.Measure), and is re-exported by `qiskit.circuit`, so the public import paths are `from qiskit.circuit.library import Measure` and `from qiskit.circuit import Measure`. | ||
|
|
||
| As a rule of thumb, if you are using Qiskit, you should import objects from the highest-level package that exports that object. |
There was a problem hiding this comment.
This does raise some questions about things like QuantumCircuit and transpile which are re-exported from the root qiskit import, but are documented in qiskit.circuit and qiskit.compiler respectively.
There was a problem hiding this comment.
I'd roughly attempted to address that with bullet point 2. Are you thinking we need to write more in the public version of the policy / do more in the documentation / something else?
There was a problem hiding this comment.
TBH, it's more just a discrepancy I hadn't thought of before. More specifically I'm wondering if this points to a deficiency in our documentation more than anything else, should we be documenting the public API as qiskit.QuantumCircuit or qiskit.circuit.QuantumCircuit and yeah the extension for that for the user facing component of the stability policy which is considered the stable access point. In my head it's both, but I'm wondering how to express that cleanly.
There was a problem hiding this comment.
Yeah, I'd say the public interface is that QuantumCircuit is available from both qiskit and qiskit.circuit. In an ideal world, we'd specify __all__ fields on all the public modules/packages of the API, and then document those, and that would define all the public API import locations. We can't actually write that in the policy right now, though, because we don't actually do it, so I guess maybe we have to use these slightly fuzzy heuristics.
DEPRECATION.md
Outdated
| From within the Qiskit package itself, objects should be imported from the highest-level package that does not cause cyclic-import concerns. | ||
| In general, imports should not reach into the internals of *other* subpackages; `qiskit.circuit` should import all its objects from `qiskit.quantum_info` and not from submodules of that, though in practice, our package hierarchy is sufficiently confused that this is not always possible). | ||
| Using package-level imports helps indicate when new code is producing confused inter-package dependencies. |
There was a problem hiding this comment.
Is this necessary, my personal practice is normally to go as deep in the import "tree" as possible partially to avoid potential cycles, but also just out of habit.
There was a problem hiding this comment.
Personally I'd say yes it's necessary - imo, going deeper than is necessary is what exacerbates the package-hierarchy problems we have; it can sometimes make imports work that shouldn't because they're topologically out-of-order and then local imports added later can't resolve the cyclic problems caused that should work.
That said, that line is definitely me legislating style by fiat right now, and it would be better to discuss that.
There was a problem hiding this comment.
I've removed that little bit 8c6c560 - it's something we should talk about separately anyway, you're right.
| - never assume that an object that is part of the public interface is not in use. | ||
|
|
||
| While the no-breaking-changes rule is only formally required *within* a major release series, you should make every effort to avoid breaking changes wherever possible. | ||
| Similarly, while it is permissible where necessary for behavior to change with no single-code path to support both the last minor of one major release and the first minor of a new major release, it is still strongly preferable if you can achieve this. |
There was a problem hiding this comment.
I think this should be single code path. For example two-code path doesn't make sense but two code paths does.
There was a problem hiding this comment.
I think I using the hyphen to make it so that in your example, the plural would have been "two-codes path", as in "the direction which requires two codes" - "path" wasn't meant in the sense of "codepath" but "direction". Happier for better wording, though.
There was a problem hiding this comment.
That does make sense. single code path in the sense of "sequence of instructions" does not in this context. But I wasn't able to find that meaning.
There was a problem hiding this comment.
I'm struggling a bit to get the precise meaning of this
behavior [to] change with no single-code path to support both the last minor of one major release and the first minor of a new major release
- So single-code path means a solution such that the user has one chunk of code that they don't need to alter between the two versions?
- Does behavior change here refer to a change in behavior without a change in API? In other words, a user runs code in version A. Then in version B, the same code runs but gives different results.
- Suppose the answer to these questions is "yes" (and say the dev insists on maintaining both) Then I don't see a way to turn this into a non-breaking change.
There was a problem hiding this comment.
Yeah, in my reading this is describing the situation of 0.46 ->1.0 where we introduced a deprecation warning in 0.46 and removed that api in 1.0. This is a breaking change and because it's for a new major version this is allowed. This is basically just saying, that while the policy allows us to to do with the major version bump, it's preferable to try and maintain compatibility if possible.
There was a problem hiding this comment.
Yeah, that's what I was going for: "you're technically allowed to make changes that don't have a single way that works on both 0.x and 1.0, but really try not to do that". If there's better wording people want, I'm happy to update.
There was a problem hiding this comment.
This LGTM and is consistent with the changes we made to the user facing side of this. It read fine to me but if there are any suggestions on the language here we can always adjust it in a follow up PR, this is a living document as it's documenting our dev processes and procedures to support our user facing guarantees around backwards compat/stability.
* Update deprecation policy document for Qiskit 1.0 This updates `DEPRECATION.md` to bring it inline with the documentation hosted on https://docs.quantum.ibm.com. The policy described in this commit is intended to match what we already agreed on and posted publicly, but this particular file got slightly out-of-sync during the initial deployment of the new hosted documentation and the split of certain documentation content from this repository. Most notably, this commit makes the exact extent of the public interface much clearer. We no longer use the "implicit based on object name" form, but only commit to publicly documented objects being part of the public interface. * Commit to raising a warning on experimental features * Remove comment on internal import policy (cherry picked from commit be8bea1)
* Update deprecation policy document for Qiskit 1.0 This updates `DEPRECATION.md` to bring it inline with the documentation hosted on https://docs.quantum.ibm.com. The policy described in this commit is intended to match what we already agreed on and posted publicly, but this particular file got slightly out-of-sync during the initial deployment of the new hosted documentation and the split of certain documentation content from this repository. Most notably, this commit makes the exact extent of the public interface much clearer. We no longer use the "implicit based on object name" form, but only commit to publicly documented objects being part of the public interface. * Commit to raising a warning on experimental features * Remove comment on internal import policy (cherry picked from commit be8bea1) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Summary
This updates
DEPRECATION.mdto bring it inline with the documentation hosted on https://docs.quantum.ibm.com. The policy described in this commit is intended to match what we already agreed on and posted publicly, but this particular file got slightly out-of-sync during the initial deployment of the new hosted documentation and the split of certain documentation content from this repository.Most notably, this commit makes the exact extent of the public interface much clearer. We no longer use the "implicit based on object name" form, but only commit to publicly documented objects being part of the public interface.
Details and comments
Originally this was going to be #11205, but that got closed because the deprecation policy was getting merged as Qiskit/documentation#366 instead. However, our
DEPRECATION.mdfile is instructions for developers, so it was re-added to Qiskit in #11218 except without the changes to the policy for the 1.0 era. This PR brings the developer-relevant components back up to date.