-
Notifications
You must be signed in to change notification settings - Fork 6
SDK Configures Edge #419
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
base: main
Are you sure you want to change the base?
SDK Configures Edge #419
Changes from all commits
157ea54
b3f35a2
993857f
237c39f
69b256d
dde221c
e7c7e96
4e89881
b0b6b82
9c45e06
ac06a60
91e5839
8446a4e
8d9464f
bce92fe
e96ea12
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 |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import time | ||
| from http import HTTPStatus | ||
|
|
||
| import requests | ||
|
|
||
| from groundlight.client import EdgeNotAvailableError | ||
| from groundlight.edge.config import EdgeEndpointConfig | ||
|
|
||
| _EDGE_METHOD_UNAVAILABLE_HINT = ( | ||
| "Make sure the client is pointed at a running edge endpoint " | ||
| "(via GROUNDLIGHT_ENDPOINT env var or the endpoint= constructor arg)." | ||
| ) | ||
|
|
||
|
|
||
| class EdgeAPI: | ||
| """Namespace for edge-endpoint operations, accessed via ``gl.edge``.""" | ||
|
|
||
| def __init__(self, client) -> None: | ||
| self._client = client | ||
|
|
||
| def _base_url(self) -> str: | ||
| return self._client.edge_base_url() | ||
|
|
||
| def _request(self, method: str, path: str, **kwargs) -> requests.Response: | ||
| url = f"{self._base_url()}{path}" | ||
| headers = self._client.get_raw_headers() | ||
| try: | ||
| response = requests.request( | ||
| method, url, headers=headers, verify=self._client.configuration.verify_ssl, timeout=10, **kwargs | ||
|
Collaborator
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. Claude has a nit: 10 second default is fine but should be overridable via kwargs
Contributor
Author
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. I think this would add complexity without any value to the SDK user. All of these methods should return very quickly. 10 seconds is a reasonable and generous timeout. If the request is taking longer than this, there is something wrong with the network, which won't be fixed by adjusting the timeout. |
||
| ) | ||
| response.raise_for_status() | ||
| except requests.exceptions.HTTPError as e: | ||
| if e.response is not None and e.response.status_code == HTTPStatus.NOT_FOUND: | ||
| raise EdgeNotAvailableError( | ||
| f"Edge method not available at {url}. {_EDGE_METHOD_UNAVAILABLE_HINT}" | ||
| ) from e | ||
| raise | ||
| except requests.exceptions.ConnectionError as e: | ||
| raise EdgeNotAvailableError( | ||
| f"Could not connect to {self._base_url()}. {_EDGE_METHOD_UNAVAILABLE_HINT}" | ||
| ) from e | ||
| return response | ||
|
|
||
| def get_config(self) -> EdgeEndpointConfig: | ||
| """Retrieve the active edge endpoint configuration.""" | ||
| response = self._request("GET", "/edge-config") | ||
| return EdgeEndpointConfig.from_payload(response.json()) | ||
|
|
||
| def get_detector_readiness(self) -> dict[str, bool]: | ||
| """Check which configured detectors have inference pods ready to serve. | ||
|
|
||
| :return: Dict mapping detector_id to readiness (True/False). | ||
| """ | ||
| response = self._request("GET", "/edge-detector-readiness") | ||
| return {det_id: info["ready"] for det_id, info in response.json().items()} | ||
|
|
||
| def set_config( | ||
| self, | ||
| config: EdgeEndpointConfig, | ||
| timeout_sec: float = 600, | ||
| ) -> EdgeEndpointConfig: | ||
| """Replace the edge endpoint configuration and wait until all detectors are ready. | ||
|
|
||
| :param config: The new configuration to apply. | ||
| :param timeout_sec: Max seconds to wait for all detectors to become ready. | ||
| :return: The applied configuration as reported by the edge endpoint. | ||
| """ | ||
| self._request("PUT", "/edge-config", json=config.to_payload()) | ||
|
|
||
| desired_ids = {d.detector_id for d in config.detectors if d.detector_id} | ||
|
Collaborator
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. Claude: d.detector_id should never be falsey, it's misleading to have the check here |
||
| if not desired_ids: | ||
| return self.get_config() | ||
|
|
||
| deadline = time.time() + timeout_sec | ||
| while time.time() < deadline: | ||
| readiness = self.get_detector_readiness() | ||
| if all(readiness.get(did, False) for did in desired_ids): | ||
| return self.get_config() | ||
| time.sleep(1) | ||
|
|
||
| raise TimeoutError( | ||
| f"Edge detectors were not all ready within {timeout_sec}s. " | ||
| "The edge endpoint may still be converging, or may have encountered an error." | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| from io import BufferedReader, BytesIO | ||
| from pathlib import Path | ||
| from typing import Any, Dict, List, Optional, Union | ||
| from urllib.parse import urlparse, urlunparse | ||
|
|
||
| import requests | ||
| from groundlight_openapi_client.api.actions_api import ActionsApi | ||
|
|
@@ -40,6 +41,7 @@ | |
| ) | ||
| from urllib3.response import HTTPResponse | ||
|
|
||
| from groundlight.edge.api import EdgeAPI | ||
| from groundlight.images import parse_supported_image_types | ||
| from groundlight.internalapi import _generate_request_id | ||
| from groundlight.optional_imports import Image, np | ||
|
|
@@ -103,6 +105,7 @@ def __init__( | |
| self.detector_reset_api = DetectorResetApi(self.api_client) | ||
|
|
||
| self.edge_api = EdgeApi(self.api_client) | ||
| self.edge = EdgeAPI(self) | ||
|
Collaborator
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. Something has gone very wrong here. I think what we're looking for is for |
||
|
|
||
| ITEMS_PER_PAGE = 100 | ||
|
|
||
|
|
@@ -817,3 +820,8 @@ def make_generic_api_request( # noqa: PLR0913 # pylint: disable=too-many-argume | |
| auth_settings=["ApiToken"], | ||
| _preload_content=False, # This returns the urllib3 response rather than trying any type of processing | ||
| ) | ||
|
|
||
| def edge_base_url(self) -> str: | ||
| """Return the scheme+host+port of the configured endpoint, without the /device-api path.""" | ||
| parsed = urlparse(self.configuration.host) | ||
| return urlunparse((parsed.scheme, parsed.netloc, "", "", "", "")) | ||
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.
Is there a reason to define this in client rather than experimental_api? This error should only be possible in while using the experimental client, no?
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.
Correct that it is only be possible from the experimental client at this point.
We don't currently have any exceptions that are specific to the experimental client, so I decided not to put it there. I considered grouping it with the edge stuff, e.g. groundlight.edge.exceptions, but I decided not to introduce a new pattern for a single exception.