Make timeouts part of RequestsSettings struct #450
Conversation
|
Walkthrough: This update introduces a unified approach to setting connection timeouts and retries across the application. It replaces hard-coded values with configurable settings via a Changes:
NotesChat with
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (6)
- encord/configs.py (1)
- encord/http/constants.py (2)
- encord/http/querier.py (2)
- encord/http/v2/api_client.py (1)
- tests/fixtures.py (2)
- tests/http/timeout_overrides_setting.py (1)
Files not reviewed due to errors (2)
- encord/http/constants.py (no response)
- encord/http/v2/api_client.py (no response)
Files skipped from review due to trivial changes (1)
- tests/fixtures.py
Review comments generated (5)
- Review: 3
- LGTM: 2
sergei-encord
force-pushed
the
fix-timeout-settings-propagation
branch
from
e3b94c3 to
17b9d27
Compare
There was a problem hiding this comment.
Files selected (7)
- encord/configs.py (1)
- encord/http/constants.py (2)
- encord/http/querier.py (2)
- encord/http/utils.py (1)
- encord/http/v2/api_client.py (1)
- tests/fixtures.py (2)
- tests/http/timeout_overrides_setting.py (1)
Files not reviewed due to errors (3)
- encord/http/constants.py (no response)
- encord/configs.py (no response)
- encord/http/v2/api_client.py (no response)
Files skipped from review due to trivial changes (1)
- tests/fixtures.py
Review comments generated (5)
- Review: 3
- LGTM: 2
| 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: |
There was a problem hiding this comment.
The create_new_session function now includes a new parameter connect_retries. This is a good change as it allows the number of connection retries to be configured, improving the flexibility and robustness of the HTTP client. However, it's not clear from this hunk whether the create_new_session function has been updated to handle this new argument. Please ensure that the function definition of create_new_session has been updated accordingly.
| 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) |
There was a problem hiding this comment.
The _request method now includes a timeout parameter. However, it's not clear from the provided context whether this timeout value is being used correctly within the Request object. If the Request class doesn't handle the timeout parameter properly, it could lead to unexpected behavior. Please ensure that the Request class or any other classes/functions using this method are updated accordingly to handle this new parameter.
|
|
||
| @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, |
There was a problem hiding this comment.
The signature of the create_new_session function has been modified to remove default values for max_retries, backoff_factor, and connect_retries. This change will require all calls to create_new_session to provide these parameters explicitly. Ensure that all parts of the codebase that call this function have been updated to pass these parameters. Otherwise, this could lead to a TypeError at runtime when the function is called with missing arguments.
There was a problem hiding this comment.
Files selected (4)
- docs/source/code_examples/tutorials/configurations/overwrite_timeouts.py (1)
- encord/configs.py (1)
- encord/http/constants.py (2)
- tests/http/timeout_overrides_setting.py (1)
Files skipped from review due to trivial changes (1)
- docs/source/code_examples/tutorials/configurations/overwrite_timeouts.py
Review comments generated (5)
- Review: 2
- LGTM: 3
| _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 |
There was a problem hiding this comment.
The changes look good. The timeouts are now part of the RequestsSettings struct and are no longer hard-coded, which improves flexibility and maintainability. The BaseConfig class now uses these settings from the requests_settings object. This is a good practice as it allows for easier configuration changes in the future.
| 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 | ||
| ) | ||
|
|
There was a problem hiding this comment.
The function verify_timeout_and_reset checks if the timeout settings are correctly propagated. However, it only checks for connect_timeout and read_timeout. The write_timeout is not checked here. If the write_timeout is also supposed to be propagated and checked, this would be a logic issue.
| 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 | |
| ) | |
| 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, requests_settings.write_timeout) | |
| for call in mock.call_args_list | |
| ) | |
| mock.reset_mock() |
Currently connection retry settings and connection timeouts are set in different ways.
This PR unifies them and makes sure they are correctly propagated to all objects.
Summary by CodeRabbit
New Feature:
RequestsSettingsstruct to unify connection timeouts and retries settings across the application.BaseConfig,http/constants.py,http/querier.py,http/v2/api_client.py,http/utils.pyto use the newRequestsSettingsstruct.tests/http/timeout_overrides_setting.pyto verify the unified timeout settings.docs/source/code_examples/tutorials/configurations/overwrite_timeouts.pyto reflect the changes.Refactor:
tests/fixtures.py.