Fix: Timeout HOT/OHSOME requests below gunicorn timeout#268
Conversation
- Add Config.DEFAULT_EXTERNAL_API_TIMEOUT (kept below the 40s gunicorn --timeout) and use it instead of hardcoded 100/500s timeouts for HOT Tasking Manager, geojson URL, and OHSOME requests. - Add ValidateApiCallTimeoutError to distinguish OHSOME timeouts from other API failures, surfacing a clear "OHSOME request timed out. Please try again." message instead of a raw requests exception.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #268 +/- ##
===========================================
- Coverage 87.55% 87.43% -0.12%
===========================================
Files 211 211
Lines 12819 12842 +23
Branches 1089 1089
===========================================
+ Hits 11224 11229 +5
- Misses 1196 1214 +18
Partials 399 399 ☔ View full report in Codecov by Harness. |
| return json | ||
|
|
||
|
|
||
| # fixme(frozenhelium): merge this function with `ohsome` and also add appropriate messages to raised exceptions |
| logger.info("Target: %s", url) | ||
| logger.info("Filter: %s", ohsome_filter) | ||
|
|
||
| # fixme(frozenhelium): use httpx for proper timeout |
There was a problem hiding this comment.
why remove this comment? the default request timeout is not reliable
There was a problem hiding this comment.
Should i migrate it to httpx?
| response = requests.post(url, data=data, timeout=timeout) | ||
| except requests.exceptions.Timeout as e: | ||
| logger.warning("ohsome element count request timed out") | ||
| raise ValidateApiCallTimeoutError("OHSOME request timed out. Please try again.") from e |
There was a problem hiding this comment.
We can drop "Please try again"
This was not directly triggered by the user
| data["properties"] = properties | ||
| logger.info("Target: %s", url) | ||
| logger.info("Filter: %s", request["filter"]) | ||
| # FIXME(tnagorra): Need to check what the timeout should be |
There was a problem hiding this comment.
Why remove the FIXME comment. Also the timeout has changed from 100
| feature_collection.model_dump_json(), | ||
| ohsome_filter, | ||
| ) | ||
| except ValidateApiCallTimeoutError as e: |
There was a problem hiding this comment.
Why do wr need this except when we already have a generic except that does the same thing
There was a problem hiding this comment.
Currently, the generic exception is used for all the Exception. Added new exception just for the Timeout exception! Should we just use the generic one?
| feature_collection.model_dump_json(), | ||
| ohsome_filter, | ||
| ) | ||
| except ValidateApiCallTimeoutError as e: |
There was a problem hiding this comment.
Why do we need this except when we already have a generic except that does the same thing
2 participants
Changes
Add Config.DEFAULT_EXTERNAL_API_TIMEOUT (kept below the 40s gunicorn --timeout) and use it instead of hardcoded 100/500s timeouts for HOT Tasking Manager, geojson URL, and OHSOME requests.
Add ValidateApiCallTimeoutError to distinguish OHSOME timeouts from other API failures, surfacing a clear "OHSOME request timed out. Please try again." message instead of a raw requests exception.
This PR doesn't introduce any:
printThis PR contains valid: