Inspections #81
Inspections #81
Conversation
pyproject.toml
Outdated
| @@ -9,7 +9,7 @@ packages = [ | |||
| {include = "**/*.py", from = "src"}, | |||
| ] | |||
| readme = "README.md" | |||
| version = "0.10.1" | |||
| version = "0.10.2" | |||
There was a problem hiding this comment.
I'd say we should call this 0.11 - not because it's backwards incompatible, but because if somebody asks "what version of the SDK do I need to use UR Cap?" it's a bit nicer to say "0.11" than "you need at least "0.10.2".
There was a problem hiding this comment.
I'm good with 0.11.0
I had originally recommended 0.10.2 to Tim because I was hoping to include the change to move inspection_id into the regular public API before bumping to 0.11.0. But that change will technically be backwards-compatible (and kinda a hidden implementation detail), so maybe you're right that we should bump to 0.11.0.
pyproject.toml
Outdated
| @@ -9,7 +9,7 @@ packages = [ | |||
| {include = "**/*.py", from = "src"}, | |||
| ] | |||
| readme = "README.md" | |||
| version = "0.10.1" | |||
| version = "0.10.2" | |||
There was a problem hiding this comment.
I'm good with 0.11.0
I had originally recommended 0.10.2 to Tim because I was hoping to include the change to move inspection_id into the regular public API before bumping to 0.11.0. But that change will technically be backwards-compatible (and kinda a hidden implementation detail), so maybe you're right that we should bump to 0.11.0.
src/groundlight/client.py
Outdated
| detector_id = detector.id if isinstance(detector, Detector) else detector | ||
|
|
||
| # Convert from Detector to detector_id if necessary | ||
| if isinstance(detector, Detector): | ||
| detector_id = detector.id | ||
| else: | ||
| detector_id = detector |
There was a problem hiding this comment.
This code is equivalent to the one-liner, and I feel like the previous version is more concise -- is there a reason you wanted to break it out like this?
| # Convert from image_query_id to ImageQuery if needed. | ||
| if isinstance(image_query, str): | ||
| image_query = self.get_image_query(image_query) |
There was a problem hiding this comment.
If we've added this, then we should update the typing above to:
image_query: Union[ImageQuery, str],
src/groundlight/internalapi.py
Outdated
| url = ( | ||
| f"{self.configuration.host}/posichecks" | ||
| f"?inspection_id={inspection_id}" | ||
| f"&predictor_id={detector_id}" | ||
| f"&patience_time={patience_time}" | ||
| ) | ||
|
|
||
| # In the API, 'send_notification' is used to control human_review escalation. This will eventually | ||
| # be deprecated, but for now we need to support it in the following manner: | ||
| if human_review == "ALWAYS": | ||
| url += "&send_notification=True" | ||
| elif human_review == "NEVER": | ||
| url += "&send_notification=False" | ||
| else: | ||
| pass # append nothing to the URL, allow "DEFAULT" behavior |
There was a problem hiding this comment.
For the query parameters, I think we can use a dict, and feed it into requests.request(..., params=params). That way, we don't have to manage the ? and & in the URL, so it's a little cleaner.
src/groundlight/internalapi.py
Outdated
| logger.info(response) | ||
| raise InternalApiError( | ||
| status=response.status_code, | ||
| reason=f"Error submitting image query with inspection ID {inspection_id} on detector {detector_id}", | ||
| ) |
There was a problem hiding this comment.
I think we should show the response through the InternalApiError. Let's make this consistent throughout -- please add http_resp=response in the different methods below, as well.
| logger.info(response) | |
| raise InternalApiError( | |
| status=response.status_code, | |
| reason=f"Error submitting image query with inspection ID {inspection_id} on detector {detector_id}", | |
| ) | |
| raise InternalApiError( | |
| status=response.status_code, | |
| reason=f"Error submitting image query with inspection ID {inspection_id} on detector {detector_id}", | |
| http_resp=response, | |
| ) |
|
|
||
| # Closing an inspection generates a new inspection PDF. Therefore, if the inspection | ||
| # is already closed, just return "COMPLETE" to avoid unnecessarily generating a new PDF. | ||
| response = requests.request("GET", url, headers=headers) |
There was a problem hiding this comment.
I think we want an is_ok() check here, as well.
| response = requests.request("POST", url, headers=headers, params=params, data=body.read()) | ||
|
|
||
| if not is_ok(response.status_code): | ||
| logger.info(response) |
There was a problem hiding this comment.
I think you can delete this line, now that response is in the error.
This PR adds several methods to Groundlight that are required for the RPC Server/URCap. They are intended to be released as undocumented features and only used to support the RPC Server/URCap on the SDK (for now at least).
Since the required functionality for these methods is not available in image_queries_api, they use the internal API directly.
New methods include:
start_inspection
update_inspection_metadata
stop_inspection
update_detector_confidence_threshold
There was also some logic added to submit_image_query related to inspections: if an image query is submitted with no inspection_id, the query will be submitted through image_queries_api (as it has been previously), however if submit_image_query is called with an inspection_id, we use the private API client instead.