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

Feature/inference api #117

Merged
merged 43 commits into from
Aug 2, 2023
Merged

Feature/inference api #117

merged 43 commits into from
Aug 2, 2023

Conversation

taekang1618
Copy link
Contributor

@taekang1618 taekang1618 commented Jul 21, 2023

From one of our client calls, we found a demand to use model deployment in automatic data pipelines using the studio python api client. In this PR, we built the api client side code for model deployment.

To test, you need to need to pull the branch in this PR from the backend code.

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2023

CLA assistant check
All committers have signed the CLA.

@ryansingman
Copy link
Member

can you fix mypy checks?

Copy link
Member

@ryansingman ryansingman left a comment

Choose a reason for hiding this comment

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

this PR is incomplete. users are expected to use the Studio class to interact w/ the API, not the API methods directly. methods need to be added to that class that call the API methods (like all of the other functionalities do)

Comment on lines 351 to 353
if modality == "text":
header_io = io.StringIO(header)
input_batch = io.StringIO("\n".join(chain(header_io, batch)))
Copy link
Member

Choose a reason for hiding this comment

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

see comment in backend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll change the lambda function to take care of this. Didn't want to touch lambda as much as possible since it's not in a state that's easy to fix and deploy right now.

def download_prediction_results(result_url: str) -> io.StringIO:
"""Downloads prediction results from presigned URL."""
res = requests.get(result_url)
return io.StringIO(res.text)
Copy link
Member

Choose a reason for hiding this comment

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

no need to do this buffering, just pass res.raw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we do need this buffering since we want to return a numpy array for the results to the user.

Comment on lines 54 to 55
# Set timeout to 10 minutes as inference won't take longer than 10 minutes typically and
# to prevent users from getting stuck in this loop indefinitely when there is a failure
Copy link
Member

Choose a reason for hiding this comment

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

failures should be reported in the status, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that would be ideal, but there has been cases where the status updates did not happen properly for lambda failures in the past and queries were stuck in the running status even after they have failed. Just wanted to make sure this case doesn't confuse users.

status = resp["status"]

if status == "error":
return resp["error_msg"]
Copy link
Member

Choose a reason for hiding this comment

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

we need to raise an error here. returning an error message is not a sensible API for users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to raising an APIError

results_converted: Predictions = pd.read_csv(results).to_numpy()
return results_converted

@functools.singledispatchmethod
Copy link
Member

Choose a reason for hiding this comment

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

should remove (relic from my dev work)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

status: Optional[str] = resp["status"]
# Set timeout to 10 minutes as inference won't take longer than 10 minutes typically and
# to prevent users from getting stuck in this loop indefinitely when there is a failure
timeout = time.time() + 60 * 10
Copy link
Member

Choose a reason for hiding this comment

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

timeout should be user configurable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add timeout as an optional parameter users can add when calling predict.

def download_prediction_results(result_url: str) -> io.StringIO:
"""Downloads prediction results from presigned URL."""
res = requests.get(result_url)
return io.StringIO(res.text)
Copy link
Member

Choose a reason for hiding this comment

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

It is unnecessary and wasteful to rebuffer the entire result here. Just use res.raw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this function entirely and used url directly in pandas to read results

@ryansingman
Copy link
Member

Failing in dev currently:

cleanlab_studio.errors.APIError: Length of values (2) does not match length of index (100)

@taekang1618
Copy link
Contributor Author

Can you let me know which model and test input you tested with?

@taekang1618
Copy link
Contributor Author

taekang1618 commented Jul 28, 2023

Looked at the input file here and it seems like there is a formatting error in the header.
,Unnamed: 0,tweet_id,airline_sentiment,airline_sentiment_confidence,negativereason,negativereason_confidence,airline,airline_sentiment_gold,name,negativereason_gold,retweet_count,text,tweet_coord,tweet_created,tweet_location,user_timezone

@taekang1618 taekang1618 requested a review from axl1313 July 28, 2023 18:53
@taekang1618 taekang1618 requested a review from axl1313 July 28, 2023 21:43
Comment on lines 59 to 68
resp = api.get_prediction_status(self._api_key, query_id)
status: Optional[str] = resp["status"]
# Set timeout to prevent users from getting stuck indefinitely when there is a failure
timeout_limit = time.time() + timeout

while status == "running" and time.time() < timeout_limit:
resp = api.get_prediction_status(self._api_key, query_id)
status = resp["status"]
# Set time.sleep so that the while loop doesn't flood backend with api calls
time.sleep(3)
Copy link
Member

Choose a reason for hiding this comment

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

make timeout configurable and cleanup logic

cleanlab_studio/studio/inference.py Outdated Show resolved Hide resolved
cleanlab_studio/studio/inference.py Outdated Show resolved Hide resolved
cleanlab_studio/studio/inference.py Outdated Show resolved Hide resolved
cleanlab_studio/internal/api/api.py Outdated Show resolved Hide resolved
cleanlab_studio/studio/studio.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@axl1313 axl1313 left a comment

Choose a reason for hiding this comment

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

lgtm! just a couple nits

cleanlab_studio/studio/inference.py Outdated Show resolved Hide resolved
cleanlab_studio/studio/inference.py Outdated Show resolved Hide resolved
cleanlab_studio/internal/api/api.py Outdated Show resolved Hide resolved
@ryansingman ryansingman merged commit bb2b1c5 into main Aug 2, 2023
22 checks passed
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.

4 participants