Skip to content

False 500 testing #57

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Oct 18, 2022
Merged

False 500 testing #57

Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b2fbbcc
push wip code for handling false 500s
kevin-xu-scale Oct 7, 2022
eca2153
query task response via uuid
kevin-xu-scale Oct 7, 2022
bfe891a
cleaning up, remove print statements
kevin-xu-scale Oct 7, 2022
244245c
raise 409 error when retry history is empty
kevin-xu-scale Oct 7, 2022
1740d96
small changes
jonathanfeng-scale Oct 18, 2022
51c18a3
Update api.py
jonathanfeng-scale Oct 18, 2022
5141ff2
more formatting
jonathanfeng-scale Oct 18, 2022
f994a6e
Update api.py
jonathanfeng-scale Oct 18, 2022
b485dbf
fomatting
jonathanfeng-scale Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion scaleapi/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,28 @@ def _api_request(
json = None
if res.status_code == 200:
json = res.json()
elif res.status_code == 409:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think our API also returns 409 (Conflict) during batch and project creation.
Probably super rarer to have a 500 during batch or project creation, but should we add further governance here to ensure that behavior applies only to task creation?

Checking the task/ included in endpoint in a POST request and checking if unique_id is available in the body can be some ways to approach that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, will do that

retry_history = res.raw.retries.history
# Example RequestHistory tuple
# RequestHistory(method='POST',
# url='/v1/task/imageannotation',
# error=None,
# status=409,
# redirect_location=None)
if retry_history != ():
# See if the first retry was a 500 error
if retry_history[0][3] == 500:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm (since I'm not super sure) but is 500 the only error returned from our API for those race conditions? (and not other 5xx errors).

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, the exact error is task creation taking longer than our timeout, so Scale sends a 500 back in the response

uuid = body["unique_id"]
newUrl = f"https://api.scale.com/v1/tasks?unique_id={uuid}"
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
newUrl = f"https://api.scale.com/v1/tasks?unique_id={uuid}"
newUrl = f"{self.base_api_url}/tasks?unique_id={uuid}"

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably would be ideal later if we have an API method that directly returns a single task for the given unique_id (similar to getting a task with task id)

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

# grab task from api
newRes = self._http_request(
"GET", newUrl, headers=headers, auth=auth
)
json = newRes.json()["docs"][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, will this json be the same as what we return after task creation, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

else:
self._raise_on_respose(res)
else:
self._raise_on_respose(res)

return json

def get_request(self, endpoint, params=None):
Expand Down