Repository: openaddresses/machine
Pull request #461 (2016-11-26 23:18:33)

Categorize source failures
Adds a "source problem" field to run states, which will later make it possible to display meaningful output in run tables.

Part of #460.


Patch:
diff --git a/openaddr/ci/objects.py b/openaddr/ci/objects.py
index 99ba179..d50d486 100644
--- a/openaddr/ci/objects.py
+++ b/openaddr/ci/objects.py
@@ -3,6 +3,13 @@ import logging; _L = logging.getLogger('openaddr.ci.objects')
 from .. import __version__
 import json, pickle
 
+# Todo: make this a Python 3 enum
+FAIL_REASONS = {
+    None, 'Source says to skip', 'Source is missing a conform object',
+    'Unknown source conform type', 'Could not download source data',
+    'Could not conform source data', 'Missing or incomplete coverage'
+    }
+
 class Job:
     '''
     '''
@@ -78,7 +85,7 @@ class RunState:
         'address count', 'version', 'fingerprint', 'cache time', 'processed',
         'output', 'process time', 'website', 'skipped', 'license',
         'share-alike', 'attribution required', 'attribution name',
-        'attribution flag', 'process hash', 'preview')}
+        'attribution flag', 'process hash', 'preview', 'source problem')}
 
     def __init__(self, json_blob):
         blob_dict = dict(json_blob or {})
@@ -104,9 +111,12 @@ class RunState:
         self.attribution_required = blob_dict.get('attribution required')
         self.attribution_name = blob_dict.get('attribution name')
         self.attribution_flag = blob_dict.get('attribution flag')
+        self.source_problem = blob_dict.get('source problem')
 
         unexpected = ', '.join(set(self.keys) - set(RunState.key_attrs.keys()))
         assert len(unexpected) == 0, 'RunState should not have keys {}'.format(unexpected)
+        
+        assert self.source_problem in FAIL_REASONS, 'Uknown failure reason {}'.format(repr(self.source_problem))
     
     def get(self, json_key):
         if json_key == 'code version':
diff --git a/openaddr/conform.py b/openaddr/conform.py
index 4856b05..192f55c 100644
--- a/openaddr/conform.py
+++ b/openaddr/conform.py
@@ -395,8 +395,14 @@ def guess_source_encoding(datasource, layer):
         or getpreferredencoding()
 
 def find_source_path(source_definition, source_paths):
-    "Figure out which of the possible paths is the actual source"
-    conform = source_definition["conform"]
+    ''' Figure out which of the possible paths is the actual source
+    '''
+    try:
+        conform = source_definition["conform"]
+    except KeyError:
+        _L.warning('Source is missing a conform object')
+        raise
+
     if conform["type"] in ("shapefile", "shapefile-polygon"):
         # TODO this code is too complicated; see XML variant below for simpler option
         # Shapefiles are named *.shp
@@ -493,7 +499,7 @@ def find_source_path(source_definition, source_paths):
             _L.warning("Could not find a .gml file")
             return None
     else:
-        _L.warning("Unknown source type %s", conform["type"])
+        _L.warning("Unknown source conform type %s", conform["type"])
         return None
 
 class ConvertToCsvTask(object):
diff --git a/openaddr/process_one.py b/openaddr/process_one.py
index 4a68e2f..1956bc9 100644
--- a/openaddr/process_one.py
+++ b/openaddr/process_one.py
@@ -13,6 +13,7 @@ import tempfile, json, csv, sys
 
 from . import cache, conform, preview, CacheResult, ConformResult
 from .compat import csvopen, csvwriter, PY2
+from .cache import DownloadError
 
 class SourceSaysSkip(RuntimeError): pass
 
@@ -51,7 +52,11 @@ def process(source, destination, do_preview, mapzen_key=None, extras=dict()):
                 raise SourceSaysSkip()
     
         # Cache source data.
-        cache_result = cache(temp_src, temp_dir, extras)
+        try:
+            cache_result = cache(temp_src, temp_dir, extras)
+        except DownloadError as e:
+            _L.warning('Could not download source data')
+            raise
     
         if not cache_result.cache:
             _L.warning('Nothing cached')
