Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@deprecate_arguments and @deprecate_function add deprecation to docstring #9790

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions qiskit/algorithms/factorizers/shor.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,9 @@ class Shor:
"""

@deprecate_function(
"""The Shor class is deprecated as of Qiskit Terra 0.22.0 and will be removed
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this diff does is convert from a multiline string to string concatenation. We can't handle \n in add_deprecation_to_docstring() (as a simplification)

no sooner than 3 months after the release date.
It is replaced by the tutorial at https://qiskit.org/textbook/ch-algorithms/shor.html
""",
"The Shor class is deprecated as of Qiskit Terra 0.22.0 and will be removed "
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
"no sooner than 3 months after the release date. It is replaced by the tutorial "
"at https://qiskit.org/textbook/ch-algorithms/shor.html",
since="0.22.0",
)
def __init__(self, quantum_instance: Optional[Union[QuantumInstance, Backend]] = None) -> None:
Expand Down
7 changes: 3 additions & 4 deletions qiskit/algorithms/linear_solvers/hhl.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,9 @@ class HHL(LinearSolver):
"""

@deprecate_function(
"""The HHL class is deprecated as of Qiskit Terra 0.22.0 and will be removed
no sooner than 3 months after the release date.
It is replaced by the tutorial at https://qiskit.org/textbook/ch-applications/hhl_tutorial.html"
""",
"The HHL class is deprecated as of Qiskit Terra 0.22.0 and will be removed "
"no sooner than 3 months after the release date. It is replaced by the tutorial at "
"https://qiskit.org/textbook/ch-applications/hhl_tutorial.html",
since="0.22.0",
)
def __init__(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ def circuit_result(self) -> Result:

@property
@deprecate_function(
"""The 'PhaseEstimationResult.most_likely_phase' attribute
is deprecated as of 0.18.0 and will be removed no earlier than 3 months
after the release date. It has been renamed as the 'phase' attribute.""",
"The 'PhaseEstimationResult.most_likely_phase' attribute is deprecated as of 0.18.0 and "
"will be removed no earlier than 3 months after the release date. It has been renamed as "
"the 'phase' attribute.",
since="0.18.0",
)
def most_likely_phase(self) -> float:
Expand Down
7 changes: 3 additions & 4 deletions qiskit/dagcircuit/dagcircuit.py
Original file line number Diff line number Diff line change
Expand Up @@ -527,10 +527,9 @@ def _add_op_node(self, op, qargs, cargs):
return node_index

@deprecate_function(
"""The DAGCircuit._copy_circuit_metadata method is deprecated as of 0.20.0. It will be removed
no earlier than 3 months after the release date. You should use the DAGCircuit.copy_empty_like
method instead, which acts identically.
""",
"The DAGCircuit._copy_circuit_metadata method is deprecated as of 0.20.0. It will be "
"removed no earlier than 3 months after the release date. You should use the "
"DAGCircuit.copy_empty_like method instead, which acts identically.",
since="0.20.0",
)
def _copy_circuit_metadata(self):
Expand Down
53 changes: 27 additions & 26 deletions qiskit/utils/deprecation.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,27 @@ def deprecate_arguments(
Callable: The decorated callable.
"""

del since # Will be used in a followup to add deprecations to our docs site.

def decorator(func):
func_name = func.__qualname__
old_kwarg_to_msg = {}
for old_arg, new_arg in kwarg_map.items():
msg_suffix = (
"will in the future be removed." if new_arg is None else f"replaced with {new_arg}."
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
)
old_kwarg_to_msg[
old_arg
] = f"{func_name} keyword argument {old_arg} is deprecated and {msg_suffix}"

@functools.wraps(func)
def wrapper(*args, **kwargs):
if kwargs:
_rename_kwargs(func.__name__, kwargs, kwarg_map, category)
_rename_kwargs(func_name, kwargs, old_kwarg_to_msg, kwarg_map, category)
return func(*args, **kwargs)

for msg in old_kwarg_to_msg.values():
add_deprecation_to_docstring(
wrapper, msg, since=since, pending=issubclass(category, PendingDeprecationWarning)
)
return wrapper

return decorator
Expand Down Expand Up @@ -73,14 +85,15 @@ def deprecate_function(
Callable: The decorated, deprecated callable.
"""

del since # Will be used in a followup to add deprecations to our docs site.

def decorator(func):
@functools.wraps(func)
def wrapper(*args, **kwargs):
warnings.warn(msg, category=category, stacklevel=stacklevel)
return func(*args, **kwargs)

add_deprecation_to_docstring(
wrapper, msg, since=since, pending=issubclass(category, PendingDeprecationWarning)
)
return wrapper

return decorator
Expand All @@ -89,30 +102,18 @@ def wrapper(*args, **kwargs):
def _rename_kwargs(
func_name: str,
kwargs: Dict[str, Any],
kwarg_map: Dict[str, str],
old_kwarg_to_msg: Dict[str, str],
kwarg_map: Dict[str, Optional[str]],
category: Type[Warning] = DeprecationWarning,
) -> None:
for old_arg, new_arg in kwarg_map.items():
if old_arg in kwargs:
if new_arg in kwargs:
raise TypeError(f"{func_name} received both {new_arg} and {old_arg} (deprecated).")

if new_arg is None:
warnings.warn(
f"{func_name} keyword argument {old_arg} is deprecated and "
"will in future be removed.",
category=category,
stacklevel=3,
)
else:
warnings.warn(
f"{func_name} keyword argument {old_arg} is deprecated and "
f"replaced with {new_arg}.",
category=category,
stacklevel=3,
)

kwargs[new_arg] = kwargs.pop(old_arg)
if old_arg not in kwargs:
continue
if new_arg in kwargs:
raise TypeError(f"{func_name} received both {new_arg} and {old_arg} (deprecated).")
warnings.warn(old_kwarg_to_msg[old_arg], category=category, stacklevel=3)
if new_arg is not None:
kwargs[new_arg] = kwargs.pop(old_arg)


# We insert deprecations in-between the description and Napoleon's meta sections. The below is from
Expand Down
84 changes: 83 additions & 1 deletion test/python/utils/test_deprecation.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,89 @@
from textwrap import dedent

from qiskit.test import QiskitTestCase
from qiskit.utils.deprecation import add_deprecation_to_docstring
from qiskit.utils.deprecation import (
add_deprecation_to_docstring,
deprecate_function,
deprecate_arguments,
)


class TestDeprecationDecorators(QiskitTestCase):
"""Test that the decorators in ``utils.deprecation`` correctly log warnings and get added to
docstring."""

def test_deprecate_arguments_message(self) -> None:
"""Test that `@deprecate_arguments` adds the correct message to the docstring."""

@deprecate_arguments(
{"old_arg1": "new_arg1", "old_arg2": None},
category=PendingDeprecationWarning,
since="9.99",
)
def my_func() -> None:
pass

self.assertEqual(
my_func.__doc__,
dedent(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about removing idents, as they are kinda important in this context. Do you think that a literal comparison is too strong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still considers indentation. Note how there are two spaces underneath the line .. deprecated::. This is how the docstring renders when there was no prior docstring, which is what we want.

Also, for context, these tests are not focused on ensuring that add_deprecation_to_docstring handles all the edge cases of indentation. We already have a whole suite of unit tests for that dedicated purpose. These unit tests here are only to check that we:

  1. Generate the right message
  2. Actually call the helper function add_deprecation_to_docstring

f"""\

.. deprecated:: 9.99_pending
{my_func.__qualname__} keyword argument old_arg1 is deprecated and replaced with \
new_arg1.

.. deprecated:: 9.99_pending
{my_func.__qualname__} keyword argument old_arg2 is deprecated and will in the \
future be removed.
ElePT marked this conversation as resolved.
Show resolved Hide resolved
"""
),
)

def test_deprecate_function_docstring(self) -> None:
"""Test that `@deprecate_function` adds the correct message to the docstring."""

@deprecate_function("Stop using my_func!", since="9.99")
def my_func() -> None:
pass

self.assertEqual(
my_func.__doc__,
dedent(
"""\

.. deprecated:: 9.99
Stop using my_func!
"""
),
)

def test_deprecate_arguments_runtime_warning(self) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of this functionality is new. But I wanted to make sure I didn't break anything.

"""Test that `@deprecate_arguments` warns whenever the arguments are used.

Also check that old arguments are passed in as their new alias.
"""

@deprecate_arguments({"arg1": None, "arg2": "new_arg2"}, since="9.99")
def my_func(*, arg1: str = "a", new_arg2: str) -> None:
del arg1
self.assertEqual(new_arg2, "z")

my_func(new_arg2="z") # No warnings if no deprecated args used.
with self.assertWarnsRegex(DeprecationWarning, "arg1"):
my_func(arg1="a", new_arg2="z")
with self.assertWarnsRegex(DeprecationWarning, "arg2"):
# `arg2` should be converted into `new_arg2`.
my_func(arg2="z") # pylint: disable=missing-kwoa

def test_deprecate_function_runtime_warning(self) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto that this functionality is not new

"""Test that `@deprecate_function` warns whenever the function is used."""

@deprecate_function("Stop using my_func!", since="9.99")
def my_func() -> None:
pass

with self.assertWarnsRegex(DeprecationWarning, "Stop using my_func!"):
my_func()


class AddDeprecationDocstringTest(QiskitTestCase):
Expand Down