-
Notifications
You must be signed in to change notification settings - Fork 6
Enable configuring HTTP transport retry behavior and add request_timeout
parameter to get_detector
#386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable configuring HTTP transport retry behavior and add request_timeout
parameter to get_detector
#386
Changes from all commits
6a16f78
ea19514
5f03bef
976ba03
1f7f045
459bcb9
0bfd0ff
56cc766
920f909
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ | |
PaginatedImageQueryList, | ||
) | ||
from urllib3.exceptions import InsecureRequestWarning | ||
from urllib3.util.retry import Retry | ||
|
||
from groundlight.binary_labels import Label, convert_internal_label_to_display | ||
from groundlight.config import API_TOKEN_MISSING_HELP_MESSAGE, API_TOKEN_VARIABLE_NAME, DISABLE_TLS_VARIABLE_NAME | ||
|
@@ -133,26 +134,31 @@ def __init__( | |
endpoint: Optional[str] = None, | ||
api_token: Optional[str] = None, | ||
disable_tls_verification: Optional[bool] = None, | ||
http_transport_retries: Optional[Union[int, Retry]] = None, | ||
): | ||
""" | ||
Initialize a new Groundlight client instance. | ||
|
||
:param endpoint: Optional custom API endpoint URL. If not specified, uses the default Groundlight endpoint. | ||
:param api_token: Authentication token for API access. | ||
If not provided, will attempt to read from the "GROUNDLIGHT_API_TOKEN" environment variable. | ||
If not provided, will attempt to read from the "GROUNDLIGHT_API_TOKEN" environment variable. | ||
:param disable_tls_verification: If True, disables SSL/TLS certificate verification for API calls. | ||
When not specified, checks the "DISABLE_TLS_VERIFY" environment variable | ||
(1=disable, 0=enable). Certificate verification is enabled by default. | ||
When not specified, checks the "DISABLE_TLS_VERIFY" environment variable (1=disable, 0=enable). | ||
Certificate verification is enabled by default. | ||
|
||
Warning: Only disable verification when connecting to a Groundlight Edge | ||
Endpoint using self-signed certificates. For security, always keep | ||
verification enabled when using the Groundlight cloud service. | ||
Warning: Only disable verification when connecting to a Groundlight Edge Endpoint using self-signed | ||
certificates. For security, always keep verification enabled when using the Groundlight cloud service. | ||
Comment on lines
+144
to
+150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just corrected the formatting on these. |
||
:param http_transport_retries: Overrides urllib3 `PoolManager` retry policy for HTTP/HTTPS (forwarded to | ||
`Configuration.retries`). Not the same as SDK 5xx retries handled by `RequestsRetryDecorator`. | ||
|
||
:return: Groundlight client | ||
""" | ||
# Specify the endpoint | ||
self.endpoint = sanitize_endpoint_url(endpoint) | ||
self.configuration = Configuration(host=self.endpoint) | ||
if http_transport_retries is not None: | ||
# Once we upgrade openapitools to ^7.7.0, retries can be passed into the constructor of Configuration above. | ||
self.configuration.retries = http_transport_retries | ||
|
||
if not api_token: | ||
try: | ||
|
@@ -264,7 +270,11 @@ def _user_is_privileged(self) -> bool: | |
obj = self.user_api.who_am_i() | ||
return obj["is_superuser"] | ||
|
||
def get_detector(self, id: Union[str, Detector]) -> Detector: # pylint: disable=redefined-builtin | ||
def get_detector( | ||
self, | ||
id: Union[str, Detector], # pylint: disable=redefined-builtin | ||
request_timeout: Optional[Union[float, Tuple[float, float]]] = None, | ||
) -> Detector: | ||
""" | ||
Get a Detector by id. | ||
|
||
|
@@ -275,6 +285,8 @@ def get_detector(self, id: Union[str, Detector]) -> Detector: # pylint: disable | |
print(detector) | ||
|
||
:param id: the detector id | ||
:param request_timeout: The request timeout for the image query submission API request. Most users will not need | ||
to modify this. If not set, the default value will be used. | ||
|
||
:return: Detector | ||
""" | ||
|
@@ -283,7 +295,8 @@ def get_detector(self, id: Union[str, Detector]) -> Detector: # pylint: disable | |
# Short-circuit | ||
return id | ||
try: | ||
obj = self.detectors_api.get_detector(id=id, _request_timeout=DEFAULT_REQUEST_TIMEOUT) | ||
request_timeout = request_timeout if request_timeout is not None else DEFAULT_REQUEST_TIMEOUT | ||
obj = self.detectors_api.get_detector(id=id, _request_timeout=request_timeout) | ||
except NotFoundException as e: | ||
raise NotFoundError(f"Detector with id '{id}' not found") from e | ||
return Detector.parse_obj(obj.to_dict()) | ||
|
@@ -662,7 +675,7 @@ def submit_image_query( # noqa: PLR0913 # pylint: disable=too-many-arguments, t | |
inspection_id: Optional[str] = None, | ||
metadata: Union[dict, str, None] = None, | ||
image_query_id: Optional[str] = None, | ||
request_timeout: Optional[float] = None, | ||
request_timeout: Optional[Union[float, Tuple[float, float]]] = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's a Tuple[float, float] do here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The request timeout has two components, the "connect timeout" and the "read timeout". If you supply a single number, that will be the value for the total timeout between both of those. If you supply a tuple with two values, it'll use those values as (connect_timeout, read_timeout). |
||
) -> ImageQuery: | ||
""" | ||
Evaluates an image with Groundlight. This is the core method for getting predictions about images. | ||
|
@@ -744,8 +757,8 @@ def submit_image_query( # noqa: PLR0913 # pylint: disable=too-many-arguments, t | |
:param image_query_id: The ID for the image query. This is to enable specific functionality | ||
and is not intended for general external use. If not set, a random ID | ||
will be generated. | ||
:param request_timeout: The total request timeout for the image query submission API request. Most users will | ||
not need to modify this. If not set, the default value will be used. | ||
:param request_timeout: The request timeout for the image query submission API request. Most users will not need | ||
to modify this. If not set, the default value will be used. | ||
|
||
:return: ImageQuery with query details and result (if wait > 0) | ||
:raises ValueError: If wait > 0 when want_async=True | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This upgrade is necessary for the
retries
parameter on theConfiguration
to actually be respected when requests are made. 2.5.0 is the most recent release - likely the fix was made earlier, but I don't see a reason not to update to the most recent (let me know if you know of a reason not to upgrade this, though).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a nasty debug for you. The upgrade makes perfect sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was indeed a hairy process!