Retry http errors by maingoh · Pull Request #58 · Deepomatic/deepomatic-client-python

maingoh · 2019-08-02T15:21:38Z

Related to #17

Retries on http errors (bad status code and requests exceptions)
Fast retry, with small timeout on most requests
Allow to override the default from a specific endpoint: network creation doesn't retry as it is an heavy request, we don't want to retry silently. Request timeout is longer
Allow to customize the retryer when instanciating the client

…eptions

maingoh · 2019-08-02T17:10:27Z

Maybe better list of exception types ? https://2.python-requests.org/en/master/_modules/requests/exceptions/
Test status codes with https://github.com/gabrielfalcao/HTTPretty (EDIT: done)

vdel

This way of implementing the retry forces the developer to remember wrapping its function call into a retry. The problem is that when new developers come and do not do that, we will still miss the retry.

I would rather implement the retry at the low-level in make_request, forwarding retry parameters from the generic retrieve, list, etc... mixups in order to keep the possibility to locally change the retry behavior.

deepomatic/api/utils.py

maingoh · 2019-08-19T12:21:52Z

I dont' understand the problem. The retry is already located in make_request. No need for the developer to change anything. Doing it at the urllib3 level was also considered but doesn't provide as much flexibility as tenacity does (see with @thomas-riccardi). We are also not sure it retries on everything we want.

Overriding the retry parameter from each endpoint is a feature we can probably add. Though for now I don't see a use case where it would be necessary ? This can be done in another PR if really needed one day, but I don't think it provides a lot of value to do it now.

vdel · 2019-08-20T07:23:12Z

I would rather implement the retry at the low-level in make_request, forwarding retry parameters from the generic retrieve, list, etc... mixups in order to keep the possibility to locally change the retry behavior.

I was referring to places like this one: https://github.com/Deepomatic/deepomatic-client-python/pull/58/files#diff-e63474de88f64ad351a531418838acc4R126 but I didn't see that the retry condition was different from a failure. So actually this comment above is void and everything looks good for me.

maingoh · 2019-08-20T10:02:06Z

I think I understand what you meant. I could have merged the retry policy of the task with the http helper retry policy instead of using two different retryer, but I don't know if it really make sense to merge the network failures with the retry on "pending" tasks. I don't think it is a big deal though ?

thomas-riccardi · 2019-08-27T13:51:31Z

I think I understand what you meant. I could have merged the retry policy of the task with the http helper retry policy instead of using two different retryer, but I don't know if it really make sense to merge the network failures with the retry on "pending" tasks. I don't think it is a big deal though ?

Let's discuss it IRL; I am not sure what we want to do, what should be avoided (double level retry?)...

vdel · 2019-08-30T10:27:26Z

There is retry in case of error 5XX for example, then we use retry to poll on the task until it completes.
For me this is okay, it's an important feature so I'm in favor of taking a decision soonish to merge it.