From 4ce1c9454264cf84c2384f1436abe061ec8f132c Mon Sep 17 00:00:00 2001 From: brandon Date: Tue, 10 Oct 2023 15:39:16 -0700 Subject: [PATCH 01/11] Update UNSURE to be UNCLEAR, catch __USURE received from the service --- src/groundlight/binary_labels.py | 16 +++++++++++----- test/integration/test_groundlight.py | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/groundlight/binary_labels.py b/src/groundlight/binary_labels.py index a0f615b4..de3216a1 100644 --- a/src/groundlight/binary_labels.py +++ b/src/groundlight/binary_labels.py @@ -15,19 +15,25 @@ class Label(str, Enum): YES = "YES" NO = "NO" - UNSURE = "UNSURE" + UNCLEAR = "UNCLEAR" -VALID_DISPLAY_LABELS = {Label.YES, Label.NO, Label.UNSURE} +VALID_DISPLAY_LABELS = {Label.YES, Label.NO, Label.UNCLEAR} class DeprecatedLabel(str, Enum): PASS = "PASS" FAIL = "FAIL" NEEDS_REVIEW = "NEEDS_REVIEW" + UNSURE = "__UNSURE" -DEPRECATED_LABEL_NAMES = {DeprecatedLabel.PASS, DeprecatedLabel.FAIL, DeprecatedLabel.NEEDS_REVIEW} +DEPRECATED_LABEL_NAMES = { + DeprecatedLabel.PASS, + DeprecatedLabel.FAIL, + DeprecatedLabel.NEEDS_REVIEW, + DeprecatedLabel.UNSURE, +} def convert_internal_label_to_display( @@ -47,8 +53,8 @@ def convert_internal_label_to_display( return Label.YES if upper in {Label.NO, DeprecatedLabel.FAIL}: return Label.NO - if upper in {Label.UNSURE, DeprecatedLabel.NEEDS_REVIEW}: - return Label.UNSURE + if upper in {Label.UNCLEAR, DeprecatedLabel.NEEDS_REVIEW, DeprecatedLabel.UNSURE}: + return Label.UNCLEAR logger.warning(f"Unrecognized internal label {label} - leaving it alone as a string.") return label diff --git a/test/integration/test_groundlight.py b/test/integration/test_groundlight.py index 59301161..397bad91 100644 --- a/test/integration/test_groundlight.py +++ b/test/integration/test_groundlight.py @@ -337,7 +337,7 @@ def test_add_label_names(gl: Groundlight, image_query_yes: ImageQuery, image_que # We may want to support something like this in the future, but not yet with pytest.raises(ValueError): - gl.add_label(iqid_yes, Label.UNSURE) + gl.add_label(iqid_yes, Label.UNCLEAR) def test_label_conversion_produces_strings(): From 05096913c02d6800456bf8bc3be1f1fda8669032 Mon Sep 17 00:00:00 2001 From: brandon Date: Tue, 10 Oct 2023 22:50:23 -0700 Subject: [PATCH 02/11] Add basic catch if api token isn't specified when cli is called --- src/groundlight/cli.py | 24 +++++++++++++++--------- src/groundlight/client.py | 10 ++-------- src/groundlight/config.py | 6 ++++++ 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/groundlight/cli.py b/src/groundlight/cli.py index 9c967fae..86adb2ee 100644 --- a/src/groundlight/cli.py +++ b/src/groundlight/cli.py @@ -2,9 +2,13 @@ from typing import Union import typer +import click +from click.testing import CliRunner from typing_extensions import get_origin from groundlight import Groundlight +from groundlight.client import ApiTokenError +from groundlight.config import API_TOKEN_HELP_MESSAGE cli_app = typer.Typer(no_args_is_help=True, context_settings={"help_option_names": ["-h", "--help"]}) @@ -30,15 +34,17 @@ def wrapper(*args, **kwargs): def groundlight(): - gl = Groundlight() - - # For each method in the Groundlight class, create a function that can be called from the command line - for name, method in vars(Groundlight).items(): - if callable(method) and not name.startswith("_"): - attached_method = method.__get__(gl) - cli_func = class_func_to_cli(attached_method) - cli_app.command()(cli_func) - cli_app() + try: + gl = Groundlight() + # For each method in the Groundlight class, create a function that can be called from the command line + for name, method in vars(Groundlight).items(): + if callable(method) and not name.startswith("_"): + attached_method = method.__get__(gl) + cli_func = class_func_to_cli(attached_method) + cli_app.command()(cli_func) + cli_app() + except ApiTokenError: + print(API_TOKEN_HELP_MESSAGE) if __name__ == "__main__": diff --git a/src/groundlight/client.py b/src/groundlight/client.py index a7d7948c..d9f6c981 100644 --- a/src/groundlight/client.py +++ b/src/groundlight/client.py @@ -11,7 +11,7 @@ from openapi_client.model.detector_creation_input import DetectorCreationInput 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.config import API_TOKEN_VARIABLE_NAME, API_TOKEN_WEB_URL, API_TOKEN_HELP_MESSAGE from groundlight.images import ByteStreamWrapper, parse_supported_image_types from groundlight.internalapi import ( GroundlightApiClient, @@ -62,13 +62,7 @@ def __init__(self, endpoint: Optional[str] = None, api_token: Optional[str] = No # Retrieve the API token from environment variable api_token = os.environ[API_TOKEN_VARIABLE_NAME] except KeyError as e: - raise ApiTokenError( - ( - "No API token found. Please put your token in an environment variable " - f'named "{API_TOKEN_VARIABLE_NAME}". If you don\'t have a token, you can ' - f"create one at {API_TOKEN_WEB_URL}" - ), - ) from e + raise ApiTokenError(API_TOKEN_HELP_MESSAGE) from e configuration.api_key["ApiToken"] = api_token diff --git a/src/groundlight/config.py b/src/groundlight/config.py index 2eb592ff..c92acb32 100644 --- a/src/groundlight/config.py +++ b/src/groundlight/config.py @@ -8,3 +8,9 @@ "API_TOKEN_VARIABLE_NAME", "DEFAULT_ENDPOINT", ] + +API_TOKEN_HELP_MESSAGE = ( + "No API token found. Please put your token in an environment variable " + f'named "{API_TOKEN_VARIABLE_NAME}". If you don\'t have a token, you can ' + f"create one at {API_TOKEN_WEB_URL}" +) From 04e4cdac266fde07dadaffca0b6c3d1e712598c6 Mon Sep 17 00:00:00 2001 From: brandon Date: Wed, 11 Oct 2023 00:00:53 -0700 Subject: [PATCH 03/11] Pushes Groundlight class instantiation up until the function is actually called with arguments. This means that the entire help args are available even if we can't instantiate the class (no api key) --- src/groundlight/cli.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/groundlight/cli.py b/src/groundlight/cli.py index 86adb2ee..dc6c3385 100644 --- a/src/groundlight/cli.py +++ b/src/groundlight/cli.py @@ -18,9 +18,19 @@ def class_func_to_cli(method): Given the class method, simplify the typing on the inputs so that Typer can accept the method """ - @wraps(method) + # We create a temporary class to bind the method to so we have the correct annotations for typer to use + # The first thing we'll do when we actually call the method is to bind it to a proper Groundlight instance + class fake_class: + pass + + fake_instance = fake_class() + fake_bound_method = method.__get__(fake_instance, fake_class) + + @wraps(fake_bound_method) def wrapper(*args, **kwargs): - print(method(*args, **kwargs)) # this is where we output to the console + gl = Groundlight() + gl_bound_method = fake_instance.fake_bound_method.__get__(gl, Groundlight) + print(gl_bound_method(*args, **kwargs)) # this is where we output to the console # not recommended practice to directly change annotations, but gets around Typer not supporting Union types for name, annotation in method.__annotations__.items(): @@ -35,12 +45,10 @@ def wrapper(*args, **kwargs): def groundlight(): try: - gl = Groundlight() # For each method in the Groundlight class, create a function that can be called from the command line for name, method in vars(Groundlight).items(): if callable(method) and not name.startswith("_"): - attached_method = method.__get__(gl) - cli_func = class_func_to_cli(attached_method) + cli_func = class_func_to_cli(method) cli_app.command()(cli_func) cli_app() except ApiTokenError: From 3eddd0bc4eb9e2f5e2af3c6835d734be74941e6c Mon Sep 17 00:00:00 2001 From: brandon Date: Wed, 11 Oct 2023 14:17:10 -0700 Subject: [PATCH 04/11] Revert "Update UNSURE to be UNCLEAR, catch __USURE received from the service" Accidentally pushed my cli commits onto the wrong branch. Removing that work to make this PR it's own concern This reverts commit 4ce1c9454264cf84c2384f1436abe061ec8f132c. --- src/groundlight/binary_labels.py | 16 +++++----------- test/integration/test_groundlight.py | 2 +- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/groundlight/binary_labels.py b/src/groundlight/binary_labels.py index de3216a1..a0f615b4 100644 --- a/src/groundlight/binary_labels.py +++ b/src/groundlight/binary_labels.py @@ -15,25 +15,19 @@ class Label(str, Enum): YES = "YES" NO = "NO" - UNCLEAR = "UNCLEAR" + UNSURE = "UNSURE" -VALID_DISPLAY_LABELS = {Label.YES, Label.NO, Label.UNCLEAR} +VALID_DISPLAY_LABELS = {Label.YES, Label.NO, Label.UNSURE} class DeprecatedLabel(str, Enum): PASS = "PASS" FAIL = "FAIL" NEEDS_REVIEW = "NEEDS_REVIEW" - UNSURE = "__UNSURE" -DEPRECATED_LABEL_NAMES = { - DeprecatedLabel.PASS, - DeprecatedLabel.FAIL, - DeprecatedLabel.NEEDS_REVIEW, - DeprecatedLabel.UNSURE, -} +DEPRECATED_LABEL_NAMES = {DeprecatedLabel.PASS, DeprecatedLabel.FAIL, DeprecatedLabel.NEEDS_REVIEW} def convert_internal_label_to_display( @@ -53,8 +47,8 @@ def convert_internal_label_to_display( return Label.YES if upper in {Label.NO, DeprecatedLabel.FAIL}: return Label.NO - if upper in {Label.UNCLEAR, DeprecatedLabel.NEEDS_REVIEW, DeprecatedLabel.UNSURE}: - return Label.UNCLEAR + 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 diff --git a/test/integration/test_groundlight.py b/test/integration/test_groundlight.py index 397bad91..59301161 100644 --- a/test/integration/test_groundlight.py +++ b/test/integration/test_groundlight.py @@ -337,7 +337,7 @@ def test_add_label_names(gl: Groundlight, image_query_yes: ImageQuery, image_que # We may want to support something like this in the future, but not yet with pytest.raises(ValueError): - gl.add_label(iqid_yes, Label.UNCLEAR) + gl.add_label(iqid_yes, Label.UNSURE) def test_label_conversion_produces_strings(): From 8c29eb48f6bbc52a8f96f19915a396bd73c60386 Mon Sep 17 00:00:00 2001 From: Auto-format Bot Date: Wed, 11 Oct 2023 21:18:41 +0000 Subject: [PATCH 05/11] Automatically reformatting code --- src/groundlight/cli.py | 2 -- src/groundlight/client.py | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/groundlight/cli.py b/src/groundlight/cli.py index dc6c3385..d6b358d4 100644 --- a/src/groundlight/cli.py +++ b/src/groundlight/cli.py @@ -2,8 +2,6 @@ from typing import Union import typer -import click -from click.testing import CliRunner from typing_extensions import get_origin from groundlight import Groundlight diff --git a/src/groundlight/client.py b/src/groundlight/client.py index d9f6c981..06096c4b 100644 --- a/src/groundlight/client.py +++ b/src/groundlight/client.py @@ -11,7 +11,7 @@ from openapi_client.model.detector_creation_input import DetectorCreationInput 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, API_TOKEN_HELP_MESSAGE +from groundlight.config import API_TOKEN_HELP_MESSAGE, API_TOKEN_VARIABLE_NAME from groundlight.images import ByteStreamWrapper, parse_supported_image_types from groundlight.internalapi import ( GroundlightApiClient, From ea5631a34010d46c25a1b0e9cd506f2b339d3720 Mon Sep 17 00:00:00 2001 From: brandon Date: Wed, 11 Oct 2023 17:12:37 -0700 Subject: [PATCH 06/11] Fixed misunderstanding with metaprogramming, added tests --- src/groundlight/cli.py | 15 +++++++++------ test/unit/test_cli.py | 23 ++++++++++++++++++++--- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/groundlight/cli.py b/src/groundlight/cli.py index dc6c3385..b5ac8223 100644 --- a/src/groundlight/cli.py +++ b/src/groundlight/cli.py @@ -15,21 +15,24 @@ def class_func_to_cli(method): """ - Given the class method, simplify the typing on the inputs so that Typer can accept the method + Given the class method, create a method with the identical signature to provide the help documentation and + but only instantiates the class when the method is actually called. """ - # We create a temporary class to bind the method to so we have the correct annotations for typer to use - # The first thing we'll do when we actually call the method is to bind it to a proper Groundlight instance + # We create a fake class and fake method to so we have the correct annotations for typer to use + # When we wrap the fake method, we only use the fake method's name to access the real method + # and attach it to a Groundlight instance that we create at function call time class fake_class: pass fake_instance = fake_class() - fake_bound_method = method.__get__(fake_instance, fake_class) + fake_method = method.__get__(fake_instance, fake_class) - @wraps(fake_bound_method) + @wraps(fake_method) def wrapper(*args, **kwargs): gl = Groundlight() - gl_bound_method = fake_instance.fake_bound_method.__get__(gl, Groundlight) + gl_method = vars(Groundlight)[fake_method.__name__] + gl_bound_method = gl_method.__get__(gl, Groundlight) print(gl_bound_method(*args, **kwargs)) # this is where we output to the console # not recommended practice to directly change annotations, but gets around Typer not supporting Union types diff --git a/test/unit/test_cli.py b/test/unit/test_cli.py index 1558245f..1e89cbaa 100644 --- a/test/unit/test_cli.py +++ b/test/unit/test_cli.py @@ -1,6 +1,17 @@ import re import subprocess from datetime import datetime +import pytest +import os + + +@pytest.fixture(name="no_api_token") +def fixture_no_api_token(): + # you need to have an API token set to run the tests, but this lets us test behavior if an API token is not set + original_api_token = os.environ.get("GROUNDLIGHT_API_TOKEN") + del os.environ["GROUNDLIGHT_API_TOKEN"] + yield + os.environ["GROUNDLIGHT_API_TOKEN"] = original_api_token def test_list_detector(): @@ -72,12 +83,18 @@ def test_detector_and_image_queries(): assert completed_process.returncode == 0 -def test_help(): +def test_help(no_api_token): completed_process = subprocess.run(["groundlight"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) assert completed_process.returncode == 0 - completed_process = subprocess.run(["groundlight"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + completed_process = subprocess.run(["groundlight", "-h"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) assert completed_process.returncode == 0 - completed_process = subprocess.run(["groundlight"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + completed_process = subprocess.run( + ["groundlight", "--help"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True + ) + assert completed_process.returncode == 0 + completed_process = subprocess.run( + ["groundlight", "get-detector", "-h"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True + ) assert completed_process.returncode == 0 From 5a06090f81a7c193ea99ee2d3bb7118dd3458c05 Mon Sep 17 00:00:00 2001 From: Auto-format Bot Date: Thu, 12 Oct 2023 00:14:39 +0000 Subject: [PATCH 07/11] Automatically reformatting code --- test/unit/test_cli.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/test_cli.py b/test/unit/test_cli.py index 1e89cbaa..51e45378 100644 --- a/test/unit/test_cli.py +++ b/test/unit/test_cli.py @@ -1,8 +1,9 @@ +import os import re import subprocess from datetime import datetime + import pytest -import os @pytest.fixture(name="no_api_token") From 823cc1951213cf3d71eb8399552b8f6bd3ac5c29 Mon Sep 17 00:00:00 2001 From: brandon Date: Fri, 13 Oct 2023 16:44:41 -0700 Subject: [PATCH 08/11] Addressing comments --- src/groundlight/cli.py | 8 ++++++-- test/unit/test_cli.py | 13 +++---------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/groundlight/cli.py b/src/groundlight/cli.py index 4a117b5e..ef051bf2 100644 --- a/src/groundlight/cli.py +++ b/src/groundlight/cli.py @@ -8,7 +8,11 @@ from groundlight.client import ApiTokenError from groundlight.config import API_TOKEN_HELP_MESSAGE -cli_app = typer.Typer(no_args_is_help=True, context_settings={"help_option_names": ["-h", "--help"]}) + +cli_app = typer.Typer( + no_args_is_help=True, + context_settings={"help_option_names": ["-h", "--help"], "max_content_width": 800}, +) def class_func_to_cli(method): @@ -17,7 +21,7 @@ def class_func_to_cli(method): but only instantiates the class when the method is actually called. """ - # We create a fake class and fake method to so we have the correct annotations for typer to use + # We create a fake class and fake method so we have the correct annotations for typer to use # When we wrap the fake method, we only use the fake method's name to access the real method # and attach it to a Groundlight instance that we create at function call time class fake_class: diff --git a/test/unit/test_cli.py b/test/unit/test_cli.py index 1e89cbaa..22949af2 100644 --- a/test/unit/test_cli.py +++ b/test/unit/test_cli.py @@ -3,15 +3,7 @@ from datetime import datetime import pytest import os - - -@pytest.fixture(name="no_api_token") -def fixture_no_api_token(): - # you need to have an API token set to run the tests, but this lets us test behavior if an API token is not set - original_api_token = os.environ.get("GROUNDLIGHT_API_TOKEN") - del os.environ["GROUNDLIGHT_API_TOKEN"] - yield - os.environ["GROUNDLIGHT_API_TOKEN"] = original_api_token +from unittest.mock import patch def test_list_detector(): @@ -83,7 +75,8 @@ def test_detector_and_image_queries(): assert completed_process.returncode == 0 -def test_help(no_api_token): +@patch.dict(os.environ, {"GROUNDLIGHT_API_TOKEN": None}) +def test_help(): completed_process = subprocess.run(["groundlight"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) assert completed_process.returncode == 0 completed_process = subprocess.run(["groundlight", "-h"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) From ea8f85331b83115d65cbdf6a9a2e517c48709946 Mon Sep 17 00:00:00 2001 From: Auto-format Bot Date: Fri, 13 Oct 2023 23:49:19 +0000 Subject: [PATCH 09/11] Automatically reformatting code --- src/groundlight/cli.py | 1 - test/unit/test_cli.py | 3 --- 2 files changed, 4 deletions(-) diff --git a/src/groundlight/cli.py b/src/groundlight/cli.py index ef051bf2..7a5a004a 100644 --- a/src/groundlight/cli.py +++ b/src/groundlight/cli.py @@ -8,7 +8,6 @@ from groundlight.client import ApiTokenError from groundlight.config import API_TOKEN_HELP_MESSAGE - cli_app = typer.Typer( no_args_is_help=True, context_settings={"help_option_names": ["-h", "--help"], "max_content_width": 800}, diff --git a/test/unit/test_cli.py b/test/unit/test_cli.py index 89099ad7..caa7a4e1 100644 --- a/test/unit/test_cli.py +++ b/test/unit/test_cli.py @@ -2,9 +2,6 @@ import re import subprocess from datetime import datetime - -import pytest -import os from unittest.mock import patch From 4580e3136b9a50ce90f30d4211e0f402a9ef3c6e Mon Sep 17 00:00:00 2001 From: brandon Date: Mon, 16 Oct 2023 14:16:04 -0700 Subject: [PATCH 10/11] caught failed test that github was ready to ignore --- test/unit/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/test_cli.py b/test/unit/test_cli.py index caa7a4e1..9e8b99fe 100644 --- a/test/unit/test_cli.py +++ b/test/unit/test_cli.py @@ -74,7 +74,7 @@ def test_detector_and_image_queries(): assert completed_process.returncode == 0 -@patch.dict(os.environ, {"GROUNDLIGHT_API_TOKEN": None}) +@patch.dict(os.environ, {}) def test_help(): completed_process = subprocess.run(["groundlight"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) assert completed_process.returncode == 0 From 1876c3225206645f4a1dfb96fa081e1f2b36fe9a Mon Sep 17 00:00:00 2001 From: brandon Date: Mon, 16 Oct 2023 15:09:53 -0700 Subject: [PATCH 11/11] lint --- src/groundlight/cli.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/groundlight/cli.py b/src/groundlight/cli.py index 7a5a004a..7ea962d5 100644 --- a/src/groundlight/cli.py +++ b/src/groundlight/cli.py @@ -23,11 +23,11 @@ def class_func_to_cli(method): # We create a fake class and fake method so we have the correct annotations for typer to use # When we wrap the fake method, we only use the fake method's name to access the real method # and attach it to a Groundlight instance that we create at function call time - class fake_class: + class FakeClass: pass - fake_instance = fake_class() - fake_method = method.__get__(fake_instance, fake_class) + fake_instance = FakeClass() + fake_method = method.__get__(fake_instance, FakeClass) @wraps(fake_method) def wrapper(*args, **kwargs):