Skip to content
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

Cli polishing #109

Merged
merged 13 commits into from
Oct 17, 2023
Merged

Cli polishing #109

merged 13 commits into from
Oct 17, 2023

Conversation

brandon-groundlight
Copy link
Collaborator

Polishing up the behavior of the CLI, granting access to the help tools before specifying an API token and raising the API token error only when trying to execute a command.

brandon-groundlight and others added 5 commits October 10, 2023 15:39
…lly called with arguments.

This means that the entire help args are available even if we can't instantiate the class (no api key)
…service"

Accidentally pushed my cli commits onto the wrong branch. Removing that work to make this PR it's own concern

This reverts commit 4ce1c94.
@@ -14,9 +16,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
Copy link
Collaborator Author

@brandon-groundlight brandon-groundlight Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some of the wildest python I've ever written. A lot of this jumps through hoops to get Typer to work, but it should all be valid

@brandon-groundlight brandon-groundlight marked this pull request as ready for review October 11, 2023 23:05
@brandon-groundlight brandon-groundlight removed the request for review from mjvogelsong October 11, 2023 23:08
Comment on lines +12 to +16
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}"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great idea to standardize this

Copy link
Contributor

@mjvogelsong mjvogelsong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, nice updates!

@@ -72,12 +84,18 @@ def test_detector_and_image_queries():
assert completed_process.returncode == 0


def test_help():
def test_help(no_api_token):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but a slightly simpler alternative to the fixture approach would be to use mock.patch.dict()

import os
from unittest.mock import patch

@patch.dict(os.environ, {'GROUNDLIGHT_API_TOKEN': None})
def test_help():

