From 45e42525224e9e1857300e7e5529273fe619c9e4 Mon Sep 17 00:00:00 2001 From: Abby Mitchell <23662430+javabster@users.noreply.github.com> Date: Mon, 20 Nov 2023 16:49:06 -0500 Subject: [PATCH] Convert deprecation policy and maintainer guide to markdown (#11218) * Converted deprecation policy and maintainer guide to markdown * Apply suggestions from code review Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * Added more context around writing docs * Update DEPRECATION.md Co-authored-by: Jake Lishman * fixed some formatting issues * Removed deprecation_policy.rst and maintainers_guide.rst * Apply suggestions from code review Co-authored-by: Jake Lishman * update index.rst --------- Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Co-authored-by: Jake Lishman --- DEPRECATION.md | 227 ++++++++++++++++++ docs/maintainers_guide.rst => MAINTAINING.md | 33 ++- docs/deprecation_policy.rst | 237 ------------------- docs/index.rst | 7 - 4 files changed, 243 insertions(+), 261 deletions(-) create mode 100644 DEPRECATION.md rename docs/maintainers_guide.rst => MAINTAINING.md (63%) delete mode 100644 docs/deprecation_policy.rst diff --git a/DEPRECATION.md b/DEPRECATION.md new file mode 100644 index 000000000000..5b0de6611261 --- /dev/null +++ b/DEPRECATION.md @@ -0,0 +1,227 @@ +# Deprecation Policy + +Many users and other packages depend on different parts of Qiskit. We must +make sure that whenever we make changes to the code, we give users ample time to +adjust without breaking code that they have already written. + +Most importantly: *do not* change any interface that is public-facing unless we +absolutely have to. Adding things is ok, taking things away is annoying for +users but can be handled reasonably with plenty notice, but changing behavior +generally means users cannot write code that will work with two subsequent +versions of Qiskit, which is not acceptable. + +Beware that users will often be using functions, classes and methods that we, +the Qiskit developers, may consider internal or not widely used. Do not make +assumptions that "this is buried, so nobody will be using it"; if it is public, +it is subject to the policy. The only exceptions here are functions and modules +that are explicitly internal, *i.e.* those whose names begin with a leading +underscore (`_`). + +The guiding principles are: + +- we must not remove or change code without active warnings for least three + months or two complete version cycles; + +- there must always be a way to achieve valid goals that does not issue any + warnings; + +- never assume that a function that isn't explicitly internal isn't in use; + +- all deprecations, changes and removals are considered API changes, and can + only occur in minor releases not patch releases, per the [stable branch policy](https://github.com/Qiskit/qiskit/blob/main/MAINTAINING.md#stable-branch-policy). + + +## Removing a feature + +When removing a feature (for example a class, function or function parameter), +we will follow this procedure: + +- The alternative path must be in place for one minor version before any + warnings are issued. For example, if we want to replace the function `foo()` + with `bar()`, we must make at least one release with both functions before + issuing any warnings within `foo()`. You may issue + `PendingDeprecationWarning`s from the old paths immediately. + + *Reason*: we need to give people time to swap over without breaking their + code as soon as they upgrade. + +- After the alternative path has been in place for at least one minor version, + [issue the deprecation warnings](#issuing-deprecation-warnings). Add a + release note with a `deprecations` section listing all deprecated paths, + their alternatives, and the reason for deprecation. [Update the tests to test the warnings](#testing-deprecated-functionality). + + *Reason*: removals must be highly visible for at least one version, to + minimize the surprise to users when they actually go. + +- Set a removal date for the old feature, and remove it (and the warnings) when + reached. This must be at least three months after the version with the + warnings was first released, and cannot be the minor version immediately + after the warnings. Add an `upgrade` release note that lists all the + removals. For example, if the alternative path was provided in `0.19.0` + and the warnings were added in `0.20.0`, the earliest version for removal + is `0.22.0`, even if `0.21.0` was released more than three months after + `0.20.0`. + + **Note: These are _minimum_** requirements. For removal of significant or core features, give + users at least an extra minor version if not longer.** + + *Reason*: there needs to be time for users to see these messages, and to give + them time to adjust. Not all users will update their version of Qiskit + immediately, and some may skip minor versions. + +When a feature is marked as deprecated it is slated for removal, but users +should still be able to rely on it to work correctly. We consider a feature +marked "deprecated" as frozen; we commit to maintaining it with critical bug +fixes until it is removed, but we won't merge new functionality to it. + + +## Changing behavior + + +Changing behavior without a removal is particularly difficult to manage, because +we need to have both options available for two versions, and be able to issue +warnings. For example, changing the type of the return value from a function +will almost invariably involve making an API break, which is frustrating for +users and makes it difficult for them to use Qiskit. + +The best solution here is often to make a new function, and then use [the procedures for removal](#removing-features) above. + +If you absolutely must change the behavior of existing code (other than fixing +bugs), you will need to use your best judgment to apply the guiding principles +at the top of this document. The most appropriate warning for behavioral +changes is usually `FutureWarning`. Some possibilities for how to effect a +change: + +- If you are changing the default behavior of a function, consider adding a + keyword argument to select between old and new behaviors. When it comes time, + you can issue a `FutureWarning` if the keyword argument is not given + (*e.g.* if it is `None`), saying that the new value will soon become the + default. You will need to go through the normal deprecation period for + removing this keyword argument after you have made the behavior change. This + will take at least six months to go through both cycles. + +- If you need to change the return type of a function, consider adding a new + function that returns the new type, and then follow the procedures for + deprecating the old function. + +- If you need to accept a new input that you cannot distinguish from an existing + possibility because of its type, consider letting it be passed by a different + keyword argument, or add a second function that only accepts the new form. + + + +## Issuing deprecation warnings + +The proper way to raise a deprecation warning is to use the decorators `@deprecate_arg` and +`@deprecate_func` from `qiskit.utils.deprecation`. These will generate a standardized message and +and add the deprecation to that function's docstring so that it shows up in the docs. + + +```python +from qiskit.utils.deprecation import deprecate_arg, deprecate_func + +@deprecate_func(since="0.24.0", additional_msg="No replacement is provided.") +def deprecated_func(): + pass + +@deprecate_arg("bad_arg", new_alias="new_name", since="0.24.0") +def another_func(bad_arg: str, new_name: str): + pass +``` + +Usually, you should set `additional_msg: str` with the format `"Instead, use ..."` so that +people know how to migrate. Read those functions' docstrings for additional arguments like +`pending: bool` and `predicate`. + +If you are deprecating outside the main Qiskit repo, set `package_name` to match your package. +Alternatively, if you prefer to use your own decorator helpers, then have them call +`add_deprecation_to_docstring` from `qiskit.utils.deprecation`. + +If `@deprecate_func` and `@deprecate_arg` cannot handle your use case, consider improving +them. Otherwise, you can directly call the `warn` function +from the [warnings module in the Python standard library](https://docs.python.org/3/library/warnings.html), +using the category `DeprecationWarning`. For example: + +```python +import warnings + +def deprecated_function(): + warnings.warn( + "The function qiskit.deprecated_function() is deprecated since " + "Qiskit 0.44.0, and will be removed 3 months or more later. " + "Instead, you should use qiskit.other_function().", + category=DeprecationWarning, + stacklevel=2, + ) + # ... the rest of the function ... + +``` + +Make sure you include the version of the package that introduced the deprecation +warning (so maintainers can easily see when it is valid to remove it), and what +the alternative path is. + +Take note of the `stacklevel` argument. This controls which function is +accused of being deprecated. Setting `stacklevel=1` (the default) means the +warning will blame the `warn` function itself, while `stacklevel=2` will +correctly blame the containing function. It is unusual to set this to anything +other than `2`, but can be useful if you use a helper function to issue the +same warning in multiple places. + + +## Testing deprecated functionality + +Whenever you add deprecation warnings, you will need to update tests involving +the functionality. The test suite should fail otherwise, because of the new +warnings. We must continue to test deprecated functionality throughout the +deprecation period, to ensure that it still works. + +To update the tests, you need to wrap each call of deprecated behavior in its +own assertion block. For subclasses of `unittest.TestCase` (which all Qiskit +test cases are), this is done by: + + +```python +class MyTestSuite(QiskitTestCase): + def test_deprecated_function(self): + with self.assertWarns(DeprecationWarning): + output = deprecated_function() + # ... do some things with output ... + self.assertEqual(output, expected) +``` + +## Documenting deprecations and breaking changes + +It is important to warn the user when your breaking changes are coming. + +`@deprecate_arg` and `@deprecate_func` will automatically add the deprecation to the docstring +for the function so that it shows up in docs. + +If you are not using those decorators, you should directly add a [Sphinx deprecated directive](https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-deprecated): + + +```python +def deprecated_function(): + """ + Short description of the deprecated function. + + .. deprecated:: 0.44.0 + The function qiskit.deprecated_function() is deprecated since + Qiskit 0.44.0, and will be removed 3 months or more later. + Instead, you should use qiskit.other_function(). + + + """ + # ... the rest of the function ... +``` + + +You should also document the deprecation in the changelog by using Reno. Explain the deprecation +and how to migrate. + +In particular situations where a deprecation or change might be a major disruptor for users, a +*migration guide* might be needed. Please write these guides in Qiskit's documentation at +https://github.com/Qiskit/documentation/tree/main/docs/api/migration-guides. Once +the migration guide is written and published, deprecation +messages and documentation should link to it (use the `additional_msg` argument for +`@deprecate_arg` and `@deprecate_func`). diff --git a/docs/maintainers_guide.rst b/MAINTAINING.md similarity index 63% rename from docs/maintainers_guide.rst rename to MAINTAINING.md index ed2a6b508cdf..bf51e67fe79d 100644 --- a/docs/maintainers_guide.rst +++ b/MAINTAINING.md @@ -1,17 +1,13 @@ -################# -Maintainers Guide -################# +# Maintainers Guide This document defines a *maintainer* as a contributor with merge privileges. The information detailed here is mostly related to Qiskit releases and other internal processes. -.. _stable_branch_policy: -Stable Branch Policy -==================== +## Stable Branch Policy The stable branch is intended to be a safe source of fixes for high-impact -bugs and security issues that have been fixed on ``main`` since a +bugs and security issues that have been fixed on `main` since a release. When reviewing a stable branch PR, we must balance the risk of any given patch with the value that it will provide to users of the stable branch. Only a limited class of changes are appropriate for @@ -28,27 +24,30 @@ change: - How self-contained the fix is: if it fixes a significant issue but also refactors a lot of code, it's probably worth thinking about what a less risky fix might look like. -- Whether the fix is already on ``main``: a change must be a backport of - a change already merged onto ``main``, unless the change simply does - not make sense on ``main``. +- Whether the fix is already on `main`: a change must be a backport of + a change already merged onto `main`, unless the change simply does + not make sense on `main`. -Backporting ------------ +### Backporting -When a PR tagged with ``stable backport potential`` is merged, or when a -merged PR is given that tag, the `Mergify bot `__ will +When a PR tagged with `stable backport potential` is merged, or when a +merged PR is given that tag, the [Mergify bot](https://mergify.com) will open a PR to the current stable branch. You can review and merge this PR like normal. -Documentation Structure -======================= +## Documentation Structure The way documentation is structured in Qiskit is to push as much of the actual documentation into the docstrings as possible. This makes it easier for additions and corrections to be made during development, because the majority -of the documentation lives near the code being changed. +of the documentation lives near the code being changed. These docstrings are then pulled into +the API Reference section of https://docs.quantum-computing.ibm.com. Refer to https://qiskit.github.io/qiskit_sphinx_theme/apidocs/index.html for how to create and write effective API documentation, such as setting up the RST files and docstrings. + +If changes you are making affect non-API reference content in https://docs.quantum-computing.ibm.com +you can open an issue (or better yet a PR) to update the relevant page in https://github.com/Qiskit/documentation. +You can also use this repo to suggest or contribute brand new content beyond updates to the API reference. \ No newline at end of file diff --git a/docs/deprecation_policy.rst b/docs/deprecation_policy.rst deleted file mode 100644 index 997aa8172fa8..000000000000 --- a/docs/deprecation_policy.rst +++ /dev/null @@ -1,237 +0,0 @@ -################## -Deprecation Policy -################## - -Many users and other packages depend on different parts of Qiskit. We must -make sure that whenever we make changes to the code, we give users ample time to -adjust without breaking code that they have already written. - -Most importantly: *do not* change any interface that is public-facing unless we -absolutely have to. Adding things is ok, taking things away is annoying for -users but can be handled reasonably with plenty notice, but changing behavior -generally means users cannot write code that will work with two subsequent -versions of Qiskit, which is not acceptable. - -Beware that users will often be using functions, classes and methods that we, -the Qiskit developers, may consider internal or not widely used. Do not make -assumptions that "this is buried, so nobody will be using it"; if it is public, -it is subject to the policy. The only exceptions here are functions and modules -that are explicitly internal, *i.e.* those whose names begin with a leading -underscore (``_``). - -The guiding principles are: - -- we must not remove or change code without active warnings for least three - months or two complete version cycles; - -- there must always be a way to achieve valid goals that does not issue any - warnings; - -- never assume that a function that isn't explicitly internal isn't in use; - -- all deprecations, changes and removals are considered API changes, and can - only occur in minor releases not patch releases, per the - :ref:`stable branch policy `. - -.. _removing-features: - -Removing a feature -================== - -When removing a feature (for example a class, function or function parameter), -we will follow this procedure: - -#. The alternative path must be in place for one minor version before any - warnings are issued. For example, if we want to replace the function ``foo()`` - with ``bar()``, we must make at least one release with both functions before - issuing any warnings within ``foo()``. You may issue - ``PendingDeprecationWarning``\ s from the old paths immediately. - - *Reason*: we need to give people time to swap over without breaking their - code as soon as they upgrade. - -#. After the alternative path has been in place for at least one minor version, - :ref:`issue the deprecation warnings `. Add a - release note with a ``deprecations`` section listing all deprecated paths, - their alternatives, and the reason for deprecation. :ref:`Update the tests - to test the warnings `. - - *Reason*: removals must be highly visible for at least one version, to - minimize the surprise to users when they actually go. - -#. Set a removal date for the old feature, and remove it (and the warnings) when - reached. This must be at least three months after the version with the - warnings was first released, and cannot be the minor version immediately - after the warnings. Add an ``upgrade`` release note that lists all the - removals. For example, if the alternative path was provided in ``0.19.0`` - and the warnings were added in ``0.20.0``, the earliest version for removal - is ``0.22.0``, even if ``0.21.0`` was released more than three months after - ``0.20.0``. - - .. note:: - - These are *minimum* requirements. For removal of significant or core features, give - users at least an extra minor version if not longer. - - *Reason*: there needs to be time for users to see these messages, and to give - them time to adjust. Not all users will update their version of Qiskit - immediately, and some may skip minor versions. - -When a feature is marked as deprecated it is slated for removal, but users -should still be able to rely on it to work correctly. We consider a feature -marked "deprecated" as frozen; we commit to maintaining it with critical bug -fixes until it is removed, but we won't merge new functionality to it. - - -Changing behavior -================= - -Changing behavior without a removal is particularly difficult to manage, because -we need to have both options available for two versions, and be able to issue -warnings. For example, changing the type of the return value from a function -will almost invariably involve making an API break, which is frustrating for -users and makes it difficult for them to use Qiskit. - -The best solution here is often to make a new function, and then use :ref:`the -procedures for removal ` above. - -If you absolutely must change the behavior of existing code (other than fixing -bugs), you will need to use your best judgment to apply the guiding principles -at the top of this document. The most appropriate warning for behavioral -changes is usually ``FutureWarning``. Some possibilities for how to effect a -change: - -- If you are changing the default behavior of a function, consider adding a - keyword argument to select between old and new behaviors. When it comes time, - you can issue a ``FutureWarning`` if the keyword argument is not given - (*e.g.* if it is ``None``), saying that the new value will soon become the - default. You will need to go through the normal deprecation period for - removing this keyword argument after you have made the behavior change. This - will take at least six months to go through both cycles. - -- If you need to change the return type of a function, consider adding a new - function that returns the new type, and then follow the procedures for - deprecating the old function. - -- If you need to accept a new input that you cannot distinguish from an existing - possibility because of its type, consider letting it be passed by a different - keyword argument, or add a second function that only accepts the new form. - - -.. _issuing-deprecation-warnings: - -Issuing deprecation warnings -============================ - -The proper way to raise a deprecation warning is to use the decorators ``@deprecate_arg`` and -``@deprecate_func`` from ``qiskit.utils.deprecation``. These will generate a standardized message and -and add the deprecation to that function's docstring so that it shows up in the docs. - -.. code-block:: python - - from qiskit.utils.deprecation import deprecate_arg, deprecate_func - - @deprecate_func(since="0.24.0", additional_msg="No replacement is provided.") - def deprecated_func(): - pass - - @deprecate_arg("bad_arg", new_alias="new_name", since="0.24.0") - def another_func(bad_arg: str, new_name: str): - pass - -Usually, you should set ``additional_msg: str `` with the format ``"Instead, use ..."`` so that -people know how to migrate. Read those functions' docstrings for additional arguments like -``pending: bool`` and ``predicate``. - -If you are deprecating outside the main Qiskit repo, set ``package_name`` to match your package. -Alternatively, if you prefer to use your own decorator helpers, then have them call -``add_deprecation_to_docstring`` from ``qiskit.utils.deprecation``. - -If ``@deprecate_func`` and ``@deprecate_arg`` cannot handle your use case, consider improving -them. Otherwise, you can directly call the ``warn`` function -from the `warnings module in the Python standard library -`__, using the category -``DeprecationWarning``. For example: - -.. code-block:: python - - import warnings - - def deprecated_function(): - warnings.warn( - "The function qiskit.deprecated_function() is deprecated since " - "Qiskit Terra 0.20.0, and will be removed 3 months or more later. " - "Instead, you should use qiskit.other_function().", - category=DeprecationWarning, - stacklevel=2, - ) - # ... the rest of the function ... - -Make sure you include the version of the package that introduced the deprecation -warning (so maintainers can easily see when it is valid to remove it), and what -the alternative path is. - -Take note of the ``stacklevel`` argument. This controls which function is -accused of being deprecated. Setting ``stacklevel=1`` (the default) means the -warning will blame the ``warn`` function itself, while ``stacklevel=2`` will -correctly blame the containing function. It is unusual to set this to anything -other than ``2``, but can be useful if you use a helper function to issue the -same warning in multiple places. - - -.. _testing-deprecated-functionality: - -Testing deprecated functionality -================================ - -Whenever you add deprecation warnings, you will need to update tests involving -the functionality. The test suite should fail otherwise, because of the new -warnings. We must continue to test deprecated functionality throughout the -deprecation period, to ensure that it still works. - -To update the tests, you need to wrap each call of deprecated behavior in its -own assertion block. For subclasses of ``unittest.TestCase`` (which all Qiskit -test cases are), this is done by: - -.. code-block:: python - - class MyTestSuite(QiskitTestCase): - def test_deprecated_function(self): - with self.assertWarns(DeprecationWarning): - output = deprecated_function() - # ... do some things with output ... - self.assertEqual(output, expected) - -Documenting deprecations and breaking changes -============================================= - -It is important to warn the user when your breaking changes are coming. - -``@deprecate_arg`` and ``@deprecate_func`` will automatically add the deprecation to the docstring -for the function so that it shows up in docs. - -If you are not using those decorators, you should directly add a `Sphinx deprecated directive -`__: - -.. code-block:: python - - def deprecated_function(): - """ - Short description of the deprecated function. - - .. deprecated:: 0.20.0 - The function qiskit.deprecated_function() is deprecated since - Qiskit Terra 0.20.0, and will be removed 3 months or more later. - Instead, you should use qiskit.other_function(). - - - """ - # ... the rest of the function ... - -You should also document the deprecation in the changelog by using Reno. Explain the deprecation -and how to migrate. - -In particular situations where a deprecation or change might be a major disruptor for users, a -*migration guide* might be needed. Once the migration guide is written and published, deprecation -messages and documentation should link to it (use the ``additional_msg: str`` argument for -``@deprecate_arg`` and ``@deprecate_func``). diff --git a/docs/index.rst b/docs/index.rst index edb770ceeeac..86c195e08363 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -78,10 +78,3 @@ Main Qiskit-related projects configuration GitHub faq - -.. toctree:: - :caption: Contributing - :hidden: - - deprecation_policy - maintainers_guide