@@ -125,6 +130,32 @@ def get_log_handler(directory):
     
     return handler
 
+def find_source_problem(log_contents, source):
+    '''
+    '''
+    if 'WARNING: Source is missing a conform object' in log_contents:
+        return 'Source is missing a conform object'
+    
+    if 'WARNING: Unknown source conform type' in log_contents:
+        return 'Unknown source conform type'
+    
+    if 'WARNING: Could not download source data' in log_contents:
+        return 'Could not download source data'
+    
+    if 'WARNING: Error doing conform; skipping' in log_contents:
+        return 'Could not conform source data'
+    
+    if 'coverage' in source:
+        coverage = source.get('coverage')
+        if 'US Census' in coverage or 'ISO 3166' in coverage or 'geometry' in coverage:
+            pass
+        else:
+            return 'Missing or incomplete coverage'
+    else:
+        return 'Missing or incomplete coverage'
+    
+    return None
+
 def write_state(source, skipped, destination, log_handler, cache_result,
                 conform_result, preview_path, temp_dir):
     '''
@@ -165,6 +196,18 @@ def write_state(source, skipped, destination, log_handler, cache_result,
     output_path = join(statedir, 'output.txt')
     copy(log_handler.stream.name, output_path)
 
+    if skipped:
+        source_problem = 'Source says to skip'
+    else:
+        with open(output_path) as file:
+            log_content = file.read()
+        if exists(source):
+            with open(source) as file:
+                source_data = json.load(file)
+        else:
+            source_data = {}
+        source_problem = find_source_problem(log_content, source_data)
+
     state = [
         ('source', basename(source)),
         ('skipped', bool(skipped)),
@@ -184,6 +227,7 @@ def write_state(source, skipped, destination, log_handler, cache_result,
         ('attribution required', boolstr(conform_result.attribution_flag)),
         ('attribution name', conform_result.attribution_name),
         ('share-alike', boolstr(conform_result.sharealike_flag)),
+        ('source problem', source_problem),
         ]
                
     with csvopen(join(statedir, 'index.txt'), 'w', encoding='utf8') as file:


Test patch:
diff --git a/openaddr/tests/__init__.py b/openaddr/tests/__init__.py
index a127f3c..5471ea8 100644
--- a/openaddr/tests/__init__.py
+++ b/openaddr/tests/__init__.py
@@ -54,6 +54,7 @@ from ..util import package_output
 from ..ci.objects import Run, RunState
 from ..cache import CacheResult
 from ..conform import ConformResult
+from ..process_one import find_source_problem
 
 class TestOA (unittest.TestCase):
     
@@ -503,17 +504,18 @@ class TestOA (unittest.TestCase):
             state_path = process_one.process(source, self.testdir, False)
         
         with open(state_path) as file:
-            state = dict(zip(*json.load(file)))
+            state = RunState(dict(zip(*json.load(file))))
         
-        self.assertFalse(state['skipped'])
-        self.assertIsNotNone(state['cache'])
+        self.assertFalse(state.skipped)
+        self.assertIsNotNone(state.cache)
         # This test data does not contain a working conform object
-        self.assertIsNone(state['processed'])
-        self.assertIsNone(state['preview'])
-        self.assertEqual(state['website'], 'http://data.openoakland.org/dataset/property-parcels/resource/df20b818-0d16-4da8-a9c1-a7b8b720ff49')
-        self.assertIsNone(state['license'])
+        self.assertEqual(state.source_problem, 'Unknown source conform type')
+        self.assertIsNone(state.processed)
+        self.assertIsNone(state.preview)
+        self.assertEqual(state.website, 'http://data.openoakland.org/dataset/property-parcels/resource/df20b818-0d16-4da8-a9c1-a7b8b720ff49')
+        self.assertIsNone(state.license)
         
-        with open(join(dirname(state_path), state['sample'])) as file:
+        with open(join(dirname(state_path), state.sample)) as file:
             sample_data = json.load(file)
         
         self.assertTrue('FID_PARCEL' in sample_data[0])
@@ -527,13 +529,14 @@ class TestOA (unittest.TestCase):
             state_path = process_one.process(source, self.testdir, False)
         
         with open(state_path) as file:
-            state = dict(zip(*json.load(file)))
+            state = RunState(dict(zip(*json.load(file))))
         
         # This test data says "skip": True
-        self.assertTrue(state['skipped'])
-        self.assertIsNone(state['cache'])
-        self.assertIsNone(state['processed'])
-        self.assertIsNone(state['preview'])
+        self.assertEqual(state.source_problem, 'Source says to skip')
+        self.assertTrue(state.skipped)
+        self.assertIsNone(state.cache)
+        self.assertIsNone(state.processed)
+        self.assertIsNone(state.preview)
 
     def test_single_berk(self):
         ''' Test complete process_one.process on Berkeley sample data.
