-
Notifications
You must be signed in to change notification settings - Fork 0
Fix: Timeout HOT/OHSOME requests below gunicorn timeout #268
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
base: develop
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,10 @@ class ValidateApiCallError(Exception): | |
| pass | ||
|
|
||
|
|
||
| class ValidateApiCallTimeoutError(ValidateApiCallError): | ||
| pass | ||
|
|
||
|
|
||
| def remove_troublesome_chars(string: str | None): | ||
| """Remove chars that cause trouble when pushed into postgres.""" | ||
| if type(string) is not str: | ||
|
|
@@ -176,16 +180,22 @@ def remove_noise_and_add_user_info(json: dict[str, Any]) -> dict[str, Any]: | |
| return json | ||
|
|
||
|
|
||
| # fixme(frozenhelium): merge this function with `ohsome` and also add appropriate messages to raised exceptions | ||
| def get_object_count_from_ohsome(area: str, ohsome_filter: PydanticLongText) -> int | None: | ||
| def get_object_count_from_ohsome( | ||
| area: str, | ||
| ohsome_filter: PydanticLongText, | ||
| timeout: int = Config.DEFAULT_EXTERNAL_API_TIMEOUT, | ||
| ) -> int | None: | ||
| url = Config.OHSOME_API_LINK + "elements/count" | ||
| data = {"bpolys": area, "filter": ohsome_filter} | ||
|
|
||
| logger.info("Target: %s", url) | ||
| logger.info("Filter: %s", ohsome_filter) | ||
|
|
||
| # fixme(frozenhelium): use httpx for proper timeout | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why remove this comment? the default request timeout is not reliable
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should i migrate it to httpx? |
||
| response = requests.post(url, data=data, timeout=100) | ||
| try: | ||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can drop "Please try again" |
||
| if response.status_code != 200: | ||
| logger.warning( | ||
| "ohsome element count request failed: check for errors in filter or geometries", | ||
|
|
@@ -210,16 +220,24 @@ def get_object_count_from_ohsome(area: str, ohsome_filter: PydanticLongText) -> | |
| return int(value) | ||
|
|
||
|
|
||
| def ohsome(request: dict[str, Any], area: str, properties: str | None = None) -> dict[str, Any]: | ||
| def ohsome( | ||
| request: dict[str, Any], | ||
| area: str, | ||
| properties: str | None = None, | ||
| timeout: int = Config.DEFAULT_EXTERNAL_API_TIMEOUT, | ||
| ) -> dict[str, Any]: | ||
| """Request data from Ohsome API.""" | ||
| url = Config.OHSOME_API_LINK + request["endpoint"] | ||
| data = {"bpolys": area, "filter": request["filter"]} | ||
| if properties: | ||
| data["properties"] = properties | ||
| logger.info("Target: %s", url) | ||
| logger.info("Filter: %s", request["filter"]) | ||
| # FIXME(tnagorra): Need to check what the timeout should be | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove the FIXME comment. Also the timeout has changed from 100 |
||
| response = requests.post(url, data=data, timeout=100) | ||
| try: | ||
| response = requests.post(url, data=data, timeout=timeout) | ||
| except requests.exceptions.Timeout as e: | ||
| logger.warning("ohsome request timed out") | ||
| raise ValidateApiCallTimeoutError("OHSOME request timed out. Please try again.") from e | ||
| if response.status_code != 200: | ||
| logger.warning( | ||
| "ohsome request failed: check for errors in filter or geometries", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,12 @@ | |
| from main.config import Config | ||
| from project_types.base import project as base_project | ||
| from project_types.tile_map_service.base.project import create_json_dump | ||
| from project_types.validate.api_calls import ValidateApiCallError, get_object_count_from_ohsome, ohsome | ||
| from project_types.validate.api_calls import ( | ||
| ValidateApiCallError, | ||
| ValidateApiCallTimeoutError, | ||
| get_object_count_from_ohsome, | ||
| ohsome, | ||
| ) | ||
| from utils import fields as custom_fields | ||
| from utils.asset_types.models import AoiGeometryAssetProperty | ||
| from utils.common import Grouping, clean_up_none_keys, to_groups | ||
|
|
@@ -157,9 +162,12 @@ def _validate_object_count(count: int | None) -> int: | |
| def test_geojson_url_project(url: str): | ||
| logger.info("Fetching object geojson from %s", url) | ||
|
|
||
| # FIXME(frozenhelium): use predefined timeout duration | ||
| # FIXME(tnagorra): handle timeout error | ||
| response = requests.get(url, timeout=500) | ||
| try: | ||
| response = requests.get(url, timeout=Config.DEFAULT_EXTERNAL_API_TIMEOUT) | ||
| except requests.exceptions.Timeout as e: | ||
| raise base_project.ValidationException( | ||
| f"Failed to fetch object geojson from {url}: request timed out", | ||
| ) from e | ||
| if response.status_code != 200: | ||
| raise base_project.ValidationException( | ||
| f"Failed to fetch object geojson from {url}", | ||
|
|
@@ -207,6 +215,8 @@ def test_ohsome_objects_from_aoi_asset( | |
| feature_collection.model_dump_json(), | ||
| ohsome_filter, | ||
| ) | ||
| except ValidateApiCallTimeoutError as e: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do wr need this except when we already have a generic except that does the same thing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| raise base_project.ValidationException(str(e)) from e | ||
| except Exception as e: | ||
| raise base_project.ValidationException("Failed to get object_count from ohsome") from e | ||
|
|
||
|
|
@@ -223,7 +233,12 @@ def test_tasking_manager_project( | |
| hot_tm_url = f"{Config.HOT_TASKING_MANAGER_PROJECT_API_LINK}projects/{hot_tm_id}/queries/aoi/?as_file=false" | ||
| logger.info("Fetching AOI geojson on HOT from %s", hot_tm_url) | ||
|
|
||
| aoi_result = requests.get(hot_tm_url, timeout=500) | ||
| try: | ||
| aoi_result = requests.get(hot_tm_url, timeout=Config.DEFAULT_EXTERNAL_API_TIMEOUT) | ||
| except requests.exceptions.Timeout as e: | ||
| raise base_project.ValidationException( | ||
| f"Failed to fetch AOI GeoJSON from HOT Tasking Manager for tm_id {hot_tm_id}: request timed out", | ||
| ) from e | ||
| if aoi_result.status_code != 200: | ||
| raise base_project.ValidationException( | ||
| f"Failed to fetch AOI GeoJSON from HOT Tasking Manager for tm_id {hot_tm_id}", | ||
|
|
@@ -269,6 +284,8 @@ def test_tasking_manager_project( | |
| feature_collection.model_dump_json(), | ||
| ohsome_filter, | ||
| ) | ||
| except ValidateApiCallTimeoutError as e: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this except when we already have a generic except that does the same thing |
||
| raise base_project.ValidationException(str(e)) from e | ||
| except Exception as e: | ||
| raise base_project.ValidationException("Failed to get object_count from ohsome") from e | ||
|
|
||
|
|
@@ -311,9 +328,13 @@ def _get_object_geometry_from_ohsome(self, geojson: dict[typing.Any, typing.Any] | |
| feature_collection.model_dump_json(), | ||
| properties="tags, metadata", | ||
| ) | ||
| except ValidateApiCallTimeoutError as e: | ||
| raise base_project.ValidationException(str(e)) from e | ||
| except ValidateApiCallError as e: | ||
| # NOTE: Handles calls from OHSOME, OSMCHA and OSM | ||
| raise base_project.ValidationException("Failed to fetch data from OHSOME/OSMCHA/OSM") from e | ||
| raise base_project.ValidationException( | ||
| "Failed to fetch data from OHSOME/OSMCHA/OSM", | ||
| ) from e | ||
| except requests.JSONDecodeError as e: | ||
| # NOTE: Handles calls from OHSOME and OSMCHA | ||
| # OSM responds in XML format | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this comment?