Skip to content

fix: Retry constructors methods support None #592

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

Merged
merged 8 commits into from
Feb 5, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 42 additions & 35 deletions google/api_core/retry/retry_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import time

from enum import Enum
from typing import Any, Callable, TYPE_CHECKING
from typing import Any, Callable, Optional, TYPE_CHECKING

import requests.exceptions

Expand Down Expand Up @@ -238,8 +238,8 @@ def __init__(
initial: float = _DEFAULT_INITIAL_DELAY,
maximum: float = _DEFAULT_MAXIMUM_DELAY,
multiplier: float = _DEFAULT_DELAY_MULTIPLIER,
timeout: float = _DEFAULT_DEADLINE,
on_error: Callable[[Exception], Any] | None = None,
timeout: Optional[float] = _DEFAULT_DEADLINE,
on_error: Optional[Callable[[Exception], Any]] = None,
**kwargs: Any,
) -> None:
self._predicate = predicate
Expand All @@ -265,48 +265,39 @@ def deadline(self) -> float | None:
def timeout(self) -> float | None:
return self._timeout

def _replace(
self,
predicate: Callable[[Exception], bool] | None = None,
initial: float | None = None,
maximum: float | None = None,
multiplier: float | None = None,
timeout: float | None = None,
on_error: Callable[[Exception], Any] | None = None,
) -> Self:
return type(self)(
predicate=predicate or self._predicate,
initial=initial or self._initial,
maximum=maximum or self._maximum,
multiplier=multiplier or self._multiplier,
timeout=timeout or self._timeout,
on_error=on_error or self._on_error,
)

def with_deadline(self, deadline: float | None) -> Self:
"""Return a copy of this retry with the given timeout.

DEPRECATED: use :meth:`with_timeout` instead. Refer to the ``Retry`` class
documentation for details.

Args:
deadline (float): How long to keep retrying, in seconds.
deadline (float|None): How long to keep retrying, in seconds. If None,
no timeout is enforced.

Returns:
Retry: A new retry instance with the given timeout.
"""
return self._replace(timeout=deadline)
return self.with_timeout(deadline)

def with_timeout(self, timeout: float) -> Self:
def with_timeout(self, timeout: float | None) -> Self:
"""Return a copy of this retry with the given timeout.

Args:
timeout (float): How long to keep retrying, in seconds.
timeout (float): How long to keep retrying, in seconds. If None,
no timeout will be enforced.

Returns:
Retry: A new retry instance with the given timeout.
"""
return self._replace(timeout=timeout)
return type(self)(
predicate=self._predicate,
initial=self._initial,
maximum=self._maximum,
multiplier=self._multiplier,
timeout=timeout,
on_error=self._on_error,
)

