Conversation
…on-sdk into tim/sdk-configures-edge
…on-sdk into tim/sdk-configures-edge
brandon-wada
left a comment
There was a problem hiding this comment.
Couple of fixes to take a look at, mostly to ensure some stylistic rules
There was a problem hiding this comment.
Maybe worth a small mocked test on the put method and detector readiness method
| self.detector_reset_api = DetectorResetApi(self.api_client) | ||
|
|
||
| self.edge_api = EdgeApi(self.api_client) | ||
| self.edge = EdgeAPI(self) |
There was a problem hiding this comment.
Something has gone very wrong here. I think what we're looking for is for edge_base_url to be defined in the EdgeApi, and for there to only be one EdgeAPI defined on the experimental client
| pass | ||
|
|
||
|
|
||
| class EdgeNotAvailableError(GroundlightClientError): |
There was a problem hiding this comment.
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.
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.
src/groundlight/edge/api.py
Outdated
| deadline = time.time() + timeout_sec | ||
| while time.time() < deadline: | ||
| readiness = self.get_detector_readiness() | ||
| if desired_ids and all(readiness.get(did, False) for did in desired_ids): |
There was a problem hiding this comment.
Via Claude: edge condition if the detector list is empty. It should either return immediately or return a different error rather than timeout
| headers = self._client.get_raw_headers() | ||
| try: | ||
| response = requests.request( | ||
| method, url, headers=headers, verify=self._client.configuration.verify_ssl, timeout=10, **kwargs |
There was a problem hiding this comment.
Claude has a nit: 10 second default is fine but should be overridable via kwargs
There was a problem hiding this comment.
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.
| self._request("PUT", "/edge-config", json=config.to_payload()) | ||
|
|
||
| poll_interval_seconds = 1 | ||
| desired_ids = {d.detector_id for d in config.detectors if d.detector_id} |
There was a problem hiding this comment.
Claude: d.detector_id should never be falsey, it's misleading to have the check here
Me: However, in the current pydantic config I don't think we actually forbid the empty string. We probably should though
Summary
Adds SDK methods for reading and modifying edge endpoint configuration at runtime, accessible via a namespaced
gl.edgepattern. Depends on companion Edge PR.New code
EdgeAPIclass (groundlight/edge/api.py)Provides three methods:
get_config()-- callsGET /edge-config, returnsEdgeEndpointConfigset_config(config, timeout_sec)-- callsPUT /edge-config, then pollsGET /edge-detector-readinessuntil all desired detectors are ready (or timeout)get_detector_readiness()-- callsGET /edge-detector-readiness, returnsdict[str, bool]All HTTP calls go through
_request(), which raisesEdgeNotAvailableErroron 404 or connection failure with an informative message guiding the user to check their endpoint configuration.EdgeNotAvailableErrorNew exception in
groundlight.client. Raised when an edge-only method is called against a non-edge endpoint (cloud API, unreachable host). The error message includes a hint about theGROUNDLIGHT_ENDPOINTenv var.gl.edgeaccess patternExperimentalApi.__init__creates anEdgeAPIinstance asself.edge. Edge methods are only accessible through this namespace.Version
Bumped to
0.26.0(minor version bump for new public API surface).Release order
The companion edge-endpoint PR must be deployed first. The
gl.edge.*methods call endpoints (/edge-config,/edge-detector-readiness) that only exist after the edge PR is deployed. Releasing this SDK version first would cause those methods to raiseEdgeNotAvailableError(404).Authentication note
These methods do not add authentication headers beyond what the SDK already sends (the standard
x-api-token). The edge endpoint does not currently enforce auth on the config or readiness endpoints. The SDK sends the token in case auth is added in the future, but it is not required today.