From 59b9070aff0f482bc28965f352d970503c37fe25 Mon Sep 17 00:00:00 2001 From: SamRemis Date: Wed, 19 Nov 2025 19:02:31 -0500 Subject: [PATCH 1/3] Add retry mode resolver and timeout error handling - Add retry mode resolver functionality with caching - Change default retry mode from simple to standard - Add deepcopy support to retry strategies - Update test coverage for new features # Conflicts: # packages/smithy-core/tests/unit/test_retries.py --- .../python/codegen/ClientGenerator.java | 26 ++++++++- .../codegen/generators/ConfigGenerator.java | 25 ++++---- .../src/smithy_core/interfaces/retries.py | 1 + .../smithy-core/src/smithy_core/retries.py | 57 +++++++++++++++++++ .../smithy-core/tests/unit/test_retries.py | 36 +++++++++++- 5 files changed, 132 insertions(+), 13 deletions(-) diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java index 1cac89621..95c6e0d5a 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java @@ -83,6 +83,8 @@ private void generateService(PythonWriter writer) { } } + writer.addDependency(SmithyPythonDependency.SMITHY_CORE); + writer.addImport("smithy_core.retries", "RetryStrategyResolver"); writer.write(""" def __init__(self, config: $1T | None = None, plugins: list[$2T] | None = None): self._config = config or $1T() @@ -95,6 +97,8 @@ def __init__(self, config: $1T | None = None, plugins: list[$2T] | None = None): for plugin in client_plugins: plugin(self._config) + + self._retry_strategy_resolver = RetryStrategyResolver() """, configSymbol, pluginSymbol, writer.consumer(w -> writeDefaultPlugins(w, defaultPlugins))); var topDownIndex = TopDownIndex.of(model); @@ -187,6 +191,8 @@ private void writeSharedOperationInit(PythonWriter writer, OperationShape operat writer.addImport("smithy_core.types", "TypedProperties"); writer.addImport("smithy_core.aio.client", "RequestPipeline"); writer.addImport("smithy_core.exceptions", "ExpectationNotMetError"); + writer.addImport("smithy_core.retries", "RetryStrategyOptions"); + writer.addImport("smithy_core.interfaces.retries", "RetryStrategy"); writer.addStdlibImport("copy", "deepcopy"); writer.write(""" @@ -200,6 +206,24 @@ private void writeSharedOperationInit(PythonWriter writer, OperationShape operat plugin(config) if config.protocol is None or config.transport is None: raise ExpectationNotMetError("protocol and transport MUST be set on the config to make calls.") + + # Resolve retry strategy from config + if isinstance(config.retry_strategy, RetryStrategy): + retry_strategy = config.retry_strategy + elif isinstance(config.retry_strategy, RetryStrategyOptions): + retry_strategy = await self._retry_strategy_resolver.resolve_retry_strategy( + options=config.retry_strategy + ) + elif config.retry_strategy is None: + retry_strategy = await self._retry_strategy_resolver.resolve_retry_strategy( + options=RetryStrategyOptions() + ) + else: + raise TypeError( + f"retry_strategy must be RetryStrategy, RetryStrategyOptions, or None, " + f"got {type(config.retry_strategy).__name__}" + ) + pipeline = RequestPipeline( protocol=config.protocol, transport=config.transport @@ -212,7 +236,7 @@ raise ExpectationNotMetError("protocol and transport MUST be set on the config t auth_scheme_resolver=config.auth_scheme_resolver, supported_auth_schemes=config.auth_schemes, endpoint_resolver=config.endpoint_resolver, - retry_strategy=config.retry_strategy, + retry_strategy=retry_strategy, ) """, writer.consumer(w -> writeDefaultPlugins(w, defaultPlugins))); diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ConfigGenerator.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ConfigGenerator.java index 45b6324d7..b17847ba9 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ConfigGenerator.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ConfigGenerator.java @@ -55,17 +55,20 @@ public final class ConfigGenerator implements Runnable { ConfigProperty.builder() .name("retry_strategy") .type(Symbol.builder() - .name("RetryStrategy") - .namespace("smithy_core.interfaces.retries", ".") - .addDependency(SmithyPythonDependency.SMITHY_CORE) + .name("RetryStrategy | RetryStrategyOptions") + .addReference(Symbol.builder() + .name("RetryStrategy") + .namespace("smithy_core.interfaces.retries", ".") + .addDependency(SmithyPythonDependency.SMITHY_CORE) + .build()) + .addReference(Symbol.builder() + .name("RetryStrategyOptions") + .namespace("smithy_core.retries", ".") + .addDependency(SmithyPythonDependency.SMITHY_CORE) + .build()) .build()) - .documentation("The retry strategy for issuing retry tokens and computing retry delays.") - .nullable(false) - .initialize(writer -> { - writer.addDependency(SmithyPythonDependency.SMITHY_CORE); - writer.addImport("smithy_core.retries", "SimpleRetryStrategy"); - writer.write("self.retry_strategy = retry_strategy or SimpleRetryStrategy()"); - }) + .documentation( + "The retry strategy or options for configuring retry behavior. Can be either a configured RetryStrategy or RetryStrategyOptions to create one.") .build(), ConfigProperty.builder() .name("endpoint_uri") @@ -379,7 +382,7 @@ private void writeInitParams(PythonWriter writer, Collection pro } private void documentProperties(PythonWriter writer, Collection properties) { - writer.writeDocs(() ->{ + writer.writeDocs(() -> { var iter = properties.iterator(); writer.write("\nConstructor.\n"); while (iter.hasNext()) { diff --git a/packages/smithy-core/src/smithy_core/interfaces/retries.py b/packages/smithy-core/src/smithy_core/interfaces/retries.py index 8ba0b00cc..a7f8c4e2d 100644 --- a/packages/smithy-core/src/smithy_core/interfaces/retries.py +++ b/packages/smithy-core/src/smithy_core/interfaces/retries.py @@ -55,6 +55,7 @@ class RetryToken(Protocol): """Delay in seconds to wait before the retry attempt.""" +@runtime_checkable class RetryStrategy(Protocol): """Issuer of :py:class:`RetryToken`s.""" diff --git a/packages/smithy-core/src/smithy_core/retries.py b/packages/smithy-core/src/smithy_core/retries.py index cbb2d3017..3032f1053 100644 --- a/packages/smithy-core/src/smithy_core/retries.py +++ b/packages/smithy-core/src/smithy_core/retries.py @@ -5,9 +5,60 @@ from collections.abc import Callable from dataclasses import dataclass from enum import Enum +from functools import lru_cache +from typing import Any, Literal from .exceptions import RetryError from .interfaces import retries as retries_interface +from .interfaces.retries import RetryStrategy + +RetryStrategyType = Literal["simple", "standard"] + + +@dataclass(kw_only=True, frozen=True) +class RetryStrategyOptions: + """Options for configuring retry behavior.""" + + retry_mode: RetryStrategyType = "standard" + """The retry mode to use.""" + + max_attempts: int | None = None + """Maximum number of attempts (initial attempt plus retries). If None, uses the strategy's default.""" + + +class RetryStrategyResolver: + """Retry strategy resolver that caches retry strategies based on configuration options. + + This resolver caches retry strategy instances based on their configuration to reuse existing + instances of RetryStrategy with the same settings. Uses LRU cache for thread-safe caching. + """ + + async def resolve_retry_strategy( + self, *, options: RetryStrategyOptions + ) -> RetryStrategy: + """Resolve a retry strategy from the provided options, using cache when possible. + + :param options: The retry strategy options to use for creating the strategy. + """ + return self._create_retry_strategy(options.retry_mode, options.max_attempts) + + @lru_cache + def _create_retry_strategy( + self, retry_mode: RetryStrategyType, max_attempts: int | None + ) -> RetryStrategy: + match retry_mode: + case "simple": + if max_attempts is None: + return SimpleRetryStrategy() + else: + return SimpleRetryStrategy(max_attempts=max_attempts) + case "standard": + if max_attempts is None: + return StandardRetryStrategy() + else: + return StandardRetryStrategy(max_attempts=max_attempts) + case _: + raise ValueError(f"Unknown retry mode: {retry_mode}") class ExponentialBackoffJitterType(Enum): @@ -244,6 +295,9 @@ def refresh_retry_token_for_retry( def record_success(self, *, token: retries_interface.RetryToken) -> None: """Not used by this retry strategy.""" + def __deepcopy__(self, memo: Any) -> "SimpleRetryStrategy": + return self + class StandardRetryQuota: """Retry quota used by :py:class:`StandardRetryStrategy`.""" @@ -414,3 +468,6 @@ def record_success(self, *, token: retries_interface.RetryToken) -> None: f"StandardRetryStrategy requires StandardRetryToken, got {type(token).__name__}" ) self._retry_quota.release(release_amount=token.quota_acquired) + + def __deepcopy__(self, memo: Any) -> "StandardRetryStrategy": + return self diff --git a/packages/smithy-core/tests/unit/test_retries.py b/packages/smithy-core/tests/unit/test_retries.py index a1ef164ea..18f9e380c 100644 --- a/packages/smithy-core/tests/unit/test_retries.py +++ b/packages/smithy-core/tests/unit/test_retries.py @@ -1,11 +1,12 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 - import pytest from smithy_core.exceptions import CallError, RetryError from smithy_core.retries import ExponentialBackoffJitterType as EBJT from smithy_core.retries import ( ExponentialRetryBackoffStrategy, + RetryStrategyOptions, + RetryStrategyResolver, SimpleRetryStrategy, StandardRetryQuota, StandardRetryStrategy, @@ -217,3 +218,36 @@ def test_retry_quota_acquire_timeout_error( acquired = retry_quota.acquire(error=timeout_error) assert acquired == StandardRetryQuota.TIMEOUT_RETRY_COST assert retry_quota.available_capacity == 0 + + +async def test_caching_retry_strategy_default_resolution() -> None: + resolver = RetryStrategyResolver() + options = RetryStrategyOptions() + + strategy = await resolver.resolve_retry_strategy(options=options) + + assert isinstance(strategy, StandardRetryStrategy) + assert strategy.max_attempts == 3 + + +async def test_caching_retry_strategy_resolver_creates_strategies_by_options() -> None: + resolver = RetryStrategyResolver() + + options1 = RetryStrategyOptions(max_attempts=3) + options2 = RetryStrategyOptions(max_attempts=5) + + strategy1 = await resolver.resolve_retry_strategy(options=options1) + strategy2 = await resolver.resolve_retry_strategy(options=options2) + + assert strategy1.max_attempts == 3 + assert strategy2.max_attempts == 5 + + +async def test_caching_retry_strategy_resolver_caches_strategies() -> None: + resolver = RetryStrategyResolver() + + options = RetryStrategyOptions(max_attempts=5) + strategy1 = await resolver.resolve_retry_strategy(options=options) + strategy2 = await resolver.resolve_retry_strategy(options=options) + + assert strategy1 is strategy2 From f88f07f4a7beae4e1977c0de6aa6f62622f38261 Mon Sep 17 00:00:00 2001 From: SamRemis Date: Wed, 19 Nov 2025 19:13:20 -0500 Subject: [PATCH 2/3] Update _create_retry_strategy to use kwargs --- packages/smithy-core/src/smithy_core/retries.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/smithy-core/src/smithy_core/retries.py b/packages/smithy-core/src/smithy_core/retries.py index 3032f1053..b7c6d95ba 100644 --- a/packages/smithy-core/src/smithy_core/retries.py +++ b/packages/smithy-core/src/smithy_core/retries.py @@ -46,17 +46,14 @@ async def resolve_retry_strategy( def _create_retry_strategy( self, retry_mode: RetryStrategyType, max_attempts: int | None ) -> RetryStrategy: + kwargs: dict[str, Any] = ( + {} if max_attempts is None else {"max_attempts": max_attempts} + ) match retry_mode: case "simple": - if max_attempts is None: - return SimpleRetryStrategy() - else: - return SimpleRetryStrategy(max_attempts=max_attempts) + return SimpleRetryStrategy(**kwargs) case "standard": - if max_attempts is None: - return StandardRetryStrategy() - else: - return StandardRetryStrategy(max_attempts=max_attempts) + return StandardRetryStrategy(**kwargs) case _: raise ValueError(f"Unknown retry mode: {retry_mode}") From ebe6fa017bed0f49accb45350ad38418a1fb686a Mon Sep 17 00:00:00 2001 From: SamRemis Date: Thu, 20 Nov 2025 13:11:27 -0500 Subject: [PATCH 3/3] Update packages/smithy-core/src/smithy_core/retries.py Co-authored-by: Nate Prewitt --- packages/smithy-core/src/smithy_core/retries.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/smithy-core/src/smithy_core/retries.py b/packages/smithy-core/src/smithy_core/retries.py index b7c6d95ba..ce990e6b4 100644 --- a/packages/smithy-core/src/smithy_core/retries.py +++ b/packages/smithy-core/src/smithy_core/retries.py @@ -46,14 +46,15 @@ async def resolve_retry_strategy( def _create_retry_strategy( self, retry_mode: RetryStrategyType, max_attempts: int | None ) -> RetryStrategy: - kwargs: dict[str, Any] = ( - {} if max_attempts is None else {"max_attempts": max_attempts} - ) + kwargs = {"max_attempts": max_attempts} + filtered_kwargs: dict[str, Any] = { + k: v for k, v in kwargs.items() if v is not None + } match retry_mode: case "simple": - return SimpleRetryStrategy(**kwargs) + return SimpleRetryStrategy(**filtered_kwargs) case "standard": - return StandardRetryStrategy(**kwargs) + return StandardRetryStrategy(**filtered_kwargs) case _: raise ValueError(f"Unknown retry mode: {retry_mode}")