def with_predicate(self, predicate: Callable[[Exception], bool]) -> Self:
"""Return a copy of this retry with the given predicate.
Expand All @@ -318,26 +309,42 @@ def with_predicate(self, predicate: Callable[[Exception], bool]) -> Self:
Returns:
Retry: A new retry instance with the given predicate.
"""
return self._replace(predicate=predicate)
return type(self)(
predicate=predicate,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we allowing predicate to be None here? If so, we would need to tweak the type annotation, and to add a test for this edge case? (The test might be starting with something with a non-trivial predicate, and then verifying that calling .with_predicate(None) sets it to None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like predicate has always been a required variable

Copy link
Contributor

@vchudnov-g vchudnov-g Feb 5, 2024

Choose a reason for hiding this comment

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

Well, in _replace, we had predicate: Callable[[Exception], bool] | None = None,, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed this comment

In the _replace helper, None represented a variable that isn't replaced (that is, it used the same value as the original Retry object).

But this doesn't work if users are passing in None for a with_timeout and expecting it to disable timeouts, which wasn't supported by the type annotations, but was used in practice. Which was the motivation for this change

initial=self._initial,
maximum=self._maximum,
multiplier=self._multiplier,
timeout=self._timeout,
on_error=self._on_error,
)

def with_delay(
self,
initial: float | None = None,
maximum: float | None = None,
multiplier: float | None = None,
initial: Optional[float] = None,
maximum: Optional[float] = None,
multiplier: Optional[float] = None,
) -> Self:
"""Return a copy of this retry with the given delay options.

Args:
initial (float): The minimum amount of time to delay (in seconds). This must
be greater than 0.
maximum (float): The maximum amount of time to delay (in seconds).
multiplier (float): The multiplier applied to the delay.
be greater than 0. If None, the current value is used.
maximum (float): The maximum amount of time to delay (in seconds). If None, the
current value is used.
multiplier (float): The multiplier applied to the delay. If None, the current
value is used.

Returns:
Retry: A new retry instance with the given predicate.
Retry: A new retry instance with the given delay options.
"""
return self._replace(initial=initial, maximum=maximum, multiplier=multiplier)
return type(self)(
predicate=self._predicate,
initial=initial if initial is not None else self._initial,
Copy link
Contributor

Choose a reason for hiding this comment

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

If these values are not None but still falsy, we still want to use them? If so, we should capture this in a test since this differs from the previous behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it differs from the previous behaviour, but it is in line with the pre-2.16.0 behaviour

Good point on the test. I had added one for timeout, but not for this one. Fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

@daniel-sanche Please could you link the specific commit for the fix in #592 (comment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, forgot to push. The new test should be there now

maximum=maximum if maximum is not None else self._maximum,
multiplier=multiplier if multiplier is not None else self._multiplier,
timeout=self._timeout,
on_error=self._on_error,
)

def __str__(self) -> str:
return (
Expand Down
2 changes: 1 addition & 1 deletion google/api_core/retry/retry_unary.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ class Retry(_BaseRetry):
must be greater than 0.
maximum (float): The maximum amount of time to delay in seconds.
multiplier (float): The multiplier applied to the delay.
timeout (float): How long to keep retrying, in seconds.
timeout (Optional[float]): How long to keep retrying, in seconds.
Note: timeout is only checked before initiating a retry, so the target may
run past the timeout value as long as it is healthy.
on_error (Callable[Exception]): A function to call while processing
Expand Down
47 changes: 34 additions & 13 deletions tests/unit/retry/test_retry_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ def test_constructor_options(self):
assert retry_._on_error is _some_function

@pytest.mark.parametrize("use_deadline", [True, False])
def test_with_timeout(self, use_deadline):
@pytest.mark.parametrize("value", [None, 0, 1, 4, 42, 5.5])
def test_with_timeout(self, use_deadline, value):
retry_ = self._make_one(
predicate=mock.sentinel.predicate,
initial=1,
Expand All @@ -148,11 +149,17 @@ def test_with_timeout(self, use_deadline):
on_error=mock.sentinel.on_error,
)
new_retry = (
retry_.with_timeout(42) if not use_deadline else retry_.with_deadline(42)
retry_.with_timeout(value)
if not use_deadline
else retry_.with_deadline(value)
Comment on lines +153 to +154
Copy link
Contributor

Choose a reason for hiding this comment

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

Under the hood, with_deadline is just calling with_timeout, so these two branches wind up testing the same code, module the trivial wrapping function. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I believe so. I think the intention is to make sure they stay in-line through any potential future refactors

)
assert retry_ is not new_retry
assert new_retry._timeout == 42
assert new_retry.timeout == 42 if not use_deadline else new_retry.deadline == 42
assert new_retry._timeout == value
assert (
new_retry.timeout == value
if not use_deadline
else new_retry.deadline == value
)

# the rest of the attributes should remain the same
assert new_retry._predicate is retry_._predicate
Expand Down Expand Up @@ -196,20 +203,34 @@ def test_with_delay_noop(self):
assert new_retry._maximum == retry_._maximum
assert new_retry._multiplier == retry_._multiplier

def test_with_delay(self):
@pytest.mark.parametrize(
"originals,updated,expected",
[
[(1, 2, 3), (4, 5, 6), (4, 5, 6)],
[(1, 2, 3), (0, 0, 0), (0, 0, 0)],
[(1, 2, 3), (None, None, None), (1, 2, 3)],
[(0, 0, 0), (None, None, None), (0, 0, 0)],
[(1, 2, 3), (None, 0.5, None), (1, 0.5, 3)],
[(1, 2, 3), (None, 0.5, 4), (1, 0.5, 4)],
[(1, 2, 3), (9, None, None), (9, 2, 3)],
],
)
def test_with_delay(self, originals, updated, expected):
retry_ = self._make_one(
predicate=mock.sentinel.predicate,
initial=1,
maximum=2,
multiplier=3,
timeout=4,
initial=originals[0],
maximum=originals[1],
multiplier=originals[2],
timeout=14,
on_error=mock.sentinel.on_error,
)
new_retry = retry_.with_delay(initial=5, maximum=6, multiplier=7)
new_retry = retry_.with_delay(
initial=updated[0], maximum=updated[1], multiplier=updated[2]
)
assert retry_ is not new_retry
assert new_retry._initial == 5
assert new_retry._maximum == 6
assert new_retry._multiplier == 7
assert new_retry._initial == expected[0]
assert new_retry._maximum == expected[1]
assert new_retry._multiplier == expected[2]

# the rest of the attributes should remain the same
assert new_retry._timeout == retry_._timeout
Expand Down