@@ -544,16 +547,17 @@ class TestOA (unittest.TestCase):
             state_path = process_one.process(source, self.testdir, False)
         
         with open(state_path) as file:
-            state = dict(zip(*json.load(file)))
+            state = RunState(dict(zip(*json.load(file))))
         
-        self.assertIsNotNone(state['cache'])
+        self.assertIsNotNone(state.cache)
         # This test data does not contain a conform object at all
-        self.assertIsNone(state['processed'])
-        self.assertIsNone(state['preview'])
-        self.assertEqual(state['website'], 'http://www.ci.berkeley.ca.us/datacatalog/')
-        self.assertIsNone(state['license'])
+        self.assertEqual(state.source_problem, 'Source is missing a conform object')
+        self.assertIsNone(state.processed)
+        self.assertIsNone(state.preview)
+        self.assertEqual(state.website, 'http://www.ci.berkeley.ca.us/datacatalog/')
+        self.assertIsNone(state.license)
         
-        with open(join(dirname(state_path), state['sample'])) as file:
+        with open(join(dirname(state_path), state.sample)) as file:
             sample_data = json.load(file)
         
         self.assertTrue('APN' in sample_data[0])
@@ -567,11 +571,12 @@ class TestOA (unittest.TestCase):
             state_path = process_one.process(source, self.testdir, False)
         
         with open(state_path) as file:
-            state = dict(zip(*json.load(file)))
+            state = RunState(dict(zip(*json.load(file))))
         
