merge 0.8 branch into main (#56)

* Bump 0.8.0rc1

* (0.8.0rc1) Return YES/NO instead of PASS/FAIL (#50)

* Convert PASS/FAIL to YES/NO

* Change to classmethod

* Remove logger

* Bump 0.8.0

* (0.8.0rc1) Only allow "YES"/"NO" in `add_label()` (#51)

* Convert PASS/FAIL to YES/NO

* Change to classmethod

* Remove logger

* Bump 0.8.0

* Only allow YES/NO labels through SDK; Add tests

* Add comments

* Remove duplicates

* Feedback

* Add type ignore

* Remove select all

* Enum

* Update pyproject.toml

---------

Co-authored-by: Michael Vogelsong <[email protected]>

Author: brandon-wada

Makefile

@@ -20,7 +20,7 @@ generate: install-generator  ## Generate the SDK from our public openapi spec
 	poetry run datamodel-codegen  --input spec/public-api.yaml --output generated/model.py
 	poetry run black .
 
-PYTEST=poetry run pytest -v --cov=src
+PYTEST=poetry run pytest -v
 
 # You can pass extra arguments to pytest by setting the TEST_ARGS environment variable.
 # For example:

code-quality/lint

@@ -36,8 +36,8 @@ echo "Linting with mypy (type checking) ..."
 poetry run mypy $TARGET_PATHS || ((errors++))
 
 if [[ $errors -gt 0 ]]; then
-    echo "🚨 $errors linters found errors!"
-    exit $errors
+  echo "🚨 $errors linters found errors!"
+  exit $errors
 fi
 
 echo "✅ Success!"

pyproject.toml

@@ -9,7 +9,7 @@ packages = [
     {include = "**/*.py", from = "src"},
 ]
 readme = "README.md"
-version = "0.7.8"
+version = "0.8.0"
 
 [tool.poetry.dependencies]
 certifi = "^2021.10.8"

samples/blocking_submit.py

@@ -1,5 +1,4 @@
-"""Example of how to wait for a confident result
-"""
+"""Example of how to wait for a confident result."""
 import logging
 
 from groundlight import Groundlight

src/groundlight/__init__.py

@@ -4,6 +4,7 @@
 
 # Imports from our code
 from .client import Groundlight
+from .binary_labels import Label
 
 try:
     import importlib.metadata

src/groundlight/binary_labels.py

@@ -1,51 +1,76 @@
-"""Defines the possible values for binary class labels like Yes/No or PASS/FAIL.
+"""Defines the possible values for binary class labels like YES/NO.
 Provides methods to convert between them.
 
 This part of the API is kinda ugly right now.  So we'll encapsulate the ugliness in one place.
 """
 import logging
-from typing import List, Union
+from enum import Enum
+from typing import Union
 
 from model import Detector, ImageQuery
 
-logger = logging.getLogger("groundlight.sdk")
+logger = logging.getLogger(__name__)
 
 
-def internal_labels_for_detector(
-    context: Union[ImageQuery, Detector, str],  # pylint: disable=unused-argument
-) -> List[str]:
-    """Returns an ordered list of class labels as strings.
-    These are the versions of labels that the API demands.
-    :param context: Can be an ImageQuery, a Detector, or a string-id for one of them.
-    """
-    # NOTE: At some point this will need to be an API call, because these will be defined per-detector
-    return ["PASS", "FAIL"]
+class Label(str, Enum):
+    YES = "YES"
+    NO = "NO"
+    UNSURE = "UNSURE"
+
+
+VALID_DISPLAY_LABELS = {Label.YES, Label.NO, Label.UNSURE}
+
+
+class DeprecatedLabel(str, Enum):
+    PASS = "PASS"
+    FAIL = "FAIL"
+    NEEDS_REVIEW = "NEEDS_REVIEW"
+
+
+DEPRECATED_LABEL_NAMES = {DeprecatedLabel.PASS, DeprecatedLabel.FAIL, DeprecatedLabel.NEEDS_REVIEW}
 
 
 def convert_internal_label_to_display(
     context: Union[ImageQuery, Detector, str],  # pylint: disable=unused-argument
     label: str,
-) -> str:
+) -> Union[Label, str]:
+    """Convert a label that comes from our API into the label string enum that we show to the user.
+
+    NOTE: We return UPPERCASE label strings to the user, unless there is a custom label (which
+    shouldn't be happening at this time).
+    """
     # NOTE: Someday we will do nothing here, when the server provides properly named classes.
+    if not isinstance(label, str):
+        raise ValueError(f"Expected a string label, but got {label} of type {type(label)}")
     upper = label.upper()
-    if upper == "PASS":
-        return "YES"
-    if upper == "FAIL":
-        return "NO"
-    if upper in ["YES", "NO"]:
-        return label
-    logger.warning(f"Unrecognized internal label {label} - leaving alone.")
+    if upper in {Label.YES, DeprecatedLabel.PASS}:
+        return Label.YES
+    if upper in {Label.NO, DeprecatedLabel.FAIL}:
+        return Label.NO
+    if upper in {Label.UNSURE, DeprecatedLabel.NEEDS_REVIEW}:
+        return Label.UNSURE
+
+    logger.warning(f"Unrecognized internal label {label} - leaving it alone as a string.")
     return label
 
 
 def convert_display_label_to_internal(
     context: Union[ImageQuery, Detector, str],  # pylint: disable=unused-argument
-    label: str,
+    label: Union[Label, str],
 ) -> str:
+    """Convert a label that comes from the user into the label string that we send to the server. We
+    are strict here, and only allow YES/NO.
+
+    NOTE: We accept case-insensitive label strings from the user, but we send UPPERCASE labels to
+    the server. E.g., user inputs "yes" -> the label is returned as "YES".
+    """
     # NOTE: In the future we should validate against actually supported labels for the detector
+    if not isinstance(label, str):
+        raise ValueError(f"Expected a string label, but got {label} of type {type(label)}")
     upper = label.upper()
-    if upper in {"PASS", "YES"}:
-        return "PASS"
-    if upper in {"FAIL", "NO"}:
-        return "FAIL"
-    raise ValueError(f'Invalid label string "{label}".  Must be one of YES,NO,PASS,FAIL')
+    if upper == Label.YES:
+        return DeprecatedLabel.PASS.value
+    if upper == Label.NO:
+        return DeprecatedLabel.FAIL.value
+
+    raise ValueError(f"Invalid label string '{label}'.  Must be one of '{Label.YES.value}','{Label.NO.value}'.")

src/groundlight/client.py

@@ -10,7 +10,7 @@
 from openapi_client.api.image_queries_api import ImageQueriesApi
 from openapi_client.model.detector_creation_input import DetectorCreationInput
 
-from groundlight.binary_labels import convert_display_label_to_internal
+from groundlight.binary_labels import Label, convert_display_label_to_internal, convert_internal_label_to_display
 from groundlight.config import API_TOKEN_VARIABLE_NAME, API_TOKEN_WEB_URL
 from groundlight.images import parse_supported_image_types
 from groundlight.internalapi import GroundlightApiClient, NotFoundError, sanitize_endpoint_url
@@ -71,6 +71,15 @@ def __init__(self, endpoint: Optional[str] = None, api_token: Optional[str] = No
         self.detectors_api = DetectorsApi(self.api_client)
         self.image_queries_api = ImageQueriesApi(self.api_client)
 
+    @classmethod
+    def _post_process_image_query(cls, iq: ImageQuery) -> ImageQuery:
+        """Post-process the image query so we don't use confusing internal labels.
+
+        TODO: Get rid of this once we clean up the mapping logic server-side.
+        """
+        iq.result.label = convert_internal_label_to_display(iq, iq.result.label)
+        return iq
+
     def get_detector(self, id: Union[str, Detector]) -> Detector:  # pylint: disable=redefined-builtin
         if isinstance(id, Detector):
             # Short-circuit
@@ -102,7 +111,12 @@ def create_detector(
         return Detector.parse_obj(obj.to_dict())
 
     def get_or_create_detector(
-        self, name: str, query: str, *, confidence_threshold: Optional[float] = None, config_name: Optional[str] = None
+        self,
+        name: str,
+        query: str,
+        *,
+        confidence_threshold: Optional[float] = None,
+        config_name: Optional[str] = None,
     ) -> Detector:
         """Tries to look up the detector by name.  If a detector with that name, query, and
         confidence exists, return it. Otherwise, create a detector with the specified query and
@@ -113,30 +127,41 @@ def get_or_create_detector(
         except NotFoundError:
             logger.debug(f"We could not find a detector with name='{name}'. So we will create a new detector ...")
             return self.create_detector(
-                name=name, query=query, confidence_threshold=confidence_threshold, config_name=config_name
+                name=name,
+                query=query,
+                confidence_threshold=confidence_threshold,
+                config_name=config_name,
             )
 
         # TODO: We may soon allow users to update the retrieved detector's fields.
         if existing_detector.query != query:
             raise ValueError(
-                f"Found existing detector with name={name} (id={existing_detector.id}) but the queries don't match."
-                f" The existing query is '{existing_detector.query}'."
+                (
+                    f"Found existing detector with name={name} (id={existing_detector.id}) but the queries don't match."
+                    f" The existing query is '{existing_detector.query}'."
+                ),
             )
         if confidence_threshold is not None and existing_detector.confidence_threshold != confidence_threshold:
             raise ValueError(
-                f"Found existing detector with name={name} (id={existing_detector.id}) but the confidence"
-                " thresholds don't match. The existing confidence threshold is"
-                f" {existing_detector.confidence_threshold}."
+                (
+                    f"Found existing detector with name={name} (id={existing_detector.id}) but the confidence"
+                    " thresholds don't match. The existing confidence threshold is"
+                    f" {existing_detector.confidence_threshold}."
+                ),
             )
         return existing_detector
 
     def get_image_query(self, id: str) -> ImageQuery:  # pylint: disable=redefined-builtin
         obj = self.image_queries_api.get_image_query(id=id)
-        return ImageQuery.parse_obj(obj.to_dict())
+        iq = ImageQuery.parse_obj(obj.to_dict())
+        return self._post_process_image_query(iq)
 
     def list_image_queries(self, page: int = 1, page_size: int = 10) -> PaginatedImageQueryList:
         obj = self.image_queries_api.list_image_queries(page=page, page_size=page_size)
-        return PaginatedImageQueryList.parse_obj(obj.to_dict())
+        image_queries = PaginatedImageQueryList.parse_obj(obj.to_dict())
+        if image_queries.results is not None:
+            image_queries.results = [self._post_process_image_query(iq) for iq in image_queries.results]
+        return image_queries
 
     def submit_image_query(
         self,
@@ -166,7 +191,7 @@ def submit_image_query(
         if wait:
             threshold = self.get_detector(detector).confidence_threshold
             image_query = self.wait_for_confident_result(image_query, confidence_threshold=threshold, timeout_sec=wait)
-        return image_query
+        return self._post_process_image_query(image_query)
 
     def wait_for_confident_result(
         self,
@@ -203,11 +228,11 @@ def wait_for_confident_result(
             image_query = self.get_image_query(image_query.id)
         return image_query
 
-    def add_label(self, image_query: Union[ImageQuery, str], label: str):
+    def add_label(self, image_query: Union[ImageQuery, str], label: Union[Label, str]):
         """A new label to an image query.  This answers the detector's question.
         :param image_query: Either an ImageQuery object (returned from `submit_image_query`) or
         an image_query id as a string.
-        :param label: The string "Yes" or the string "No" in answer to the query.
+        :param label: The string "YES" or the string "NO" in answer to the query.
         """
         if isinstance(image_query, ImageQuery):
             image_query_id = image_query.id

src/groundlight/internalapi.py

@@ -133,7 +133,7 @@ def _get_detector_by_name(self, name: str) -> Detector:
 
         if not is_ok(response.status_code):
             raise InternalApiError(
-                f"Error getting detector by name '{name}' (status={response.status_code}): {response.text}"
+                f"Error getting detector by name '{name}' (status={response.status_code}): {response.text}",
             )
 
         parsed = response.json()
@@ -142,6 +142,6 @@ def _get_detector_by_name(self, name: str) -> Detector:
             raise NotFoundError(f"Detector with name={name} not found.")
         if parsed["count"] > 1:
             raise RuntimeError(
-                f"We found multiple ({parsed['count']}) detectors with the same name. This shouldn't happen."
+                f"We found multiple ({parsed['count']}) detectors with the same name. This shouldn't happen.",
             )
         return Detector.parse_obj(parsed["results"][0])