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

Improve the plumbing and documentation for some of the ask* methods #125

Merged
merged 9 commits into from
Oct 18, 2023

Conversation

tyler-romero
Copy link
Member

@tyler-romero tyler-romero commented Oct 18, 2023

Address a some documentation papercuts for the new ask_* methods. Plus, improve plumbing of arguments to submit_image_query and to wait_for_confident_result.

"""Evaluates an image with Groundlight waiting until an answer above the confidence threshold
of the detector is reached or the wait period has passed.
"""
Evaluates an image with Groundlight waiting until an answer above the confidence threshold
Copy link
Member Author

Choose a reason for hiding this comment

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

@tyler-romero tyler-romero requested review from sunildkumar, robotrapta and brandon-groundlight and removed request for sunildkumar October 18, 2023 21:39
Comment on lines +491 to +501
:param patience_time: How long to wait (in seconds) for a confident answer for this image query.
The longer the patience_time, the more likely Groundlight will arrive at a confident answer.
Within patience_time, Groundlight will update ML predictions based on stronger findings,
and, additionally, Groundlight will prioritize human review of the image query if necessary.
This is a soft server-side timeout. If not set, use the detector's patience_time.
:type patience_time: float

:param confidence_threshold: The confidence threshold to wait for.
If not set, use the detector's confidence threshold.
:type confidence_threshold: float

Copy link
Member

Choose a reason for hiding this comment

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

I absolutely love this description of patience time

@tyler-romero tyler-romero marked this pull request as ready for review October 18, 2023 21:45
Comment on lines +581 to +584
if confidence_threshold is None:
if isinstance(image_query, str):
image_query = self.get_image_query(image_query)
confidence_threshold = self.get_detector(image_query.detector_id).confidence_threshold
Copy link
Member

Choose a reason for hiding this comment

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

my only concern here is that this introduces another request from the server. I recall @robotrapta suggesting that we should try to minimize this to minimize overall latency, but maybe I am mistaken. I am guessing that the alternative would be to force the user to provide the detector as an argument to the function which is kinda ugly.

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually call get_image_query if image_query is a string as the very first step in _wait_for_result, so this just brings the call forwards a bit.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok nice

def confidence_above_thresh(iq):
return iq_is_confident(iq, confidence_threshold=confidence_threshold)

confidence_above_thresh = partial(iq_is_confident, confidence_threshold=confidence_threshold)
Copy link
Member

Choose a reason for hiding this comment

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

thanks for teaching me about partial - this is very haskell-y.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha its honestly really nice sometimes. I love it because its a bit of a functional holdover

@@ -623,6 +662,7 @@ def add_label(self, image_query: Union[ImageQuery, str], label: Union[Label, str
else:
image_query_id = str(image_query)
# Some old imagequery id's started with "chk_"
# TODO: handle iqe
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what you mean here, so I'm concerned that others might not as well, but maybe I'm just out of the loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Edge detectors return image queries with the prefix iqe_, so we need to work on a way to handle that sanely

Copy link
Collaborator

Choose a reason for hiding this comment

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

This what the edge server will return, right? This just opens up the sdk to work with the edge

Copy link
Member

Choose a reason for hiding this comment

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

got it!

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly Brandon, just updated the comment to be a bit more clear

Comment on lines +71 to +72
image_query = gl.wait_for_confident_result(id=image_query.id) # Poll for a confident result from Groundlight
result = image_query.result
Copy link
Member

Choose a reason for hiding this comment

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

I like showing this as the default use case.

Copy link
Member

@sunildkumar sunildkumar left a comment

Choose a reason for hiding this comment

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

nice! Thanks for your feedback on all of the documentation this afternoon.

Copy link
Collaborator

@brandon-groundlight brandon-groundlight left a comment

Choose a reason for hiding this comment

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

Did we also want to attach confidence_threshold to the object sent to the BE, or does that require some BE work first?

@@ -623,6 +662,7 @@ def add_label(self, image_query: Union[ImageQuery, str], label: Union[Label, str
else:
image_query_id = str(image_query)
# Some old imagequery id's started with "chk_"
# TODO: handle iqe
Copy link
Collaborator

Choose a reason for hiding this comment

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

This what the edge server will return, right? This just opens up the sdk to work with the edge

@tyler-romero
Copy link
Member Author

That will require a bit of BE work Brandon, this is just the intermediate solution

@tyler-romero tyler-romero merged commit ff6da32 into main Oct 18, 2023
8 checks passed
@tyler-romero tyler-romero deleted the tyler/ask-method-improvements-and-documentation branch October 18, 2023 22:52
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