AmericanRedCross / street-view-green-view

BSD 3-Clause "New" or "Revised" License

8 stars 9 forks source link

fix: Stamina Retry on all HTTPError and RequestException #52

Closed dragonejt closed 2 weeks ago

dragonejt commented 3 weeks ago

Description

An attempt to avoid stopping execution if a ChunkedEncodingError is encountered in #47

Changes

Change the stamina retry condition to be urllib3.exceptions.HTTPError and requests.RequestException
I don't think there are any issues if we retry on all HTTPError or RequestException, but there may be an edge case in which we don't want to retry an API call or image download that I haven't though of

Testing

python -m src.assign_images data/interim/Three_Rivers_Michigan_USA_points.gpkg MAPILLARY data/raw/mapillary/downloaded many images successfully, and stamina retried in the logs

dragonejt commented 2 weeks ago

While I agree that there are cases where a retry may not change anything, I also don't see any cases where a retry would actively disrupt our logic flow, so these exceptions should be "safe" to retry even if they still fail. There are also cases where, for example, the Mapillary API times out but actually returns an HTTP 400 Bad Request. I agree that we can punt this to an issue to determine the best exceptions to retry on later.

jayqi commented 2 weeks ago

I also don't see any cases where a retry would actively disrupt our logic flow, so these exceptions should be "safe" to retry even if they still fail.

It's not just about "safe" from runtime errors. Repeating requests that are known to fail (if request is invalid, or 403 or 404 errors), then we (a) will cause the user to waste a bunch of time making repeated failed requests and waiting for the exponential backoff, and (b) incur unnecessary server load on Mapillary, which is bad etiquette.

That said, I'm okay with this being a quick fix for now.