feat(spp_demo): curated PHL geodata + spp_demo_phl_luzon demo module (re-land from #76)#277
feat(spp_demo): curated PHL geodata + spp_demo_phl_luzon demo module (re-land from #76)#277gonzalesedwin1123 wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a geodata preparation script (prepare_phl_geodata.py) and updates the spp_demo module with refreshed Philippine geodata using PSA/HDX p-codes. It also introduces a new companion module, spp_demo_phl_luzon, which provides Luzon-scale demo areas, population weights, and an area loader. The review feedback highlights three key improvements: optimizing an N+1 query pattern in the Luzon area loader's shape-loading method, wrapping the streaming HTTP request in a context manager to prevent connection leaks, and updating the script's documented dependencies to include pandas and openpyxl so that population data processing does not silently fail.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| features = geojson_data.get("features", []) | ||
| shapes_loaded = 0 | ||
|
|
||
| for feature in features: | ||
| properties = feature.get("properties", {}) | ||
| geometry = feature.get("geometry") | ||
|
|
||
| code = properties.get("code") | ||
| if not code or not geometry: | ||
| continue | ||
|
|
||
| area = self.env["spp.area"].search([("code", "=", code)], limit=1) | ||
| if not area: | ||
| continue | ||
|
|
||
| try: | ||
| from shapely.geometry import shape | ||
|
|
||
| geom = shape(geometry) | ||
| area.write({"geo_polygon": geom.wkt}) | ||
| shapes_loaded += 1 | ||
| except Exception as e: | ||
| _logger.warning("Could not set shape for %s: %s", code, e) |
There was a problem hiding this comment.
An N+1 query pattern is present in _load_shapes(). The method performs a database search query inside the loop for every single feature in the GeoJSON file:
area = self.env["spp.area"].search([("code", "=", code)], limit=1)
With over 700 municipalities and other administrative areas in Luzon, this results in hundreds of individual database queries, significantly degrading performance.
To optimize this, all relevant spp.area records should be fetched in a single batched query before the loop, and then mapped by their code.
features = geojson_data.get("features", [])
shapes_loaded = 0
# Batch search all areas to avoid N+1 database queries inside the loop
codes = [f.get("properties", {}).get("code") for f in features if f.get("properties", {}).get("code")]
areas = self.env["spp.area"].search([("code", "in", codes)])
area_by_code = {area.code: area for area in areas}
for feature in features:
properties = feature.get("properties", {})
geometry = feature.get("geometry")
code = properties.get("code")
if not code or not geometry:
continue
area = area_by_code.get(code)
if not area:
continue
try:
from shapely.geometry import shape
geom = shape(geometry)
area.write({"geo_polygon": geom.wkt})
shapes_loaded += 1
except Exception as e:
_logger.warning("Could not set shape for %s: %s", code, e)References
- When processing records in bulk, avoid evaluating filters or permissions individually per record to prevent O(N * M) database query patterns. Instead, perform individual checks first, and then evaluate filters in a single batched query.
| resp = requests.get(url, stream=True, timeout=300) | ||
| resp.raise_for_status() | ||
|
|
||
| total = int(resp.headers.get("content-length", 0)) | ||
| downloaded = 0 | ||
| with open(cached_path, "wb") as f: | ||
| for chunk in resp.iter_content(chunk_size=1024 * 1024): | ||
| f.write(chunk) | ||
| downloaded += len(chunk) | ||
| if total: | ||
| pct = downloaded * 100 // total | ||
| print(f"\r Progress: {pct}% ({downloaded // (1024 * 1024)}MB / {total // (1024 * 1024)}MB)", end="") | ||
| print() |
There was a problem hiding this comment.
The requests.get call is made with stream=True but is not wrapped in a with statement or explicitly closed. When stream=True is used, the connection is kept open until all data is read or the response is closed, which can lead to a connection leak if an exception occurs or if the stream is not fully consumed.
Wrapping the request in a with statement ensures that the connection is reliably closed and released back to the pool.
| resp = requests.get(url, stream=True, timeout=300) | |
| resp.raise_for_status() | |
| total = int(resp.headers.get("content-length", 0)) | |
| downloaded = 0 | |
| with open(cached_path, "wb") as f: | |
| for chunk in resp.iter_content(chunk_size=1024 * 1024): | |
| f.write(chunk) | |
| downloaded += len(chunk) | |
| if total: | |
| pct = downloaded * 100 // total | |
| print(f"\r Progress: {pct}% ({downloaded // (1024 * 1024)}MB / {total // (1024 * 1024)}MB)", end="") | |
| print() | |
| with requests.get(url, stream=True, timeout=300) as resp: | |
| resp.raise_for_status() | |
| total = int(resp.headers.get("content-length", 0)) | |
| downloaded = 0 | |
| with open(cached_path, "wb") as f: | |
| for chunk in resp.iter_content(chunk_size=1024 * 1024): | |
| f.write(chunk) | |
| downloaded += len(chunk) | |
| if total: | |
| pct = downloaded * 100 // total | |
| print(f"\r Progress: {pct}% ({downloaded // (1024 * 1024)}MB / {total // (1024 * 1024)}MB)", end="") | |
| print() |
| # Requires: geopandas, shapely, requests | ||
| uv run --with geopandas --with requests scripts/prepare_phl_geodata.py |
There was a problem hiding this comment.
The script imports pandas and uses pd.read_excel (which requires openpyxl or xlrd as an engine) to parse the population projection data. However, the documented uv run command only specifies --with geopandas --with requests.
If a user runs the script using the documented command, the population data download will silently fail (due to the broad except Exception block catching the ImportError) and fall back to equal weights.
Updating the usage instructions to include pandas and openpyxl ensures that the population weights are correctly processed.
| # Requires: geopandas, shapely, requests | |
| uv run --with geopandas --with requests scripts/prepare_phl_geodata.py | |
| # Requires: geopandas, shapely, requests, pandas, openpyxl | |
| uv run --with geopandas --with requests --with pandas --with openpyxl scripts/prepare_phl_geodata.py |
… module (from #76) Re-lands the PHL demo data portion of reverted PR #76: curated PHL geojson shapes and areas, demo data generator and area loader test updates, the prepare_phl_geodata.py preparation script, and the new spp_demo_phl_luzon module (Luzon demo areas + population weights). Files restored verbatim from the pre-revert merged state (8bf9a3a). spp_demo bumped to 19.0.2.1.0 with a HISTORY entry.
gonzalesedwin1123
force-pushed
the
reland/phl-demo
branch
from
32a47c5 to
537b32e
Compare
- area_loader: batch-fetch areas by code instead of per-feature search (N+1 over ~700 Luzon features). - prepare_phl_geodata.py: close streamed download via context manager; usage docs include pandas/openpyxl needed for population weights.
|
All three gemini-code-assist findings applied (commit 91c9e06): batched area lookup replacing the per-feature N+1 search, streamed download wrapped in a context manager, and usage docs now include pandas/openpyxl. |
1 participant
Re-lands the PHL demo-data portion of reverted PR #76 (revert: #271).
Summary
spp_demo: curated PHL shapes/areas (phl_curated.geojson,areas.xml) and demo generator updates.spp_demo_phl_luzonmodule: Luzon administrative areas, population weights, area loader (depends on spp_demo's loader changes).scripts/prepare_phl_geodata.py: the script that generates the curated geodata.Verification
./spp t spp_demo: 121 passed, 0 failed./spp t spp_demo_phl_luzon: 17 passed, 0 failed