From e3b94c34e2d204a9c38af3014a1da07363e66d8f Mon Sep 17 00:00:00 2001 From: Sergei Chertkov Date: Tue, 17 Oct 2023 17:26:36 +0100 Subject: [PATCH] Make timeouts part of RequestsSettings struct --- encord/configs.py | 10 +- encord/http/constants.py | 13 +++ encord/http/querier.py | 4 +- encord/http/v2/api_client.py | 6 +- tests/fixtures.py | 17 ++-- tests/http/timeout_overrides_setting.py | 124 ++++++++++++++++++++++++ 6 files changed, 153 insertions(+), 21 deletions(-) create mode 100644 tests/http/timeout_overrides_setting.py diff --git a/encord/configs.py b/encord/configs.py index 3a593eeca..12b738979 100644 --- a/encord/configs.py +++ b/encord/configs.py @@ -50,10 +50,6 @@ _ENCORD_SSH_KEY = "ENCORD_SSH_KEY" _ENCORD_SSH_KEY_FILE = "ENCORD_SSH_KEY_FILE" -READ_TIMEOUT = 180 # In seconds -WRITE_TIMEOUT = 180 # In seconds -CONNECT_TIMEOUT = 180 # In seconds - logger = logging.getLogger(__name__) from encord.exceptions import ResourceNotFoundError @@ -61,9 +57,9 @@ class BaseConfig(ABC): def __init__(self, endpoint: str, requests_settings: RequestsSettings = DEFAULT_REQUESTS_SETTINGS): - self.read_timeout: int = READ_TIMEOUT - self.write_timeout: int = WRITE_TIMEOUT - self.connect_timeout: int = CONNECT_TIMEOUT + self.read_timeout: int = requests_settings.read_timeout + self.write_timeout: int = requests_settings.write_timeout + self.connect_timeout: int = requests_settings.connection_timeout self.endpoint: str = endpoint self.requests_settings = requests_settings diff --git a/encord/http/constants.py b/encord/http/constants.py index 67aa36d0f..5ef3940cb 100644 --- a/encord/http/constants.py +++ b/encord/http/constants.py @@ -4,6 +4,10 @@ DEFAULT_MAX_RETRIES = 3 DEFAULT_BACKOFF_FACTOR = 1.5 +DEFAULT_READ_TIMEOUT = 180 # In seconds +DEFAULT_WRITE_TIMEOUT = 180 # In seconds +DEFAULT_CONNECT_TIMEOUT = 180 # In seconds + @dataclass class RequestsSettings: @@ -20,5 +24,14 @@ class RequestsSettings: connection_retries: int = DEFAULT_CONNECTION_RETRIES """Number of allowed retries to establish TCP connection when a request is sent.""" + connection_timeout: int = DEFAULT_CONNECT_TIMEOUT + """Maximum number of seconds from connection establishment to the first byte of response received""" + + read_timeout: int = DEFAULT_READ_TIMEOUT + """Maximum number of seconds to obtain full response""" + + write_timeout: int = DEFAULT_WRITE_TIMEOUT + """Maximum number of seconds to send request payload""" + DEFAULT_REQUESTS_SETTINGS = RequestsSettings() diff --git a/encord/http/querier.py b/encord/http/querier.py index 321adc1d0..0ceba7768 100644 --- a/encord/http/querier.py +++ b/encord/http/querier.py @@ -156,7 +156,7 @@ def _exception_context(request: requests.PreparedRequest) -> RequestContext: except Exception: return RequestContext() - def _request(self, method: QueryMethods, db_object_type: Type[T], uid, timeout, payload=None): + def _request(self, method: QueryMethods, db_object_type: Type[T], uid, timeout: int, payload=None) -> Request: request = Request(method, db_object_type, uid, timeout, self._config.connect_timeout, payload) request.headers = self._config.define_headers(request.data) @@ -205,7 +205,7 @@ def _execute(self, request: Request, retryable=False, enable_logging=True) -> Tu @contextmanager def create_new_session( - max_retries: Optional[int] = None, backoff_factor: float = 0, connect_retries=3 + max_retries: Optional[int], backoff_factor: float, connect_retries ) -> Generator[Session, None, None]: retry_policy = Retry( connect=connect_retries, diff --git a/encord/http/v2/api_client.py b/encord/http/v2/api_client.py index 1df27d104..027969fe0 100644 --- a/encord/http/v2/api_client.py +++ b/encord/http/v2/api_client.py @@ -64,10 +64,12 @@ def get(self, path: Path, params: Optional[BaseDTO], result_type: Type[T]) -> T: req = sign_request(req, self._config.public_key_hex, self._config.private_key) - timeouts = (self._config.write_timeout, self._config.read_timeout) + timeouts = (self._config.connect_timeout, self._config.read_timeout) req_settings = self._config.requests_settings with create_new_session( - max_retries=req_settings.max_retries, backoff_factor=req_settings.backoff_factor + max_retries=req_settings.max_retries, + backoff_factor=req_settings.backoff_factor, + connect_retries=req_settings.connection_retries, ) as session: res = session.send(req, timeout=timeouts) context = self._exception_context_from_response(res) diff --git a/tests/fixtures.py b/tests/fixtures.py index 5f1df0347..1ec76cf46 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -4,7 +4,7 @@ from cryptography.hazmat.primitives import serialization from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey -from encord import EncordUserClient +from encord import EncordUserClient, Project from encord.client import EncordClientProject from encord.http.querier import Querier from encord.ontology import Ontology @@ -24,26 +24,23 @@ @pytest.fixture -def ontology(): - ontology = Ontology(None, None, OrmOntology.from_dict(ONTOLOGY_BLURB)) - return ontology +def ontology() -> Ontology: + return Ontology(None, None, OrmOntology.from_dict(ONTOLOGY_BLURB)) @pytest.fixture -def user_client(): - client = EncordUserClient.create_with_ssh_private_key(DUMMY_PRIVATE_KEY) - return client +def user_client() -> EncordUserClient: + return EncordUserClient.create_with_ssh_private_key(DUMMY_PRIVATE_KEY) @pytest.fixture @patch.object(EncordClientProject, "get_project") @patch.object(Querier, "basic_getter") -def project(querier_mock: Querier, client_project_mock, user_client: EncordUserClient, ontology: Ontology): +def project(querier_mock: Querier, client_project_mock, user_client: EncordUserClient, ontology: Ontology) -> Project: querier_mock.return_value = OrmOntology.from_dict(ONTOLOGY_BLURB) client_project_mock.return_value = OrmProject( {"ontology_hash": "dummy-ontology-hash", "project_hash": "dummy-project-hash"} ) - project = user_client.get_project("dummy-project-hash") - return project + return user_client.get_project("dummy-project-hash") diff --git a/tests/http/timeout_overrides_setting.py b/tests/http/timeout_overrides_setting.py new file mode 100644 index 000000000..d86905440 --- /dev/null +++ b/tests/http/timeout_overrides_setting.py @@ -0,0 +1,124 @@ +import json +from datetime import datetime +from unittest.mock import MagicMock, patch +from uuid import uuid4 + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey +from requests import Session + +from encord.configs import UserConfig +from encord.http.constants import RequestsSettings +from encord.http.v2.api_client import ApiClient +from encord.user_client import EncordUserClient +from tests.fixtures import DUMMY_PRIVATE_KEY + +PRIVATE_KEY = Ed25519PrivateKey.generate() +PROJECT_HASH = uuid4().hex +ONTOLOGY_HASH = uuid4().hex +DATASET_HASH = uuid4().hex + + +@pytest.fixture +def api_client(): + return ApiClient(config=UserConfig(PRIVATE_KEY)) + + +def stub_responses(*args, **kwargs) -> MagicMock: + body_json = json.loads(args[0].body) + request_type = body_json["query_type"] + + if request_type == "project": + response = { + "project_hash": PROJECT_HASH, + "title": "Test project", + "description": "", + "created_at": datetime.utcnow(), + "last_edited_at": datetime.utcnow(), + "editor_ontology": {}, + "datasets": [uuid4().hex], + "label_rows": [], + "ontology_hash": ONTOLOGY_HASH, + } + elif request_type == "ontology": + response = { + "title": "Test ontology", + "description": "", + "ontology_hash": ONTOLOGY_HASH, + "created_at": datetime.utcnow(), + "last_edited_at": datetime.utcnow(), + "editor": { + "objects": [], + "classifications": [], + }, + } + elif request_type == "dataset": + response = { + "dataset_hash": DATASET_HASH, + "title": "Test dataset", + "description": "", + "dataset_type": "cord", + "data_rows": [], + } + elif request_type in ["datarows", "labelrowmetadata"]: + response = {} # These objects are not used in tests, so it's ok to return empty descriptor for now + else: + assert False, f"Unsupported request: {request_type}" + + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = {"status": 200, "response": response} + + return mock_response + + +def verify_timeout_and_reset(mock: MagicMock, requests_settings: RequestsSettings, expected_call_count=1) -> None: + assert mock.call_count == expected_call_count + assert all( + call[1]["timeout"] == (requests_settings.connection_timeout, requests_settings.read_timeout) + for call in mock.call_args_list + ) + + mock.reset_mock() + + +@patch.object(Session, "send") +def test_request_timeout_settings_correctly_propagated(send: MagicMock, api_client: ApiClient): + requests_settings = RequestsSettings( + connection_timeout=1001, + read_timeout=1002, + write_timeout=1003, + ) + + send.side_effect = stub_responses + + user_client = EncordUserClient.create_with_ssh_private_key( + ssh_private_key=DUMMY_PRIVATE_KEY, requests_settings=requests_settings + ) + project = user_client.get_project(PROJECT_HASH) + verify_timeout_and_reset(send, requests_settings, 2) + + project.list_label_rows_v2() + verify_timeout_and_reset(send, requests_settings) + + dataset = user_client.get_dataset(DATASET_HASH) + verify_timeout_and_reset(send, requests_settings) + + dataset.list_data_rows() + verify_timeout_and_reset(send, requests_settings) + + ontology = user_client.get_ontology(ONTOLOGY_HASH) + verify_timeout_and_reset(send, requests_settings) + + ontology.refetch_data() + verify_timeout_and_reset(send, requests_settings) + + # Re-mocking for V2 API + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = {"results": []} + send.side_effect = None + send.return_value = mock_response + + _ = list(project.list_collaborator_timers(datetime.now())) + verify_timeout_and_reset(send, requests_settings)