From 0d490bbb19d110e19d0a53fd36c05b10208fbf94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Thu, 19 Jul 2018 20:52:59 +0200 Subject: [PATCH 01/26] Extract EMPTY_RESPONSE --- .../lib/python/cartodb_services/cartodb_services/geocoder.py | 2 ++ .../cartodb_services/cartodb_services/google/geocoder.py | 3 +-- .../cartodb_services/cartodb_services/mapbox/geocoder.py | 5 ++--- .../cartodb_services/cartodb_services/tomtom/geocoder.py | 3 +-- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 54e80f1..5a9f0a6 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -9,6 +9,8 @@ import json PRECISION_PRECISE = 'precise' PRECISION_INTERPOLATED = 'interpolated' +EMPTY_RESPONSE = [[], {}] + def geocoder_metadata(relevance, precision, match_types): return { diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index fb75c76..30a62cf 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -4,11 +4,10 @@ from urlparse import parse_qs from exceptions import MalformedResult -from cartodb_services.geocoder import compose_address, geocoder_metadata, PRECISION_PRECISE, PRECISION_INTERPOLATED +from cartodb_services.geocoder import compose_address, geocoder_metadata, PRECISION_PRECISE, PRECISION_INTERPOLATED, EMPTY_RESPONSE from cartodb_services.google.exceptions import InvalidGoogleCredentials from client_factory import GoogleMapsClientFactory -EMPTY_RESPONSE = [[], {}] PARTIAL_FACTOR = 0.8 RELEVANCE_BY_LOCATION_TYPE = { 'ROOFTOP': 1, diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index d76937a..2af2c6f 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -5,7 +5,7 @@ Python client for the Mapbox Geocoder service. import json import requests from mapbox import Geocoder -from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -23,8 +23,6 @@ ENTRY_COORDINATES = 'coordinates' ENTRY_TYPE = 'type' TYPE_POINT = 'Point' -EMPTY_RESPONSE = [[], {}] - MATCH_TYPE_BY_MATCH_LEVEL = { 'poi': 'point_of_interest', 'poi.landmark': 'point_of_interest', @@ -154,6 +152,7 @@ class MapboxGeocoder(Traceable): try: free_search = ';'.join([self._escape(fs) for fs in free_searches]) response = self._geocoder.forward(address=free_search.decode('utf-8'), + limit=1, country=country) if response.status_code == requests.codes.ok: diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py index 46cdbda..3f077bd 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py @@ -5,7 +5,7 @@ import json import requests from uritemplate import URITemplate from math import tanh -from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -20,7 +20,6 @@ ENTRY_RESULTS = 'results' ENTRY_POSITION = 'position' ENTRY_LON = 'lon' ENTRY_LAT = 'lat' -EMPTY_RESPONSE = [[], {}] SCORE_NORMALIZATION_FACTOR = 0.15 PRECISION_SCORE_THRESHOLD = 0.5 From 96fbf3080a23f5a91b8f0948d1a58d54ab979e5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Thu, 19 Jul 2018 21:08:07 +0200 Subject: [PATCH 02/26] Base run_street_point_geocoder test --- .../test/test_cartodb_services_geocoder.py | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py diff --git a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py new file mode 100644 index 0000000..a23554c --- /dev/null +++ b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py @@ -0,0 +1,29 @@ +#!/usr/local/bin/python +# -*- coding: utf-8 -*- + +from unittest import TestCase +from mock import Mock, MagicMock +from nose.tools import assert_not_equal, assert_equal, assert_true +from cartodb_services.geocoder import run_street_point_geocoder + + +class TestRunStreetPointGeocoder(TestCase): + def test_count_increment_total_service_use_on_error(self): + quota_service_mock = Mock() + + service_manager_mock = Mock() + service_manager_mock.quota_service = quota_service_mock + + searches = [] + + logger_config_mock = MagicMock(min_log_level='debug', + log_file_path='/tmp/ptest.log') + with(self.assertRaises(BaseException)): + run_street_point_geocoder(Mock(), + {'logger_config': logger_config_mock}, + None, + service_manager_mock, + None, None, searches) + + quota_service_mock.increment_total_service_use. \ + assert_called_once_with() From fd097724f11a127f488ecdffb253fea2fade1d29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Thu, 19 Jul 2018 21:17:09 +0200 Subject: [PATCH 03/26] In case of general error, total service use should be incremented by searches length --- server/lib/python/cartodb_services/cartodb_services/geocoder.py | 2 +- .../cartodb_services/test/test_cartodb_services_geocoder.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 5a9f0a6..2f1b55a 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -62,7 +62,7 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org service_manager.logger.error('Error trying to bulk geocode street point', sys.exc_info(), data={"username": username, "orgname": orgname}) raise Exception('Error trying to bulk geocode street') finally: - service_manager.quota_service.increment_total_service_use() + service_manager.quota_service.increment_total_service_use(len(searches)) StreetGeocoderSearch = namedtuple('StreetGeocoderSearch', 'id address city state country') diff --git a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py index a23554c..ea3deac 100644 --- a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py +++ b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py @@ -26,4 +26,4 @@ class TestRunStreetPointGeocoder(TestCase): None, None, searches) quota_service_mock.increment_total_service_use. \ - assert_called_once_with() + assert_called_once_with(len(searches)) From cd5e6510a68f4740fba1743bc787edb59fb0e3be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Thu, 19 Jul 2018 21:19:00 +0200 Subject: [PATCH 04/26] In case of general error, failed service use should be incremented by searches length --- .../python/cartodb_services/cartodb_services/geocoder.py | 4 +++- .../test/test_cartodb_services_geocoder.py | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 2f1b55a..1678552 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -54,11 +54,13 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org service_manager.quota_service.increment_empty_service_use(len(searches)) return [] except QuotaExceededException as qe: + logger.debug('QuotaExceededException at run_street_point_geocoder', qe, + data={"username": username, "orgname": orgname}) service_manager.quota_service.increment_failed_service_use(len(searches)) return [] except BaseException as e: import sys - service_manager.quota_service.increment_failed_service_use() + service_manager.quota_service.increment_failed_service_use(len(searches)) service_manager.logger.error('Error trying to bulk geocode street point', sys.exc_info(), data={"username": username, "orgname": orgname}) raise Exception('Error trying to bulk geocode street') finally: diff --git a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py index ea3deac..044dca0 100644 --- a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py +++ b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py @@ -8,11 +8,13 @@ from cartodb_services.geocoder import run_street_point_geocoder class TestRunStreetPointGeocoder(TestCase): - def test_count_increment_total_service_use_on_error(self): + def test_count_increment_total_and_failed_service_use_on_error(self): quota_service_mock = Mock() service_manager_mock = Mock() service_manager_mock.quota_service = quota_service_mock + service_manager_mock.assert_within_limits = \ + Mock(side_effect=Exception('Fail!')) searches = [] @@ -27,3 +29,6 @@ class TestRunStreetPointGeocoder(TestCase): quota_service_mock.increment_total_service_use. \ assert_called_once_with(len(searches)) + + quota_service_mock.increment_failed_service_use. \ + assert_called_once_with(len(searches)) From abbaf83e9769bd40711a3d67c21a719294919931 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 10:28:38 +0200 Subject: [PATCH 05/26] run_street_point_geocoder tests --- .../test/test_cartodb_services_geocoder.py | 63 ++++++++++++++----- 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py index 044dca0..4670fe6 100644 --- a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py +++ b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py @@ -4,31 +4,64 @@ from unittest import TestCase from mock import Mock, MagicMock from nose.tools import assert_not_equal, assert_equal, assert_true -from cartodb_services.geocoder import run_street_point_geocoder +from cartodb_services.geocoder import run_street_point_geocoder, StreetGeocoderSearch class TestRunStreetPointGeocoder(TestCase): - def test_count_increment_total_and_failed_service_use_on_error(self): - quota_service_mock = Mock() - service_manager_mock = Mock() - service_manager_mock.quota_service = quota_service_mock - service_manager_mock.assert_within_limits = \ + def setUp(self): + point = [0,0] + self.plpy_mock = Mock() + self.plpy_mock.execute = MagicMock(return_value=[{'the_geom': point}]) + + self.logger_config_mock = MagicMock(min_log_level='debug', + log_file_path='/tmp/ptest.log') + self.gd_mock = {'logger_config': self.logger_config_mock} + + self.geocoder_mock = Mock() + + self.quota_service_mock = Mock() + + self.service_manager_mock = Mock() + self.service_manager_mock.quota_service = self.quota_service_mock + self.service_manager_mock.assert_within_limits = MagicMock() + + def test_count_increment_total_and_failed_service_use_on_error(self): + self.service_manager_mock.assert_within_limits = \ Mock(side_effect=Exception('Fail!')) searches = [] - logger_config_mock = MagicMock(min_log_level='debug', - log_file_path='/tmp/ptest.log') with(self.assertRaises(BaseException)): - run_street_point_geocoder(Mock(), - {'logger_config': logger_config_mock}, - None, - service_manager_mock, - None, None, searches) + run_street_point_geocoder(self.plpy_mock, self.gd_mock, + self.geocoder_mock, + self.service_manager_mock, + 'any_username', None, searches) - quota_service_mock.increment_total_service_use. \ + self.quota_service_mock.increment_total_service_use. \ assert_called_once_with(len(searches)) - quota_service_mock.increment_failed_service_use. \ + self.quota_service_mock.increment_failed_service_use. \ assert_called_once_with(len(searches)) + + def test_increment_success_service_use_on_complete_response(self): + searches = [ + StreetGeocoderSearch(id=1, address='Paseo Zorrilla 1, Valladolid', + city=None, state=None, country=None), + StreetGeocoderSearch(id=2, address='Paseo Zorrilla 2, Valladolid', + city=None, state=None, country=None) + ] + results = [ + (1, [0, 0], {}), + (2, [0, 0], {}), + ] + self.geocoder_mock.bulk_geocode = MagicMock(return_value=results) + run_street_point_geocoder(self.plpy_mock, self.gd_mock, + self.geocoder_mock, + self.service_manager_mock, + 'any_username', None, searches) + + self.quota_service_mock.increment_success_service_use.\ + assert_called_once_with(len(results)) + + From 2862c8002530c9b291bd9bbad67efa94e9f71833 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 11:53:02 +0200 Subject: [PATCH 06/26] Proper empty count on bulk geocoding --- .../cartodb_services/geocoder.py | 17 +-- .../test/test_cartodb_services_geocoder.py | 102 ++++++++++++++++-- 2 files changed, 102 insertions(+), 17 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 1678552..edb33bc 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -30,11 +30,13 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org logger = Logger(logger_config) + success_count, failed_count = 0, 0 + try: service_manager.assert_within_limits(quota=False) geocode_results = geocoder.bulk_geocode(searches=searches) + results = [] if geocode_results: - results = [] for result in geocode_results: if len(result) > 2: metadata = json.dumps(result[2]) @@ -46,13 +48,16 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326) as the_geom; ", ["double precision", "double precision"]) point = plpy.execute(plan, result[1], 1)[0] results.append([result[0], point['the_geom'], metadata]) + success_count += 1 else: results.append([result[0], None, metadata]) - service_manager.quota_service.increment_success_service_use(len(results)) - return results - else: - service_manager.quota_service.increment_empty_service_use(len(searches)) - return [] + + empty_count = len(searches) - success_count - failed_count + service_manager.quota_service.increment_success_service_use(success_count) + service_manager.quota_service.increment_empty_service_use(empty_count) + service_manager.quota_service.increment_failed_service_use(failed_count) + + return results except QuotaExceededException as qe: logger.debug('QuotaExceededException at run_street_point_geocoder', qe, data={"username": username, "orgname": orgname}) diff --git a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py index 4670fe6..240e6f7 100644 --- a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py +++ b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py @@ -4,9 +4,44 @@ from unittest import TestCase from mock import Mock, MagicMock from nose.tools import assert_not_equal, assert_equal, assert_true +from cartodb_services.tools import QuotaExceededException from cartodb_services.geocoder import run_street_point_geocoder, StreetGeocoderSearch +SEARCH_FIXTURES = { + 'two': [ + StreetGeocoderSearch(id=1, address='Paseo Zorrilla 1, Valladolid', + city=None, state=None, country=None), + StreetGeocoderSearch(id=2, address='Paseo Zorrilla 2, Valladolid', + city=None, state=None, country=None) + ], + 'wrong': [ + StreetGeocoderSearch(id=100, address='deowpfjoepwjfopejwpofjewpojgf', + city=None, state=None, country=None), + ] +} + +BULK_RESULTS_FIXTURES = { + 'two': [ + (1, [0, 0], {}), + (2, [0, 0], {}), + ], + 'wrong': [ + (100, [], {}) + ] +} + +EXPECTED_RESULTS_FIXTURES = { + 'two': [ + [1, [0, 0], '{}'], + [2, [0, 0], '{}'], + ], + 'wrong': [ + [100, None, '{}'] + ] +} + + class TestRunStreetPointGeocoder(TestCase): def setUp(self): @@ -44,24 +79,69 @@ class TestRunStreetPointGeocoder(TestCase): self.quota_service_mock.increment_failed_service_use. \ assert_called_once_with(len(searches)) + def test_count_increment_failed_service_use_on_quota_error(self): + self.service_manager_mock.assert_within_limits = \ + Mock(side_effect=QuotaExceededException()) + + searches = SEARCH_FIXTURES['two'] + + result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, + self.geocoder_mock, + self.service_manager_mock, + 'any_username', None, searches) + assert_equal(result, []) + + self.quota_service_mock.increment_failed_service_use. \ + assert_called_once_with(len(searches)) + def test_increment_success_service_use_on_complete_response(self): - searches = [ - StreetGeocoderSearch(id=1, address='Paseo Zorrilla 1, Valladolid', - city=None, state=None, country=None), - StreetGeocoderSearch(id=2, address='Paseo Zorrilla 2, Valladolid', - city=None, state=None, country=None) - ] + searches = SEARCH_FIXTURES['two'] results = [ (1, [0, 0], {}), (2, [0, 0], {}), ] + expected_results = [ + [1, [0, 0], '{}'], + [2, [0, 0], '{}'], + ] self.geocoder_mock.bulk_geocode = MagicMock(return_value=results) - run_street_point_geocoder(self.plpy_mock, self.gd_mock, - self.geocoder_mock, - self.service_manager_mock, - 'any_username', None, searches) + result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, + self.geocoder_mock, + self.service_manager_mock, + 'any_username', None, searches) + assert_equal(result, expected_results) - self.quota_service_mock.increment_success_service_use.\ + self.quota_service_mock.increment_success_service_use. \ assert_called_once_with(len(results)) + def test_increment_empty_service_use_on_complete_response(self): + searches = SEARCH_FIXTURES['two'] + results = [] + self.geocoder_mock.bulk_geocode = MagicMock(return_value=results) + result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, + self.geocoder_mock, + self.service_manager_mock, + 'any_username', None, searches) + + assert_equal(result, results) + + self.quota_service_mock.increment_empty_service_use. \ + assert_called_once_with(len(searches)) + + def test_increment_mixed_empty_service_use_on_complete_response(self): + searches = SEARCH_FIXTURES['two'] + SEARCH_FIXTURES['wrong'] + bulk_results = BULK_RESULTS_FIXTURES['two'] + BULK_RESULTS_FIXTURES['wrong'] + self.geocoder_mock.bulk_geocode = MagicMock(return_value=bulk_results) + result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, + self.geocoder_mock, + self.service_manager_mock, + 'any_username', None, searches) + + assert_equal(result, EXPECTED_RESULTS_FIXTURES['two'] + EXPECTED_RESULTS_FIXTURES['wrong']) + + self.quota_service_mock.increment_success_service_use. \ + assert_called_once_with(len(SEARCH_FIXTURES['two'])) + self.quota_service_mock.increment_empty_service_use. \ + assert_called_once_with(len(SEARCH_FIXTURES['wrong'])) + From 1cebbe7af0946ca8a6182fe3e7cc29514a2fd018 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 12:16:48 +0200 Subject: [PATCH 07/26] Missing warning mock and fix for debug --- server/lib/python/cartodb_services/test/mock_plpy.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/test/mock_plpy.py b/server/lib/python/cartodb_services/test/mock_plpy.py index 0fe067f..c088670 100644 --- a/server/lib/python/cartodb_services/test/mock_plpy.py +++ b/server/lib/python/cartodb_services/test/mock_plpy.py @@ -52,11 +52,14 @@ class MockPlPy: self._logged_queries = [] self._log_executed_queries = True + def warning(self, msg): + self.warnings.append(msg) + def notice(self, msg): self.notices.append(msg) def debug(self, msg): - self.notices.append(msg) + self.debugs.append(msg) def info(self, msg): self.infos.append(msg) From 9a1b1e2832030a91704c0dafb9e1fcc89f339ab2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 12:18:26 +0200 Subject: [PATCH 08/26] Error count --- .../cartodb_services/geocoder.py | 18 ++++++++---- .../test/test_cartodb_services_geocoder.py | 29 ++++++++++++++++++- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index edb33bc..8d5d456 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -39,18 +39,24 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org if geocode_results: for result in geocode_results: if len(result) > 2: - metadata = json.dumps(result[2]) + metadata = result[2] else: - logger.warning('Geocoding for {} without metadata'.format(username)) - metadata = '{}' + logger.warning('Geocoding without metadata', + data={"username": username, "orgname": orgname}) + metadata = {} - if result[1] and len(result[1]) == 2: + if metadata.get('error', None): + logger.warning('Geocoding error', + data={"username": username, "orgname": orgname, "error": metadata['error']}) + results.append([result[0], None, json.dumps(metadata)]) + failed_count += 1 + elif result[1] and len(result[1]) == 2: plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326) as the_geom; ", ["double precision", "double precision"]) point = plpy.execute(plan, result[1], 1)[0] - results.append([result[0], point['the_geom'], metadata]) + results.append([result[0], point['the_geom'], json.dumps(metadata)]) success_count += 1 else: - results.append([result[0], None, metadata]) + results.append([result[0], None, json.dumps(metadata)]) empty_count = len(searches) - success_count - failed_count service_manager.quota_service.increment_success_service_use(success_count) diff --git a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py index 240e6f7..6dd199c 100644 --- a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py +++ b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py @@ -18,6 +18,10 @@ SEARCH_FIXTURES = { 'wrong': [ StreetGeocoderSearch(id=100, address='deowpfjoepwjfopejwpofjewpojgf', city=None, state=None, country=None), + ], + 'error': [ + StreetGeocoderSearch(id=200, address=None, city=None, state=None, + country=None), ] } @@ -28,6 +32,9 @@ BULK_RESULTS_FIXTURES = { ], 'wrong': [ (100, [], {}) + ], + 'error': [ + (200, [], {'error': 'Something wrong happened'}) ] } @@ -38,6 +45,9 @@ EXPECTED_RESULTS_FIXTURES = { ], 'wrong': [ [100, None, '{}'] + ], + 'error': [ + [200, None, '{"error": "Something wrong happened"}'] ] } @@ -50,7 +60,8 @@ class TestRunStreetPointGeocoder(TestCase): self.plpy_mock.execute = MagicMock(return_value=[{'the_geom': point}]) self.logger_config_mock = MagicMock(min_log_level='debug', - log_file_path='/tmp/ptest.log') + log_file_path='/tmp/ptest.log', + rollbar_api_key=None) self.gd_mock = {'logger_config': self.logger_config_mock} self.geocoder_mock = Mock() @@ -144,4 +155,20 @@ class TestRunStreetPointGeocoder(TestCase): self.quota_service_mock.increment_empty_service_use. \ assert_called_once_with(len(SEARCH_FIXTURES['wrong'])) + def test_increment_mixed_error_service_use_on_complete_response(self): + searches = SEARCH_FIXTURES['two'] + SEARCH_FIXTURES['error'] + bulk_results = BULK_RESULTS_FIXTURES['two'] + BULK_RESULTS_FIXTURES['error'] + self.geocoder_mock.bulk_geocode = MagicMock(return_value=bulk_results) + result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, + self.geocoder_mock, + self.service_manager_mock, + 'any_username', None, searches) + + assert_equal(result, EXPECTED_RESULTS_FIXTURES['two'] + EXPECTED_RESULTS_FIXTURES['error']) + + self.quota_service_mock.increment_success_service_use. \ + assert_called_once_with(len(SEARCH_FIXTURES['two'])) + self.quota_service_mock.increment_failed_service_use. \ + assert_called_once_with(len(SEARCH_FIXTURES['error'])) + From 1ff512839d60114cb645c061d9e259022045ee70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 13:01:46 +0200 Subject: [PATCH 09/26] Fixes empty results count --- .../cartodb_services/geocoder.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 8d5d456..f23695f 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -24,7 +24,7 @@ def compose_address(street, city=None, state=None, country=None): return ', '.join(filter(None, [street, city, state, country])) -def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, orgname, searches): +def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, orgname, searches_string): plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") logger_config = GD["logger_config"] @@ -32,9 +32,15 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org success_count, failed_count = 0, 0 + try: + searches = json.loads(searches_string) + except Exception as e: + logger.error('Parsing searches', exception=e, data={'searches': searches_string}) + raise e + try: service_manager.assert_within_limits(quota=False) - geocode_results = geocoder.bulk_geocode(searches=searches) + geocode_results = geocoder.bulk_geocode(searches) results = [] if geocode_results: for result in geocode_results: @@ -94,20 +100,14 @@ class StreetPointBulkGeocoder: SEARCH_KEYS = ['id', 'address', 'city', 'state', 'country'] - def bulk_geocode(self, searches): + def bulk_geocode(self, decoded_searches): """ - :param searches: array of StreetGeocoderSearch + :param decoded_searches: array of StreetGeocoderSearch :return: array of tuples with three elements: * id * latitude and longitude (array of two elements) * empty array (future use: metadata) """ - try: - decoded_searches = json.loads(searches) - except Exception as e: - self._logger.error('General error', exception=e) - raise e - street_geocoder_searches = [] for search in decoded_searches: search_id, address, city, state, country = \ From c5d9db61e64686ad24a6c7d877dd2d4b169748e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 15:48:32 +0200 Subject: [PATCH 10/26] Mapbox error handling --- .../cartodb_services/geocoder.py | 18 +++++++-- .../cartodb_services/mapbox/bulk_geocoder.py | 4 +- .../cartodb_services/mapbox/geocoder.py | 37 ++++++++++++------- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index f23695f..cde5182 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -9,9 +9,6 @@ import json PRECISION_PRECISE = 'precise' PRECISION_INTERPOLATED = 'interpolated' -EMPTY_RESPONSE = [[], {}] - - def geocoder_metadata(relevance, precision, match_types): return { 'relevance': round(relevance, 2), @@ -20,6 +17,18 @@ def geocoder_metadata(relevance, precision, match_types): } +def geocoder_error_response(message): + return [[], {'error': message}] + + +# Single empty result +EMPTY_RESPONSE = [[], {}] +# HTTP 429 and related +TOO_MANY_REQUESTS_ERROR_RESPONSE = geocoder_error_response('Rate limit exceeded') +# Full empty _batch_geocode response +EMPTY_BATCH_RESPONSE = [] + + def compose_address(street, city=None, state=None, country=None): return ', '.join(filter(None, [street, city, state, country])) @@ -42,7 +51,7 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org service_manager.assert_within_limits(quota=False) geocode_results = geocoder.bulk_geocode(searches) results = [] - if geocode_results: + if not geocode_results == EMPTY_BATCH_RESPONSE: for result in geocode_results: if len(result) > 2: metadata = result[2] @@ -65,6 +74,7 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org results.append([result[0], None, json.dumps(metadata)]) empty_count = len(searches) - success_count - failed_count + logger.debug("--> Success: {}; empty: {}; failed: {}".format(success_count, empty_count, failed_count)) service_manager.quota_service.increment_success_service_use(success_count) service_manager.quota_service.increment_empty_service_use(empty_count) service_manager.quota_service.increment_failed_service_use(failed_count) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py index 2563d43..a939716 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py @@ -1,8 +1,6 @@ -import json, requests, time -from requests.adapters import HTTPAdapter +import requests from cartodb_services import StreetPointBulkGeocoder from cartodb_services.mapbox import MapboxGeocoder -from cartodb_services.tools.exceptions import ServiceException from iso3166 import countries from cartodb_services.tools.country import country_to_iso3 diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 2af2c6f..39d9da5 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -5,7 +5,7 @@ Python client for the Mapbox Geocoder service. import json import requests from mapbox import Geocoder -from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE, EMPTY_BATCH_RESPONSE, TOO_MANY_REQUESTS_ERROR_RESPONSE, geocoder_error_response from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -69,7 +69,7 @@ class MapboxGeocoder(Traceable): result.append(EMPTY_RESPONSE) return result else: - return EMPTY_RESPONSE + return EMPTY_BATCH_RESPONSE def _extract_lng_lat_from_feature(self, feature): geometry = feature[ENTRY_GEOMETRY] @@ -118,9 +118,17 @@ class MapboxGeocoder(Traceable): :param city: :param state_province: :param country: Country ISO 3166 code - :return: [x, y] on success, [] on error + :return: [x, y] on success, raises ServiceException on error """ - return self.geocode_meta(searchtext, city, state_province, country)[0] + response = self.geocode_meta(searchtext, city, state_province, country) + if response: + error_message = response[1].get('error', None) + if error_message: + raise ServiceException(error_message, None) + else: + return response[0] + else: + return EMPTY_RESPONSE @qps_retry(qps=10) def geocode_meta(self, searchtext, city=None, state_province=None, @@ -138,7 +146,8 @@ class MapboxGeocoder(Traceable): free_search = ', '.join(address) - return self.geocode_free_text_meta([free_search], country)[0] + response = self.geocode_free_text_meta([free_search], country) + return response[0] if response else EMPTY_RESPONSE @qps_retry(qps=10) def geocode_free_text_meta(self, free_searches, country=None): @@ -157,24 +166,26 @@ class MapboxGeocoder(Traceable): if response.status_code == requests.codes.ok: return self._parse_geocoder_response(response.text) + elif response.status_code == requests.codes.too_many_requests: + return [TOO_MANY_REQUESTS_ERROR_RESPONSE] * len(free_searches) elif response.status_code == requests.codes.bad_request: - return EMPTY_RESPONSE + return EMPTY_BATCH_RESPONSE elif response.status_code == requests.codes.unprocessable_entity: - return EMPTY_RESPONSE + return EMPTY_BATCH_RESPONSE else: - raise ServiceException(response.status_code, response) + msg = "Unkown status: {}".format(response.status_code) + return [geocoder_error_response(msg)] * len(free_searches) except requests.Timeout as te: # In case of timeout we want to stop the job because the server # could be down - self._logger.error('Timeout connecting to Mapbox geocoding server', - te) - raise ServiceException('Error geocoding {0} using Mapbox'.format( - free_search), None) + msg = 'Timeout connecting to Mapbox geocoding server' + self._logger.error(msg, te) + return [geocoder_error_response(msg)] * len(free_searches) except requests.ConnectionError as ce: # Don't raise the exception to continue with the geocoding job self._logger.error('Error connecting to Mapbox geocoding server', exception=ce) - return EMPTY_RESPONSE + return EMPTY_BATCH_RESPONSE def _escape(self, free_search): # Semicolon is used to separate batch geocoding; there's no documented From bcb34d1ceacc8ed8f2dd038cfcc63e07fcdaeddd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 15:57:32 +0200 Subject: [PATCH 11/26] Adjustments on street level fixtures --- test/integration/test_street_functions.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 2d02bdb..a36cb18 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -11,7 +11,7 @@ class TestStreetFunctionsSetUp(TestCase): fixture_points = None GOOGLE_POINTS = { - 'Plaza Mayor, Valladolid': [-4.728252, 41.6517025], + 'Plaza Mayor 1, Valladolid': [-4.728252, 41.6517025], 'Paseo Zorrilla, Valladolid': [-4.7404453, 41.6314339], '1900 amphitheatre parkway': [-122.0875324, 37.4227968], '1901 amphitheatre parkway': [-122.0885504, 37.4238657], @@ -26,7 +26,7 @@ class TestStreetFunctionsSetUp(TestCase): } HERE_POINTS = { - 'Plaza Mayor, Valladolid': [-4.72979, 41.65258], + 'Plaza Mayor 1, Valladolid': [-4.72979, 41.65258], 'Paseo Zorrilla, Valladolid': [-4.73869, 41.63817], '1900 amphitheatre parkway': [-122.0879468, 37.4234763], '1901 amphitheatre parkway': [-122.0879253, 37.4238725], @@ -42,13 +42,13 @@ class TestStreetFunctionsSetUp(TestCase): TOMTOM_POINTS = HERE_POINTS.copy() TOMTOM_POINTS.update({ - 'Plaza Mayor, Valladolid': [-4.72183, 41.5826], + 'Plaza Mayor 1, Valladolid': [-4.7286, 41.6523], 'Paseo Zorrilla, Valladolid': [-4.74031, 41.63181], 'Valladolid': [-4.72838, 41.6542], 'Valladolid, Spain': [-4.72838, 41.6542], 'Madrid': [-3.70035, 40.42028], 'Logroño, Spain': [-2.44998, 42.46592], - 'Plaza España, Barcelona': [2.1497, 41.37516] + 'Plaza España, Barcelona': [2.14856, 41.37516] }) MAPBOX_POINTS = GOOGLE_POINTS.copy() @@ -174,7 +174,7 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point(" \ "'select 1 as cartodb_id, ''Spain'' as country, " \ "''Castilla y León'' as state, ''Valladolid'' as city, " \ - "''Plaza Mayor'' as street " \ + "''Plaza Mayor 1'' as street " \ "UNION " \ "select 2 as cartodb_id, ''Spain'' as country, " \ "''Castilla y León'' as state, ''Valladolid'' as city, " \ @@ -183,7 +183,7 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): response = self._run_authenticated(query) points_by_cartodb_id = { - 1: self.fixture_points['Plaza Mayor, Valladolid'], + 1: self.fixture_points['Plaza Mayor 1, Valladolid'], 2: self.fixture_points['Paseo Zorrilla, Valladolid'] } self.assert_close_points(self._x_y_by_cartodb_id(response), points_by_cartodb_id) From 07f5be92076070d1ace035d4984b61afba375097 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 17:17:38 +0200 Subject: [PATCH 12/26] TomTom error handling --- .../cartodb_services/tomtom/bulk_geocoder.py | 13 ++++++++++-- .../cartodb_services/tomtom/geocoder.py | 20 ++++++++++++------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py index 8fc2e92..400cc2a 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py @@ -1,6 +1,7 @@ import json, requests, time from requests.adapters import HTTPAdapter from cartodb_services import StreetPointBulkGeocoder +from cartodb_services.geocoder import geocoder_error_response from cartodb_services.tomtom import TomTomGeocoder from cartodb_services.tools.exceptions import ServiceException @@ -43,13 +44,21 @@ class TomTomBulkGeocoder(TomTomGeocoder, StreetPointBulkGeocoder): return results def _batch_geocode(self, searches): - location = self._send_batch(searches) - full_results = self._download_results(location) + full_results = self._geocode_searches(searches) results = [] for s, r in zip(searches, full_results): results.append((s[0], r[0], r[1])) return results + def _geocode_searches(self, searches): + try: + location = self._send_batch(searches) + return self._download_results(location) + except Exception as e: + msg = "Error running TomTom batch geocode: {}".format(e) + self._logger.error(msg, e) + return [geocoder_error_response(msg)] * len(searches) + def _send_batch(self, searches): body = {'batchItems': [{'query': self._query(s)} for s in searches]} request_params = { diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py index 3f077bd..b5ec592 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py @@ -5,7 +5,7 @@ import json import requests from uritemplate import URITemplate from math import tanh -from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE, geocoder_error_response from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -73,7 +73,12 @@ class TomTomGeocoder(Traceable): @qps_retry(qps=5) def geocode(self, searchtext, city=None, state_province=None, country=None): - return self.geocode_meta(searchtext, city, state_province, country)[0] + response = self.geocode_meta(searchtext, city, state_province, country) + error_message = response[1].get('error', None) + if error_message: + raise ServiceException(error_message, None) + else: + return response[0] @qps_retry(qps=5) def geocode_meta(self, searchtext, city=None, state_province=None, @@ -106,10 +111,9 @@ class TomTomGeocoder(Traceable): except requests.Timeout as te: # In case of timeout we want to stop the job because the server # could be down - self._logger.error('Timeout connecting to TomTom geocoding server', - te) - raise ServiceException('Error geocoding {0} using TomTom'.format( - searchtext), None) + msg = 'Timeout connecting to TomTom geocoding server' + self._logger.error(msg, te) + return geocoder_error_response(msg) except requests.ConnectionError as ce: # Don't raise the exception to continue with the geocoding job self._logger.error('Error connecting to TomTom geocoding server', @@ -125,7 +129,9 @@ class TomTomGeocoder(Traceable): return EMPTY_RESPONSE else: msg = 'Unknown response {}: {}'.format(str(status_code), text) - raise ServiceException(msg, None) + self._logger.warning('Error parsing TomTom geocoding response', + data={'msg': msg}) + return geocoder_error_response(msg) def _parse_geocoder_response(self, response): json_response = json.loads(response) \ From 5d2303e1deb8d57d5bd2c3095182cba305cc479a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 17:23:08 +0200 Subject: [PATCH 13/26] Log a failed one if any --- .../python/cartodb_services/cartodb_services/geocoder.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index cde5182..c07ed08 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -51,6 +51,7 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org service_manager.assert_within_limits(quota=False) geocode_results = geocoder.bulk_geocode(searches) results = [] + a_failed_one = None if not geocode_results == EMPTY_BATCH_RESPONSE: for result in geocode_results: if len(result) > 2: @@ -64,6 +65,7 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org logger.warning('Geocoding error', data={"username": username, "orgname": orgname, "error": metadata['error']}) results.append([result[0], None, json.dumps(metadata)]) + a_failed_one = result failed_count += 1 elif result[1] and len(result[1]) == 2: plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326) as the_geom; ", ["double precision", "double precision"]) @@ -75,6 +77,13 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org empty_count = len(searches) - success_count - failed_count logger.debug("--> Success: {}; empty: {}; failed: {}".format(success_count, empty_count, failed_count)) + if a_failed_one: + logger.warning("failed geocoding", + data={ + "username": username, + "orgname": orgname, + "failed": str(a_failed_one) + }) service_manager.quota_service.increment_success_service_use(success_count) service_manager.quota_service.increment_empty_service_use(empty_count) service_manager.quota_service.increment_failed_service_use(failed_count) From faf9b7237b4936be14dec6435db88c92b9340c5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 17:36:06 +0200 Subject: [PATCH 14/26] Adjustments on street level fixtures --- test/integration/test_street_functions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index a36cb18..b7557c9 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -26,7 +26,7 @@ class TestStreetFunctionsSetUp(TestCase): } HERE_POINTS = { - 'Plaza Mayor 1, Valladolid': [-4.72979, 41.65258], + 'Plaza Mayor 1, Valladolid': [-4.729, 41.65258], 'Paseo Zorrilla, Valladolid': [-4.73869, 41.63817], '1900 amphitheatre parkway': [-122.0879468, 37.4234763], '1901 amphitheatre parkway': [-122.0879253, 37.4238725], From 1b31c089ce09655a1f0530e654f44cdae009cc84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 18:16:40 +0200 Subject: [PATCH 15/26] Global error handling for batched geocoding --- .../cartodb_services/geocoder.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index c07ed08..289fde1 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -121,7 +121,7 @@ class StreetPointBulkGeocoder: def bulk_geocode(self, decoded_searches): """ - :param decoded_searches: array of StreetGeocoderSearch + :param decoded_searches: JSON array :return: array of tuples with three elements: * id * latitude and longitude (array of two elements) @@ -136,10 +136,19 @@ class StreetPointBulkGeocoder: if len(street_geocoder_searches) > self.MAX_BATCH_SIZE: raise Exception("Batch size can't be larger than {}".format(self.MAX_BATCH_SIZE)) - if self._should_use_batch(street_geocoder_searches): - return self._batch_geocode(street_geocoder_searches) - else: - return self._serial_geocode(street_geocoder_searches) + try: + if self._should_use_batch(street_geocoder_searches): + return self._batch_geocode(street_geocoder_searches) + else: + return self._serial_geocode(street_geocoder_searches) + except Exception as e: + msg = "Error running geocode: {}".format(e) + self._logger.error(msg, e) + errors = [geocoder_error_response(msg)] * len(decoded_searches) + results = [] + for s, r in zip(decoded_searches, errors): + results.append((s['id'], r[0], r[1])) + return results def _batch_geocode(self, street_geocoder_searches): raise NotImplementedError('Subclasses must implement _batch_geocode') From 8162bff2041da4eb4ba0fb267116f4ff01960030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 18:27:02 +0200 Subject: [PATCH 16/26] Serial geocoding error handling --- .../cartodb_services/here/bulk_geocoder.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index 8316c44..772787c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -8,7 +8,7 @@ from collections import namedtuple from requests.adapters import HTTPAdapter from cartodb_services import StreetPointBulkGeocoder from cartodb_services.here import HereMapsGeocoder -from cartodb_services.geocoder import geocoder_metadata +from cartodb_services.geocoder import geocoder_metadata, geocoder_error_response from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException @@ -42,7 +42,11 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): results = [] for search in searches: (search_id, address, city, state, country) = search - result = self.geocode_meta(searchtext=address, city=city, state=state, country=country) + try: + result = self.geocode_meta(searchtext=address, city=city, state=state, country=country) + except Exception as e: + self._logger.error("Error geocoding", e) + result = geocoder_error_response("Error geocoding") results.append((search_id, result[0], result[1])) return results From 4be3aa88fda2a555981a5b8e325854ca5d777f51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 18:30:33 +0200 Subject: [PATCH 17/26] Constant extraction refactor --- .../cartodb_services/cartodb_services/here/geocoder.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index 23dd871..cbf2576 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -6,7 +6,7 @@ import requests from requests.adapters import HTTPAdapter from exceptions import * -from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE from cartodb_services.metrics import Traceable class HereMapsGeocoder(Traceable): @@ -90,7 +90,7 @@ class HereMapsGeocoder(Traceable): if value and value.strip(): params[key] = value if not params: - return [[], {}] + return EMPTY_RESPONSE return self._execute_geocode(params) def _execute_geocode(self, params): @@ -102,7 +102,7 @@ class HereMapsGeocoder(Traceable): return [self._extract_lng_lat_from_result(result), self._extract_metadata_from_result(result)] except IndexError: - return [[], {}] + return EMPTY_RESPONSE except KeyError as e: self._logger.error('params: {}'.format(params), e) raise MalformedResult() @@ -127,7 +127,7 @@ class HereMapsGeocoder(Traceable): self._logger.warning('Error 4xx trying to geocode street using HERE', data={"response": response.json(), "params": params}) - return [] + return EMPTY_RESPONSE else: self._logger.error('Error trying to geocode street using HERE', data={"response": response.json(), "params": From fc75f1afc8f326f49b0d15b0f6a9c961274d004d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 18:41:47 +0200 Subject: [PATCH 18/26] Google batch geocoder error handling --- .../cartodb_services/google/bulk_geocoder.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py index 0e1ab1f..27a52b6 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py @@ -1,7 +1,7 @@ from multiprocessing import Pool from exceptions import MalformedResult from cartodb_services import StreetPointBulkGeocoder -from cartodb_services.geocoder import compose_address +from cartodb_services.geocoder import compose_address, geocoder_error_response from cartodb_services.google import GoogleMapsGeocoder @@ -25,8 +25,13 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): results = [] for search in searches: (cartodb_id, street, city, state, country) = search - lng_lat, metadata = self.geocode_meta(street, city, state, country) - results.append((cartodb_id, lng_lat, metadata)) + try: + lng_lat, metadata = self.geocode_meta(street, city, state, country) + result = (cartodb_id, lng_lat, metadata) + except Exception as e: + self._logger.error("Error geocoding", e) + result = geocoder_error_response("Error geocoding") + results.append(result) return results def _batch_geocode(self, searches): @@ -49,14 +54,12 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): try: lng_lat, metadata = self._process_results(bulk_result.get()) except Exception as e: - self._logger.error('Error at Google async_geocoder', e) - lng_lat, metadata = [[], {}] + msg = 'Error at Google async_geocoder' + self._logger.error(msg, e) + lng_lat, metadata = geocoder_error_response(msg) results.append((cartodb_id, lng_lat, metadata)) return results - except KeyError as e: - self._logger.error('KeyError error', exception=e) - raise MalformedResult() except Exception as e: self._logger.error('General error', exception=e) raise e From 3a5360c96cdb3929756921a9cde1819062bbaf78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 19:10:35 +0200 Subject: [PATCH 19/26] Refactor and fix for actual searches type --- .../test/test_cartodb_services_geocoder.py | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py index 6dd199c..a58d5c2 100644 --- a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py +++ b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py @@ -1,6 +1,7 @@ #!/usr/local/bin/python # -*- coding: utf-8 -*- +import json from unittest import TestCase from mock import Mock, MagicMock from nose.tools import assert_not_equal, assert_equal, assert_true @@ -53,6 +54,17 @@ EXPECTED_RESULTS_FIXTURES = { class TestRunStreetPointGeocoder(TestCase): + def _run_geocoder(self, plpy=None, gd=None, geocoder=None, + service_manager=None, username=None, orgname=None, + searches=None): + return run_street_point_geocoder( + plpy if plpy else self.plpy_mock, + gd if gd else self.gd_mock, + geocoder if geocoder else self.geocoder_mock, + service_manager if service_manager else self.service_manager_mock, + username if username else 'any_username', + orgname if orgname else None, + json.dumps(searches) if searches else '[]') def setUp(self): point = [0,0] @@ -79,10 +91,8 @@ class TestRunStreetPointGeocoder(TestCase): searches = [] with(self.assertRaises(BaseException)): - run_street_point_geocoder(self.plpy_mock, self.gd_mock, - self.geocoder_mock, - self.service_manager_mock, - 'any_username', None, searches) + self._run_geocoder(service_manager=self.service_manager_mock, + searches=searches) self.quota_service_mock.increment_total_service_use. \ assert_called_once_with(len(searches)) @@ -96,10 +106,8 @@ class TestRunStreetPointGeocoder(TestCase): searches = SEARCH_FIXTURES['two'] - result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, - self.geocoder_mock, - self.service_manager_mock, - 'any_username', None, searches) + result = self._run_geocoder(service_manager=self.service_manager_mock, + searches=searches) assert_equal(result, []) self.quota_service_mock.increment_failed_service_use. \ @@ -116,10 +124,8 @@ class TestRunStreetPointGeocoder(TestCase): [2, [0, 0], '{}'], ] self.geocoder_mock.bulk_geocode = MagicMock(return_value=results) - result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, - self.geocoder_mock, - self.service_manager_mock, - 'any_username', None, searches) + result = self._run_geocoder(geocoder=self.geocoder_mock, + searches=searches) assert_equal(result, expected_results) self.quota_service_mock.increment_success_service_use. \ @@ -129,10 +135,8 @@ class TestRunStreetPointGeocoder(TestCase): searches = SEARCH_FIXTURES['two'] results = [] self.geocoder_mock.bulk_geocode = MagicMock(return_value=results) - result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, - self.geocoder_mock, - self.service_manager_mock, - 'any_username', None, searches) + result = self._run_geocoder(geocoder=self.geocoder_mock, + searches=searches) assert_equal(result, results) @@ -143,10 +147,8 @@ class TestRunStreetPointGeocoder(TestCase): searches = SEARCH_FIXTURES['two'] + SEARCH_FIXTURES['wrong'] bulk_results = BULK_RESULTS_FIXTURES['two'] + BULK_RESULTS_FIXTURES['wrong'] self.geocoder_mock.bulk_geocode = MagicMock(return_value=bulk_results) - result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, - self.geocoder_mock, - self.service_manager_mock, - 'any_username', None, searches) + result = self._run_geocoder(geocoder=self.geocoder_mock, + searches=searches) assert_equal(result, EXPECTED_RESULTS_FIXTURES['two'] + EXPECTED_RESULTS_FIXTURES['wrong']) @@ -159,10 +161,8 @@ class TestRunStreetPointGeocoder(TestCase): searches = SEARCH_FIXTURES['two'] + SEARCH_FIXTURES['error'] bulk_results = BULK_RESULTS_FIXTURES['two'] + BULK_RESULTS_FIXTURES['error'] self.geocoder_mock.bulk_geocode = MagicMock(return_value=bulk_results) - result = run_street_point_geocoder(self.plpy_mock, self.gd_mock, - self.geocoder_mock, - self.service_manager_mock, - 'any_username', None, searches) + result = self._run_geocoder(geocoder=self.geocoder_mock, + searches=searches) assert_equal(result, EXPECTED_RESULTS_FIXTURES['two'] + EXPECTED_RESULTS_FIXTURES['error']) From d060ab3d4117a582f6d58cb3a6feb2a450638bca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 19:11:23 +0200 Subject: [PATCH 20/26] Empty vs missing count detail --- .../cartodb_services/geocoder.py | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 289fde1..03ad014 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -39,7 +39,7 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org logger = Logger(logger_config) - success_count, failed_count = 0, 0 + success_count, failed_count, empty_count = 0, 0, 0 try: searches = json.loads(searches_string) @@ -54,16 +54,9 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org a_failed_one = None if not geocode_results == EMPTY_BATCH_RESPONSE: for result in geocode_results: - if len(result) > 2: - metadata = result[2] - else: - logger.warning('Geocoding without metadata', - data={"username": username, "orgname": orgname}) - metadata = {} + metadata = result[2] if len(result) > 2 else {} if metadata.get('error', None): - logger.warning('Geocoding error', - data={"username": username, "orgname": orgname, "error": metadata['error']}) results.append([result[0], None, json.dumps(metadata)]) a_failed_one = result failed_count += 1 @@ -74,18 +67,25 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org success_count += 1 else: results.append([result[0], None, json.dumps(metadata)]) + empty_count += 1 - empty_count = len(searches) - success_count - failed_count - logger.debug("--> Success: {}; empty: {}; failed: {}".format(success_count, empty_count, failed_count)) + missing_count = len(searches) - success_count - failed_count - empty_count + + logger.debug("--> Success: {}; empty: {}; missing: {}, failed: {}". + format(success_count, empty_count, missing_count, failed_count)) if a_failed_one: logger.warning("failed geocoding", data={ "username": username, "orgname": orgname, - "failed": str(a_failed_one) + "failed": str(a_failed_one), + "success_count": success_count, + "empty_count": empty_count, + "missing_count": missing_count, + "failed_count": failed_count }) service_manager.quota_service.increment_success_service_use(success_count) - service_manager.quota_service.increment_empty_service_use(empty_count) + service_manager.quota_service.increment_empty_service_use(empty_count + missing_count) service_manager.quota_service.increment_failed_service_use(failed_count) return results From fa3d7db5f89077849023275d3c1832a5916f59b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 21:52:54 +0200 Subject: [PATCH 21/26] Fix Google geocoder error handling --- .../cartodb_services/google/bulk_geocoder.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py index 27a52b6..cde9f1c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py @@ -27,11 +27,10 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): (cartodb_id, street, city, state, country) = search try: lng_lat, metadata = self.geocode_meta(street, city, state, country) - result = (cartodb_id, lng_lat, metadata) except Exception as e: self._logger.error("Error geocoding", e) - result = geocoder_error_response("Error geocoding") - results.append(result) + lng_lat, metadata = geocoder_error_response("Error geocoding") + results.append((cartodb_id, lng_lat, metadata)) return results def _batch_geocode(self, searches): From 80dcde2db09718e6caa05b75ca4ba4df35fc6c17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 22:00:16 +0200 Subject: [PATCH 22/26] Log Mapbox unknown status --- .../python/cartodb_services/cartodb_services/mapbox/geocoder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 39d9da5..f50eff1 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -163,7 +163,6 @@ class MapboxGeocoder(Traceable): response = self._geocoder.forward(address=free_search.decode('utf-8'), limit=1, country=country) - if response.status_code == requests.codes.ok: return self._parse_geocoder_response(response.text) elif response.status_code == requests.codes.too_many_requests: @@ -174,6 +173,7 @@ class MapboxGeocoder(Traceable): return EMPTY_BATCH_RESPONSE else: msg = "Unkown status: {}".format(response.status_code) + self._logger.warning(msg, data={"searches": free_searches}) return [geocoder_error_response(msg)] * len(free_searches) except requests.Timeout as te: # In case of timeout we want to stop the job because the server From 3524ee1e241afb2da7c21b0804b87683a722fbe1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 24 Jul 2018 11:31:05 +0200 Subject: [PATCH 23/26] Handle postprocessing error --- .../cartodb_services/geocoder.py | 28 ++++++---- .../test/test_cartodb_services_geocoder.py | 55 ++++++++++++++++--- 2 files changed, 63 insertions(+), 20 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 03ad014..66dffc7 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -55,19 +55,25 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org if not geocode_results == EMPTY_BATCH_RESPONSE: for result in geocode_results: metadata = result[2] if len(result) > 2 else {} - - if metadata.get('error', None): + try: + if metadata.get('error', None): + results.append([result[0], None, json.dumps(metadata)]) + a_failed_one = result + failed_count += 1 + elif result[1] and len(result[1]) == 2: + plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326) as the_geom; ", ["double precision", "double precision"]) + point = plpy.execute(plan, result[1], 1)[0] + results.append([result[0], point['the_geom'], json.dumps(metadata)]) + success_count += 1 + else: + results.append([result[0], None, json.dumps(metadata)]) + empty_count += 1 + except Exception as e: + import sys + logger.error("Error processing geocode", sys.exc_info(), data={"username": username, "orgname": orgname}) + metadata['processing_error'] = 'Error: {}'.format(e.message) results.append([result[0], None, json.dumps(metadata)]) - a_failed_one = result failed_count += 1 - elif result[1] and len(result[1]) == 2: - plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326) as the_geom; ", ["double precision", "double precision"]) - point = plpy.execute(plan, result[1], 1)[0] - results.append([result[0], point['the_geom'], json.dumps(metadata)]) - success_count += 1 - else: - results.append([result[0], None, json.dumps(metadata)]) - empty_count += 1 missing_count = len(searches) - success_count - failed_count - empty_count diff --git a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py index a58d5c2..ad36f72 100644 --- a/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py +++ b/server/lib/python/cartodb_services/test/test_cartodb_services_geocoder.py @@ -23,7 +23,15 @@ SEARCH_FIXTURES = { 'error': [ StreetGeocoderSearch(id=200, address=None, city=None, state=None, country=None), - ] + ], + 'broken_middle': [ + StreetGeocoderSearch(id=301, address='Paseo Zorrilla 1, Valladolid', + city=None, state=None, country=None), + StreetGeocoderSearch(id=302, address='Marsopolis', + city=None, state=None, country=None), + StreetGeocoderSearch(id=303, address='Paseo Zorrilla 2, Valladolid', + city=None, state=None, country=None) + ], } BULK_RESULTS_FIXTURES = { @@ -36,6 +44,11 @@ BULK_RESULTS_FIXTURES = { ], 'error': [ (200, [], {'error': 'Something wrong happened'}) + ], + 'broken_middle': [ + (301, [0, 0], {}), + (302, ['a', 'b'], {}), + (303, [0, 0], {}), ] } @@ -49,6 +62,11 @@ EXPECTED_RESULTS_FIXTURES = { ], 'error': [ [200, None, '{"error": "Something wrong happened"}'] + ], + 'broken_middle': [ + [301, [0, 0], '{}'], + [302, None, '{"processing_error": "Error: NO!"}'], + [303, [0, 0], '{}'], ] } @@ -87,7 +105,6 @@ class TestRunStreetPointGeocoder(TestCase): def test_count_increment_total_and_failed_service_use_on_error(self): self.service_manager_mock.assert_within_limits = \ Mock(side_effect=Exception('Fail!')) - searches = [] with(self.assertRaises(BaseException)): @@ -96,20 +113,17 @@ class TestRunStreetPointGeocoder(TestCase): self.quota_service_mock.increment_total_service_use. \ assert_called_once_with(len(searches)) - self.quota_service_mock.increment_failed_service_use. \ assert_called_once_with(len(searches)) def test_count_increment_failed_service_use_on_quota_error(self): self.service_manager_mock.assert_within_limits = \ Mock(side_effect=QuotaExceededException()) - searches = SEARCH_FIXTURES['two'] result = self._run_geocoder(service_manager=self.service_manager_mock, searches=searches) assert_equal(result, []) - self.quota_service_mock.increment_failed_service_use. \ assert_called_once_with(len(searches)) @@ -124,10 +138,10 @@ class TestRunStreetPointGeocoder(TestCase): [2, [0, 0], '{}'], ] self.geocoder_mock.bulk_geocode = MagicMock(return_value=results) + result = self._run_geocoder(geocoder=self.geocoder_mock, searches=searches) assert_equal(result, expected_results) - self.quota_service_mock.increment_success_service_use. \ assert_called_once_with(len(results)) @@ -135,11 +149,11 @@ class TestRunStreetPointGeocoder(TestCase): searches = SEARCH_FIXTURES['two'] results = [] self.geocoder_mock.bulk_geocode = MagicMock(return_value=results) + result = self._run_geocoder(geocoder=self.geocoder_mock, searches=searches) assert_equal(result, results) - self.quota_service_mock.increment_empty_service_use. \ assert_called_once_with(len(searches)) @@ -147,11 +161,11 @@ class TestRunStreetPointGeocoder(TestCase): searches = SEARCH_FIXTURES['two'] + SEARCH_FIXTURES['wrong'] bulk_results = BULK_RESULTS_FIXTURES['two'] + BULK_RESULTS_FIXTURES['wrong'] self.geocoder_mock.bulk_geocode = MagicMock(return_value=bulk_results) + result = self._run_geocoder(geocoder=self.geocoder_mock, searches=searches) assert_equal(result, EXPECTED_RESULTS_FIXTURES['two'] + EXPECTED_RESULTS_FIXTURES['wrong']) - self.quota_service_mock.increment_success_service_use. \ assert_called_once_with(len(SEARCH_FIXTURES['two'])) self.quota_service_mock.increment_empty_service_use. \ @@ -161,14 +175,37 @@ class TestRunStreetPointGeocoder(TestCase): searches = SEARCH_FIXTURES['two'] + SEARCH_FIXTURES['error'] bulk_results = BULK_RESULTS_FIXTURES['two'] + BULK_RESULTS_FIXTURES['error'] self.geocoder_mock.bulk_geocode = MagicMock(return_value=bulk_results) + result = self._run_geocoder(geocoder=self.geocoder_mock, searches=searches) assert_equal(result, EXPECTED_RESULTS_FIXTURES['two'] + EXPECTED_RESULTS_FIXTURES['error']) - self.quota_service_mock.increment_success_service_use. \ assert_called_once_with(len(SEARCH_FIXTURES['two'])) self.quota_service_mock.increment_failed_service_use. \ assert_called_once_with(len(SEARCH_FIXTURES['error'])) + def test_controlled_failure_on_query_break(self): + searches = SEARCH_FIXTURES['broken_middle'] + bulk_results = BULK_RESULTS_FIXTURES['broken_middle'] + self.geocoder_mock.bulk_geocode = MagicMock(return_value=bulk_results) + def break_on_302(*args): + if len(args) == 3: + plan, values, limit = args + if values[0] == 'a': + raise Exception('NO!') + + return [{'the_geom': [0,0]}] + self.plpy_mock.execute = break_on_302 + + result = self._run_geocoder(geocoder=self.geocoder_mock, + searches=searches) + + assert_equal(result, EXPECTED_RESULTS_FIXTURES['broken_middle']) + self.quota_service_mock.increment_success_service_use. \ + assert_called_once_with(2) + self.quota_service_mock.increment_failed_service_use. \ + assert_called_once_with(1) + + From c6720bf689cee021dffe418b222396cfbf2798e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 24 Jul 2018 11:57:54 +0200 Subject: [PATCH 24/26] Better debug message --- .../cartodb_services/cartodb_services/geocoder.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 66dffc7..a2c250c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -77,8 +77,6 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org missing_count = len(searches) - success_count - failed_count - empty_count - logger.debug("--> Success: {}; empty: {}; missing: {}, failed: {}". - format(success_count, empty_count, missing_count, failed_count)) if a_failed_one: logger.warning("failed geocoding", data={ @@ -90,6 +88,16 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org "missing_count": missing_count, "failed_count": failed_count }) + else: + logger.debug("finished geocoding", + data={ + "username": username, + "orgname": orgname, + "success_count": success_count, + "empty_count": empty_count, + "missing_count": missing_count, + "failed_count": failed_count + }) service_manager.quota_service.increment_success_service_use(success_count) service_manager.quota_service.increment_empty_service_use(empty_count + missing_count) service_manager.quota_service.increment_failed_service_use(failed_count) From 11ec6075c3fae060017086f0b37023b6f0a022d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 24 Jul 2018 11:59:29 +0200 Subject: [PATCH 25/26] Python library version 0.19.1 --- NEWS.md | 2 +- server/lib/python/cartodb_services/setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index 9883417..4571e01 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,6 @@ Jul 19th, 2018 ============== -* Version `0.25.0` of the client, `0.32.0` of the server, and `0.19.0` of the Python library. +* Version `0.25.0` of the client, `0.32.0` of the server, and `0.19.1` of the Python library. * Support for batch street-level geocoding. May 7th, 2018 diff --git a/server/lib/python/cartodb_services/setup.py b/server/lib/python/cartodb_services/setup.py index 3ea820e..7699f06 100644 --- a/server/lib/python/cartodb_services/setup.py +++ b/server/lib/python/cartodb_services/setup.py @@ -10,7 +10,7 @@ from setuptools import setup, find_packages setup( name='cartodb_services', - version='0.19.0', + version='0.19.1', description='CartoDB Services API Python Library', From 075f602a7f3ed5bd5fa76407c8d89afffca65a3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 25 Jul 2018 11:22:31 +0200 Subject: [PATCH 26/26] WELL_KNOWN_SHAPE and WELL_KNOWN_LENGTH fixture update --- .../test/test_mapboxrouting.py | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/server/lib/python/cartodb_services/test/test_mapboxrouting.py b/server/lib/python/cartodb_services/test/test_mapboxrouting.py index 160b2a5..2101535 100644 --- a/server/lib/python/cartodb_services/test/test_mapboxrouting.py +++ b/server/lib/python/cartodb_services/test/test_mapboxrouting.py @@ -17,17 +17,16 @@ VALID_PROFILE = DEFAULT_PROFILE INVALID_PROFILE = 'invalid_profile' WELL_KNOWN_SHAPE = [(40.73312, -73.98891), (40.73353, -73.98987), - (40.73398, -73.99095), (40.73453, -73.99227), - (40.73531, -73.99412), (40.73467, -73.99459), - (40.73442, -73.99477), (40.73435, -73.99482), - (40.73403, -73.99505), (40.73344, -73.99549), - (40.73286, -73.9959), (40.73226, -73.99635), - (40.73186, -73.99664), (40.73147, -73.99693), - (40.73141, -73.99698), (40.73147, -73.99707), - (40.73219, -73.99856), (40.73222, -73.99861), - (40.73225, -73.99868), (40.73293, -74.00007), - (40.733, -74.00001)] -WELL_KNOWN_LENGTH = 1317.9 + (40.73398, -73.99095), (40.73321, -73.99111), + (40.73245, -73.99129), (40.7333, -73.99332), + (40.7338, -73.99449), (40.73403, -73.99505), + (40.73344, -73.99549), (40.73286, -73.9959), + (40.73226, -73.99635), (40.73186, -73.99664), + (40.73147, -73.99693), (40.73141, -73.99698), + (40.73147, -73.99707), (40.73219, -73.99856), + (40.73222, -73.99861), (40.73225, -73.99868), + (40.73293, -74.00007), (40.733, -74.00001)] +WELL_KNOWN_LENGTH = 1384.8 class MapboxRoutingTestCase(unittest.TestCase):