"""

@wraps(method)
# We create a fake class and fake method to so we have the correct annotations for typer to use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the help message, it truncates the descriptions of the sub-commands. Is there a way to have it print out the full line?

Commands:
  add-label                       Add a new label to an image query.
  create-detector                 Create a new detector with a...
  get-detector                    Get a detector by id
  get-detector-by-name            Get a detector by name
  get-image-query                 Get an image query by id
  get-or-create-detector          Tries to look up the detector by...
  list-detectors                  List out detectors you own
  list-image-queries              List out image queries you own
  start-inspection                For users with Inspection Reports...
  stop-inspection                 For users with Inspection Reports...
  submit-image-query              Evaluates an image with Groundlight.
  update-detector-confidence-threshold
                                  Updates the confidence threshold...
  update-inspection-metadata      For users with Inspection Reports...
  wait-for-confident-result       Waits for an image query result's...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the help page for a specific command, the description's formatting can be a little weird because it doesn't respect single newlines. (I understand that typer.Argument() would fix this if we could use it, but it doesn't work because of Union types).

❯ poetry run groundlight add-label -h
Usage: groundlight add-label [OPTIONS] IMAGE_QUERY LABEL

  Add 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.

Arguments:
  IMAGE_QUERY  [required]
  LABEL        [required]

Options:
  -h, --help  Show this message and exit.

But I think the fix is actually pretty easy -- when we want a newline to show up in the generated message, we should just put in a blank line in the docstring. For example:

    def add_label(self, image_query: Union[ImageQuery, str], label: Union[Label, str]):
        """Add 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.
        """

Which then looks better:

poetry run groundlight add-label -h
Usage: groundlight add-label [OPTIONS] IMAGE_QUERY LABEL

  Add 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstrings are undergoing continual improvement, they'll all hopefully use the double new line once we finish working out the sphinx stuff.

Turns out not truncating lines was just a matter of adding a param to the context_settings of the typer app.

@brandon-groundlight brandon-groundlight merged commit 59dcc76 into version_0.12.0 Oct 17, 2023
6 of 7 checks passed
brandon-groundlight added a commit that referenced this pull request Oct 17, 2023
* bump version

* Update UNSURE to be UNCLEAR, catch __USURE received from the service (#105)

* ask_async (#102)

* initial commit

* added ask_async to submit_image_query

* Automatically reformatting code

* added ask async method

* Automatically reformatting code

* added integration tests (requires BE merge first)

* Automatically reformatting code

* satisfying mypy

* Automatically reformatting code

* fix comments

* change what type of exception test is catching

* Automatically reformatting code

* fix imports organization issue

* fix implementation - wait must be 0 not None

* Automatically reformatting code

* forgot to make wait=0 in relevant test

* feedback from PR review

* Automatically reformatting code

* ensure want_async is a serializable bool

* add description

* updated sphinx reqs to render some of the dependencies

* updated docstring for ask_async and fixed small sphinx bugs in other folk's docstrings

* Tests aren't passing because I didn't update the autogenerated code to expect a new param

* Revert "Tests aren't passing because I didn't update the autogenerated code to expect a new param"

This reverts commit 2477fd5.

* fix generated

* Automatically reformatting code

* fix lint

* Automatically reformatting code

* Revert "Automatically reformatting code"

This reverts commit cb9359e.

* Revert "fix generated"

This reverts commit 935c036.

* Revert "Revert "Tests aren't passing because I didn't update the autogenerated code to expect a new param""

This reverts commit 07670e3.

* Revert "Tests aren't passing because I didn't update the autogenerated code to expect a new param"

This reverts commit 2477fd5.

* Revert "updated docstring for ask_async and fixed small sphinx bugs in other folk's docstrings"

This reverts commit 67e3edd.

* third time at generated docs is the charm

---------

Co-authored-by: Auto-format Bot <[email protected]>

* Cli polishing (#109)

* Add basic catch if api token isn't specified when cli is called

* 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)

* Fixed misunderstanding with metaprogramming, added tests

* Addressing comments

---------

Co-authored-by: Auto-format Bot <[email protected]>

* Add ask_confident and ask_ml (#99)

* Adding ask_confident and ask_fast

* Fixing ask_ml behavior

* Unhide wait functions, merging logic, fixed iq_is_answered logic

* Rewriting doc strings in Sphinx style

* fixed sphinx docstring return types

* Making iq submission with inspection work with newly optional patience time


---------

Co-authored-by: Auto-format Bot <[email protected]>

* Linting

* fix ask_async docstring

---------

Co-authored-by: brandon <[email protected]>
Co-authored-by: Brandon <[email protected]>
Co-authored-by: Auto-format Bot <[email protected]>
sunildkumar added a commit that referenced this pull request Oct 18, 2023
* bump version

* Update UNSURE to be UNCLEAR, catch __USURE received from the service (#105)

* rough draft of demo

* ask_async (#102)

* initial commit

* added ask_async to submit_image_query

* Automatically reformatting code

* added ask async method

* Automatically reformatting code

* added integration tests (requires BE merge first)

* Automatically reformatting code

* satisfying mypy

* Automatically reformatting code

* fix comments

* change what type of exception test is catching

* Automatically reformatting code

* fix imports organization issue

* fix implementation - wait must be 0 not None

* Automatically reformatting code

* forgot to make wait=0 in relevant test

* feedback from PR review

* Automatically reformatting code

* ensure want_async is a serializable bool

* add description

* updated sphinx reqs to render some of the dependencies

* updated docstring for ask_async and fixed small sphinx bugs in other folk's docstrings

* Tests aren't passing because I didn't update the autogenerated code to expect a new param

* Revert "Tests aren't passing because I didn't update the autogenerated code to expect a new param"

This reverts commit 2477fd5.

* fix generated

* Automatically reformatting code

* fix lint

* Automatically reformatting code

* Revert "Automatically reformatting code"

This reverts commit cb9359e.

* Revert "fix generated"

This reverts commit 935c036.

* Revert "Revert "Tests aren't passing because I didn't update the autogenerated code to expect a new param""

This reverts commit 07670e3.

* Revert "Tests aren't passing because I didn't update the autogenerated code to expect a new param"

This reverts commit 2477fd5.

* Revert "updated docstring for ask_async and fixed small sphinx bugs in other folk's docstrings"

This reverts commit 67e3edd.

* third time at generated docs is the charm

* Automatically reformatting code

* finish making tests work

* Automatically reformatting code

---------

Co-authored-by: Auto-format Bot <[email protected]>

* Cli polishing (#109)

* Add basic catch if api token isn't specified when cli is called

* 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)

* Fixed misunderstanding with metaprogramming, added tests

* Addressing comments

---------

Co-authored-by: Auto-format Bot <[email protected]>

* Add ask_confident and ask_ml (#99)

* Adding ask_confident and ask_fast

* Automatically reformatting code

* Fixing ask_ml behavior

* Adding to test

* Automatically reformatting code

* set default wait for ask_ml

* Unhide wait functions, merging logic, fixed iq_is_answered logic

* Automatically reformatting code

* Rewriting doc strings in Sphinx style

* ask_fast to ask_ml in the tests

* fixed sphinx docstring return types

* Cleaning the lint trap

* Last bits of lint

* Making iq submission with inspection work with newly optional patience time

* single char typo

* Reorder functions to trick Git's LCS alg to be correct

* Automatically reformatting code

---------

Co-authored-by: Auto-format Bot <[email protected]>

* Linting

* addressed self comments

* fix ask_async docstring

* small fixes

* cleaning up a bit

* Update docs/docs/building-applications/5-async-queries.md

Co-authored-by: blaise-muhirwa <[email protected]>

* Update docs/docs/building-applications/5-async-queries.md

Co-authored-by: robotrapta <[email protected]>

* small fixes based on PR feedback from Leo and Blaise

---------

Co-authored-by: brandon <[email protected]>
Co-authored-by: Brandon <[email protected]>
Co-authored-by: Auto-format Bot <[email protected]>
Co-authored-by: blaise-muhirwa <[email protected]>
Co-authored-by: robotrapta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants