Skip to content

Ensure pre-commit tests by adding actions #1352

Merged
bw4sz merged 2 commits into
mainfrom
henrykironde-patch-3
Mar 19, 2026
Merged

Ensure pre-commit tests by adding actions #1352
bw4sz merged 2 commits into
mainfrom
henrykironde-patch-3

Conversation

@henrykironde
Copy link
Copy Markdown
Contributor

@henrykironde henrykironde commented Mar 14, 2026

Add GitHub Actions to enforce pre-commit tests alongside existing app tests

henrykironde added a commit to henrykironde/DeepForest that referenced this pull request Mar 14, 2026
@musaqlain
Copy link
Copy Markdown
Contributor

@henrykironde I have a proposed solution for replacing the Image.MAX_IMAGE_PIXELS = None line with a tiled raster check, based on your feedback here: #1324 (comment)

Want me to open a follow-up PR that implements it? That way the # to fix marker can be resolved properly.

@vickysharma-prog
Copy link
Copy Markdown
Contributor

vickysharma-prog commented Mar 14, 2026

@henrykironde Following up on your tiled raster check suggestion — I've looked deeper into the implementation:

The current global MAX_IMAGE_PIXELS = None (#1326) works but disables PIL's protection entirely. Your original idea of a pre-flight check is cleaner and safer:

import rasterio
import warnings

# PIL default: int(1024 * 1024 * 1024 // 4 // 3) = 89,478,485
PIL_PIXEL_LIMIT = 89_478_485

def _check_large_image(raster_path):
    """Pre-flight check: detect large images and route to optimal strategy."""
    try:
        with rasterio.open(raster_path) as src:
            total_pixels = src.width * src.height
            is_tiled = src.profile.get('tiled', False)
            
            if total_pixels > PIL_PIXEL_LIMIT:
                if is_tiled:
                    warnings.warn(
                        f"Large tiled raster ({total_pixels:,} pixels). "
                        f"Auto-switching to 'window' strategy."
                    )
                    return "window"
                else:
                    raise ValueError(
                        f"Image ({total_pixels:,} pixels) exceeds PIL limit "
                        f"and is not tiled. Convert with:\n"
                        f"  gdal_translate -of GTiff -co TILED=YES input.tif output.tif"
                    )
    except rasterio.errors.RasterioIOError:
        # Non-raster format (PNG/JPG) - continue with default strategy
        pass
    
    return None

Edge cases I've identified:

Non-raster fallback: rasterio.open() fails on PNG/JPG — needs graceful handling (not just crash)
predict_tile dual input: Accepts both raster_path (file) and image (numpy array) — check only applies to file paths
Consistency: download.py (line 22) also sets MAX_IMAGE_PIXELS = None — both should use the same approach
Dual PIL thresholds: Warning at ~89.5M pixels, hard error at ~178M — should we handle these differently?
This would be placed at the start of predict_tile, before any dataset creation — zero memory overhead since rasterio.open() only reads metadata.

I'd like to implement this as a follow-up PR to #1326 with tests. Happy to get started if this direction works!

* Move Image.MAX_IMAGE_PIXELS assignment to after import statements

* Add precommit step to actions

fix #1352
@henrykironde henrykironde changed the title Test pre-commit of MAX_IMAGE_PIXELS in main.py Ensure pre-commit tests by adding actions Mar 15, 2026
@henrykironde
Copy link
Copy Markdown
Contributor Author

Thanks @musaqlain and @vickysharma-prog for working on this issue. I’m still reviewing how we should handle large files, and once I finalize the approach I’ll update you on the next steps regarding the PIL pixel limit issue and the overall design.

@bw4sz bw4sz self-requested a review March 19, 2026 02:16
@bw4sz bw4sz merged commit b23598a into main Mar 19, 2026
8 checks passed
@bw4sz bw4sz deleted the henrykironde-patch-3 branch March 19, 2026 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants