-
-
Notifications
You must be signed in to change notification settings - Fork 9
Add OpenStreetMap querier with unit and smoke tests #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,61 @@ | ||
| from datetime import datetime, timezone | ||
| import httpx, time | ||
| from etl.base.querier import BaseQuerier | ||
| from etl.dtos import RawLocation | ||
| from etl.dtos import DataSource, RawLocation | ||
|
|
||
| OVERPASS_URL = "https://overpass-api.de/api/interpreter" | ||
| # Overpass asks clients to identify themselves. | ||
| USER_AGENT = "boston-circular-economy-etl/0.1 (https://github.com/codeforboston/boston-circular-economy)" | ||
| # sometimes the Overpass API is overloaded or the server times out, so we retry a few times. 429 = too many requests, 502/503/504 = server error. | ||
| RETRYABLE = {429, 502, 503, 504} | ||
|
|
||
|
|
||
| # based on coordinates set in repair-00.json in /data-explorations | ||
| DEFAULT_BBOX = (42.2, -71.2, 42.5, -70.9) # south, west, north, east | ||
|
|
||
|
|
||
| class OpenStreetMapQuerier(BaseQuerier): | ||
|
|
||
| def __init__(self, tag_filters: list[str], bbox=DEFAULT_BBOX, timeout_s: int = 60): | ||
| # tag_filters are Overpass selector strings, e.g. '["shop"="tailor"]' | ||
| self.tag_filters = tag_filters | ||
| self.bbox = bbox | ||
| self.timeout_s = timeout_s | ||
|
|
||
| def fetch(self) -> list[RawLocation]: | ||
| pass | ||
| query = self._build_query() | ||
| elements = self._run(query) | ||
| return self._to_raw_locations(elements) | ||
|
|
||
| def _build_query(self) -> str: | ||
| s, w, n, e = self.bbox | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since most queriers will support boundaries, it might be good to define a class to represent the boundaries. It'll help prevent mix-ups if another querier gets the order wrong. |
||
| bbox = f"({s},{w},{n},{e})" | ||
| lines = "".join(f"nwr{f}{bbox};" for f in self.tag_filters) | ||
| return f"[out:json][timeout:{self.timeout_s}];({lines});out center;" | ||
|
|
||
| def _run(self, query: str, attempts: int = 3) -> list[dict]: | ||
| for attempt in range(attempts): | ||
| response = httpx.post( | ||
| OVERPASS_URL, data={"data": query}, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's good practice for each arg to live on it's own line when they're broken up like this |
||
| timeout=self.timeout_s + 10, | ||
| headers={"User-Agent": USER_AGENT}, | ||
| ) | ||
| if response.status_code == 200: | ||
| return response.json()["elements"] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - it's sometimes nice to separate method calls from indexing: body = response.json()
return body["elements"] |
||
| if response.status_code in RETRYABLE and attempt < attempts - 1: | ||
| time.sleep(2 ** attempt * 5) # 5s, 10s | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good that we're waiting longer each attempt. Most APIs expect this to some degree. |
||
| continue | ||
| raise RuntimeError(f"Overpass returned {response.status_code}: {response.text[:500]}") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't hurt to include the attempt # & an explanation if there is one. Looking at the httpx documentation, maybe |
||
|
|
||
| def _to_raw_locations(self, elements: list[dict]) -> list[RawLocation]: | ||
| now = datetime.now(timezone.utc) | ||
| by_id: dict[str, RawLocation] = {} | ||
| for el in elements: | ||
| key = f"{el['type']}/{el['id']}" # stable native ID | ||
| by_id[key] = RawLocation( # dict = dedup by ID | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we worried about getting duplicate entries from OSM? |
||
| data_source=DataSource.OPENSTREETMAP, | ||
| data_source_id=key, | ||
| fetched_at=now, | ||
| payload=el, | ||
| ) | ||
| return list(by_id.values()) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good job making sure this included tests. It's very easy to exclude them when no one is forcing you to. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| """Tests for the OpenStreetMap querier. | ||
|
|
||
| Unit tests run offline and cover query building and RawLocation mapping. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit(pick) - a unit test that makes a web request imo isn't a unit test.
|
||
| The smoke test is marked `network` and hits the live Overpass API. | ||
|
|
||
| uv run pytest tests/sources/openstreetmap/ -v # everything | ||
| uv run pytest tests/sources/openstreetmap/ -v -m "not network" # offline only | ||
| uv run pytest tests/sources/openstreetmap/ -v -m network -s # smoke only, with output | ||
| """ | ||
|
|
||
| import pytest | ||
|
|
||
| from etl.dtos import DataSource, RawLocation | ||
| from etl.sources.openstreetmap.querier import DEFAULT_BBOX, OpenStreetMapQuerier | ||
|
|
||
| CLOTHING_FILTERS = [ | ||
| '["shop"="tailor"]', | ||
| '["craft"="tailor"]', | ||
| '["shop"="second_hand"]', | ||
| '["shop"="clothes"]["second_hand"="yes"]', | ||
| '["shop"="charity"]', | ||
| '["amenity"="recycling"]["recycling:clothes"="yes"]', | ||
| ] | ||
|
|
||
|
|
||
| # -------------------------------------------------------------------------- | ||
| # Fixtures: hand-written Overpass elements. | ||
| # -------------------------------------------------------------------------- | ||
|
|
||
| NODE_WITH_NAME = { | ||
| "type": "node", | ||
| "id": 2707308543, | ||
| "lat": 42.3641243, | ||
| "lon": -71.1019370, | ||
| "tags": {"name": "Boomerangs", "shop": "second_hand"}, | ||
| } | ||
|
|
||
| # Ways carry coordinates under "center", which only appears when the query | ||
| # ends with `out center;`. | ||
| WAY_WITH_CENTER = { | ||
| "type": "way", | ||
| "id": 674356526, | ||
| "center": {"lat": 42.4626283, "lon": -70.9473823}, | ||
| "nodes": [6315476620, 6315476621], | ||
| "tags": {"name": "The Salvation Army", "shop": "charity"}, | ||
| } | ||
|
|
||
| # A real pattern from the donation data: a recycling container with no name. | ||
| UNNAMED_BIN = { | ||
| "type": "node", | ||
| "id": 13048760434, | ||
| "lat": 42.3672692, | ||
| "lon": -71.1148759, | ||
| "tags": { | ||
| "amenity": "recycling", | ||
| "operator": "City of Cambridge", | ||
| "recycling:clothes": "yes", | ||
| }, | ||
| } | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def querier(): | ||
| return OpenStreetMapQuerier(tag_filters=CLOTHING_FILTERS) | ||
|
|
||
|
|
||
| # -------------------------------------------------------------------------- | ||
| # Unit tests: _build_query | ||
| # -------------------------------------------------------------------------- | ||
|
|
||
| def test_build_query_exact_output(): | ||
| q = OpenStreetMapQuerier(tag_filters=['["shop"="tailor"]'], bbox=(1.0, 2.0, 3.0, 4.0), timeout_s=25) | ||
| assert q._build_query() == '[out:json][timeout:25];(nwr["shop"="tailor"](1.0,2.0,3.0,4.0););out center;' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not normally a fan of writing unit tests for private methods (or having public methods that are only externally called by unit tests). However, since the alternative to is use mocking & over-complicate things, it's fine as is. |
||
|
|
||
|
|
||
| def test_build_query_unions_all_filters(querier): | ||
| query = querier._build_query() | ||
| for tag_filter in CLOTHING_FILTERS: | ||
| assert tag_filter in query | ||
| # one nwr statement per filter | ||
| assert query.count("nwr") == len(CLOTHING_FILTERS) | ||
|
|
||
|
|
||
| def test_build_query_requests_center(querier): | ||
| # Without `out center;` ways and relations come back with no coordinates. | ||
| assert querier._build_query().endswith("out center;") | ||
|
|
||
|
|
||
| def test_build_query_uses_bbox_in_south_west_north_east_order(querier): | ||
| south, west, north, east = DEFAULT_BBOX | ||
| assert f"({south},{west},{north},{east})" in querier._build_query() | ||
|
|
||
|
|
||
| # -------------------------------------------------------------------------- | ||
| # Unit tests: _to_raw_locations | ||
| # -------------------------------------------------------------------------- | ||
|
|
||
| def test_to_raw_locations_builds_valid_dtos(querier): | ||
| result = querier._to_raw_locations([NODE_WITH_NAME]) | ||
| assert len(result) == 1 | ||
| raw = result[0] | ||
| assert isinstance(raw, RawLocation) | ||
| assert raw.data_source is DataSource.OPENSTREETMAP | ||
| assert raw.data_source_id == "node/2707308543" | ||
| assert raw.fetched_at is not None | ||
|
|
||
|
|
||
| def test_data_source_id_includes_element_type(querier): | ||
| # node/123 and way/123 are different objects; the type prefix keeps them distinct. | ||
| same_id_different_types = [ | ||
| {"type": "node", "id": 123, "lat": 1.0, "lon": 2.0, "tags": {}}, | ||
| {"type": "way", "id": 123, "center": {"lat": 1.0, "lon": 2.0}, "tags": {}}, | ||
| ] | ||
| result = querier._to_raw_locations(same_id_different_types) | ||
| assert {r.data_source_id for r in result} == {"node/123", "way/123"} | ||
|
|
||
|
|
||
| def test_duplicate_elements_are_deduped(querier): | ||
| # MergeProcessor.match() assumes no duplicate entries per source. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unit tests tend to only worry about the class they are testing |
||
| result = querier._to_raw_locations([NODE_WITH_NAME, NODE_WITH_NAME, NODE_WITH_NAME]) | ||
| assert len(result) == 1 | ||
|
|
||
|
|
||
| def test_payload_is_preserved_unmodified(querier): | ||
| result = querier._to_raw_locations([WAY_WITH_CENTER]) | ||
| # Interpretation belongs to the normalizer, so the raw element passes through intact. | ||
| assert result[0].payload == WAY_WITH_CENTER | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whenever you're checking an index, it's good to assert the result has the expected length (similar to other tests) |
||
| assert result[0].payload["center"] == {"lat": 42.4626283, "lon": -70.9473823} | ||
|
|
||
|
|
||
| def test_unnamed_elements_are_kept(querier): | ||
| # Nameless donation bins are real data; dropping them is the normalizer's decision, not the querier's. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - negative testing like this is usually limited; typically if there's some sort of if statement involved i.e. if input < 0:
return this.do_something()
return 1The fact that this comment is referencing another class kind of hints at that. |
||
| result = querier._to_raw_locations([UNNAMED_BIN]) | ||
| assert len(result) == 1 | ||
| assert "name" not in result[0].payload["tags"] | ||
| assert result[0].payload["tags"]["operator"] == "City of Cambridge" | ||
|
|
||
|
|
||
| def test_empty_response_returns_empty_list(querier): | ||
| assert querier._to_raw_locations([]) == [] | ||
|
|
||
|
|
||
| # -------------------------------------------------------------------------- | ||
| # Smoke test: hits the live Overpass API. | ||
| # -------------------------------------------------------------------------- | ||
|
|
||
| @pytest.mark.network | ||
| def test_smoke_fetch_against_live_overpass(querier): | ||
| results = querier.fetch() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may throw. Might be good to catch the exception & evaluate why. |
||
|
|
||
| print(f"\n fetched {len(results)} locations") | ||
|
|
||
| # Ballpark from prior Overpass Turbo runs (tailor ~23, second_hand ~18, | ||
| # charity ~10, plus clothing recycling bins). Wide bounds — OSM data changes. | ||
| assert 20 < len(results) < 400, f"unexpected count: {len(results)}" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validating the # of results we get feels a little iffy. It's not necessarily our fault if OSM gives us bad data. |
||
|
|
||
| ids = [r.data_source_id for r in results] | ||
| assert len(ids) == len(set(ids)), "duplicate data_source_id in results" | ||
| assert all(i.split("/")[0] in {"node", "way", "relation"} for i in ids) | ||
|
|
||
| # Every element must be placeable: nodes carry lat/lon, ways/relations carry center. | ||
| for raw in results: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel like were iterating on results in different places we we could iterate once & validate in a separate method: for raw in results:
validate_result(raw)
# or even break it up into a couple methods
validate_id(raw.data_source_id)
validate_tags(raw.payload) |
||
| p = raw.payload | ||
| assert ("lat" in p and "lon" in p) or "center" in p, f"no coordinates: {raw.data_source_id}" | ||
|
|
||
| named = [r for r in results if r.payload.get("tags", {}).get("name")] | ||
| print(f" {len(named)} named, {len(results) - len(named)} unnamed") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does pytest have some sort of logger? Would give some control over where & when these message would get printed. Would probably label these as debug logs. |
||
|
|
||
| shops = {} | ||
| for raw in results: | ||
| tags = raw.payload.get("tags", {}) | ||
| key = tags.get("shop") or tags.get("craft") or tags.get("amenity") or "(other)" | ||
| shops[key] = shops.get(key, 0) + 1 | ||
| print(f" breakdown: {dict(sorted(shops.items(), key=lambda kv: -kv[1]))}") | ||
| print(f" sample: {results[0].data_source_id} -> {results[0].payload.get('tags', {}).get('name')}") | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to not reference files outside of the ETL pipeline.
Can lead to confusion if they end up being removed.