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

Retry upload 3 times in case of failure #703

Merged
merged 13 commits into from
Jul 7, 2023
Merged

Retry upload 3 times in case of failure #703

merged 13 commits into from
Jul 7, 2023

Conversation

franz101
Copy link
Contributor

@franz101 franz101 commented Jul 6, 2023

Adds 3 tries for retrying to upload in case of failure.

  • Does not depend on external libraries

@franz101 franz101 marked this pull request as ready for review July 6, 2023 13:48
@franz101 franz101 requested review from a team and dcaustin33 as code owners July 6, 2023 13:48
Copy link
Contributor

@elboy3 elboy3 left a comment

Choose a reason for hiding this comment

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

I like the addition of retries but we should consider using tenacity rather than implementing ourselves

https://tenacity.readthedocs.io/en/latest/

@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2023

Codecov Report

Merging #703 (4f65b8b) into main (59727cc) will decrease coverage by 0.03%.
The diff coverage is 66.66%.

❗ Current head 4f65b8b differs from pull request most recent head 577850e. Consider uploading reports for the commit 577850e to get more accurate results

@@            Coverage Diff             @@
##             main     #703      +/-   ##
==========================================
- Coverage   89.62%   89.59%   -0.03%     
==========================================
  Files         166      166              
  Lines       13276    13278       +2     
==========================================
- Hits        11898    11897       -1     
- Misses       1378     1381       +3     
Impacted Files Coverage Δ
dataquality/utils/ultralytics.py 55.92% <0.00%> (ø)
dataquality/clients/objectstore.py 45.58% <100.00%> (+1.64%) ⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pyproject.toml Outdated
@@ -67,7 +67,7 @@ test = [
"coverage[toml]>=7.0.5",
"pytest-cov>=4.0.0",
"scikit-learn>=1.0",
"tensorflow>=2.9.1",
"tensorflow>=2.9.1,<2.13.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

so folks can never use a higher version of tensorflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs to be fixed sperately

Copy link
Contributor

Choose a reason for hiding this comment

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

This req is in the test extras. But if they do happen to use tf >= 2.13.0 our method https://github.com/rungalileo/dataquality/blob/9b8af3636decb6bb5a7828df7fe6ac7e755188f3/dataquality/utils/keras.py#L46C1-L54C6 will fail and it seems to be used everywhere.

It's a prob in TF so I opened an issue on TF tensorflow/tensorflow#61204 (comment)

Comment on lines 87 to 103
max_retries = 3 if retry else 1
try:
for attempt in Retrying(stop=stop_after_attempt(max_retries)):
with attempt:
res = RequestType.get_method(request.value)(
url,
json=body,
params=params,
headers=header,
data=data,
timeout=timeout,
files=files,
)
except RetryError as e:
raise GalileoException(
f"Failed to make request after {max_retries} attempts. " f"Error: {e}"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's chat about this. i would just use the tenacity decorator. 1 line instead of 17

Copy link
Contributor

@bogdan-galileo bogdan-galileo left a comment

Choose a reason for hiding this comment

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

LGTM !

@elboy3 elboy3 merged commit be83366 into main Jul 7, 2023
@elboy3 elboy3 deleted the chore/retry_upload branch July 7, 2023 18:33
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