From fb12b5d9be3e70cb95b162d1b8bae2c4ac3e03c2 Mon Sep 17 00:00:00 2001 From: Naoto Ono Date: Wed, 5 Mar 2025 12:41:29 +0900 Subject: [PATCH 1/3] [AIENG-73] Disable retry when ReadTimeout occurs --- launchable/utils/http_client.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/launchable/utils/http_client.py b/launchable/utils/http_client.py index c5d3eeca9..118d9aea5 100644 --- a/launchable/utils/http_client.py +++ b/launchable/utils/http_client.py @@ -8,7 +8,7 @@ from click import Context from requests import Session from requests.adapters import HTTPAdapter -from requests.packages.urllib3.util.retry import Retry # type: ignore +from urllib3.util.retry import Retry from launchable.version import __version__ @@ -51,6 +51,9 @@ def __init__(self, base_url: str = "", session: Optional[Session] = None, if session is None: strategy = Retry( total=3, + # When Launchable server is unstable, ReadTimeout can occur. + # To prevent the execution from slowing down, we disable retrying the request in this case. + read=0, allowed_methods=["GET", "PUT", "PATCH", "DELETE"], status_forcelist=[429, 500, 502, 503, 504], backoff_factor=2 From bec461416041da555333a3211bea9947750b402c Mon Sep 17 00:00:00 2001 From: Naoto Ono Date: Wed, 5 Mar 2025 13:57:11 +0900 Subject: [PATCH 2/3] Fix based on reviews --- launchable/commands/subset.py | 14 ++++++++++++++ launchable/utils/http_client.py | 25 +++++++++++++++---------- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/launchable/commands/subset.py b/launchable/commands/subset.py index f69b5056c..112686020 100644 --- a/launchable/commands/subset.py +++ b/launchable/commands/subset.py @@ -8,9 +8,12 @@ from typing import Any, Callable, Dict, List, Optional, Sequence, TextIO, Tuple, Union import click +from requests import Session +from requests.adapters import HTTPAdapter from tabulate import tabulate from launchable.utils.authentication import get_org_workspace +from launchable.utils.http_client import MAX_RETRIES, default_retry_strategy from launchable.utils.session import parse_session from launchable.utils.tracking import Tracking, TrackingClient @@ -505,8 +508,19 @@ def run(self): else: try: test_runner = context.invoked_subcommand + + strategy = default_retry_strategy() + # Requesting a subset may require retrying the requests because it takes time to load + # models on the Launchable side. + # Therefore, we make an exception to allow retries. + strategy.read = MAX_RETRIES + adapter = HTTPAdapter(max_retries=strategy) + s = Session() + s.mount("http://", adapter) + s.mount("https://", adapter) client = LaunchableClient( test_runner=test_runner, + session=s, app=app, tracking_client=tracking_client) diff --git a/launchable/utils/http_client.py b/launchable/utils/http_client.py index 118d9aea5..9956e93ea 100644 --- a/launchable/utils/http_client.py +++ b/launchable/utils/http_client.py @@ -24,11 +24,25 @@ DEFAULT_TIMEOUT: Tuple[int, int] = (5, 60) DEFAULT_GET_TIMEOUT: Tuple[int, int] = (5, 15) +MAX_RETRIES = 3 + def get_base_url(): return os.getenv(BASE_URL_KEY) or DEFAULT_BASE_URL +def default_retry_strategy(): + return Retry( + total=MAX_RETRIES, + # When Launchable server is unstable, ReadTimeout can occur. + # To prevent the execution from slowing down, we disable retrying the request in this case. + read=0, + allowed_methods=["GET", "PUT", "PATCH", "DELETE"], + status_forcelist=[429, 500, 502, 503, 504], + backoff_factor=2 + ) + + class DryRunResponse: def __init__(self, status_code, payload): self.status_code = status_code @@ -49,16 +63,7 @@ def __init__(self, base_url: str = "", session: Optional[Session] = None, self.skip_cert_verification = bool(app and app.skip_cert_verification) if session is None: - strategy = Retry( - total=3, - # When Launchable server is unstable, ReadTimeout can occur. - # To prevent the execution from slowing down, we disable retrying the request in this case. - read=0, - allowed_methods=["GET", "PUT", "PATCH", "DELETE"], - status_forcelist=[429, 500, 502, 503, 504], - backoff_factor=2 - ) - + strategy = default_retry_strategy() adapter = HTTPAdapter(max_retries=strategy) s = Session() s.mount("http://", adapter) From 9875b3061e70addcfab299606a496b46e65497b9 Mon Sep 17 00:00:00 2001 From: Naoto Ono Date: Thu, 6 Mar 2025 11:48:46 +0900 Subject: [PATCH 3/3] Fix based on reviews --- launchable/commands/subset.py | 14 -------------- launchable/utils/env_keys.py | 1 + launchable/utils/http_client.py | 28 +++++++++++++--------------- 3 files changed, 14 insertions(+), 29 deletions(-) diff --git a/launchable/commands/subset.py b/launchable/commands/subset.py index 112686020..f69b5056c 100644 --- a/launchable/commands/subset.py +++ b/launchable/commands/subset.py @@ -8,12 +8,9 @@ from typing import Any, Callable, Dict, List, Optional, Sequence, TextIO, Tuple, Union import click -from requests import Session -from requests.adapters import HTTPAdapter from tabulate import tabulate from launchable.utils.authentication import get_org_workspace -from launchable.utils.http_client import MAX_RETRIES, default_retry_strategy from launchable.utils.session import parse_session from launchable.utils.tracking import Tracking, TrackingClient @@ -508,19 +505,8 @@ def run(self): else: try: test_runner = context.invoked_subcommand - - strategy = default_retry_strategy() - # Requesting a subset may require retrying the requests because it takes time to load - # models on the Launchable side. - # Therefore, we make an exception to allow retries. - strategy.read = MAX_RETRIES - adapter = HTTPAdapter(max_retries=strategy) - s = Session() - s.mount("http://", adapter) - s.mount("https://", adapter) client = LaunchableClient( test_runner=test_runner, - session=s, app=app, tracking_client=tracking_client) diff --git a/launchable/utils/env_keys.py b/launchable/utils/env_keys.py index ed1980a7f..a45af6cc0 100644 --- a/launchable/utils/env_keys.py +++ b/launchable/utils/env_keys.py @@ -3,3 +3,4 @@ ORGANIZATION_KEY = "LAUNCHABLE_ORGANIZATION" WORKSPACE_KEY = "LAUNCHABLE_WORKSPACE" BASE_URL_KEY = "LAUNCHABLE_BASE_URL" +SKIP_TIMEOUT_RETRY = "LAUNCHABLE_SKIP_TIMEOUT_RETRY" diff --git a/launchable/utils/http_client.py b/launchable/utils/http_client.py index 9956e93ea..0044ecb99 100644 --- a/launchable/utils/http_client.py +++ b/launchable/utils/http_client.py @@ -8,13 +8,13 @@ from click import Context from requests import Session from requests.adapters import HTTPAdapter -from urllib3.util.retry import Retry +from requests.packages.urllib3.util.retry import Retry # type: ignore from launchable.version import __version__ from ..app import Application from .authentication import authentication_headers -from .env_keys import BASE_URL_KEY +from .env_keys import BASE_URL_KEY, SKIP_TIMEOUT_RETRY from .gzipgen import compress as gzipgen_compress from .logger import AUDIT_LOG_FORMAT, Logger @@ -31,18 +31,6 @@ def get_base_url(): return os.getenv(BASE_URL_KEY) or DEFAULT_BASE_URL -def default_retry_strategy(): - return Retry( - total=MAX_RETRIES, - # When Launchable server is unstable, ReadTimeout can occur. - # To prevent the execution from slowing down, we disable retrying the request in this case. - read=0, - allowed_methods=["GET", "PUT", "PATCH", "DELETE"], - status_forcelist=[429, 500, 502, 503, 504], - backoff_factor=2 - ) - - class DryRunResponse: def __init__(self, status_code, payload): self.status_code = status_code @@ -63,7 +51,17 @@ def __init__(self, base_url: str = "", session: Optional[Session] = None, self.skip_cert_verification = bool(app and app.skip_cert_verification) if session is None: - strategy = default_retry_strategy() + read = MAX_RETRIES + if os.getenv(SKIP_TIMEOUT_RETRY): + read = 0 + strategy = Retry( + total=MAX_RETRIES, + read=read, + allowed_methods=["GET", "PUT", "PATCH", "DELETE"], + status_forcelist=[429, 500, 502, 503, 504], + backoff_factor=2 + ) + adapter = HTTPAdapter(max_retries=strategy) s = Session() s.mount("http://", adapter)