-        self.assertIsNone(state['cache'])
-        self.assertIsNone(state['processed'])
-        self.assertIsNone(state['preview'])
+        self.assertEqual(state.source_problem, 'Could not download source data')
+        self.assertIsNone(state.cache)
+        self.assertIsNone(state.processed)
+        self.assertIsNone(state.preview)
         
     def test_single_berk_apn(self):
         ''' Test complete process_one.process on Berkeley sample data.
@@ -688,16 +693,18 @@ class TestOA (unittest.TestCase):
             state_path = process_one.process(source, self.testdir, False)
         
         with open(state_path) as file:
-            state = dict(zip(*json.load(file)))
-
-        self.assertIsNotNone(state['sample'])
-        self.assertIsNone(state['preview'])
-        self.assertEqual(state['website'], 'http://nlftp.mlit.go.jp/isj/index.html')
-        self.assertEqual(state['license'], u'http://nlftp.mlit.go.jp/ksj/other/yakkan§.html')
-        self.assertEqual(state['attribution required'], 'true')
-        self.assertIn('Ministry of Land', state['attribution name'])
-        
-        with open(join(dirname(state_path), state['sample'])) as file:
+            state = RunState(dict(zip(*json.load(file))))
+
+        self.assertIsNotNone(state.sample)
+        self.assertEqual(state.source_problem, 'Could not conform source data')
+        self.assertIsNone(state.processed)
+        self.assertIsNone(state.preview)
+        self.assertEqual(state.website, 'http://nlftp.mlit.go.jp/isj/index.html')
+        self.assertEqual(state.license, u'http://nlftp.mlit.go.jp/ksj/other/yakkan§.html')
+        self.assertEqual(state.attribution_required, 'true')
+        self.assertIn('Ministry of Land', state.attribution_name)
+        
+        with open(join(dirname(state_path), state.sample)) as file:
             sample_data = json.load(file)
         
         self.assertEqual(len(sample_data), 6)
@@ -1027,9 +1034,11 @@ class TestOA (unittest.TestCase):
             csv.field_size_limit(ofs)
 
         with open(state_path) as file:
-            state = dict(zip(*json.load(file)))
+            state = RunState(dict(zip(*json.load(file))))
 
-        self.assertIsNone(state['sample'], 'Sample should be missing when csv.field_size_limit() is too short')
+        self.assertIsNone(state.sample, 'Sample should be missing when csv.field_size_limit() is too short')
+        self.assertEqual(state.source_problem, 'Could not conform source data')
+        self.assertIsNone(state.processed)
 
         source = join(self.src_dir, 'us/tx/city_of_waco.json')
 
@@ -1040,12 +1049,14 @@ class TestOA (unittest.TestCase):
             csv.field_size_limit(ofs)
 
         with open(state_path) as file:
-            state = dict(zip(*json.load(file)))
+            state = RunState(dict(zip(*json.load(file))))
 
-        self.assertIsNotNone(state['sample'], 'Sample should be present when csv.field_size_limit() is long enough')
-        self.assertIsNone(state['preview'])
+        self.assertIsNotNone(state.sample, 'Sample should be present when csv.field_size_limit() is long enough')
+        self.assertIsNone(state.source_problem)
+        self.assertIsNotNone(state.processed)
+        self.assertIsNone(state.preview)
 
-        output_path = join(dirname(state_path), state['processed'])
+        output_path = join(dirname(state_path), state.processed)
         
         with csvopen(output_path, encoding='utf8') as input:
             rows = list(csvDictReader(input, encoding='utf8'))
@@ -1356,6 +1367,19 @@ class TestState (unittest.TestCase):
         self.assertEqual(state2['source'], 'bar.json')
         self.assertEqual(state2['skipped'], True)
         self.assertEqual(state2['attribution required'], 'false')
+    
+    def test_find_source_problem(self):
+        '''
+        '''
+        self.assertIsNone(RunState({'source problem': find_source_problem('', {'coverage': {'US Census': None}})}).source_problem)
+        self.assertIsNone(RunState({'source problem': find_source_problem('', {'coverage': {'US Census': None}})}).source_problem)
+        self.assertIsNone(RunState({'source problem': find_source_problem('', {'coverage': {'ISO 3166': None}})}).source_problem)
+
+        self.assertEqual(RunState({'source problem': find_source_problem('', {})}).source_problem, 'Missing or incomplete coverage')
+        self.assertEqual(RunState({'source problem': find_source_problem('WARNING: Error doing conform; skipping', {})}).source_problem, 'Could not conform source data')
+        self.assertEqual(RunState({'source problem': find_source_problem('WARNING: Could not download source data', {})}).source_problem, 'Could not download source data')
+        self.assertEqual(RunState({'source problem': find_source_problem('WARNING: Unknown source conform type', {})}).source_problem, 'Unknown source conform type')
+        self.assertEqual(RunState({'source problem': find_source_problem('WARNING: Source is missing a conform object', {})}).source_problem, 'Source is missing a conform object')
 
 class TestPackage (unittest.TestCase):
 
diff --git a/openaddr/tests/ci.py b/openaddr/tests/ci.py
index 7cae4ca..42a73ec 100644
--- a/openaddr/tests/ci.py
+++ b/openaddr/tests/ci.py
@@ -102,6 +102,13 @@ class TestObjects (unittest.TestCase):
             attr = key.replace(' ', '_').replace('-', '_')
             self.assertEqual(getattr(state, attr), value)
 
+        # special case for source problem
+        value = None
+        state = RunState({'source problem': value})
+        self.assertEqual(state.get('source problem'), value)
+        self.assertEqual(state.source_problem, value)
+
+        # special case for code version
         value = str(uuid4())
         state = RunState({'version': value})
         self.assertEqual(state.get('version'), value)