feat(backend): clip DEM to buffered AOI before upload to reduce QFiel… - #864
feat(backend): clip DEM to buffered AOI before upload to reduce QFiel…#864s4h1r wants to merge 3 commits into
Conversation
…d package size Signed-off-by: Sahir <sahir22@proton.me>
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
Implements Issue #790 by clipping downloaded DEM rasters to a project’s buffered AOI (50m) before uploading to S3, reducing QField package size while keeping a safe fallback to the original DEM when clipping cannot be performed.
Changes:
- Added
clip_dem_to_aoi()GDAL-based clipping helper and integrated it into thedownload_and_upload_dem()workflow using a PostGIS-derived buffered outline. - Added ARQ-worker-side fallback behavior for missing AOI / missing DB pool / clipping errors (uploads unclipped DEM).
- Added a new test suite covering clipping invocation, fallback paths, and basic clipping behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/backend/app/jaxa/upload_dem.py | Adds GDAL clipping helper and clips DEM to a 50m-buffered AOI from PostGIS before upload, with fallback behavior on failure. |
| src/backend/tests/test_upload_dem.py | Adds tests for the new clipping + fallback workflow and direct helper behavior. |
Suppressed comments (3)
src/backend/tests/test_upload_dem.py:154
- This test writes a dummy merged.tif to tmp_path/merged.tif, but download_and_upload_dem() reads/writes tmp_path/tif_processing/<project_id>/merged.tif (based on tempfile.gettempdir()). Creating the file at the actual expected path makes the test setup match production behavior and avoids false confidence if future code starts checking for file existence before clipping/uploading.
merged = tmp_path / "merged.tif"
merged.write_bytes(b"fake-tif")
monkeypatch.setattr(upload_dem.tempfile, "gettempdir", lambda: str(tmp_path))
src/backend/tests/test_upload_dem.py:196
- This test writes a dummy merged.tif to tmp_path/merged.tif, but download_and_upload_dem() reads/writes tmp_path/tif_processing/<project_id>/merged.tif (based on tempfile.gettempdir()). Creating the file at the actual expected path makes the test setup match production behavior and avoids false confidence if future code starts checking for file existence before clipping/uploading.
merged = tmp_path / "merged.tif"
merged.write_bytes(b"fake-tif")
monkeypatch.setattr(upload_dem.tempfile, "gettempdir", lambda: str(tmp_path))
src/backend/tests/test_upload_dem.py:238
- This test writes a dummy merged.tif to tmp_path/merged.tif, but download_and_upload_dem() reads/writes tmp_path/tif_processing/<project_id>/merged.tif (based on tempfile.gettempdir()). Creating the file at the actual expected path makes the test setup match production behavior and avoids false confidence if future code starts checking for file existence before clipping/uploading.
merged = tmp_path / "merged.tif"
merged.write_bytes(b"fake-tif")
monkeypatch.setattr(upload_dem.tempfile, "gettempdir", lambda: str(tmp_path))
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fd, cutline_path = tempfile.mkstemp(suffix=".geojson", prefix="cutline_") | ||
| os.close(fd) | ||
| clipped_path = dem_path + ".clipped.tif" |
|
I have read the CONTRIBUTING.md document and I hereby sign and agree with the guidelines |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Sahir <sahir22@proton.me>
|
Thanks! I'll review asap 🙏 |
It's been quite some time. A response would be highly appreciated. Thanks |
|
Thanks for the prompt! It hasn't been that long though 😉 Open source maintainers can take a while to reply - you'll get used to it - due to overwhelming demand on their attention. Especially a problem with LLM-assisted contributions, which take a maintainer orders of magnitude longer to validate and review PRs than it does for devs to produce them. That all said, thanks again, I'll review this today or tomorrow! In the meantime, did you manage to test this code on an actual example, other than automated tests? 🙏 A good way to do that:
To fully confirm, you could do the pre and post this PR, so compare clipped and not clipped DEMs 👍 |
What type of PR is this? (check all applicable)
Related Issue
Fixes #790
Describe this PR
This PR clips downloaded DEMs to the project AOI with a 50 m buffer before uploading them to S3. This reduces unnecessary DEM coverage in QField packages.
The clipping uses GDAL with a PostGIS-derived cutline and is integrated into the existing DEM download/upload workflow. If clipping cannot be performed, the original DEM is uploaded as a fallback.
Tests cover successful clipping, fallback paths, invalid geometry, temporary-file cleanup, and error handling.
AI Tool Usage
If AI-assisted:
Review follow-up
I had a small hiccup while applying the Copilot review suggestions and initially accepted the wrong suggested fix, but I caught it, corrected the implementation, and pushed the final version. The PR is now up to date and ready for review.
Screenshots
N/A — backend-only change.
Review Guide
just lint— passedpre-commit run --hook-stage pre-push --all-files— all 16 hooks passedjust test backend— 231/231 tests passed