Make timeouts part of RequestsSettings struct (#450)

* Make timeouts part of RequestsSettings struct

docs/source/code_examples/tutorials/configurations/overwrite_timeouts.py

@@ -1,29 +1,14 @@
-from encord import Dataset, EncordUserClient
-from encord.client import EncordClientDataset
-
-user_client: EncordUserClient = EncordUserClient.create_with_ssh_private_key(
-    "<your_private_key>",
-)
+from encord import EncordUserClient
+from encord.http.constants import RequestsSettings
 
 NEW_TIMEOUT = 300  # seconds
 
-# For user_client requests specifically. These will NOT automatically
-# propagate to the Dataset or Project objects
-user_client.user_config.read_timeout = NEW_TIMEOUT
-user_client.user_config.write_timeout = NEW_TIMEOUT
-user_client.user_config.connect_timeout = NEW_TIMEOUT
-
-# The same procedure works for the Project class that is returned from
-# `user_client.get_project("<project_hash>")
-dataset: Dataset = user_client.get_dataset("<dataset_hash>")
-dataset._client._config.read_timeout = NEW_TIMEOUT
-dataset._client._config.write_timeout = NEW_TIMEOUT
-dataset._client._config.connect_timeout = NEW_TIMEOUT
+requests_settings = RequestsSettings(
+    connect_timeout=NEW_TIMEOUT,
+    read_timeout=NEW_TIMEOUT,
+    write_timeout=NEW_TIMEOUT,
+)
 
-# If you are using the deprecated EncordClientDataset instead
-dataset_client: EncordClientDataset = user_client.get_dataset_client(
-    "<dataset_hash>"
+user_client: EncordUserClient = EncordUserClient.create_with_ssh_private_key(
+    "<your_private_key>", requests_settings=requests_settings
 )
-dataset_client._config.read_timeout = NEW_TIMEOUT
-dataset_client._config.write_timeout = NEW_TIMEOUT
-dataset_client._config.connect_timeout = NEW_TIMEOUT

encord/configs.py

@@ -50,20 +50,16 @@
 _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
 
 
 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.connect_timeout
 
         self.endpoint: str = endpoint
         self.requests_settings = requests_settings

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."""
 
+    connect_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()

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,

encord/http/utils.py

@@ -148,7 +148,9 @@ def _upload_single_file(
     backoff_factor: float,
     cache_max_age: int = CACHE_DURATION_IN_SECONDS,
 ) -> None:
-    with create_new_session(max_retries=max_retries, backoff_factor=backoff_factor) as session:
+    with create_new_session(
+        max_retries=max_retries, backoff_factor=backoff_factor, connect_retries=max_retries
+    ) as session:
         url = signed_url["signed_url"]
 
         with open(file_path, "rb") as f:

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)

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")

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.connect_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(
+        connect_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)