-
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
Conversation
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. |
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.
Just corrected the formatting on these.
requests = "^2.28.2" | ||
typer = "^0.15.4" | ||
urllib3 = "^1.26.9" | ||
urllib3 = "^2.5.0" |
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 the Configuration
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!
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.
Makes sense, I like it
requests = "^2.28.2" | ||
typer = "^0.15.4" | ||
urllib3 = "^1.26.9" | ||
urllib3 = "^2.5.0" |
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
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 comment
The 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 comment
The 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).
This PR includes two distinct changes.
http_transport_retries
to theGroundlight
constructor, enabling the retry behavior to be modified when initializing aGroundlight
instance.Groundlight
instance.Groundlight
instance has been initialized (this is technically possible but awkward).request_timeout
parameter to theget_detector
method which enables changing the request timeout values for the request.submit_image_query
. Eventually I think we'll want to make this available for all methods, but for now we don't need that.These changes are intended to be utilized by the edge endpoint to improve behavior under poor network connectivity.