From 79840206d712f6412e0b9614974312facbc1adb2 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Wed, 31 Jan 2024 15:12:50 -0800 Subject: [PATCH 1/7] use sentinal value for _replace --- google/api_core/retry/retry_base.py | 39 +++++++++++++++++------------ 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/google/api_core/retry/retry_base.py b/google/api_core/retry/retry_base.py index efd6d8f7..6f64fb70 100644 --- a/google/api_core/retry/retry_base.py +++ b/google/api_core/retry/retry_base.py @@ -47,6 +47,8 @@ _LOGGER = logging.getLogger("google.api_core.retry") +# sentinal value for _replace to indicate arguments that should not be replaced +_DO_NOT_REPLACE = object() def if_exception_type( *exception_types: type[Exception], @@ -267,20 +269,20 @@ def timeout(self) -> float | None: 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, + predicate: Callable[[Exception], bool] | _DO_NOT_REPLACE = _DO_NOT_REPLACE, + initial: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, + maximum: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, + multiplier: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, + timeout: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, + on_error: Callable[[Exception], Any] | None | _DO_NOT_REPLACE = _DO_NOT_REPLACE, ) -> 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, + predicate=self._predicate if predicate is _DO_NOT_REPLACE else predicate, + initial=self._initial if initial is _DO_NOT_REPLACE else initial, + maximum=self._maximum if maximum is _DO_NOT_REPLACE else maximum, + multiplier=self._multiplier if multiplier is _DO_NOT_REPLACE else multiplier, + timeout=self._timeout if timeout is _DO_NOT_REPLACE else timeout, + on_error=self._on_error if on_error is _DO_NOT_REPLACE else on_error, ) def with_deadline(self, deadline: float | None) -> Self: @@ -330,13 +332,18 @@ def with_delay( 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. """ + initial = _DO_NOT_REPLACE if initial is None else initial + maximum = _DO_NOT_REPLACE if maximum is None else maximum + multiplier = _DO_NOT_REPLACE if multiplier is None else multiplier return self._replace(initial=initial, maximum=maximum, multiplier=multiplier) def __str__(self) -> str: From d228d98cac4681bcd077d7d4db1b84ca1b1c4443 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Wed, 31 Jan 2024 15:22:59 -0800 Subject: [PATCH 2/7] accept None in timeout --- google/api_core/retry/retry_base.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/google/api_core/retry/retry_base.py b/google/api_core/retry/retry_base.py index 6f64fb70..a9265bc8 100644 --- a/google/api_core/retry/retry_base.py +++ b/google/api_core/retry/retry_base.py @@ -240,7 +240,7 @@ def __init__( initial: float = _DEFAULT_INITIAL_DELAY, maximum: float = _DEFAULT_MAXIMUM_DELAY, multiplier: float = _DEFAULT_DELAY_MULTIPLIER, - timeout: float = _DEFAULT_DEADLINE, + timeout: float | None = _DEFAULT_DEADLINE, on_error: Callable[[Exception], Any] | None = None, **kwargs: Any, ) -> None: @@ -273,7 +273,7 @@ def _replace( initial: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, maximum: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, multiplier: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, - timeout: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, + timeout: float | None | _DO_NOT_REPLACE = _DO_NOT_REPLACE, on_error: Callable[[Exception], Any] | None | _DO_NOT_REPLACE = _DO_NOT_REPLACE, ) -> Self: return type(self)( @@ -292,18 +292,20 @@ def with_deadline(self, deadline: float | None) -> Self: 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) - 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. From ed60cc100c380205a5c69ff878969b8634447945 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Wed, 31 Jan 2024 15:40:59 -0800 Subject: [PATCH 3/7] remove _replace function --- google/api_core/retry/retry_base.py | 52 ++++++++++++++--------------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/google/api_core/retry/retry_base.py b/google/api_core/retry/retry_base.py index a9265bc8..589994d5 100644 --- a/google/api_core/retry/retry_base.py +++ b/google/api_core/retry/retry_base.py @@ -47,8 +47,6 @@ _LOGGER = logging.getLogger("google.api_core.retry") -# sentinal value for _replace to indicate arguments that should not be replaced -_DO_NOT_REPLACE = object() def if_exception_type( *exception_types: type[Exception], @@ -267,24 +265,6 @@ def deadline(self) -> float | None: def timeout(self) -> float | None: return self._timeout - def _replace( - self, - predicate: Callable[[Exception], bool] | _DO_NOT_REPLACE = _DO_NOT_REPLACE, - initial: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, - maximum: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, - multiplier: float | _DO_NOT_REPLACE = _DO_NOT_REPLACE, - timeout: float | None | _DO_NOT_REPLACE = _DO_NOT_REPLACE, - on_error: Callable[[Exception], Any] | None | _DO_NOT_REPLACE = _DO_NOT_REPLACE, - ) -> Self: - return type(self)( - predicate=self._predicate if predicate is _DO_NOT_REPLACE else predicate, - initial=self._initial if initial is _DO_NOT_REPLACE else initial, - maximum=self._maximum if maximum is _DO_NOT_REPLACE else maximum, - multiplier=self._multiplier if multiplier is _DO_NOT_REPLACE else multiplier, - timeout=self._timeout if timeout is _DO_NOT_REPLACE else timeout, - on_error=self._on_error if on_error is _DO_NOT_REPLACE else on_error, - ) - def with_deadline(self, deadline: float | None) -> Self: """Return a copy of this retry with the given timeout. @@ -298,7 +278,7 @@ def with_deadline(self, deadline: float | None) -> Self: 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 | None) -> Self: """Return a copy of this retry with the given timeout. @@ -310,7 +290,14 @@ def with_timeout(self, timeout: float | None) -> Self: 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. @@ -322,7 +309,14 @@ 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, + initial=self._initial, + maximum=self._maximum, + multiplier=self._multiplier, + timeout=self._timeout, + on_error=self._on_error, + ) def with_delay( self, @@ -343,10 +337,14 @@ def with_delay( Returns: Retry: A new retry instance with the given delay options. """ - initial = _DO_NOT_REPLACE if initial is None else initial - maximum = _DO_NOT_REPLACE if maximum is None else maximum - multiplier = _DO_NOT_REPLACE if multiplier is None else multiplier - 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, + 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 ( From 07ae10bcfdaa2d5cf133a45d07d9f9fa34acf119 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Wed, 31 Jan 2024 15:44:29 -0800 Subject: [PATCH 4/7] added test --- tests/unit/retry/test_retry_base.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/unit/retry/test_retry_base.py b/tests/unit/retry/test_retry_base.py index fa55d935..5db1de01 100644 --- a/tests/unit/retry/test_retry_base.py +++ b/tests/unit/retry/test_retry_base.py @@ -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, @@ -148,11 +149,11 @@ 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) ) 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 From 9d6fcefae2e69fc814e47eab8df623d555e20ce4 Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Wed, 31 Jan 2024 23:55:59 +0000 Subject: [PATCH 5/7] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20po?= =?UTF-8?q?st-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- tests/unit/retry/test_retry_base.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/unit/retry/test_retry_base.py b/tests/unit/retry/test_retry_base.py index 5db1de01..3a3be342 100644 --- a/tests/unit/retry/test_retry_base.py +++ b/tests/unit/retry/test_retry_base.py @@ -149,11 +149,17 @@ def test_with_timeout(self, use_deadline, value): on_error=mock.sentinel.on_error, ) new_retry = ( - retry_.with_timeout(value) if not use_deadline else retry_.with_deadline(value) + retry_.with_timeout(value) + if not use_deadline + else retry_.with_deadline(value) ) assert retry_ is not new_retry assert new_retry._timeout == value - assert new_retry.timeout == value if not use_deadline else new_retry.deadline == 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 From fba807b61f83b33ac0f2861251e47afdd3b41c5b Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 1 Feb 2024 09:40:27 -0800 Subject: [PATCH 6/7] use Optional for type annotations --- google/api_core/retry/retry_base.py | 12 ++++++------ google/api_core/retry/retry_unary.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/google/api_core/retry/retry_base.py b/google/api_core/retry/retry_base.py index 589994d5..1606e0fe 100644 --- a/google/api_core/retry/retry_base.py +++ b/google/api_core/retry/retry_base.py @@ -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 @@ -238,8 +238,8 @@ def __init__( initial: float = _DEFAULT_INITIAL_DELAY, maximum: float = _DEFAULT_MAXIMUM_DELAY, multiplier: float = _DEFAULT_DELAY_MULTIPLIER, - timeout: float | None = _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 @@ -320,9 +320,9 @@ def with_predicate(self, predicate: Callable[[Exception], bool]) -> Self: 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. diff --git a/google/api_core/retry/retry_unary.py b/google/api_core/retry/retry_unary.py index ae59d514..ab1b4030 100644 --- a/google/api_core/retry/retry_unary.py +++ b/google/api_core/retry/retry_unary.py @@ -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 From acf7c43f8cfbdaca330ce64059fd1f3a3b2470a8 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 2 Feb 2024 14:38:08 -0800 Subject: [PATCH 7/7] added test cases to test_with_delay --- tests/unit/retry/test_retry_base.py | 32 +++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/tests/unit/retry/test_retry_base.py b/tests/unit/retry/test_retry_base.py index 3a3be342..a0c6776b 100644 --- a/tests/unit/retry/test_retry_base.py +++ b/tests/unit/retry/test_retry_base.py @@ -203,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