Add offline map tile prefetch tooling#18
Conversation
Greptile Summary
Confidence Score: 5/5Safe to merge; all previously flagged P1s are resolved and only a minor zoom-cap guard remains. Every P1/P0 issue from the prior review round (OSM policy, antimeridian distance, write-error cleanup, URL template validation) is addressed in the current HEAD. The single new finding is a P2 missing upper-bound guard on --max-zoom, which does not cause wrong data or incorrect downloads — only a potential performance/resource problem for unusually high user-supplied values. P2-only → 5/5 per severity anchoring. tools/prefetch_map_tiles.py — zoom-level upper-bound guard at line 278. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([main]) --> B{validate args}
B -- invalid --> Z1([exit 2])
B -- valid --> C[enumerate tiles per zoom level\nbounding_box → tile_x_ranges\n+ tile_intersects_radius filter]
C --> D{dry-run?}
D -- yes --> Z2([print counts, exit 0])
D -- no --> E[for each tile]
E --> F{dest exists\n& no overwrite?}
F -- skip --> G[skipped++]
F -- download --> H[build_url\ndownload_tile]
H --> I{HTTP success?}
I -- yes --> J[write_bytes to\ntiles/z/x/y.png]
J --> K[downloaded++]
I -- fail/retry --> L{attempts\nexhausted?}
L -- no --> H
L -- yes --> M[unlink partial\nFAIL log]
M --> N[failed++]
G & K & N --> O{more tiles?}
O -- yes --> E
O -- no --> P([print summary\nexit 0 or 1])
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
tools/prefetch_map_tiles.py:278-280
No upper bound is enforced on `--max-zoom`. Tile counts grow as 4× per zoom level: the default 200 km radius produces ~55 k tiles at zoom 14, but ~3.5 M tiles at zoom 19 and billions above that. Because `enumerate_tiles` builds the complete tile list in memory before any downloading begins, an overly high value can exhaust RAM or lock the script into days of computation with no warning.
```suggestion
MAX_ZOOM_LIMIT = 19
if args.min_zoom < 0 or args.max_zoom < 0 or args.min_zoom > args.max_zoom:
print("invalid zoom range", file=sys.stderr)
return 2
if args.max_zoom > MAX_ZOOM_LIMIT:
print(f"--max-zoom {args.max_zoom} exceeds {MAX_ZOOM_LIMIT}; tile counts grow exponentially and most servers cap at z19.", file=sys.stderr)
return 2
```
Reviews (5): Last reviewed commit: "Harden prefetch tile write failures" | Re-trigger Greptile |
| FAIL https://tile.openstreetmap.org/13/2552/2984.png -> tiles\13\2552\2984.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2552/2985.png -> tiles\13\2552\2985.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2552/2986.png -> tiles\13\2552\2986.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2552/2987.png -> tiles\13\2552\2987.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2552/2988.png -> tiles\13\2552\2988.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2552/2989.png -> tiles\13\2552\2989.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2552/2990.png -> tiles\13\2552\2990.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2552/2991.png -> tiles\13\2552\2991.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2553/2984.png -> tiles\13\2553\2984.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2553/2985.png -> tiles\13\2553\2985.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2553/2986.png -> tiles\13\2553\2986.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2553/2987.png -> tiles\13\2553\2987.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2553/2988.png -> tiles\13\2553\2988.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2553/2989.png -> tiles\13\2553\2989.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2553/2990.png -> tiles\13\2553\2990.png (HTTP Error 404: Not Found) | ||
| FAIL https://tile.openstreetmap.org/13/2553/2991.png -> tiles\13\2553\2991.png (HTTP Error 404: Not Found) |
There was a problem hiding this comment.
Log files should not be committed
These log files (prefetch_map_tiles.stderr.log and prefetch_map_tiles.stdout.log) are runtime output artifacts from a test run and should not be tracked in the repository. Consider removing them from the commit and adding *.log or these specific filenames to .gitignore.
Prompt To Fix With AI
This is a comment left during a code review.
Path: prefetch_map_tiles.stderr.log
Line: 1-16
Comment:
**Log files should not be committed**
These log files (`prefetch_map_tiles.stderr.log` and `prefetch_map_tiles.stdout.log`) are runtime output artifacts from a test run and should not be tracked in the repository. Consider removing them from the commit and adding `*.log` or these specific filenames to `.gitignore`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
@brothercorvo please remove the log files from your PR
| - The script defaults to the same OpenStreetMap tile URL currently used by the firmware. | ||
| - If you use a different tile provider, pass `--url-template`. |
There was a problem hiding this comment.
Missing OSM tile usage policy warning
The README should note that the default tile server (tile.openstreetmap.org) prohibits bulk downloading and offline prefetching per the OSM Tile Usage Policy. Users should be directed to use a self-hosted tile server or a provider that explicitly permits offline/bulk downloads.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/README.md
Line: 53-54
Comment:
**Missing OSM tile usage policy warning**
The README should note that the default tile server (`tile.openstreetmap.org`) prohibits bulk downloading and offline prefetching per the [OSM Tile Usage Policy](https://operations.osmfoundation.org/policies/tiles/). Users should be directed to use a self-hosted tile server or a provider that explicitly permits offline/bulk downloads.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
@brothercorvo Greptile is right, here is a screenshot from the usage policy. This is against the OSM Tile Usage Policy linked above
|
@torlando-tech this is also ready for review |
Summary
Notes
Testing