From fed934c73ac7b0a306167d075bb4a00b8368cba4 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 27 Jan 2022 00:46:21 -0500 Subject: [PATCH 1/6] Implement buffer_size overrides See https://github.com/openmaptiles/openmaptiles/issues/1345 Layer file changes: * Added support for an optional `min_buffer_size` value, which is only used in case of a global override, but if set, it must be less than or equal to `buffer_size`. * Layer's `buffer_size` is now optional if `min_buffer_size` is set. Tileset file changes: * Added support for an optional `overrides` section (similar to `defaults`). * Currently only suppports optional `overrides.buffer_size` int value. If set, this value will override the `buffer_size` set in the layer. If layer has `min_buffer_size`, the largest of the two will be used. * An environment variable `TILE_BUFFER_SIZE` can be used instead of overrides.buffer_size, and takes precedent. --- openmaptiles/mbtile_tools.py | 7 +++--- openmaptiles/tileset.py | 44 ++++++++++++++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/openmaptiles/mbtile_tools.py b/openmaptiles/mbtile_tools.py index 18fa7ba9..023bebed 100644 --- a/openmaptiles/mbtile_tools.py +++ b/openmaptiles/mbtile_tools.py @@ -2,6 +2,7 @@ import os import sqlite3 from datetime import datetime +from os import getenv from pathlib import Path from sqlite3 import Cursor from typing import Dict, List, Optional, Tuple @@ -315,7 +316,7 @@ def show_tile(self, zoom, x, y, show_names, summary): def _update_metadata(self, metadata, auto_minmax, reset, file, center_zoom=None): def update_from_env(param, env_var): - val = os.environ.get(env_var) + val = getenv(env_var) if val is not None: metadata[param] = val @@ -328,10 +329,10 @@ def update_from_env(param, env_var): metadata['filesize'] = os.path.getsize(file) - bbox_str = os.environ.get('BBOX') + bbox_str = getenv('BBOX') if bbox_str: bbox = Bbox(bbox=bbox_str, - center_zoom=os.environ.get('CENTER_ZOOM', center_zoom)) + center_zoom=getenv('CENTER_ZOOM', center_zoom)) metadata['bounds'] = bbox.bounds_str() metadata['center'] = bbox.center_str() diff --git a/openmaptiles/tileset.py b/openmaptiles/tileset.py index eea16e4f..1a5b3196 100644 --- a/openmaptiles/tileset.py +++ b/openmaptiles/tileset.py @@ -1,6 +1,7 @@ from dataclasses import dataclass +from os import getenv from pathlib import Path -from typing import List, Union, Dict, Any, Callable +from typing import List, Union, Dict, Any, Callable, Optional import sys import warnings @@ -181,7 +182,30 @@ def description(self) -> str: @property def buffer_size(self) -> int: - return self.definition['layer']['buffer_size'] + """Each layer must have a default buffer size, with either `buffer_size` or `min_buffer_size` or both. + If both are set, buffer_size must be >= min_buffer_size. + min_buffer_size is only used when there is a global buffer size override, + e.g. if global is set to 0, and layer's min_buffer_size is set to 4, the result is 4. + """ + size = self.definition['layer'].get('buffer_size') + min_size = self.definition['layer'].get('min_buffer_size') + if size is None and min_size is None: + raise ValueError(f'Layer "{self.id}" is missing an integer buffer_size and/or min_buffer_size') + elif size is not None and min_size is not None: + if size < min_size: + raise ValueError(f'Layer "{self.id}" has buffer_size less than min_buffer_size') + elif size is None: + # size is not set, use min_size as default + size = min_size + else: + # size is set, min_size is not set + min_size = 0 + + override = self.tileset.override_buffer_size if self.tileset else None + if override is not None: + size = max(override, min_size) + + return size @property def max_size(self) -> int: @@ -360,6 +384,22 @@ def minzoom(self) -> int: def name(self) -> str: return self.definition['name'] + @property + def overrides(self) -> dict: + return self.definition.get('overrides', {}) + + @property + def override_buffer_size(self) -> Optional[int]: + """Layer's buffer size can be overridden globally with a TILE_BUFFER_SIZE env var (used first), + or with the `overrides.buffer_size` value in the tileset yaml file.""" + size = getenv('TILE_BUFFER_SIZE') + if size is not None: + try: + return int(size) + except ValueError: + raise ValueError('Unable to parse TILE_BUFFER_SIZE env var as an integer') + return self.overrides.get('buffer_size') + @property def pixel_scale(self) -> int: return self.definition['pixel_scale'] From ba82c5af5dd482f5e3e2e055be2b4422ba556210 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Fri, 28 Jan 2022 13:40:49 -0500 Subject: [PATCH 2/6] layer overrides --- openmaptiles/tileset.py | 115 +++++++++++++++++++++++---------------- tests/python/README.md | 9 +++ tests/python/test_sql.py | 58 ++++++++++++++++++-- 3 files changed, 132 insertions(+), 50 deletions(-) create mode 100644 tests/python/README.md diff --git a/openmaptiles/tileset.py b/openmaptiles/tileset.py index 1a5b3196..f9291160 100644 --- a/openmaptiles/tileset.py +++ b/openmaptiles/tileset.py @@ -1,7 +1,7 @@ from dataclasses import dataclass -from os import getenv +import os from pathlib import Path -from typing import List, Union, Dict, Any, Callable, Optional +from typing import List, Union, Dict, Any, Callable, Optional, NewType import sys import warnings @@ -11,6 +11,9 @@ from .utils import print_err +GetEnv = NewType('GetEnv', Callable[[str, Optional[str]], Optional[str]]) + + def tag_fields_to_sql(fields): """Converts a list of fields stored in the tags hstore into a list of SQL fields: name:en => NULLIF(tags->'name:en', '') AS name:en @@ -18,6 +21,25 @@ def tag_fields_to_sql(fields): return [f"NULLIF(tags->'{fld}', '') AS \"{fld}\"" for fld in fields] +def assert_int(value, name: str, min_val: Optional[int] = None, max_val: Optional[int] = None, required=False): + if value is None: + if required: + raise ValueError(f'Value {name} does not exist') + return None + elif isinstance(value, str): + try: + value = int(value) + except ValueError: + raise ValueError(f'Unable to parse {name} value "{value}" as an integer') + elif not isinstance(value, int): + raise ValueError(f'The {name} value was expected to be an integer, but found {type(value).__name__} "{value}"') + if min_val is not None and value < min_val: + raise ValueError(f'The {name} value {value} is less than the minimum allowed {min_val}') + if max_val is not None and value > max_val: + raise ValueError(f'The {name} value {value} is more than the maximum allowed {max_val}') + return value + + @dataclass class ParsedData: data: Union[dict, str] @@ -71,14 +93,17 @@ def parse(layer_source: Union[str, Path, ParsedData]) -> 'Layer': def __init__(self, layer_source: Union[str, Path, ParsedData], tileset: 'Tileset' = None, - index: int = None): + index: int = None, + overrides: dict = None): """ :param layer_source: load layer from this source, e.g. a file :param tileset: parent tileset object (optional) :param index: layer's position index within the tileset + :param overrides: additional override parameters for this layer """ self.tileset = tileset self.index = index + self.overrides = overrides if isinstance(layer_source, ParsedData): self.filename = layer_source.path @@ -117,7 +142,7 @@ def __init__(self, requires = requires.copy() # dict will be modified to detect unrecognized properties err = 'If set, "requires" parameter must be a map with optional "layers", "tables", and "functions" sub-elements. Each sub-element must be a string or a list of strings. If "requires" is a list or a string itself, it is treated as a list of layers. ' + \ - 'Optionally add "helpText" sub-element string to help the user with generating missing tables and functions.' + 'Optionally add "helpText" sub-element string to help the user with generating missing tables and functions.' self.requires_layers = get_requires_prop( requires, 'layers', err + '"requires.layers" must be an ID of another layer, or a list of layer IDs.') @@ -148,17 +173,6 @@ def __init__(self, # osm_id column twice - once for feature_id, and once as an attribute raise ValueError('key_field_as_attribute=yes is not yet implemented') - @deprecated(version='3.2.0', reason='use named properties instead') - def __getitem__(self, attr): - if attr in self.definition: - return self.definition[attr] - elif attr == 'fields': - return {} - elif attr == 'description': - return '' - else: - raise KeyError - def get_fields(self) -> List[str]: """Get a list of field names this layer generates. Geometry field is not included.""" @@ -186,9 +200,11 @@ def buffer_size(self) -> int: If both are set, buffer_size must be >= min_buffer_size. min_buffer_size is only used when there is a global buffer size override, e.g. if global is set to 0, and layer's min_buffer_size is set to 4, the result is 4. + Per layer tileset overrides are allowed for both buffer_size and min_buffer_size. + Per layer overrides have higher priority than global overrides, but less than ENV var. """ - size = self.definition['layer'].get('buffer_size') - min_size = self.definition['layer'].get('min_buffer_size') + size = assert_int(self.definition['layer'].get('buffer_size'), 'buffer_size', min_val=0) + min_size = assert_int(self.definition['layer'].get('min_buffer_size'), 'min_buffer_size', min_val=0) if size is None and min_size is None: raise ValueError(f'Layer "{self.id}" is missing an integer buffer_size and/or min_buffer_size') elif size is not None and min_size is not None: @@ -201,10 +217,27 @@ def buffer_size(self) -> int: # size is set, min_size is not set min_size = 0 - override = self.tileset.override_buffer_size if self.tileset else None - if override is not None: - size = max(override, min_size) - + if self.tileset: + val = assert_int(self.tileset.overrides.get('buffer_size'), 'buffer_size global override', min_val=0) + if val is not None: + size = val + if self.overrides: + val = assert_int(self.overrides.get('buffer_size'), 'buffer_size layer override', min_val=0) + min_val = assert_int(self.overrides.get('min_buffer_size'), 'min_buffer_size layer override', min_val=0) + if val is not None and min_val is not None and val < min_val: + raise ValueError(f'Layer overrides for "{self.id}" have buffer_size less than min_buffer_size') + if val is not None: + size = val + if min_val is not None: + min_size = min_val + # Allow empty env var to be the same as unset. + getenv = self.tileset.getenv if self.tileset else os.getenv + tbs = getenv('TILE_BUFFER_SIZE', '') + val = assert_int(tbs if tbs != '' else None, 'TILE_BUFFER_SIZE env var', min_val=0) + if val is not None: + size = val + if size < min_size: + size = min_size return size @property @@ -279,24 +312,33 @@ class Tileset: filename: Path definition: dict layers: List[Layer] + getenv: GetEnv @staticmethod def parse(tileset_source: Union[str, Path, ParsedData]) -> 'Tileset': return Tileset(tileset_source) - def __init__(self, tileset_source: Union[str, Path, ParsedData]): + def __init__(self, tileset_source: Union[str, Path, ParsedData], getenv: GetEnv = None): + """Create a new tileset from a file (str|Path), or already parsed. + Optionally provide environment variables (used in unit testing).""" + self.getenv = getenv or os.getenv if isinstance(tileset_source, ParsedData): self.filename = tileset_source.path - self.definition = tileset_source.data + data = tileset_source.data else: self.filename = Path(tileset_source).resolve() - self.definition = parse_file(self.filename) + data = parse_file(self.filename) - self.definition = self.definition['tileset'] + self.definition = data['tileset'] self.layers = [] self.layers_by_id = {} - for index, layer_filename in enumerate(self.definition['layers']): - layer = Layer(layer_filename, self, index) + + layer_obj: Union[str, Path, ParsedData, dict] + for index, layer_obj in enumerate(self.definition['layers']): + if isinstance(layer_obj, dict): + layer = Layer(layer_obj['file'], self, index, layer_obj) + else: + layer = Layer(layer_obj, self, index, overrides=None) if layer.id in self.layers_by_id: raise ValueError(f"Layer '{layer.id}' is defined more than once") self.layers.append(layer) @@ -325,13 +367,6 @@ def __init__(self, tileset_source: Union[str, Path, ParsedData]): validate_properties(self, f'Tileset {self.filename}') - @deprecated(version='3.2.0', reason='use named properties instead') - def __getitem__(self, attr): - if attr in self.definition: - return self.definition[attr] - else: - raise KeyError - @property def attribution(self) -> str: return self.definition['attribution'] @@ -388,18 +423,6 @@ def name(self) -> str: def overrides(self) -> dict: return self.definition.get('overrides', {}) - @property - def override_buffer_size(self) -> Optional[int]: - """Layer's buffer size can be overridden globally with a TILE_BUFFER_SIZE env var (used first), - or with the `overrides.buffer_size` value in the tileset yaml file.""" - size = getenv('TILE_BUFFER_SIZE') - if size is not None: - try: - return int(size) - except ValueError: - raise ValueError('Unable to parse TILE_BUFFER_SIZE env var as an integer') - return self.overrides.get('buffer_size') - @property def pixel_scale(self) -> int: return self.definition['pixel_scale'] diff --git a/tests/python/README.md b/tests/python/README.md new file mode 100644 index 00000000..349da27c --- /dev/null +++ b/tests/python/README.md @@ -0,0 +1,9 @@ +# Python Unit Tests + +These tests can be quickly ran using `make bash` (from the root of the tools project): + +```bash +host$ make bash + +openmaptiles@...:/tileset$ python -m unittest +``` diff --git a/tests/python/test_sql.py b/tests/python/test_sql.py index 37ffe103..4169b6f3 100644 --- a/tests/python/test_sql.py +++ b/tests/python/test_sql.py @@ -1,7 +1,7 @@ import warnings from dataclasses import dataclass from pathlib import Path -from typing import List, Union, Dict +from typing import List, Union, Dict, Optional from unittest import main, TestCase from openmaptiles.sql import collect_sql, sql_assert_table, sql_assert_func @@ -43,16 +43,16 @@ def parsed_data(layers: Union[Case, List[Case]]): defaults=dict(srs='test_srs', datasource=dict(srid='test_datasource')), id='id1', layers=[ - ParsedData(dict( + dict(file=ParsedData(dict( layer=dict( - buffer_size='test_buffer_size', + buffer_size='10', datasource=dict(query='test_query'), id=v.id, fields={}, requires=[v.reqs] if isinstance(v.reqs, str) else v.reqs or [] ), schema=[ParsedData(v.query, Path(v.id + '_s.yaml'))] if v.query else [], - ), Path(f'./{v.id}.yaml')) for v in ([layers] if isinstance(layers, Case) else layers) + ), Path(f'./{v.id}.yaml'))) for v in ([layers] if isinstance(layers, Case) else layers) ], maxzoom='test_maxzoom', minzoom='test_minzoom', @@ -158,6 +158,7 @@ def _ts_parse(self, reqs, expected_layers, expected_tables, expected_funcs, extr self.assertEqual(layer.requires_layers, expected_layers) self.assertEqual(layer.requires_tables, expected_tables) self.assertEqual(layer.requires_functions, expected_funcs) + self.assertEqual(layer.buffer_size, 10) # This test can be deleted once we remove the deprecated property in some future version with warnings.catch_warnings(): @@ -181,6 +182,55 @@ def test_ts_parse(self): self._ts_parse(dict(layers=['c1'], tables=['a', 'b'], functions=['x', 'y']), ['c1'], ['a', 'b'], ['x', 'y'], extra) + def _ts_overrides(self, expected_layer: dict, + layer: Optional[dict] = None, + override_ts: Optional[dict] = None, + override_layer: Optional[dict] = None, + env: Optional[dict] = None): + data = parsed_data([Case('my_id', 'my_query;')]) + + ts_data = data.data['tileset'] + if override_ts is not None: + ts_data['overrides'] = override_ts + if layer is not None: + ts_data['layers'][0]['file'].data['layer'].update(layer) + if override_layer is not None: + ts_data['layers'][0].update(override_layer) + + ts = Tileset(data, getenv=env.get if env else None) + for k in expected_layer.keys(): + self.assertEqual(getattr(ts.layers_by_id['my_id'], k), expected_layer[k]) + + def test_overrides(self): + buf_0 = dict(buffer_size=0) + buf_1 = dict(buffer_size=1) + buf_2 = dict(buffer_size=2) + buf_3 = dict(buffer_size=3) + min_1 = dict(min_buffer_size=1) + min_2 = dict(min_buffer_size=2) + min_3 = dict(min_buffer_size=3) + + self._ts_overrides(buf_2, buf_2) + self._ts_overrides(buf_0, buf_2, override_ts=buf_0) + self._ts_overrides(buf_1, buf_2, override_ts=buf_1) + self._ts_overrides(buf_3, buf_2, override_ts=buf_3) + self._ts_overrides(buf_1, min_1 | buf_2, override_ts=buf_0) + self._ts_overrides(buf_1, buf_2, override_layer=buf_1) + self._ts_overrides(buf_2, min_2 | buf_3, override_layer=buf_1) + self._ts_overrides(buf_3, min_1 | buf_2, override_layer=min_3) + self._ts_overrides(buf_3, min_1 | buf_2, override_layer=min_2 | buf_3, override_ts=buf_0) + + env_0 = dict(TILE_BUFFER_SIZE='0') + env_2 = dict(TILE_BUFFER_SIZE='2') + self._ts_overrides(buf_2, min_1 | buf_2, override_layer=min_2 | buf_3, override_ts=buf_0, env=env_0) + self._ts_overrides(buf_2, buf_1, env=env_2) + self._ts_overrides(buf_2, min_2 | buf_3, env=env_0) + self._ts_overrides(buf_2, min_1 | buf_3, override_ts=buf_0, env=env_2) + self._ts_overrides(buf_2, buf_2, dict(TILE_BUFFER_SIZE='')) + + # str parsing + self._ts_overrides(dict(buffer_size=2), dict(buffer_size='2')) + if __name__ == '__main__': main() From f4e804c9a3e1f75ee435827e22a08647d71e191c Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 29 Jan 2022 21:34:48 -0500 Subject: [PATCH 3/6] Update openmaptiles/tileset.py Co-authored-by: zstadler --- openmaptiles/tileset.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openmaptiles/tileset.py b/openmaptiles/tileset.py index f9291160..c7446d52 100644 --- a/openmaptiles/tileset.py +++ b/openmaptiles/tileset.py @@ -197,7 +197,7 @@ def description(self) -> str: @property def buffer_size(self) -> int: """Each layer must have a default buffer size, with either `buffer_size` or `min_buffer_size` or both. - If both are set, buffer_size must be >= min_buffer_size. + If both are set, `buffer_size` must be >= `min_buffer_size`. min_buffer_size is only used when there is a global buffer size override, e.g. if global is set to 0, and layer's min_buffer_size is set to 4, the result is 4. Per layer tileset overrides are allowed for both buffer_size and min_buffer_size. From 996927dc083a9e30176c56fff9491240e59fc594 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 29 Jan 2022 21:35:58 -0500 Subject: [PATCH 4/6] Update openmaptiles/tileset.py Co-authored-by: zstadler --- openmaptiles/tileset.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openmaptiles/tileset.py b/openmaptiles/tileset.py index c7446d52..8db1504b 100644 --- a/openmaptiles/tileset.py +++ b/openmaptiles/tileset.py @@ -199,7 +199,7 @@ def buffer_size(self) -> int: """Each layer must have a default buffer size, with either `buffer_size` or `min_buffer_size` or both. If both are set, `buffer_size` must be >= `min_buffer_size`. min_buffer_size is only used when there is a global buffer size override, - e.g. if global is set to 0, and layer's min_buffer_size is set to 4, the result is 4. + e.g. if global `buffer_size` is set to 0, and layer's `min_buffer_size` is set to 4, the result is 4. Per layer tileset overrides are allowed for both buffer_size and min_buffer_size. Per layer overrides have higher priority than global overrides, but less than ENV var. """ From 917fe65c879fbbbfd12e3d263f19759441368221 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 29 Jan 2022 21:51:13 -0500 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: zstadler --- openmaptiles/tileset.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openmaptiles/tileset.py b/openmaptiles/tileset.py index 8db1504b..0fa8d4ee 100644 --- a/openmaptiles/tileset.py +++ b/openmaptiles/tileset.py @@ -200,7 +200,7 @@ def buffer_size(self) -> int: If both are set, `buffer_size` must be >= `min_buffer_size`. min_buffer_size is only used when there is a global buffer size override, e.g. if global `buffer_size` is set to 0, and layer's `min_buffer_size` is set to 4, the result is 4. - Per layer tileset overrides are allowed for both buffer_size and min_buffer_size. + Per layer tileset overrides are allowed for both `buffer_size` and `min_buffer_size`. Per layer overrides have higher priority than global overrides, but less than ENV var. """ size = assert_int(self.definition['layer'].get('buffer_size'), 'buffer_size', min_val=0) From 381f6bb5eada2f32a6ca29092496d7d40b3218d6 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 29 Jan 2022 22:05:27 -0500 Subject: [PATCH 6/6] clarify buffer logic --- openmaptiles/tileset.py | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/openmaptiles/tileset.py b/openmaptiles/tileset.py index 0fa8d4ee..b9875e44 100644 --- a/openmaptiles/tileset.py +++ b/openmaptiles/tileset.py @@ -196,13 +196,26 @@ def description(self) -> str: @property def buffer_size(self) -> int: - """Each layer must have a default buffer size, with either `buffer_size` or `min_buffer_size` or both. - If both are set, `buffer_size` must be >= `min_buffer_size`. - min_buffer_size is only used when there is a global buffer size override, - e.g. if global `buffer_size` is set to 0, and layer's `min_buffer_size` is set to 4, the result is 4. - Per layer tileset overrides are allowed for both `buffer_size` and `min_buffer_size`. - Per layer overrides have higher priority than global overrides, but less than ENV var. """ + Layer's buffer size is computed from `buffer_size` and `min_buffer_size` from layer and tileset files, + as well the TILE_BUFFER_SIZE env var using this logic: + + max( + first_found_value( + TILE_BUFFER_SIZE env variable, + buffer_size set in the tileset yaml file layer's section (per layer override), + buffer_size set in the tileset yaml file at the top level (global override), + buffer_size set in the layer yaml file, + 0), + first_found_value( + min_buffer_size set in the tileset yaml file layer's section (per layer override), + min_buffer_size set in the layer yaml file, + 0) + ) + + Note that the layer yaml file must define either buffer_size or min_buffer_size or both. + """ + # Read layer yaml file size = assert_int(self.definition['layer'].get('buffer_size'), 'buffer_size', min_val=0) min_size = assert_int(self.definition['layer'].get('min_buffer_size'), 'min_buffer_size', min_val=0) if size is None and min_size is None: @@ -211,16 +224,17 @@ def buffer_size(self) -> int: if size < min_size: raise ValueError(f'Layer "{self.id}" has buffer_size less than min_buffer_size') elif size is None: - # size is not set, use min_size as default - size = min_size + # size is not set, will use min_size as default (at the end) + size = 0 else: # size is set, min_size is not set min_size = 0 - + # Override with tileset global values if self.tileset: val = assert_int(self.tileset.overrides.get('buffer_size'), 'buffer_size global override', min_val=0) if val is not None: size = val + # Override with tileset per-layer values if self.overrides: val = assert_int(self.overrides.get('buffer_size'), 'buffer_size layer override', min_val=0) min_val = assert_int(self.overrides.get('min_buffer_size'), 'min_buffer_size layer override', min_val=0) @@ -230,12 +244,14 @@ def buffer_size(self) -> int: size = val if min_val is not None: min_size = min_val + # Override with ENV variables # Allow empty env var to be the same as unset. getenv = self.tileset.getenv if self.tileset else os.getenv tbs = getenv('TILE_BUFFER_SIZE', '') val = assert_int(tbs if tbs != '' else None, 'TILE_BUFFER_SIZE env var', min_val=0) if val is not None: size = val + # Ensure buffer is no less than the minimum if size < min_size: size = min_size return size