From 7eb52b3a3521c69ff9d3306d1f65e5d2f6e93239 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Fri, 2 Aug 2019 17:20:10 +0200 Subject: [PATCH 01/26] Factorize retrying and allow to retry on status code and requests exceptions --- deepomatic/api/client.py | 7 ++++++- deepomatic/api/http_helper.py | 34 +++++++++++++++++++++++++++++--- deepomatic/api/resources/task.py | 30 ++++++++++++++-------------- deepomatic/api/utils.py | 28 ++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 19 deletions(-) diff --git a/deepomatic/api/client.py b/deepomatic/api/client.py index b3bc902..5179f1e 100644 --- a/deepomatic/api/client.py +++ b/deepomatic/api/client.py @@ -44,7 +44,7 @@ def __init__(self, *args, **kwargs): If it fails raise a `DeepomaticException`. :type api_key: string :param verify_ssl (optional): whether to ask `requests` to verify the TLS/SSL certificates. - Defaults to `None`. + Defaults to `None`. If `None` try to get it from the `DEEPOMATIC_API_VERIFY_TLS` environment variable (`0`: False, `1`: True). If not found it is set to True. :type verify_ssl: bool @@ -60,6 +60,11 @@ def __init__(self, *args, **kwargs): :param pool_maxsize (optional): Set `requests.adapters.HTTPAdapter.pool_maxsize` for concurrent calls. Defaults to 20. :type pool_maxsize: int + :param retry_if (optional): predicate to retry on requests errors. + More details directly in tenacity source code: + - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/__init__.py#L179 + - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/retry.py + :type retry_if: tenacity.retry_base :return: :class:`Client` object :rtype: deepomatic.api.client.Client diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 8aa755c..782736a 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -25,22 +25,32 @@ import os import json import requests +from requests.exceptions import RequestException import sys import platform from requests.structures import CaseInsensitiveDict from six import string_types +from tenacity import retry_if_result, retry_if_exception_type + +from deepomatic.api.utils import Functor, retry from deepomatic.api.exceptions import DeepomaticException, BadStatus from deepomatic.api.version import __title__, __version__ API_HOST = 'https://api.deepomatic.com' API_VERSION = 0.7 +DEFAULT_RETRY_STATUS_CODES = [502, 503, 504] +DEFAULT_RETRY_EXCEPTION_TYPES = [RequestException] + ############################################################################### class HTTPHelper(object): - def __init__(self, app_id=None, api_key=None, verify_ssl=None, host=None, version=API_VERSION, check_query_parameters=True, user_agent_prefix='', user_agent_suffix='', pool_maxsize=20): + def __init__(self, app_id=None, api_key=None, verify_ssl=None, + host=None, version=API_VERSION, check_query_parameters=True, + user_agent_prefix='', user_agent_suffix='', pool_maxsize=20, + retry_if=None): """ Init the HTTP helper with API key and secret """ @@ -55,6 +65,15 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, host=None, versio if app_id is None or api_key is None: raise DeepomaticException("Please specify 'app_id' and 'api_key' either by passing those values to the client or by defining the DEEPOMATIC_APP_ID and DEEPOMATIC_API_KEY environment variables.") + self.retry_status_code = {} + self.retry_if = retry_if + + if self.retry_if is None: + self.retry_status_code = set(DEFAULT_RETRY_STATUS_CODES) + self.retry_if = retry_if_result(self.retry_if_status_code) + for exception_type in DEFAULT_RETRY_EXCEPTION_TYPES: + self.retry_if |= retry_if_exception_type(exception_type) + if not isinstance(version, string_types): version = 'v%g' % version elif version[0] != 'v': @@ -157,7 +176,12 @@ def recursive_json_dump(prefix, obj, data_dict, omit_dot=False): return new_dict - def make_request(self, func, resource, params=None, data=None, content_type='application/json', files=None, stream=False, *args, **kwargs): + def retry_if_status_code(self, response): + return response.status_code in self.retry_status_code + + def make_request(self, func, resource, params=None, data=None, + content_type='application/json', files=None, + stream=False, *args, **kwargs): if content_type is not None: if content_type.strip() == 'application/json': @@ -204,7 +228,11 @@ def make_request(self, func, resource, params=None, data=None, content_type='app if not resource.startswith('http'): resource = self.resource_prefix + resource - response = func(resource, params=params, data=data, files=files, headers=headers, verify=self.verify, stream=stream, *args, **kwargs) + + functor = Functor(func, resource, *args, params=params, + data=data, files=files, headers=headers, + verify=self.verify, stream=stream, **kwargs) + response = retry(functor, self.retry_if) # Close opened files for file in opened_files: diff --git a/deepomatic/api/resources/task.py b/deepomatic/api/resources/task.py index d6ba23d..85dc773 100644 --- a/deepomatic/api/resources/task.py +++ b/deepomatic/api/resources/task.py @@ -22,8 +22,9 @@ THE SOFTWARE. """ -from tenacity import Retrying, wait_random_exponential, stop_after_delay, retry_if_result, before_log, after_log, RetryError +from tenacity import RetryError, retry_if_result +from deepomatic.api.utils import retry, Functor from deepomatic.api.resource import Resource from deepomatic.api.mixins import ListableResource from deepomatic.api.exceptions import TaskError, TaskTimeout @@ -62,17 +63,14 @@ def list(self, task_ids): assert(isinstance(task_ids, list)) return super(Task, self).list(task_ids=task_ids) - def wait(self, timeout=60, wait_exp_multiplier=0.05, wait_exp_max=1.0): + def wait(self, **retry_kwargs): """ Wait until task is completed. Expires after 'timeout' seconds. """ try: - retryer = Retrying(wait=wait_random_exponential(multiplier=wait_exp_multiplier, max=wait_exp_max), - stop=stop_after_delay(timeout), - retry=retry_if_result(is_pending_status), - before=before_log(logger, logging.DEBUG), - after=after_log(logger, logging.DEBUG)) - retryer(self._refresh_status) + retry(self._refresh_status, + retry_if_result(is_pending_status), + **retry_kwargs) except RetryError: raise TaskTimeout(self.data()) @@ -106,7 +104,7 @@ def _refresh_tasks_status(self, pending_tasks, success_tasks, error_tasks, posit return pending_tasks - def batch_wait(self, tasks, timeout=300, wait_exp_multiplier=0.05, wait_exp_max=1.0): + def batch_wait(self, tasks, **retry_kwargs): """ Wait until a list of task are completed. Expires after 'timeout' seconds. @@ -114,6 +112,7 @@ def batch_wait(self, tasks, timeout=300, wait_exp_multiplier=0.05, wait_exp_max= Each list contains a couple (original_position, task) sorted by original_position asc original_position gives the original index in the input tasks list parameter. This helps to keep the order. """ + retry_kwargs['timeout'] = retry_kwargs.get('timeout', 300) try: positions = {} pending_tasks = [] @@ -122,12 +121,13 @@ def batch_wait(self, tasks, timeout=300, wait_exp_multiplier=0.05, wait_exp_max= pending_tasks.append((pos, task)) success_tasks = [] error_tasks = [] - retryer = Retrying(wait=wait_random_exponential(multiplier=wait_exp_multiplier, max=wait_exp_max), - stop=stop_after_delay(timeout), - retry=retry_if_result(has_pending_tasks), - before=before_log(logger, logging.DEBUG), - after=after_log(logger, logging.DEBUG)) - retryer(self._refresh_tasks_status, pending_tasks, success_tasks, error_tasks, positions) + + functor = Functor(self._refresh_tasks_status, pending_tasks, + success_tasks, error_tasks, positions) + retry(functor, + retry_if_result(has_pending_tasks), + **retry_kwargs) + except RetryError: pass diff --git a/deepomatic/api/utils.py b/deepomatic/api/utils.py index 2b94b48..c722670 100644 --- a/deepomatic/api/utils.py +++ b/deepomatic/api/utils.py @@ -22,11 +22,19 @@ THE SOFTWARE. """ +import logging +from tenacity import (Retrying, wait_random_exponential, + stop_after_delay, retry_if_result, + retry_if_exception_type, + before_log, after_log) from deepomatic.api.exceptions import DeepomaticException from deepomatic.api.resources.task import Task from deepomatic.api.inputs import format_inputs +logger = logging.getLogger(__name__) + + ############################################################################### class InferenceResource(object): @@ -50,3 +58,23 @@ def inference(self, return_task=False, wait_task=True, **kwargs): ############################################################################### + +class Functor(object): + def __init__(self, func, *args, **kwargs): + self.func = func + self.args = args + self.kwargs = kwargs + + def __call__(self): + return self.func(*self.args, **self.kwargs) + + +def retry(apply_func, retry, + timeout=60, wait_exp_multiplier=0.05, wait_exp_max=1.0): + retryer = Retrying(wait=wait_random_exponential(multiplier=wait_exp_multiplier, + max=wait_exp_max), + stop=stop_after_delay(timeout), + retry=retry, + before=before_log(logger, logging.DEBUG), + after=after_log(logger, logging.DEBUG)) + return retryer(apply_func) From e14c5ef7f5511192e2157273d5ee6cc8e94bf83d Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Fri, 2 Aug 2019 18:35:23 +0200 Subject: [PATCH 02/26] break circular import --- deepomatic/api/inference.py | 23 +++++++++++++++++++++ deepomatic/api/resources/network.py | 2 +- deepomatic/api/resources/recognition.py | 3 +-- deepomatic/api/utils.py | 27 ------------------------- 4 files changed, 25 insertions(+), 30 deletions(-) create mode 100644 deepomatic/api/inference.py diff --git a/deepomatic/api/inference.py b/deepomatic/api/inference.py new file mode 100644 index 0000000..1c94f01 --- /dev/null +++ b/deepomatic/api/inference.py @@ -0,0 +1,23 @@ +from deepomatic.api.exceptions import DeepomaticException +from deepomatic.api.resources.task import Task +from deepomatic.api.inputs import format_inputs + + +class InferenceResource(object): + def inference(self, return_task=False, wait_task=True, **kwargs): + assert(self._pk is not None) + + inputs = kwargs.pop('inputs', None) + if inputs is None: + raise DeepomaticException("Missing keyword argument: inputs") + content_type, data, files = format_inputs(inputs, kwargs) + result = self._helper.post(self._uri(pk=self._pk, suffix='/inference'), content_type=content_type, data=data, files=files) + task_id = result['task_id'] + task = Task(self._helper, pk=task_id) + if wait_task: + task.wait() + + if return_task: + return task + else: + return task['data'] diff --git a/deepomatic/api/resources/network.py b/deepomatic/api/resources/network.py index 28dae55..b1726b2 100644 --- a/deepomatic/api/resources/network.py +++ b/deepomatic/api/resources/network.py @@ -26,7 +26,7 @@ import numpy as np from deepomatic.api.resource import Resource -from deepomatic.api.utils import InferenceResource +from deepomatic.api.inference import InferenceResource from deepomatic.api.mixins import CreateableResource, ListableResource, UpdatableResource, DeletableResource from deepomatic.api.mixins import RequiredArg, OptionnalArg, ImmutableArg diff --git a/deepomatic/api/resources/recognition.py b/deepomatic/api/resources/recognition.py index b8e2974..d46b212 100644 --- a/deepomatic/api/resources/recognition.py +++ b/deepomatic/api/resources/recognition.py @@ -25,7 +25,7 @@ from six import string_types from deepomatic.api.resource import Resource, ResourceList -from deepomatic.api.utils import InferenceResource +from deepomatic.api.inference import InferenceResource from deepomatic.api.mixins import CreateableResource, ListableResource, UpdatableResource, DeletableResource from deepomatic.api.mixins import RequiredArg, OptionnalArg, ImmutableArg, UpdateOnlyArg @@ -76,4 +76,3 @@ class RecognitionVersion(CreateableResource, 'network_id': RequiredArg(), 'post_processings': RequiredArg(), } - diff --git a/deepomatic/api/utils.py b/deepomatic/api/utils.py index c722670..8828af1 100644 --- a/deepomatic/api/utils.py +++ b/deepomatic/api/utils.py @@ -27,38 +27,11 @@ stop_after_delay, retry_if_result, retry_if_exception_type, before_log, after_log) -from deepomatic.api.exceptions import DeepomaticException -from deepomatic.api.resources.task import Task -from deepomatic.api.inputs import format_inputs logger = logging.getLogger(__name__) -############################################################################### - -class InferenceResource(object): - def inference(self, return_task=False, wait_task=True, **kwargs): - assert(self._pk is not None) - - inputs = kwargs.pop('inputs', None) - if inputs is None: - raise DeepomaticException("Missing keyword argument: inputs") - content_type, data, files = format_inputs(inputs, kwargs) - result = self._helper.post(self._uri(pk=self._pk, suffix='/inference'), content_type=content_type, data=data, files=files) - task_id = result['task_id'] - task = Task(self._helper, pk=task_id) - if wait_task: - task.wait() - - if return_task: - return task - else: - return task['data'] - - -############################################################################### - class Functor(object): def __init__(self, func, *args, **kwargs): self.func = func From bf5873c98496f694b2c050ebda818e93e4a7d735 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Fri, 2 Aug 2019 19:18:53 +0200 Subject: [PATCH 03/26] Test and doc --- deepomatic/api/client.py | 6 ++++++ deepomatic/api/http_helper.py | 5 +++-- deepomatic/api/utils.py | 10 ++++++++-- tests/test_client.py | 21 ++++++++++++++++----- 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/deepomatic/api/client.py b/deepomatic/api/client.py index 5179f1e..6a6317a 100644 --- a/deepomatic/api/client.py +++ b/deepomatic/api/client.py @@ -65,6 +65,12 @@ def __init__(self, *args, **kwargs): - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/__init__.py#L179 - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/retry.py :type retry_if: tenacity.retry_base + :param retry_kwargs (optional): dict of retry parameters: + - timeout (default=60): raise tenacity.RetryError when timeout is reached (in seconds). + If None, retry indefinitely. + - wait_exp_multiplier (default=0.05): wait exponential multiplier + -wait_exp_max (default=1.0): wait exponential maximum (in seconds) + :type retry_kwargs: dict :return: :class:`Client` object :rtype: deepomatic.api.client.Client diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 782736a..bef84d8 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -50,7 +50,7 @@ class HTTPHelper(object): def __init__(self, app_id=None, api_key=None, verify_ssl=None, host=None, version=API_VERSION, check_query_parameters=True, user_agent_prefix='', user_agent_suffix='', pool_maxsize=20, - retry_if=None): + retry_if=None, retry_kwargs=None): """ Init the HTTP helper with API key and secret """ @@ -67,6 +67,7 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, self.retry_status_code = {} self.retry_if = retry_if + self.retry_kwargs = retry_kwargs or {} if self.retry_if is None: self.retry_status_code = set(DEFAULT_RETRY_STATUS_CODES) @@ -232,7 +233,7 @@ def make_request(self, func, resource, params=None, data=None, functor = Functor(func, resource, *args, params=params, data=data, files=files, headers=headers, verify=self.verify, stream=stream, **kwargs) - response = retry(functor, self.retry_if) + response = retry(functor, self.retry_if, **self.retry_kwargs) # Close opened files for file in opened_files: diff --git a/deepomatic/api/utils.py b/deepomatic/api/utils.py index 8828af1..c1e7f50 100644 --- a/deepomatic/api/utils.py +++ b/deepomatic/api/utils.py @@ -25,7 +25,7 @@ import logging from tenacity import (Retrying, wait_random_exponential, stop_after_delay, retry_if_result, - retry_if_exception_type, + retry_if_exception_type, stop_never, before_log, after_log) @@ -44,9 +44,15 @@ def __call__(self): def retry(apply_func, retry, timeout=60, wait_exp_multiplier=0.05, wait_exp_max=1.0): + + if timeout is None: + stop = stop_never + else: + stop = stop_after_delay(timeout) + retryer = Retrying(wait=wait_random_exponential(multiplier=wait_exp_multiplier, max=wait_exp_max), - stop=stop_after_delay(timeout), + stop=stop, retry=retry, before=before_log(logger, logging.DEBUG), after=after_log(logger, logging.DEBUG)) diff --git a/tests/test_client.py b/tests/test_client.py index d861330..52ca421 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,18 +1,19 @@ import os import base64 -import tarfile import pytest import tempfile import hashlib import shutil import requests import zipfile +from tenacity import RetryError from deepomatic.api.version import __title__, __version__ from deepomatic.api.client import Client from deepomatic.api.inputs import ImageInput from pytest_voluptuous import S from voluptuous.validators import All, Length, Any import six +import time import logging logging.basicConfig(level=os.getenv('LOG_LEVEL', 'INFO')) @@ -20,6 +21,8 @@ DEMO_URL = "https://static.deepomatic.com/resources/demos/api-clients/dog1.jpg" +USER_AGENT_PREFIX = '{}-tests/{}'.format(__title__, __version__) + def ExactLen(nb): return Length(min=nb, max=nb) @@ -41,10 +44,7 @@ def download_file(url): @pytest.fixture(scope='session') def client(): - api_host = os.getenv('DEEPOMATIC_API_URL') - app_id = os.environ['DEEPOMATIC_APP_ID'] - api_key = os.environ['DEEPOMATIC_API_KEY'] - yield Client(app_id, api_key, host=api_host, user_agent_prefix='{}-tests/{}'.format(__title__, __version__)) + yield Client(user_agent_prefix=USER_AGENT_PREFIX) @pytest.fixture(scope='session') @@ -286,3 +286,14 @@ def test_batch_wait(self, client): for pos, success in success_tasks: assert(tasks[pos].pk == success.pk) assert inference_schema(2, 0, 'golden retriever', 0.8) == success['data'] + + +def test_retry_client(): + timeout = 5 + client = Client(host='http://unknown-domain.com', retry_kwargs={'timeout': timeout}) + spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') + start_time = time.time() + with pytest.raises(RetryError) as e: + print(spec.data()) + + assert time.time() - start_time > timeout From 5227c68b3ae4c533d31d161f8b5c62151d3b9ef9 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Fri, 2 Aug 2019 19:20:43 +0200 Subject: [PATCH 04/26] test --- tests/test_client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_client.py b/tests/test_client.py index 52ca421..8f22246 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -296,4 +296,5 @@ def test_retry_client(): with pytest.raises(RetryError) as e: print(spec.data()) - assert time.time() - start_time > timeout + diff = time.time() - start_time + assert diff > timeout and diff < timeout + 5 From 4fa079d5d1de45612559fffb5f6fedbad36018bf Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Fri, 2 Aug 2019 19:28:09 +0200 Subject: [PATCH 05/26] user agent --- tests/test_client.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_client.py b/tests/test_client.py index 8f22246..49f57e1 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -290,11 +290,13 @@ def test_batch_wait(self, client): def test_retry_client(): timeout = 5 - client = Client(host='http://unknown-domain.com', retry_kwargs={'timeout': timeout}) + client = Client(host='http://unknown-domain.com', + user_agent_prefix=USER_AGENT_PREFIX, + retry_kwargs={'timeout': timeout}) spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') start_time = time.time() - with pytest.raises(RetryError) as e: + with pytest.raises(RetryError): print(spec.data()) diff = time.time() - start_time - assert diff > timeout and diff < timeout + 5 + assert diff > timeout and diff < timeout + 2 From 97b07175d5ca79db45d19be9d1b59bda0751bf63 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Mon, 19 Aug 2019 15:02:50 +0200 Subject: [PATCH 06/26] Using functools.partial instead of Functor --- deepomatic/api/http_helper.py | 24 ++++++++++++------------ deepomatic/api/resources/task.py | 19 +++++++++---------- deepomatic/api/utils.py | 16 ++-------------- 3 files changed, 23 insertions(+), 36 deletions(-) diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index bef84d8..84ad6f2 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -22,20 +22,20 @@ THE SOFTWARE. """ -import os +import functools import json +import os +import platform +import sys + import requests +from deepomatic.api.exceptions import BadStatus, DeepomaticException +from deepomatic.api.utils import retry +from deepomatic.api.version import __title__, __version__ from requests.exceptions import RequestException -import sys -import platform from requests.structures import CaseInsensitiveDict from six import string_types - -from tenacity import retry_if_result, retry_if_exception_type - -from deepomatic.api.utils import Functor, retry -from deepomatic.api.exceptions import DeepomaticException, BadStatus -from deepomatic.api.version import __title__, __version__ +from tenacity import retry_if_exception_type, retry_if_result API_HOST = 'https://api.deepomatic.com' API_VERSION = 0.7 @@ -230,9 +230,9 @@ def make_request(self, func, resource, params=None, data=None, if not resource.startswith('http'): resource = self.resource_prefix + resource - functor = Functor(func, resource, *args, params=params, - data=data, files=files, headers=headers, - verify=self.verify, stream=stream, **kwargs) + functor = functools.partial(func, resource, *args, params=params, + data=data, files=files, headers=headers, + verify=self.verify, stream=stream, **kwargs) response = retry(functor, self.retry_if, **self.retry_kwargs) # Close opened files diff --git a/deepomatic/api/resources/task.py b/deepomatic/api/resources/task.py index 85dc773..353256f 100644 --- a/deepomatic/api/resources/task.py +++ b/deepomatic/api/resources/task.py @@ -23,13 +23,14 @@ """ -from tenacity import RetryError, retry_if_result -from deepomatic.api.utils import retry, Functor -from deepomatic.api.resource import Resource -from deepomatic.api.mixins import ListableResource -from deepomatic.api.exceptions import TaskError, TaskTimeout +import functools import logging +from deepomatic.api.exceptions import TaskError, TaskTimeout +from deepomatic.api.mixins import ListableResource +from deepomatic.api.resource import Resource +from deepomatic.api.utils import retry +from tenacity import RetryError, retry_if_result logger = logging.getLogger(__name__) @@ -122,11 +123,9 @@ def batch_wait(self, tasks, **retry_kwargs): success_tasks = [] error_tasks = [] - functor = Functor(self._refresh_tasks_status, pending_tasks, - success_tasks, error_tasks, positions) - retry(functor, - retry_if_result(has_pending_tasks), - **retry_kwargs) + functor = functools.partial(self._refresh_tasks_status, pending_tasks, + success_tasks, error_tasks, positions) + retry(functor, retry_if_result(has_pending_tasks), **retry_kwargs) except RetryError: pass diff --git a/deepomatic/api/utils.py b/deepomatic/api/utils.py index c1e7f50..1a69854 100644 --- a/deepomatic/api/utils.py +++ b/deepomatic/api/utils.py @@ -23,25 +23,13 @@ """ import logging -from tenacity import (Retrying, wait_random_exponential, - stop_after_delay, retry_if_result, - retry_if_exception_type, stop_never, - before_log, after_log) +from tenacity import (Retrying, after_log, before_log, stop_after_delay, + stop_never, wait_random_exponential) logger = logging.getLogger(__name__) -class Functor(object): - def __init__(self, func, *args, **kwargs): - self.func = func - self.args = args - self.kwargs = kwargs - - def __call__(self): - return self.func(*self.args, **self.kwargs) - - def retry(apply_func, retry, timeout=60, wait_exp_multiplier=0.05, wait_exp_max=1.0): From 433ee6fa4665de5777d1e2be90c070d0870bc9b9 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Tue, 20 Aug 2019 11:36:48 +0200 Subject: [PATCH 07/26] Improved tests with httpretty --- .travis.yml | 2 +- deploy/install.sh | 5 ++- tests/test_client.py | 72 +++++++++++++++++++++++++++++++------------- 3 files changed, 56 insertions(+), 23 deletions(-) diff --git a/.travis.yml b/.travis.yml index 39e9445..f7c19f4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,6 +14,6 @@ install: # command to run tests script: - - pip install pytest==4.0.2 pytest-voluptuous==1.1.0 # for testing + - pip install pytest==4.0.2 pytest-voluptuous==1.1.0 httpretty==0.9.6 # for testing - LOG_LEVEL=DEBUG python -m pytest --junit-xml=junit.xml -vv app/tests - LOG_LEVEL=DEBUG python app/demo.py diff --git a/deploy/install.sh b/deploy/install.sh index 2c05442..494bdbc 100755 --- a/deploy/install.sh +++ b/deploy/install.sh @@ -4,4 +4,7 @@ set -e apt-get update && apt-get install -y build-essential pip install -r requirements.txt -pip install pytest==4.0.2 pytest-cov==2.6.1 pytest-voluptuous==1.1.0 # for testing +pip install pytest==4.0.2 \ + pytest-cov==2.6.1 \ + pytest-voluptuous==1.1.0 \ + httpretty==0.9.6 # for testing diff --git a/tests/test_client.py b/tests/test_client.py index 49f57e1..17f0c97 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,21 +1,26 @@ -import os import base64 -import pytest -import tempfile import hashlib +import logging +import os +import re import shutil -import requests +import tempfile +import time import zipfile -from tenacity import RetryError -from deepomatic.api.version import __title__, __version__ + +import httpretty +import pytest +import requests +import six from deepomatic.api.client import Client from deepomatic.api.inputs import ImageInput +from deepomatic.api.version import __title__, __version__ +from requests.exceptions import ConnectionError +from tenacity import RetryError + from pytest_voluptuous import S -from voluptuous.validators import All, Length, Any -import six -import time +from voluptuous.validators import All, Any, Length -import logging logging.basicConfig(level=os.getenv('LOG_LEVEL', 'INFO')) logger = logging.getLogger(__name__) @@ -288,15 +293,40 @@ def test_batch_wait(self, client): assert inference_schema(2, 0, 'golden retriever', 0.8) == success['data'] -def test_retry_client(): - timeout = 5 - client = Client(host='http://unknown-domain.com', - user_agent_prefix=USER_AGENT_PREFIX, - retry_kwargs={'timeout': timeout}) - spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') - start_time = time.time() - with pytest.raises(RetryError): - print(spec.data()) +class TestClientRetry(object): + DEFAULT_TIMEOUT = 5 + DEFAULT_MIN_ATTEMPT_NUMBER = 10 - diff = time.time() - start_time - assert diff > timeout and diff < timeout + 2 + def expect_retry(self, client, timeout, min_attempt_number): + spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') + start_time = time.time() + with pytest.raises(RetryError) as exc: + print(spec.data()) + + diff = time.time() - start_time + assert diff > timeout and diff < timeout + 2 + last_attempt = exc.value.last_attempt + assert last_attempt.attempt_number > min_attempt_number + return last_attempt + + def test_retry_network_failure(self): + client = Client(host='http://unknown-domain.com', + retry_kwargs={'timeout': self.DEFAULT_TIMEOUT}) + last_attempt = self.expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) + exc = last_attempt.exception(timeout=0) + assert isinstance(exc, ConnectionError) + assert 'Name or service not known' in str(exc) + + @httpretty.activate + def test_retry_bad_http_status(self): + httpretty.register_uri( + httpretty.GET, + re.compile(r'https?://.*'), + status=502, + content_type="application/json", + ) + client = Client(retry_kwargs={'timeout': self.DEFAULT_TIMEOUT}) + last_attempt = self.expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) + assert last_attempt.exception(timeout=0) is None # no exception raised during retry + last_response = last_attempt.result() + assert 502 == last_response.status_code From e21c99ec248a36fd42b0000a80805c352c21718f Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Thu, 12 Sep 2019 19:06:23 +0200 Subject: [PATCH 08/26] Better handling of retries --- deepomatic/api/client.py | 18 ++---- deepomatic/api/http_helper.py | 34 ++++------- deepomatic/api/http_retryer.py | 91 +++++++++++++++++++++++++++++ deepomatic/api/mixins.py | 8 +-- deepomatic/api/resources/network.py | 23 ++++++-- deepomatic/api/resources/task.py | 34 ++++++++--- deepomatic/api/utils.py | 37 +++++++----- tests/test_client.py | 45 +++++++++----- 8 files changed, 208 insertions(+), 82 deletions(-) create mode 100644 deepomatic/api/http_retryer.py diff --git a/deepomatic/api/client.py b/deepomatic/api/client.py index 6a6317a..22a9744 100644 --- a/deepomatic/api/client.py +++ b/deepomatic/api/client.py @@ -23,10 +23,11 @@ """ from deepomatic.api.http_helper import HTTPHelper +from deepomatic.api.resources.account import Account from deepomatic.api.resources.network import Network -from deepomatic.api.resources.recognition import RecognitionSpec, RecognitionVersion +from deepomatic.api.resources.recognition import (RecognitionSpec, + RecognitionVersion) from deepomatic.api.resources.task import Task -from deepomatic.api.resources.account import Account class Client(object): @@ -60,17 +61,8 @@ def __init__(self, *args, **kwargs): :param pool_maxsize (optional): Set `requests.adapters.HTTPAdapter.pool_maxsize` for concurrent calls. Defaults to 20. :type pool_maxsize: int - :param retry_if (optional): predicate to retry on requests errors. - More details directly in tenacity source code: - - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/__init__.py#L179 - - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/retry.py - :type retry_if: tenacity.retry_base - :param retry_kwargs (optional): dict of retry parameters: - - timeout (default=60): raise tenacity.RetryError when timeout is reached (in seconds). - If None, retry indefinitely. - - wait_exp_multiplier (default=0.05): wait exponential multiplier - -wait_exp_max (default=1.0): wait exponential maximum (in seconds) - :type retry_kwargs: dict + :param http_retryer (optional): Customize the retry of http errors + :type http_retryer: http_retryer.HTTPRetryer :return: :class:`Client` object :rtype: deepomatic.api.client.Client diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 84ad6f2..3ce1811 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -22,7 +22,6 @@ THE SOFTWARE. """ -import functools import json import os import platform @@ -30,19 +29,14 @@ import requests from deepomatic.api.exceptions import BadStatus, DeepomaticException -from deepomatic.api.utils import retry +from deepomatic.api.http_retryer import HTTPRetryer from deepomatic.api.version import __title__, __version__ -from requests.exceptions import RequestException from requests.structures import CaseInsensitiveDict from six import string_types -from tenacity import retry_if_exception_type, retry_if_result API_HOST = 'https://api.deepomatic.com' API_VERSION = 0.7 -DEFAULT_RETRY_STATUS_CODES = [502, 503, 504] -DEFAULT_RETRY_EXCEPTION_TYPES = [RequestException] - ############################################################################### @@ -50,7 +44,7 @@ class HTTPHelper(object): def __init__(self, app_id=None, api_key=None, verify_ssl=None, host=None, version=API_VERSION, check_query_parameters=True, user_agent_prefix='', user_agent_suffix='', pool_maxsize=20, - retry_if=None, retry_kwargs=None): + http_retryer=None): """ Init the HTTP helper with API key and secret """ @@ -65,15 +59,7 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, if app_id is None or api_key is None: raise DeepomaticException("Please specify 'app_id' and 'api_key' either by passing those values to the client or by defining the DEEPOMATIC_APP_ID and DEEPOMATIC_API_KEY environment variables.") - self.retry_status_code = {} - self.retry_if = retry_if - self.retry_kwargs = retry_kwargs or {} - - if self.retry_if is None: - self.retry_status_code = set(DEFAULT_RETRY_STATUS_CODES) - self.retry_if = retry_if_result(self.retry_if_status_code) - for exception_type in DEFAULT_RETRY_EXCEPTION_TYPES: - self.retry_if |= retry_if_exception_type(exception_type) + self.http_retryer = http_retryer or HTTPRetryer() if not isinstance(version, string_types): version = 'v%g' % version @@ -177,9 +163,6 @@ def recursive_json_dump(prefix, obj, data_dict, omit_dot=False): return new_dict - def retry_if_status_code(self, response): - return response.status_code in self.retry_status_code - def make_request(self, func, resource, params=None, data=None, content_type='application/json', files=None, stream=False, *args, **kwargs): @@ -230,10 +213,13 @@ def make_request(self, func, resource, params=None, data=None, if not resource.startswith('http'): resource = self.resource_prefix + resource - functor = functools.partial(func, resource, *args, params=params, - data=data, files=files, headers=headers, - verify=self.verify, stream=stream, **kwargs) - response = retry(functor, self.retry_if, **self.retry_kwargs) + http_retryer = kwargs.pop('http_retryer', None) or self.http_retryer + + response = http_retryer.retry(func, resource, *args, + params=params, data=data, + files=files, headers=headers, + verify=self.verify, + stream=stream, **kwargs) # Close opened files for file in opened_files: diff --git a/deepomatic/api/http_retryer.py b/deepomatic/api/http_retryer.py new file mode 100644 index 0000000..2b1fe01 --- /dev/null +++ b/deepomatic/api/http_retryer.py @@ -0,0 +1,91 @@ +import functools + +from deepomatic.api.utils import retry +from requests.exceptions import (ProxyError, RequestException, + TooManyRedirects, URLRequired) +from tenacity import (retry_if_exception, retry_if_result, stop_after_delay, + wait_chain, wait_fixed, wait_random_exponential) + +DEFAULT_REQUESTS_TIMEOUT = (3.05, 10) +DEFAULT_RETRY_EXP_MAX = 10. +DEFAULT_RETRY_EXP_MULTIPLIER = 0.5 +DEFAULT_RETRY_STATUS_CODES = [502, 503, 504] +DEFAULT_RETRY_EXCEPTION_TYPES = (RequestException, ) +DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST = (ValueError, ProxyError, TooManyRedirects, URLRequired) + + +class retry_if_exception_type(retry_if_exception): + def __predicate(self, e): + return (isinstance(e, self.exception_types) and + not isinstance(e, self.exception_types_blacklist)) + + def __init__(self, exception_types=Exception, + exception_types_blacklist=()): + self.exception_types = exception_types + self.exception_types_blacklist = exception_types_blacklist + super(retry_if_exception_type, self).__init__(self.__predicate) + + +class HTTPRetryer(object): + """ + :param retry_if (optional): predicate to retry on requests errors. + More details directly in tenacity source code: + - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/__init__.py#L179 + - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/retry.py + If not provided, the default behavior is: + - Retry on status code from DEFAULT_RETRY_STATUS_CODES + - Retry on exceptions from DEFAULT_RETRY_EXCEPTION_TYPES excluding those from DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST + :type retry_if: tenacity.retry_base + :param wait (optional): how to wait between retry + More details directly in tenacity source code https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/wait.py + + if not provided, the default behavior is: + ``` + random_wait = wait_random_exponential(multiplier=DEFAULT_RETRY_EXP_MULTIPLIER, + max=DEFAULT_RETRY_EXP_MAX) + wait_chain(wait_fixed(0.05), + wait_fixed(0.1), + wait_fixed(0.1) + random_wait) + ``` + :type wait: tenacity.wait_base + :param stop (optional). Tell when to stop retrying. By default it stops retrying after a delay of 60 seconds. A last retry can be done just before this delay is reached, thus the total amount of elapsed time might be a bit higher. More details in tenacity source code https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/stop.py + Raises tenacity.RetryError when timeout is reached. + :type timeout: tenacity.stop_base + :param requests_timeout: timeout of each request. More details in the `requests` documentation: https://2.python-requests.org//en/master/user/advanced/#timeouts + :param requests_timeout: float or tuple(float, float) + + """ + def __init__(self, retry_if=None, wait=None, stop=None, + requests_timeout=DEFAULT_REQUESTS_TIMEOUT): + self.retry_status_code = {} + self.retry_if = retry_if + self.requests_timeout = requests_timeout + self.wait = wait + self.stop = stop + if self.stop is None: + self.stop = stop_after_delay(60) + + if self.retry_if is None: + self.retry_status_code = set(DEFAULT_RETRY_STATUS_CODES) + self.retry_if = (retry_if_result(self.retry_if_status_code) | + retry_if_exception_type(DEFAULT_RETRY_EXCEPTION_TYPES, + DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST)) + if self.wait is None: + random_wait = wait_random_exponential(multiplier=DEFAULT_RETRY_EXP_MULTIPLIER, + max=DEFAULT_RETRY_EXP_MAX) + self.wait = wait_chain(wait_fixed(0.05), + wait_fixed(0.1), + wait_fixed(0.1) + random_wait) + + + def retry_if_status_code(self, response): + return response.status_code in self.retry_status_code + + def retry(self, *args, **kwargs): + try: + requests_timeout = kwargs.pop('timeout') + except KeyError: + requests_timeout = self.requests_timeout + + functor = functools.partial(*args, timeout=requests_timeout, **kwargs) + return retry(functor, self.retry_if, self.wait, self.stop) diff --git a/deepomatic/api/mixins.py b/deepomatic/api/mixins.py index 0ca100b..eb88e65 100644 --- a/deepomatic/api/mixins.py +++ b/deepomatic/api/mixins.py @@ -25,7 +25,6 @@ from deepomatic.api.exceptions import DeepomaticException from deepomatic.api.resource import ResourceList - ############################################################################### class Arg(object): @@ -85,7 +84,7 @@ def delete(self): ############################################################################### class CreateableResource(object): - def create(self, content_type='application/json', files=None, **kwargs): + def create(self, content_type='application/json', files=None, http_retryer=None, **kwargs): if self._helper.check_query_parameters: for arg_name in kwargs: if arg_name not in self.object_template or self.object_template[arg_name]._shoud_be_present_when_adding is False: @@ -96,7 +95,8 @@ def create(self, content_type='application/json', files=None, **kwargs): if files is not None: content_type = 'multipart/mixed' - data = self._helper.post(self._uri(), data=kwargs, content_type=content_type, files=files) + data = self._helper.post(self._uri(), data=kwargs, content_type=content_type, + files=files, http_retryer=http_retryer) return self.__class__(self._helper, pk=data['id'], data=data) @@ -108,5 +108,3 @@ def list(self, offset=0, limit=100, **kwargs): ############################################################################### - - diff --git a/deepomatic/api/resources/network.py b/deepomatic/api/resources/network.py index b1726b2..cec666c 100644 --- a/deepomatic/api/resources/network.py +++ b/deepomatic/api/resources/network.py @@ -22,17 +22,24 @@ THE SOFTWARE. """ -from six import string_types import numpy as np - -from deepomatic.api.resource import Resource +from deepomatic.api.http_retryer import HTTPRetryer from deepomatic.api.inference import InferenceResource -from deepomatic.api.mixins import CreateableResource, ListableResource, UpdatableResource, DeletableResource -from deepomatic.api.mixins import RequiredArg, OptionnalArg, ImmutableArg - +from deepomatic.api.mixins import (CreateableResource, DeletableResource, + ImmutableArg, ListableResource, + OptionnalArg, RequiredArg, + UpdatableResource) +from deepomatic.api.resource import Resource +from six import string_types +from tenacity import stop_after_attempt ############################################################################### +# No retry on network create as this is an heavy request +NETWORK_CREATE_RETRYER = HTTPRetryer(requests_timeout=(3.05, 600), + stop=stop_after_attempt(0)) + + class Network(ListableResource, CreateableResource, UpdatableResource, @@ -66,6 +73,10 @@ def inference(self, convert_to_numpy=True, return_task=False, **kwargs): else: return result + def create(self, *args, **kwargs): + kwargs['http_retryer'] = kwargs.get('http_retryer', NETWORK_CREATE_RETRYER) + return super(Network, self).create(*args, **kwargs) + @staticmethod def _convert_result_to_numpy(result): new_result = {} diff --git a/deepomatic/api/resources/task.py b/deepomatic/api/resources/task.py index 353256f..4b84cf6 100644 --- a/deepomatic/api/resources/task.py +++ b/deepomatic/api/resources/task.py @@ -29,8 +29,10 @@ from deepomatic.api.exceptions import TaskError, TaskTimeout from deepomatic.api.mixins import ListableResource from deepomatic.api.resource import Resource -from deepomatic.api.utils import retry -from tenacity import RetryError, retry_if_result +from deepomatic.api.utils import retry, warn_on_http_retry_error +from tenacity import (RetryError, retry_if_exception_type, retry_if_result, + stop_after_delay, wait_chain, wait_fixed, + wait_random_exponential) logger = logging.getLogger(__name__) @@ -53,6 +55,18 @@ def is_success_status(status): def has_pending_tasks(pending_tasks): return len(pending_tasks) > 0 +def retry_get_tasks(apply_func, retry_if, timeout=60, + wait_exp_multiplier=0.05, wait_exp_max=1.0): + if timeout is None: + stop = stop_never + else: + stop = stop_after_delay(timeout) + + wait = wait_chain(wait_fixed(0.05), + wait_fixed(0.1) + wait_random_exponential(multiplier=wait_exp_multiplier, + max=min(timeout, wait_exp_max))) + return retry(apply_func, retry_if, wait, stop) + class Task(ListableResource, Resource): base_uri = '/tasks/' @@ -69,9 +83,9 @@ def wait(self, **retry_kwargs): Wait until task is completed. Expires after 'timeout' seconds. """ try: - retry(self._refresh_status, - retry_if_result(is_pending_status), - **retry_kwargs) + retry_get_tasks(self._refresh_status, + retry_if_result(is_pending_status), + **retry_kwargs) except RetryError: raise TaskTimeout(self.data()) @@ -82,13 +96,17 @@ def wait(self, **retry_kwargs): def _refresh_status(self): logger.debug("Refreshing Task {}".format(self)) - self.refresh() + warn_on_http_retry_error(self.refresh, suffix="Retrying until Task.wait timeouts.") return self['status'] def _refresh_tasks_status(self, pending_tasks, success_tasks, error_tasks, positions): logger.debug("Refreshing batch of Task {}".format(pending_tasks)) task_ids = [task.pk for idx, task in pending_tasks] - refreshed_tasks = self.list(task_ids=task_ids) + functor = functools.partial(self.list, task_ids=task_ids) + refreshed_tasks = warn_on_http_retry_error(functor, suffix="Retrying until Task.batch_wait timeouts.") + if refreshed_tasks is None: + return pending_tasks + pending_tasks[:] = [] # clear the list (we have to keep the reference) for task in refreshed_tasks: status = task['status'] @@ -125,7 +143,7 @@ def batch_wait(self, tasks, **retry_kwargs): functor = functools.partial(self._refresh_tasks_status, pending_tasks, success_tasks, error_tasks, positions) - retry(functor, retry_if_result(has_pending_tasks), **retry_kwargs) + retry_get_tasks(functor, retry_if_result(has_pending_tasks), **retry_kwargs) except RetryError: pass diff --git a/deepomatic/api/utils.py b/deepomatic/api/utils.py index 1a69854..ec20959 100644 --- a/deepomatic/api/utils.py +++ b/deepomatic/api/utils.py @@ -24,24 +24,35 @@ import logging -from tenacity import (Retrying, after_log, before_log, stop_after_delay, - stop_never, wait_random_exponential) +from tenacity import (RetryError, Retrying, after_log, before_log, + stop_after_delay, stop_never, wait_random_exponential) logger = logging.getLogger(__name__) -def retry(apply_func, retry, - timeout=60, wait_exp_multiplier=0.05, wait_exp_max=1.0): - - if timeout is None: - stop = stop_never - else: - stop = stop_after_delay(timeout) - - retryer = Retrying(wait=wait_random_exponential(multiplier=wait_exp_multiplier, - max=wait_exp_max), +def retry(apply_func, retry_if, wait, stop): + retryer = Retrying(retry=retry_if, + wait=wait, stop=stop, - retry=retry, before=before_log(logger, logging.DEBUG), after=after_log(logger, logging.DEBUG)) return retryer(apply_func) + + +def warn_on_http_retry_error(http_func, suffix=''): + # http helper can raise a RetryError + try: + # this should be an http_helper call + return http_func() + except RetryError as e: + last_exception = e.last_attempt.exception(timeout=0) + msg = "HTTPHelper failed to refresh task status. In the last attempt, " + if last_exception is None: + last_response = last_attempt.result() + msg += 'the status code was {}.'.format(last_response.status_code) + else: + msg += 'an exception occured: {}.'.format(last_exception) + if suffix: + msg += ' ' + suffix + logger.warning(msg) + return None diff --git a/tests/test_client.py b/tests/test_client.py index 17f0c97..d178a61 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -13,10 +13,11 @@ import requests import six from deepomatic.api.client import Client +from deepomatic.api.http_retryer import DEFAULT_RETRY_EXP_MAX, HTTPRetryer from deepomatic.api.inputs import ImageInput from deepomatic.api.version import __title__, __version__ from requests.exceptions import ConnectionError -from tenacity import RetryError +from tenacity import RetryError, stop_after_delay from pytest_voluptuous import S from voluptuous.validators import All, Any, Length @@ -295,38 +296,56 @@ def test_batch_wait(self, client): class TestClientRetry(object): DEFAULT_TIMEOUT = 5 - DEFAULT_MIN_ATTEMPT_NUMBER = 10 + DEFAULT_MIN_ATTEMPT_NUMBER = 4 def expect_retry(self, client, timeout, min_attempt_number): - spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') + spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') # doesn't make any http call start_time = time.time() with pytest.raises(RetryError) as exc: - print(spec.data()) + print(spec.data()) # does make a http call diff = time.time() - start_time - assert diff > timeout and diff < timeout + 2 + assert diff > timeout and diff < timeout + DEFAULT_RETRY_EXP_MAX last_attempt = exc.value.last_attempt assert last_attempt.attempt_number > min_attempt_number return last_attempt def test_retry_network_failure(self): + http_retryer = HTTPRetryer(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) client = Client(host='http://unknown-domain.com', - retry_kwargs={'timeout': self.DEFAULT_TIMEOUT}) + http_retryer=http_retryer) last_attempt = self.expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) exc = last_attempt.exception(timeout=0) assert isinstance(exc, ConnectionError) assert 'Name or service not known' in str(exc) + def register_uri(self, methods, status): + for method in methods: + httpretty.register_uri( + method, + re.compile(r'https?://.*'), + status=502, + content_type="application/json" + ) + @httpretty.activate def test_retry_bad_http_status(self): - httpretty.register_uri( - httpretty.GET, - re.compile(r'https?://.*'), - status=502, - content_type="application/json", - ) - client = Client(retry_kwargs={'timeout': self.DEFAULT_TIMEOUT}) + self.register_uri([httpretty.GET, httpretty.POST], 502) + http_retryer = HTTPRetryer(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) + client = Client(http_retryer=http_retryer) last_attempt = self.expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) assert last_attempt.exception(timeout=0) is None # no exception raised during retry last_response = last_attempt.result() assert 502 == last_response.status_code + + # Creating network doesn't retry + with pytest.raises(RetryError) as exc: + network = client.Network.create(name="My first network", + framework='tensorflow-1.x', + preprocessing=["useless"], + files=["useless"]) + + last_attempt = exc.value.last_attempt + assert last_attempt.attempt_number == 1 + last_response = last_attempt.result() + assert 502 == last_response.status_code From 08bb6b389a2f0ed858b904dbe9119009501b3bd8 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Thu, 12 Sep 2019 19:18:40 +0200 Subject: [PATCH 09/26] added tests on blacklist exceptions --- tests/test_client.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/test_client.py b/tests/test_client.py index d178a61..7cec6db 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -16,7 +16,7 @@ from deepomatic.api.http_retryer import DEFAULT_RETRY_EXP_MAX, HTTPRetryer from deepomatic.api.inputs import ImageInput from deepomatic.api.version import __title__, __version__ -from requests.exceptions import ConnectionError +from requests.exceptions import ConnectionError, MissingSchema from tenacity import RetryError, stop_after_delay from pytest_voluptuous import S @@ -349,3 +349,10 @@ def test_retry_bad_http_status(self): assert last_attempt.attempt_number == 1 last_response = last_attempt.result() assert 502 == last_response.status_code + + def test_no_retry_blacklist_exception(self): + http_retryer = HTTPRetryer(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) + client = Client(http_retryer=http_retryer) + # check that there is no retry on exceptions from DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST + with pytest.raises(MissingSchema) as exc: + client.http_helper.http_retryer.retry(requests.get, '') From 78390c43d7c19d84b5199f8b3f13eb746afe2bf6 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Fri, 13 Sep 2019 19:23:02 +0200 Subject: [PATCH 10/26] Fix most comments --- deepomatic/api/client.py | 4 +- deepomatic/api/http_helper.py | 30 ++++++--- .../api/{http_retryer.py => http_retry.py} | 64 ++++++++++++------- deepomatic/api/mixins.py | 18 +++++- deepomatic/api/resources/network.py | 10 +-- tests/test_client.py | 38 ++++++----- 6 files changed, 104 insertions(+), 60 deletions(-) rename deepomatic/api/{http_retryer.py => http_retry.py} (70%) diff --git a/deepomatic/api/client.py b/deepomatic/api/client.py index 22a9744..a12844b 100644 --- a/deepomatic/api/client.py +++ b/deepomatic/api/client.py @@ -61,8 +61,8 @@ def __init__(self, *args, **kwargs): :param pool_maxsize (optional): Set `requests.adapters.HTTPAdapter.pool_maxsize` for concurrent calls. Defaults to 20. :type pool_maxsize: int - :param http_retryer (optional): Customize the retry of http errors - :type http_retryer: http_retryer.HTTPRetryer + :param http_retry (optional): Customize the retry of http errors + :type http_retry: http_retry.HTTPRetry :return: :class:`Client` object :rtype: deepomatic.api.client.Client diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 3ce1811..b1504b1 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -29,7 +29,7 @@ import requests from deepomatic.api.exceptions import BadStatus, DeepomaticException -from deepomatic.api.http_retryer import HTTPRetryer +from deepomatic.api.http_retry import HTTPRetry from deepomatic.api.version import __title__, __version__ from requests.structures import CaseInsensitiveDict from six import string_types @@ -44,7 +44,7 @@ class HTTPHelper(object): def __init__(self, app_id=None, api_key=None, verify_ssl=None, host=None, version=API_VERSION, check_query_parameters=True, user_agent_prefix='', user_agent_suffix='', pool_maxsize=20, - http_retryer=None): + http_retry=None): """ Init the HTTP helper with API key and secret """ @@ -59,7 +59,7 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, if app_id is None or api_key is None: raise DeepomaticException("Please specify 'app_id' and 'api_key' either by passing those values to the client or by defining the DEEPOMATIC_APP_ID and DEEPOMATIC_API_KEY environment variables.") - self.http_retryer = http_retryer or HTTPRetryer() + self.http_retry = http_retry or HTTPRetry.DEFAULT if not isinstance(version, string_types): version = 'v%g' % version @@ -213,13 +213,25 @@ def make_request(self, func, resource, params=None, data=None, if not resource.startswith('http'): resource = self.resource_prefix + resource - http_retryer = kwargs.pop('http_retryer', None) or self.http_retryer + try: + http_retry = kwargs.pop('http_retry') + except KeyError: + http_retry = self.http_retry + + if http_retry is not None: + response = http_retry.retry(func, resource, *args, + params=params, data=data, + files=files, headers=headers, + verify=self.verify, + stream=stream, **kwargs) + else: + # Call requests directly, no retry + response = func(resource, *args, + params=params, data=data, + files=files, headers=headers, + verify=self.verify, + stream=stream, **kwargs) - response = http_retryer.retry(func, resource, *args, - params=params, data=data, - files=files, headers=headers, - verify=self.verify, - stream=stream, **kwargs) # Close opened files for file in opened_files: diff --git a/deepomatic/api/http_retryer.py b/deepomatic/api/http_retry.py similarity index 70% rename from deepomatic/api/http_retryer.py rename to deepomatic/api/http_retry.py index 2b1fe01..57a9a85 100644 --- a/deepomatic/api/http_retryer.py +++ b/deepomatic/api/http_retry.py @@ -6,13 +6,6 @@ from tenacity import (retry_if_exception, retry_if_result, stop_after_delay, wait_chain, wait_fixed, wait_random_exponential) -DEFAULT_REQUESTS_TIMEOUT = (3.05, 10) -DEFAULT_RETRY_EXP_MAX = 10. -DEFAULT_RETRY_EXP_MULTIPLIER = 0.5 -DEFAULT_RETRY_STATUS_CODES = [502, 503, 504] -DEFAULT_RETRY_EXCEPTION_TYPES = (RequestException, ) -DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST = (ValueError, ProxyError, TooManyRedirects, URLRequired) - class retry_if_exception_type(retry_if_exception): def __predicate(self, e): @@ -26,23 +19,30 @@ def __init__(self, exception_types=Exception, super(retry_if_exception_type, self).__init__(self.__predicate) -class HTTPRetryer(object): +class RequestsTimeout(object): + FAST = (3.05, 10.) + MEDIUM = (3.05, 60.) + SLOW = (3.05, 600.) + + +class HTTPRetry(object): + """ :param retry_if (optional): predicate to retry on requests errors. More details directly in tenacity source code: - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/__init__.py#L179 - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/retry.py If not provided, the default behavior is: - - Retry on status code from DEFAULT_RETRY_STATUS_CODES - - Retry on exceptions from DEFAULT_RETRY_EXCEPTION_TYPES excluding those from DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST + - Retry on status code from Default.RETRY_STATUS_CODES + - Retry on exceptions from Default.RETRY_EXCEPTION_TYPES excluding those from Default.RETRY_EXCEPTION_TYPES_BLACKLIST :type retry_if: tenacity.retry_base :param wait (optional): how to wait between retry More details directly in tenacity source code https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/wait.py if not provided, the default behavior is: ``` - random_wait = wait_random_exponential(multiplier=DEFAULT_RETRY_EXP_MULTIPLIER, - max=DEFAULT_RETRY_EXP_MAX) + random_wait = wait_random_exponential(multiplier=Default.RETRY_EXP_MULTIPLIER, + max=Default.RETRY_EXP_MAX) wait_chain(wait_fixed(0.05), wait_fixed(0.1), wait_fixed(0.1) + random_wait) @@ -55,37 +55,53 @@ class HTTPRetryer(object): :param requests_timeout: float or tuple(float, float) """ + + class Default(object): + RETRY_EXP_MAX = 10. + RETRY_EXP_MULTIPLIER = 0.5 + RETRY_STATUS_CODES = [500, 502, 503, 504] + RETRY_EXCEPTION_TYPES = (RequestException, ) + RETRY_EXCEPTION_TYPES_BLACKLIST = (ValueError, ProxyError, TooManyRedirects, URLRequired) + + def __init__(self, retry_if=None, wait=None, stop=None, - requests_timeout=DEFAULT_REQUESTS_TIMEOUT): + requests_timeout=RequestsTimeout.FAST): self.retry_status_code = {} self.retry_if = retry_if self.requests_timeout = requests_timeout self.wait = wait self.stop = stop + if self.stop is None: self.stop = stop_after_delay(60) if self.retry_if is None: - self.retry_status_code = set(DEFAULT_RETRY_STATUS_CODES) + self.retry_status_code = set(HTTPRetry.Default.RETRY_STATUS_CODES) self.retry_if = (retry_if_result(self.retry_if_status_code) | - retry_if_exception_type(DEFAULT_RETRY_EXCEPTION_TYPES, - DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST)) + retry_if_exception_type(HTTPRetry.Default.RETRY_EXCEPTION_TYPES, + HTTPRetry.Default.RETRY_EXCEPTION_TYPES_BLACKLIST)) + if self.wait is None: - random_wait = wait_random_exponential(multiplier=DEFAULT_RETRY_EXP_MULTIPLIER, - max=DEFAULT_RETRY_EXP_MAX) + random_wait = wait_random_exponential(multiplier=HTTPRetry.Default.RETRY_EXP_MULTIPLIER, + max=HTTPRetry.Default.RETRY_EXP_MAX) self.wait = wait_chain(wait_fixed(0.05), wait_fixed(0.1), wait_fixed(0.1) + random_wait) - - def retry_if_status_code(self, response): - return response.status_code in self.retry_status_code - - def retry(self, *args, **kwargs): + def retry(self, requests_callable, *args, **kwargs): try: + # this is the timeout of requests module + # not the one of the retry requests_timeout = kwargs.pop('timeout') except KeyError: requests_timeout = self.requests_timeout - functor = functools.partial(*args, timeout=requests_timeout, **kwargs) + functor = functools.partial(requests_callable, *args, + timeout=requests_timeout, **kwargs) return retry(functor, self.retry_if, self.wait, self.stop) + + def retry_if_status_code(self, response): + return response.status_code in self.retry_status_code + + +HTTPRetry.DEFAULT = HTTPRetry() diff --git a/deepomatic/api/mixins.py b/deepomatic/api/mixins.py index eb88e65..5fb17bb 100644 --- a/deepomatic/api/mixins.py +++ b/deepomatic/api/mixins.py @@ -84,7 +84,17 @@ def delete(self): ############################################################################### class CreateableResource(object): - def create(self, content_type='application/json', files=None, http_retryer=None, **kwargs): + def create(self, content_type='application/json', files=None, **kwargs): + post_kwargs = {} + # TODO: this is a hack, kwargs shouldn't be the data to post + # it should be the requests kwargs + # the fix will be a breaking change, so for now we just pop them + for key in ['http_retry', 'timeout']: + try: + post_kwargs[key] = kwargs.pop(key) + except KeyError: + pass + if self._helper.check_query_parameters: for arg_name in kwargs: if arg_name not in self.object_template or self.object_template[arg_name]._shoud_be_present_when_adding is False: @@ -95,8 +105,10 @@ def create(self, content_type='application/json', files=None, http_retryer=None, if files is not None: content_type = 'multipart/mixed' - data = self._helper.post(self._uri(), data=kwargs, content_type=content_type, - files=files, http_retryer=http_retryer) + + data = self._helper.post(self._uri(), data=kwargs, + content_type=content_type, + files=files, **post_kwargs) return self.__class__(self._helper, pk=data['id'], data=data) diff --git a/deepomatic/api/resources/network.py b/deepomatic/api/resources/network.py index cec666c..5c2e8e7 100644 --- a/deepomatic/api/resources/network.py +++ b/deepomatic/api/resources/network.py @@ -23,7 +23,7 @@ """ import numpy as np -from deepomatic.api.http_retryer import HTTPRetryer +from deepomatic.api.http_retry import HTTPRetry, RequestsTimeout from deepomatic.api.inference import InferenceResource from deepomatic.api.mixins import (CreateableResource, DeletableResource, ImmutableArg, ListableResource, @@ -35,9 +35,7 @@ ############################################################################### -# No retry on network create as this is an heavy request -NETWORK_CREATE_RETRYER = HTTPRetryer(requests_timeout=(3.05, 600), - stop=stop_after_attempt(0)) + class Network(ListableResource, @@ -74,7 +72,9 @@ def inference(self, convert_to_numpy=True, return_task=False, **kwargs): return result def create(self, *args, **kwargs): - kwargs['http_retryer'] = kwargs.get('http_retryer', NETWORK_CREATE_RETRYER) + # No retry on network create by default as this is an heavy request + kwargs['http_retry'] = kwargs.get('http_retry') + kwargs['timeout'] = kwargs.get('timeout', RequestsTimeout.SLOW) return super(Network, self).create(*args, **kwargs) @staticmethod diff --git a/tests/test_client.py b/tests/test_client.py index 7cec6db..52cbb56 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -13,7 +13,8 @@ import requests import six from deepomatic.api.client import Client -from deepomatic.api.http_retryer import DEFAULT_RETRY_EXP_MAX, HTTPRetryer +from deepomatic.api.exceptions import BadStatus +from deepomatic.api.http_retry import HTTPRetry from deepomatic.api.inputs import ImageInput from deepomatic.api.version import __title__, __version__ from requests.exceptions import ConnectionError, MissingSchema @@ -295,7 +296,7 @@ def test_batch_wait(self, client): class TestClientRetry(object): - DEFAULT_TIMEOUT = 5 + DEFAULT_TIMEOUT = 2 DEFAULT_MIN_ATTEMPT_NUMBER = 4 def expect_retry(self, client, timeout, min_attempt_number): @@ -305,15 +306,15 @@ def expect_retry(self, client, timeout, min_attempt_number): print(spec.data()) # does make a http call diff = time.time() - start_time - assert diff > timeout and diff < timeout + DEFAULT_RETRY_EXP_MAX + assert diff > timeout and diff < timeout + HTTPRetry.Default.RETRY_EXP_MAX last_attempt = exc.value.last_attempt assert last_attempt.attempt_number > min_attempt_number return last_attempt def test_retry_network_failure(self): - http_retryer = HTTPRetryer(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) + http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) client = Client(host='http://unknown-domain.com', - http_retryer=http_retryer) + http_retry=http_retry) last_attempt = self.expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) exc = last_attempt.exception(timeout=0) assert isinstance(exc, ConnectionError) @@ -331,28 +332,31 @@ def register_uri(self, methods, status): @httpretty.activate def test_retry_bad_http_status(self): self.register_uri([httpretty.GET, httpretty.POST], 502) - http_retryer = HTTPRetryer(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) - client = Client(http_retryer=http_retryer) + http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) + client = Client(http_retry=http_retry) last_attempt = self.expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) assert last_attempt.exception(timeout=0) is None # no exception raised during retry last_response = last_attempt.result() assert 502 == last_response.status_code - # Creating network doesn't retry - with pytest.raises(RetryError) as exc: + @httpretty.activate + def test_no_retry_create_network(self): + self.register_uri([httpretty.GET, httpretty.POST], 502) + http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) + client = Client(http_retry=http_retry) + # Creating network doesn't retry, we directly get a 502 + t = time.time() + with pytest.raises(BadStatus) as exc: network = client.Network.create(name="My first network", framework='tensorflow-1.x', preprocessing=["useless"], files=["useless"]) - - last_attempt = exc.value.last_attempt - assert last_attempt.attempt_number == 1 - last_response = last_attempt.result() - assert 502 == last_response.status_code + assert 502 == exc.status_code + assert time.time() - t < 0.3 def test_no_retry_blacklist_exception(self): - http_retryer = HTTPRetryer(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) - client = Client(http_retryer=http_retryer) + http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) + client = Client(http_retry=http_retry) # check that there is no retry on exceptions from DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST with pytest.raises(MissingSchema) as exc: - client.http_helper.http_retryer.retry(requests.get, '') + client.http_helper.http_retry.retry(requests.get, '') From 6512040fc0a3e295d58e948324cbc3624e250aaa Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Fri, 13 Sep 2019 20:22:39 +0200 Subject: [PATCH 11/26] fix attempt number --- tests/test_client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_client.py b/tests/test_client.py index 52cbb56..8e981a8 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -297,7 +297,7 @@ def test_batch_wait(self, client): class TestClientRetry(object): DEFAULT_TIMEOUT = 2 - DEFAULT_MIN_ATTEMPT_NUMBER = 4 + DEFAULT_MIN_ATTEMPT_NUMBER = 3 def expect_retry(self, client, timeout, min_attempt_number): spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') # doesn't make any http call @@ -308,7 +308,7 @@ def expect_retry(self, client, timeout, min_attempt_number): diff = time.time() - start_time assert diff > timeout and diff < timeout + HTTPRetry.Default.RETRY_EXP_MAX last_attempt = exc.value.last_attempt - assert last_attempt.attempt_number > min_attempt_number + assert last_attempt.attempt_number >= min_attempt_number return last_attempt def test_retry_network_failure(self): From 1f2fbd291b96682c1957a5db8e0633b6094b5df2 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Tue, 17 Sep 2019 15:25:46 +0200 Subject: [PATCH 12/26] fix comments --- deepomatic/api/http_retry.py | 15 +++++++++------ requirements.txt | 2 +- tests/test_client.py | 8 +++++--- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/deepomatic/api/http_retry.py b/deepomatic/api/http_retry.py index 57a9a85..5e864ff 100644 --- a/deepomatic/api/http_retry.py +++ b/deepomatic/api/http_retry.py @@ -8,6 +8,8 @@ class retry_if_exception_type(retry_if_exception): + # Taken from https://github.com/jd/tenacity/blob/2775f13b34b3ec67a774061a77fcd4e1e9b4157c/tenacity/retry.py#L72 + # Extented to support blacklist types def __predicate(self, e): return (isinstance(e, self.exception_types) and not isinstance(e, self.exception_types_blacklist)) @@ -30,14 +32,15 @@ class HTTPRetry(object): """ :param retry_if (optional): predicate to retry on requests errors. More details directly in tenacity source code: - - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/__init__.py#L179 - - https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/retry.py + + - https://github.com/jd/tenacity/blob/5.1.1/tenacity/__init__.py#L179 + - https://github.com/jd/tenacity/blob/5.1.1/tenacity/retry.py If not provided, the default behavior is: - Retry on status code from Default.RETRY_STATUS_CODES - Retry on exceptions from Default.RETRY_EXCEPTION_TYPES excluding those from Default.RETRY_EXCEPTION_TYPES_BLACKLIST :type retry_if: tenacity.retry_base :param wait (optional): how to wait between retry - More details directly in tenacity source code https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/wait.py + More details directly in tenacity source code https://github.com/jd/tenacity/blob/5.1.1/tenacity/wait.py if not provided, the default behavior is: ``` @@ -48,11 +51,11 @@ class HTTPRetry(object): wait_fixed(0.1) + random_wait) ``` :type wait: tenacity.wait_base - :param stop (optional). Tell when to stop retrying. By default it stops retrying after a delay of 60 seconds. A last retry can be done just before this delay is reached, thus the total amount of elapsed time might be a bit higher. More details in tenacity source code https://github.com/jd/tenacity/blob/1d05520276766d8c53fbb35b2b8368cc43a6c52c/tenacity/stop.py + :param stop (optional). Tell when to stop retrying. By default it stops retrying after a delay of 60 seconds. A last retry can be done just before this delay is reached, thus the total amount of elapsed time might be a bit higher. More details in tenacity source code https://github.com/jd/tenacity/blob/5.1.1/tenacity/stop.py Raises tenacity.RetryError when timeout is reached. :type timeout: tenacity.stop_base :param requests_timeout: timeout of each request. More details in the `requests` documentation: https://2.python-requests.org//en/master/user/advanced/#timeouts - :param requests_timeout: float or tuple(float, float) + :type requests_timeout: float or tuple(float, float) """ @@ -63,7 +66,6 @@ class Default(object): RETRY_EXCEPTION_TYPES = (RequestException, ) RETRY_EXCEPTION_TYPES_BLACKLIST = (ValueError, ProxyError, TooManyRedirects, URLRequired) - def __init__(self, retry_if=None, wait=None, stop=None, requests_timeout=RequestsTimeout.FAST): self.retry_status_code = {} @@ -90,6 +92,7 @@ def __init__(self, retry_if=None, wait=None, stop=None, def retry(self, requests_callable, *args, **kwargs): try: + # requests_callable must be a method from the requests module # this is the timeout of requests module # not the one of the retry requests_timeout = kwargs.pop('timeout') diff --git a/requirements.txt b/requirements.txt index 6b5ec8c..9e16833 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,4 +2,4 @@ numpy>=1.10.0,<2 promise>=2.1,<3 six>=1.10.0,<2 requests>=2.19.0,<3 # will not work below in python3 -tenacity>=4.12.0,<5 +tenacity>=5.1,<6 diff --git a/tests/test_client.py b/tests/test_client.py index 8e981a8..38d75f8 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -299,7 +299,7 @@ class TestClientRetry(object): DEFAULT_TIMEOUT = 2 DEFAULT_MIN_ATTEMPT_NUMBER = 3 - def expect_retry(self, client, timeout, min_attempt_number): + def send_request_and_expect_retry(self, client, timeout, min_attempt_number): spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') # doesn't make any http call start_time = time.time() with pytest.raises(RetryError) as exc: @@ -315,7 +315,8 @@ def test_retry_network_failure(self): http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) client = Client(host='http://unknown-domain.com', http_retry=http_retry) - last_attempt = self.expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) + last_attempt = self.send_request_and_expect_retry(client, self.DEFAULT_TIMEOUT, + self.DEFAULT_MIN_ATTEMPT_NUMBER) exc = last_attempt.exception(timeout=0) assert isinstance(exc, ConnectionError) assert 'Name or service not known' in str(exc) @@ -334,7 +335,8 @@ def test_retry_bad_http_status(self): self.register_uri([httpretty.GET, httpretty.POST], 502) http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) client = Client(http_retry=http_retry) - last_attempt = self.expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) + last_attempt = self.send_request_and_expect_retry(client, self.DEFAULT_TIMEOUT, + self.DEFAULT_MIN_ATTEMPT_NUMBER) assert last_attempt.exception(timeout=0) is None # no exception raised during retry last_response = last_attempt.result() assert 502 == last_response.status_code From d980c8dbf6c953eabd1871917d66a6be50c68eea Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Tue, 17 Sep 2019 17:34:49 +0200 Subject: [PATCH 13/26] fix comments --- deepomatic/api/client.py | 6 +++- deepomatic/api/http_helper.py | 55 ++++++++++++++++++----------- deepomatic/api/http_retry.py | 25 ++----------- deepomatic/api/resources/network.py | 5 +-- tests/test_client.py | 19 +++++----- 5 files changed, 53 insertions(+), 57 deletions(-) diff --git a/deepomatic/api/client.py b/deepomatic/api/client.py index a12844b..9fe8453 100644 --- a/deepomatic/api/client.py +++ b/deepomatic/api/client.py @@ -61,7 +61,11 @@ def __init__(self, *args, **kwargs): :param pool_maxsize (optional): Set `requests.adapters.HTTPAdapter.pool_maxsize` for concurrent calls. Defaults to 20. :type pool_maxsize: int - :param http_retry (optional): Customize the retry of http errors + :param requests_timeout: timeout of each request. + Defaults to http_helper.RequestsTimeout.FAST. + More details in the `requests` documentation: https://2.python-requests.org//en/master/user/advanced/#timeouts + :type requests_timeout: float or tuple(float, float) + :param http_retry (optional): Customize the retry of http errors. :type http_retry: http_retry.HTTPRetry :return: :class:`Client` object diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index b1504b1..274f007 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -22,6 +22,7 @@ THE SOFTWARE. """ +import functools import json import os import platform @@ -40,11 +41,17 @@ ############################################################################### +class RequestsTimeout(object): + FAST = (3.05, 10.) + MEDIUM = (3.05, 60.) + SLOW = (3.05, 600.) + + class HTTPHelper(object): def __init__(self, app_id=None, api_key=None, verify_ssl=None, host=None, version=API_VERSION, check_query_parameters=True, user_agent_prefix='', user_agent_suffix='', pool_maxsize=20, - http_retry=None): + requests_timeout=RequestsTimeout.FAST, http_retry=None): """ Init the HTTP helper with API key and secret """ @@ -60,6 +67,7 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, raise DeepomaticException("Please specify 'app_id' and 'api_key' either by passing those values to the client or by defining the DEEPOMATIC_APP_ID and DEEPOMATIC_API_KEY environment variables.") self.http_retry = http_retry or HTTPRetry.DEFAULT + self.requests_timeout = requests_timeout if not isinstance(version, string_types): version = 'v%g' % version @@ -163,6 +171,28 @@ def recursive_json_dump(prefix, obj, data_dict, omit_dot=False): return new_dict + def send_request(self, requests_callable, *args, **kwargs): + # requests_callable must be a method from the requests module + try: + # this is the timeout of requests module + requests_timeout = kwargs.pop('timeout') + except KeyError: + requests_timeout = self.requests_timeout + + try: + http_retry = kwargs.pop('http_retry') + except KeyError: + http_retry = self.http_retry + + functor = functools.partial(requests_callable, *args, + verify=self.verify, + timeout=requests_timeout, **kwargs) + + if http_retry is not None: + return http_retry.retry(functor) + + return functor() + def make_request(self, func, resource, params=None, data=None, content_type='application/json', files=None, stream=False, *args, **kwargs): @@ -213,25 +243,10 @@ def make_request(self, func, resource, params=None, data=None, if not resource.startswith('http'): resource = self.resource_prefix + resource - try: - http_retry = kwargs.pop('http_retry') - except KeyError: - http_retry = self.http_retry - - if http_retry is not None: - response = http_retry.retry(func, resource, *args, - params=params, data=data, - files=files, headers=headers, - verify=self.verify, - stream=stream, **kwargs) - else: - # Call requests directly, no retry - response = func(resource, *args, - params=params, data=data, - files=files, headers=headers, - verify=self.verify, - stream=stream, **kwargs) - + response = self.send_request(func, resource, *args, + params=params, data=data, + files=files, headers=headers, + stream=stream, **kwargs) # Close opened files for file in opened_files: diff --git a/deepomatic/api/http_retry.py b/deepomatic/api/http_retry.py index 5e864ff..6c48a3e 100644 --- a/deepomatic/api/http_retry.py +++ b/deepomatic/api/http_retry.py @@ -21,12 +21,6 @@ def __init__(self, exception_types=Exception, super(retry_if_exception_type, self).__init__(self.__predicate) -class RequestsTimeout(object): - FAST = (3.05, 10.) - MEDIUM = (3.05, 60.) - SLOW = (3.05, 600.) - - class HTTPRetry(object): """ @@ -54,9 +48,6 @@ class HTTPRetry(object): :param stop (optional). Tell when to stop retrying. By default it stops retrying after a delay of 60 seconds. A last retry can be done just before this delay is reached, thus the total amount of elapsed time might be a bit higher. More details in tenacity source code https://github.com/jd/tenacity/blob/5.1.1/tenacity/stop.py Raises tenacity.RetryError when timeout is reached. :type timeout: tenacity.stop_base - :param requests_timeout: timeout of each request. More details in the `requests` documentation: https://2.python-requests.org//en/master/user/advanced/#timeouts - :type requests_timeout: float or tuple(float, float) - """ class Default(object): @@ -66,11 +57,9 @@ class Default(object): RETRY_EXCEPTION_TYPES = (RequestException, ) RETRY_EXCEPTION_TYPES_BLACKLIST = (ValueError, ProxyError, TooManyRedirects, URLRequired) - def __init__(self, retry_if=None, wait=None, stop=None, - requests_timeout=RequestsTimeout.FAST): + def __init__(self, retry_if=None, wait=None, stop=None): self.retry_status_code = {} self.retry_if = retry_if - self.requests_timeout = requests_timeout self.wait = wait self.stop = stop @@ -90,17 +79,7 @@ def __init__(self, retry_if=None, wait=None, stop=None, wait_fixed(0.1), wait_fixed(0.1) + random_wait) - def retry(self, requests_callable, *args, **kwargs): - try: - # requests_callable must be a method from the requests module - # this is the timeout of requests module - # not the one of the retry - requests_timeout = kwargs.pop('timeout') - except KeyError: - requests_timeout = self.requests_timeout - - functor = functools.partial(requests_callable, *args, - timeout=requests_timeout, **kwargs) + def retry(self, functor): return retry(functor, self.retry_if, self.wait, self.stop) def retry_if_status_code(self, response): diff --git a/deepomatic/api/resources/network.py b/deepomatic/api/resources/network.py index 5c2e8e7..94f564c 100644 --- a/deepomatic/api/resources/network.py +++ b/deepomatic/api/resources/network.py @@ -23,7 +23,7 @@ """ import numpy as np -from deepomatic.api.http_retry import HTTPRetry, RequestsTimeout +from deepomatic.api.http_helper import RequestsTimeout from deepomatic.api.inference import InferenceResource from deepomatic.api.mixins import (CreateableResource, DeletableResource, ImmutableArg, ListableResource, @@ -31,13 +31,10 @@ UpdatableResource) from deepomatic.api.resource import Resource from six import string_types -from tenacity import stop_after_attempt ############################################################################### - - class Network(ListableResource, CreateableResource, UpdatableResource, diff --git a/tests/test_client.py b/tests/test_client.py index 38d75f8..5cb6af0 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,4 +1,5 @@ import base64 +import functools import hashlib import logging import os @@ -349,16 +350,16 @@ def test_no_retry_create_network(self): # Creating network doesn't retry, we directly get a 502 t = time.time() with pytest.raises(BadStatus) as exc: - network = client.Network.create(name="My first network", - framework='tensorflow-1.x', - preprocessing=["useless"], - files=["useless"]) + client.Network.create(name="My first network", + framework='tensorflow-1.x', + preprocessing=["useless"], + files=["useless"]) assert 502 == exc.status_code assert time.time() - t < 0.3 def test_no_retry_blacklist_exception(self): - http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) - client = Client(http_retry=http_retry) - # check that there is no retry on exceptions from DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST - with pytest.raises(MissingSchema) as exc: - client.http_helper.http_retry.retry(requests.get, '') + http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) + client = Client(http_retry=http_retry) + # check that there is no retry on exceptions from DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST + with pytest.raises(MissingSchema): + client.http_helper.http_retry.retry(functools.partial(requests.get, '')) From dcabdcce74dd77eb66394d2e15c8982c1583005a Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Tue, 17 Sep 2019 19:23:37 +0200 Subject: [PATCH 14/26] fix comments --- deepomatic/api/http_helper.py | 2 +- deepomatic/api/http_retry.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 274f007..6cefdbd 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -66,7 +66,7 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, if app_id is None or api_key is None: raise DeepomaticException("Please specify 'app_id' and 'api_key' either by passing those values to the client or by defining the DEEPOMATIC_APP_ID and DEEPOMATIC_API_KEY environment variables.") - self.http_retry = http_retry or HTTPRetry.DEFAULT + self.http_retry = http_retry or HTTPRetry() self.requests_timeout = requests_timeout if not isinstance(version, string_types): diff --git a/deepomatic/api/http_retry.py b/deepomatic/api/http_retry.py index 6c48a3e..5dc4e8c 100644 --- a/deepomatic/api/http_retry.py +++ b/deepomatic/api/http_retry.py @@ -1,6 +1,6 @@ import functools -from deepomatic.api.utils import retry +from deepomatic.api import utils from requests.exceptions import (ProxyError, RequestException, TooManyRedirects, URLRequired) from tenacity import (retry_if_exception, retry_if_result, stop_after_delay, @@ -80,10 +80,7 @@ def __init__(self, retry_if=None, wait=None, stop=None): wait_fixed(0.1) + random_wait) def retry(self, functor): - return retry(functor, self.retry_if, self.wait, self.stop) + return utils.retry(functor, self.retry_if, self.wait, self.stop) def retry_if_status_code(self, response): return response.status_code in self.retry_status_code - - -HTTPRetry.DEFAULT = HTTPRetry() From 5b3524c4e85bae1b24254594ed4334967fb1ca04 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Mon, 23 Sep 2019 20:53:35 +0200 Subject: [PATCH 15/26] Nested retry errors --- deepomatic/api/exceptions.py | 20 ++++++++++-- deepomatic/api/http_helper.py | 8 +++-- deepomatic/api/http_retry.py | 3 +- deepomatic/api/resources/task.py | 20 ++++++------ deepomatic/api/utils.py | 18 ++++++----- tests/test_client.py | 53 ++++++++++++++++++++++++++------ 6 files changed, 90 insertions(+), 32 deletions(-) diff --git a/deepomatic/api/exceptions.py b/deepomatic/api/exceptions.py index 1546061..4aeeba4 100644 --- a/deepomatic/api/exceptions.py +++ b/deepomatic/api/exceptions.py @@ -23,13 +23,15 @@ """ import json +from tenacity import RetryError ############################################################################### class DeepomaticException(Exception): - def __init__(self, msg): - super(DeepomaticException, self).__init__(msg) + pass + # def __init__(self, msg): + # super(DeepomaticException, self).__init__(msg) ############################################################################### @@ -83,11 +85,23 @@ def get_task_id(self): ############################################################################### class TaskTimeout(DeepomaticException): - def __init__(self, task): + def __init__(self, task, retry_error=None): self.task = task + self.retry_error = retry_error def __str__(self): return "Timeout on task:\n{}".format(json.dumps(self.task)) def get_task_id(self): return self.task['id'] + + +############################################################################### + + +class HTTPRetryError(RetryError): + pass + + +class TaskRetryError(RetryError): + pass diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 6cefdbd..5a1ac35 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -51,7 +51,7 @@ class HTTPHelper(object): def __init__(self, app_id=None, api_key=None, verify_ssl=None, host=None, version=API_VERSION, check_query_parameters=True, user_agent_prefix='', user_agent_suffix='', pool_maxsize=20, - requests_timeout=RequestsTimeout.FAST, http_retry=None): + requests_timeout=RequestsTimeout.FAST, http_retry=HTTPRetry()): """ Init the HTTP helper with API key and secret """ @@ -66,7 +66,11 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, if app_id is None or api_key is None: raise DeepomaticException("Please specify 'app_id' and 'api_key' either by passing those values to the client or by defining the DEEPOMATIC_APP_ID and DEEPOMATIC_API_KEY environment variables.") - self.http_retry = http_retry or HTTPRetry() + # WARNING for developers: `self.http_retry` should not be modified in this class. + # It should be a constant object. + # This will affect the default parameter otherwise. + self.http_retry = http_retry + self.requests_timeout = requests_timeout if not isinstance(version, string_types): diff --git a/deepomatic/api/http_retry.py b/deepomatic/api/http_retry.py index 5dc4e8c..a9c7ded 100644 --- a/deepomatic/api/http_retry.py +++ b/deepomatic/api/http_retry.py @@ -1,6 +1,7 @@ import functools from deepomatic.api import utils +from deepomatic.api.exceptions import HTTPRetryError from requests.exceptions import (ProxyError, RequestException, TooManyRedirects, URLRequired) from tenacity import (retry_if_exception, retry_if_result, stop_after_delay, @@ -80,7 +81,7 @@ def __init__(self, retry_if=None, wait=None, stop=None): wait_fixed(0.1) + random_wait) def retry(self, functor): - return utils.retry(functor, self.retry_if, self.wait, self.stop) + return utils.retry(functor, self.retry_if, self.wait, self.stop, retry_error_cls=HTTPRetryError) def retry_if_status_code(self, response): return response.status_code in self.retry_status_code diff --git a/deepomatic/api/resources/task.py b/deepomatic/api/resources/task.py index 4b84cf6..d392dcb 100644 --- a/deepomatic/api/resources/task.py +++ b/deepomatic/api/resources/task.py @@ -26,7 +26,7 @@ import functools import logging -from deepomatic.api.exceptions import TaskError, TaskTimeout +from deepomatic.api.exceptions import TaskError, TaskTimeout, HTTPRetryError, TaskRetryError from deepomatic.api.mixins import ListableResource from deepomatic.api.resource import Resource from deepomatic.api.utils import retry, warn_on_http_retry_error @@ -55,6 +55,7 @@ def is_success_status(status): def has_pending_tasks(pending_tasks): return len(pending_tasks) > 0 + def retry_get_tasks(apply_func, retry_if, timeout=60, wait_exp_multiplier=0.05, wait_exp_max=1.0): if timeout is None: @@ -65,7 +66,7 @@ def retry_get_tasks(apply_func, retry_if, timeout=60, wait = wait_chain(wait_fixed(0.05), wait_fixed(0.1) + wait_random_exponential(multiplier=wait_exp_multiplier, max=min(timeout, wait_exp_max))) - return retry(apply_func, retry_if, wait, stop) + return retry(apply_func, retry_if, wait, stop, retry_error_cls=TaskRetryError) class Task(ListableResource, Resource): @@ -84,28 +85,27 @@ def wait(self, **retry_kwargs): """ try: retry_get_tasks(self._refresh_status, + retry_if_exception_type(HTTPRetryError) | retry_if_result(is_pending_status), **retry_kwargs) - except RetryError: - raise TaskTimeout(self.data()) + except TaskRetryError as retry_error: + raise TaskTimeout(self._data, retry_error) - if is_error_status(self['status']): - raise TaskError(self.data()) + if is_error_status(self._data['status']): + raise TaskError(self._data) return self def _refresh_status(self): logger.debug("Refreshing Task {}".format(self)) - warn_on_http_retry_error(self.refresh, suffix="Retrying until Task.wait timeouts.") + warn_on_http_retry_error(self.refresh, suffix="Retrying until Task.wait timeouts.", reraise=True) return self['status'] def _refresh_tasks_status(self, pending_tasks, success_tasks, error_tasks, positions): logger.debug("Refreshing batch of Task {}".format(pending_tasks)) task_ids = [task.pk for idx, task in pending_tasks] functor = functools.partial(self.list, task_ids=task_ids) - refreshed_tasks = warn_on_http_retry_error(functor, suffix="Retrying until Task.batch_wait timeouts.") - if refreshed_tasks is None: - return pending_tasks + refreshed_tasks = warn_on_http_retry_error(functor, suffix="Retrying until Task.batch_wait timeouts.", reraise=True) pending_tasks[:] = [] # clear the list (we have to keep the reference) for task in refreshed_tasks: diff --git a/deepomatic/api/utils.py b/deepomatic/api/utils.py index ec20959..15e26e7 100644 --- a/deepomatic/api/utils.py +++ b/deepomatic/api/utils.py @@ -24,28 +24,31 @@ import logging +from deepomatic.api.exceptions import HTTPRetryError from tenacity import (RetryError, Retrying, after_log, before_log, stop_after_delay, stop_never, wait_random_exponential) logger = logging.getLogger(__name__) -def retry(apply_func, retry_if, wait, stop): +def retry(apply_func, retry_if, wait, stop, **kwargs): retryer = Retrying(retry=retry_if, wait=wait, stop=stop, before=before_log(logger, logging.DEBUG), - after=after_log(logger, logging.DEBUG)) + after=after_log(logger, logging.DEBUG), + **kwargs) return retryer(apply_func) -def warn_on_http_retry_error(http_func, suffix=''): - # http helper can raise a RetryError +def warn_on_http_retry_error(http_func, suffix='', reraise=True): + # http helper can raise a HTTPRetryError try: # this should be an http_helper call return http_func() - except RetryError as e: - last_exception = e.last_attempt.exception(timeout=0) + except HTTPRetryError as e: + last_attempt = e.last_attempt + last_exception = last_attempt.exception(timeout=0) msg = "HTTPHelper failed to refresh task status. In the last attempt, " if last_exception is None: last_response = last_attempt.result() @@ -55,4 +58,5 @@ def warn_on_http_retry_error(http_func, suffix=''): if suffix: msg += ' ' + suffix logger.warning(msg) - return None + if reraise: + raise diff --git a/tests/test_client.py b/tests/test_client.py index 5cb6af0..9ec1d88 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -14,7 +14,7 @@ import requests import six from deepomatic.api.client import Client -from deepomatic.api.exceptions import BadStatus +from deepomatic.api.exceptions import BadStatus, TaskTimeout, HTTPRetryError, TaskRetryError from deepomatic.api.http_retry import HTTPRetry from deepomatic.api.inputs import ImageInput from deepomatic.api.version import __title__, __version__ @@ -50,9 +50,13 @@ def download_file(url): return filename +def get_client(*args, **kwargs): + return Client(*args, user_agent_prefix=USER_AGENT_PREFIX, **kwargs) + + @pytest.fixture(scope='session') def client(): - yield Client(user_agent_prefix=USER_AGENT_PREFIX) + yield get_client() @pytest.fixture(scope='session') @@ -300,6 +304,10 @@ class TestClientRetry(object): DEFAULT_TIMEOUT = 2 DEFAULT_MIN_ATTEMPT_NUMBER = 3 + def get_client_with_retry(self): + http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) + return get_client(http_retry=http_retry) + def send_request_and_expect_retry(self, client, timeout, min_attempt_number): spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') # doesn't make any http call start_time = time.time() @@ -314,7 +322,7 @@ def send_request_and_expect_retry(self, client, timeout, min_attempt_number): def test_retry_network_failure(self): http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) - client = Client(host='http://unknown-domain.com', + client = get_client(host='http://unknown-domain.com', http_retry=http_retry) last_attempt = self.send_request_and_expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) @@ -334,8 +342,7 @@ def register_uri(self, methods, status): @httpretty.activate def test_retry_bad_http_status(self): self.register_uri([httpretty.GET, httpretty.POST], 502) - http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) - client = Client(http_retry=http_retry) + client = self.get_client_with_retry() last_attempt = self.send_request_and_expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) assert last_attempt.exception(timeout=0) is None # no exception raised during retry @@ -345,8 +352,7 @@ def test_retry_bad_http_status(self): @httpretty.activate def test_no_retry_create_network(self): self.register_uri([httpretty.GET, httpretty.POST], 502) - http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) - client = Client(http_retry=http_retry) + client = self.get_client_with_retry() # Creating network doesn't retry, we directly get a 502 t = time.time() with pytest.raises(BadStatus) as exc: @@ -357,9 +363,38 @@ def test_no_retry_create_network(self): assert 502 == exc.status_code assert time.time() - t < 0.3 + def test_retry_task_with_http_errors(self): + # We create two clients on purpose because of a bug in httpretty + # https://github.com/gabrielfalcao/HTTPretty/issues/381 + # Also this allow us to test a simple requests with no retryer + + client = get_client(http_retry=None) + spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') + task = spec.inference(inputs=[ImageInput(DEMO_URL)], return_task=True, wait_task=False) + + client = self.get_client_with_retry() + with httpretty.enabled(): + task = client.Task.retrieve(task.pk) + httpretty.register_uri( + httpretty.GET, + re.compile(r'https?://.*?/tasks/\d+/?'), + status=502, + ) + + with pytest.raises(TaskTimeout) as task_timeout: + task.wait(timeout=5) + + # Test nested retry errors + # TaskRetryError has been raised because of too many HTTPRetryError + # (couldn't refresh the status once) + retry_error = task_timeout.value.retry_error + assert isinstance(retry_error, TaskRetryError) + last_exception = retry_error.last_attempt.exception(timeout=0) + assert isinstance(last_exception, HTTPRetryError) + assert 502 == last_exception.last_attempt.result().status_code + def test_no_retry_blacklist_exception(self): - http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) - client = Client(http_retry=http_retry) + client = self.get_client_with_retry() # check that there is no retry on exceptions from DEFAULT_RETRY_EXCEPTION_TYPES_BLACKLIST with pytest.raises(MissingSchema): client.http_helper.http_retry.retry(functools.partial(requests.get, '')) From c4ff0cf2c758d4069b234add21ff7f81dcf927a6 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Mon, 23 Sep 2019 20:55:23 +0200 Subject: [PATCH 16/26] rollback comment --- deepomatic/api/exceptions.py | 5 ++--- deepomatic/api/utils.py | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/deepomatic/api/exceptions.py b/deepomatic/api/exceptions.py index 4aeeba4..7587ee9 100644 --- a/deepomatic/api/exceptions.py +++ b/deepomatic/api/exceptions.py @@ -29,9 +29,8 @@ ############################################################################### class DeepomaticException(Exception): - pass - # def __init__(self, msg): - # super(DeepomaticException, self).__init__(msg) + def __init__(self, msg): + super(DeepomaticException, self).__init__(msg) ############################################################################### diff --git a/deepomatic/api/utils.py b/deepomatic/api/utils.py index 15e26e7..01d4784 100644 --- a/deepomatic/api/utils.py +++ b/deepomatic/api/utils.py @@ -60,3 +60,4 @@ def warn_on_http_retry_error(http_func, suffix='', reraise=True): logger.warning(msg) if reraise: raise + return None From 9d903a6f78a34ba6b4cc73d74aa1f12461327510 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Tue, 24 Sep 2019 12:13:34 +0200 Subject: [PATCH 17/26] Default HTTPRetry using kwargs --- deepomatic/api/http_helper.py | 10 +++++----- deepomatic/api/resources/task.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 5a1ac35..2247c6c 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -51,7 +51,7 @@ class HTTPHelper(object): def __init__(self, app_id=None, api_key=None, verify_ssl=None, host=None, version=API_VERSION, check_query_parameters=True, user_agent_prefix='', user_agent_suffix='', pool_maxsize=20, - requests_timeout=RequestsTimeout.FAST, http_retry=HTTPRetry()): + requests_timeout=RequestsTimeout.FAST, **kwargs): """ Init the HTTP helper with API key and secret """ @@ -66,10 +66,10 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, if app_id is None or api_key is None: raise DeepomaticException("Please specify 'app_id' and 'api_key' either by passing those values to the client or by defining the DEEPOMATIC_APP_ID and DEEPOMATIC_API_KEY environment variables.") - # WARNING for developers: `self.http_retry` should not be modified in this class. - # It should be a constant object. - # This will affect the default parameter otherwise. - self.http_retry = http_retry + try: + self.http_retry = kwargs.pop('http_retry') + except KeyError: + self.http_retry = HTTPRetry() self.requests_timeout = requests_timeout diff --git a/deepomatic/api/resources/task.py b/deepomatic/api/resources/task.py index d392dcb..99c8d3f 100644 --- a/deepomatic/api/resources/task.py +++ b/deepomatic/api/resources/task.py @@ -99,7 +99,7 @@ def wait(self, **retry_kwargs): def _refresh_status(self): logger.debug("Refreshing Task {}".format(self)) warn_on_http_retry_error(self.refresh, suffix="Retrying until Task.wait timeouts.", reraise=True) - return self['status'] + return self._data['status'] def _refresh_tasks_status(self, pending_tasks, success_tasks, error_tasks, positions): logger.debug("Refreshing batch of Task {}".format(pending_tasks)) From e0d8345be0c6645fa6209e4fc47aeb6fe49d501f Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Tue, 24 Sep 2019 12:19:53 +0200 Subject: [PATCH 18/26] doc --- deepomatic/api/client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/deepomatic/api/client.py b/deepomatic/api/client.py index 9fe8453..72e887e 100644 --- a/deepomatic/api/client.py +++ b/deepomatic/api/client.py @@ -62,10 +62,12 @@ def __init__(self, *args, **kwargs): Defaults to 20. :type pool_maxsize: int :param requests_timeout: timeout of each request. - Defaults to http_helper.RequestsTimeout.FAST. + Defaults to `http_helper.RequestsTimeout.FAST`. More details in the `requests` documentation: https://2.python-requests.org//en/master/user/advanced/#timeouts :type requests_timeout: float or tuple(float, float) :param http_retry (optional): Customize the retry of http errors. + Defaults to `HTTPRetry()`. Check out `http_retry.HTTPRetry` documentation for more information about the parameters and default values. + If None, no retry will be done on errors. :type http_retry: http_retry.HTTPRetry :return: :class:`Client` object From 9d41dbbb4272f87231bc729b91afd0db7458ed89 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Tue, 24 Sep 2019 12:23:29 +0200 Subject: [PATCH 19/26] doc --- deepomatic/api/http_helper.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 2247c6c..0618d02 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -53,8 +53,17 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, user_agent_prefix='', user_agent_suffix='', pool_maxsize=20, requests_timeout=RequestsTimeout.FAST, **kwargs): """ - Init the HTTP helper with API key and secret + Init the HTTP helper with API key and secret. + Check out `client.Client` documentation for more details about the parameters. """ + + try: + self.http_retry = kwargs.pop('http_retry') + except KeyError: + self.http_retry = HTTPRetry() + + self.requests_timeout = requests_timeout + if host is None: host = os.getenv('DEEPOMATIC_API_URL', API_HOST) if verify_ssl is None: @@ -66,13 +75,6 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, if app_id is None or api_key is None: raise DeepomaticException("Please specify 'app_id' and 'api_key' either by passing those values to the client or by defining the DEEPOMATIC_APP_ID and DEEPOMATIC_API_KEY environment variables.") - try: - self.http_retry = kwargs.pop('http_retry') - except KeyError: - self.http_retry = HTTPRetry() - - self.requests_timeout = requests_timeout - if not isinstance(version, string_types): version = 'v%g' % version elif version[0] != 'v': From 8ed1a149f97761d04c5a46e5266a328d2ad53a4e Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Tue, 24 Sep 2019 12:23:48 +0200 Subject: [PATCH 20/26] doc --- deepomatic/api/http_helper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 0618d02..caa9c74 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -54,7 +54,7 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, requests_timeout=RequestsTimeout.FAST, **kwargs): """ Init the HTTP helper with API key and secret. - Check out `client.Client` documentation for more details about the parameters. + Check out the `client.Client` documentation for more details about the parameters. """ try: From ec5f6cc424eafd9874ad3f486a8de6e6e272d68a Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Wed, 25 Sep 2019 10:25:09 +0200 Subject: [PATCH 21/26] Update deepomatic/api/client.py Co-Authored-By: Thomas Riccardi --- deepomatic/api/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deepomatic/api/client.py b/deepomatic/api/client.py index 72e887e..c02d538 100644 --- a/deepomatic/api/client.py +++ b/deepomatic/api/client.py @@ -63,7 +63,7 @@ def __init__(self, *args, **kwargs): :type pool_maxsize: int :param requests_timeout: timeout of each request. Defaults to `http_helper.RequestsTimeout.FAST`. - More details in the `requests` documentation: https://2.python-requests.org//en/master/user/advanced/#timeouts + More details in the `requests` documentation: https://2.python-requests.org/en/master/user/advanced/#timeouts :type requests_timeout: float or tuple(float, float) :param http_retry (optional): Customize the retry of http errors. Defaults to `HTTPRetry()`. Check out `http_retry.HTTPRetry` documentation for more information about the parameters and default values. From 59be1cbaa80b4d1129fb9c36ed65a8c928749292 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Wed, 25 Sep 2019 10:25:22 +0200 Subject: [PATCH 22/26] Update deepomatic/api/client.py Co-Authored-By: Thomas Riccardi --- deepomatic/api/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deepomatic/api/client.py b/deepomatic/api/client.py index c02d538..f41a20b 100644 --- a/deepomatic/api/client.py +++ b/deepomatic/api/client.py @@ -67,7 +67,7 @@ def __init__(self, *args, **kwargs): :type requests_timeout: float or tuple(float, float) :param http_retry (optional): Customize the retry of http errors. Defaults to `HTTPRetry()`. Check out `http_retry.HTTPRetry` documentation for more information about the parameters and default values. - If None, no retry will be done on errors. + If `None`, no retry will be done on errors. :type http_retry: http_retry.HTTPRetry :return: :class:`Client` object From 90ee0b84c6bb9a730ad8e417f287c6fdb8e32083 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Wed, 25 Sep 2019 10:36:29 +0200 Subject: [PATCH 23/26] Update deepomatic/api/resources/network.py Co-Authored-By: Thomas Riccardi --- deepomatic/api/resources/network.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deepomatic/api/resources/network.py b/deepomatic/api/resources/network.py index 94f564c..5ae60aa 100644 --- a/deepomatic/api/resources/network.py +++ b/deepomatic/api/resources/network.py @@ -70,7 +70,7 @@ def inference(self, convert_to_numpy=True, return_task=False, **kwargs): def create(self, *args, **kwargs): # No retry on network create by default as this is an heavy request - kwargs['http_retry'] = kwargs.get('http_retry') + kwargs['http_retry'] = kwargs.get('http_retry', None) kwargs['timeout'] = kwargs.get('timeout', RequestsTimeout.SLOW) return super(Network, self).create(*args, **kwargs) From 2ed5fb2f440f8a5e86fda63e8698eb26320c30b7 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Wed, 25 Sep 2019 10:42:05 +0200 Subject: [PATCH 24/26] fix conflicts --- deepomatic/api/http_helper.py | 22 ++++++++-------------- deepomatic/api/resources/network.py | 2 +- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index caa9c74..21b34b6 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -57,10 +57,10 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, Check out the `client.Client` documentation for more details about the parameters. """ - try: - self.http_retry = kwargs.pop('http_retry') - except KeyError: - self.http_retry = HTTPRetry() + self.http_retry = kwargs.pop('http_retry', HTTPRetry()) + + if len(kwargs) > 0: + raise TypeError("Too many parameters. HTTPRetry does not handle kwargs: {}".format(kwargs)) self.requests_timeout = requests_timeout @@ -179,16 +179,10 @@ def recursive_json_dump(prefix, obj, data_dict, omit_dot=False): def send_request(self, requests_callable, *args, **kwargs): # requests_callable must be a method from the requests module - try: - # this is the timeout of requests module - requests_timeout = kwargs.pop('timeout') - except KeyError: - requests_timeout = self.requests_timeout - - try: - http_retry = kwargs.pop('http_retry') - except KeyError: - http_retry = self.http_retry + + # this is the timeout of requests module + requests_timeout = kwargs.pop('timeout', self.requests_timeout) + http_retry = kwargs.pop('http_retry', self.http_retry) functor = functools.partial(requests_callable, *args, verify=self.verify, diff --git a/deepomatic/api/resources/network.py b/deepomatic/api/resources/network.py index 5ae60aa..10c2be9 100644 --- a/deepomatic/api/resources/network.py +++ b/deepomatic/api/resources/network.py @@ -69,7 +69,7 @@ def inference(self, convert_to_numpy=True, return_task=False, **kwargs): return result def create(self, *args, **kwargs): - # No retry on network create by default as this is an heavy request + # No retry on Network.create() errors by default as this is a large request kwargs['http_retry'] = kwargs.get('http_retry', None) kwargs['timeout'] = kwargs.get('timeout', RequestsTimeout.SLOW) return super(Network, self).create(*args, **kwargs) From 0f911407d0d5dd701072549c719872d076c529eb Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Wed, 25 Sep 2019 10:59:52 +0200 Subject: [PATCH 25/26] fix flake warnings --- deepomatic/api/__init__.py | 2 ++ deepomatic/api/mixins.py | 1 + deepomatic/api/resources/task.py | 2 +- deepomatic/api/utils.py | 3 +-- setup.py | 6 ++++-- tests/test_client.py | 4 ++-- 6 files changed, 11 insertions(+), 7 deletions(-) diff --git a/deepomatic/api/__init__.py b/deepomatic/api/__init__.py index 97cf7a6..1850c0f 100644 --- a/deepomatic/api/__init__.py +++ b/deepomatic/api/__init__.py @@ -1 +1,3 @@ from deepomatic.api.version import __version__ + +__all__ = ["__version__"] diff --git a/deepomatic/api/mixins.py b/deepomatic/api/mixins.py index 5fb17bb..a6857e2 100644 --- a/deepomatic/api/mixins.py +++ b/deepomatic/api/mixins.py @@ -25,6 +25,7 @@ from deepomatic.api.exceptions import DeepomaticException from deepomatic.api.resource import ResourceList + ############################################################################### class Arg(object): diff --git a/deepomatic/api/resources/task.py b/deepomatic/api/resources/task.py index 99c8d3f..2112794 100644 --- a/deepomatic/api/resources/task.py +++ b/deepomatic/api/resources/task.py @@ -31,7 +31,7 @@ from deepomatic.api.resource import Resource from deepomatic.api.utils import retry, warn_on_http_retry_error from tenacity import (RetryError, retry_if_exception_type, retry_if_result, - stop_after_delay, wait_chain, wait_fixed, + stop_after_delay, wait_chain, wait_fixed, stop_never, wait_random_exponential) logger = logging.getLogger(__name__) diff --git a/deepomatic/api/utils.py b/deepomatic/api/utils.py index 01d4784..1a53a28 100644 --- a/deepomatic/api/utils.py +++ b/deepomatic/api/utils.py @@ -25,8 +25,7 @@ import logging from deepomatic.api.exceptions import HTTPRetryError -from tenacity import (RetryError, Retrying, after_log, before_log, - stop_after_delay, stop_never, wait_random_exponential) +from tenacity import (Retrying, after_log, before_log) logger = logging.getLogger(__name__) diff --git a/setup.py b/setup.py index bed6697..58addf5 100644 --- a/setup.py +++ b/setup.py @@ -2,9 +2,11 @@ import io from setuptools import find_packages, setup -try: # for pip >= 10 +try: + # for pip >= 10 from pip._internal.req import parse_requirements -except ImportError: # for pip <= 9.0.3 +except ImportError: + # for pip <= 9.0.3 from pip.req import parse_requirements diff --git a/tests/test_client.py b/tests/test_client.py index 9ec1d88..e58e24d 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -312,7 +312,7 @@ def send_request_and_expect_retry(self, client, timeout, min_attempt_number): spec = client.RecognitionSpec.retrieve('imagenet-inception-v3') # doesn't make any http call start_time = time.time() with pytest.raises(RetryError) as exc: - print(spec.data()) # does make a http call + print(spec.data()) # does make a http call diff = time.time() - start_time assert diff > timeout and diff < timeout + HTTPRetry.Default.RETRY_EXP_MAX @@ -323,7 +323,7 @@ def send_request_and_expect_retry(self, client, timeout, min_attempt_number): def test_retry_network_failure(self): http_retry = HTTPRetry(stop=stop_after_delay(self.DEFAULT_TIMEOUT)) client = get_client(host='http://unknown-domain.com', - http_retry=http_retry) + http_retry=http_retry) last_attempt = self.send_request_and_expect_retry(client, self.DEFAULT_TIMEOUT, self.DEFAULT_MIN_ATTEMPT_NUMBER) exc = last_attempt.exception(timeout=0) From 6ebfe51bd41673aee7ce9e0112520ee5f28766c0 Mon Sep 17 00:00:00 2001 From: Hugo Maingonnat Date: Wed, 25 Sep 2019 11:09:30 +0200 Subject: [PATCH 26/26] comment --- deepomatic/api/http_helper.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/deepomatic/api/http_helper.py b/deepomatic/api/http_helper.py index 21b34b6..3dadbd9 100644 --- a/deepomatic/api/http_helper.py +++ b/deepomatic/api/http_helper.py @@ -57,6 +57,8 @@ def __init__(self, app_id=None, api_key=None, verify_ssl=None, Check out the `client.Client` documentation for more details about the parameters. """ + # `http_retry` is retrieved from `kwargs` because a default parameter `http_retry=HTTPRetry()` is dangerous + # If the rest of the code mutates `self.http_retry`, it would change the default parameter for all other `Client` instances self.http_retry = kwargs.pop('http_retry', HTTPRetry()) if len(kwargs) > 0: