Skip to content

Scope max_pixels to the chunk when chunks= is supplied (#2501)#2502

Merged
brendancol merged 4 commits into
mainfrom
issue-2501
May 27, 2026
Merged

Scope max_pixels to the chunk when chunks= is supplied (#2501)#2502
brendancol merged 4 commits into
mainfrom
issue-2501

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Summary

  • open_geotiff(path, chunks=N, max_pixels=M) no longer rejects the read up front when the full image exceeds max_pixels. The cap now bounds each chunk's materialised buffer instead of the full lazy region.
  • Per-chunk enforcement was already wired through _delayed_read_window -> _read_to_array -> _check_dimensions. The change drops the redundant full-extent guard in _backends/dask.py and the GPU+dask GDS path in _backends/gpu.py. The eager (no-chunks) path is unchanged.
  • Docstring updates for open_geotiff, read_geotiff_dask, and read_geotiff_gpu describe the new contract.

Backend coverage: numpy (eager, unchanged), dask+numpy (new chunk semantics), dask+cupy (new chunk semantics, GDS fast path and CPU-fallback path). VRT chunked path is out of scope; VRT mosaicing has its own composition rules.

Closes #2501

Test plan

  • pytest xrspatial/geotiff/tests/test_security.py -k max_pixels
  • pytest xrspatial/geotiff/tests/parity/test_pixel_equality.py -k max_pixels
  • pytest xrspatial/geotiff/tests/ -k "max_pixels or pixel_safety" -- 19 passed
  • Full geotiff CPU suite -- 5589 passed, 78 skipped, 1 xfailed
  • CI parity on remote (GPU + cloud paths)

`open_geotiff(path, chunks=N, max_pixels=M)` used to reject the read
up front when the full windowed extent exceeded `max_pixels`, defeating
the point of `chunks=` for large rasters: callers had to widen the cap
to the full file just to build the lazy graph, which then disabled the
per-chunk safety guard for every task too.

Drop the full-extent guard in both the CPU dask reader
(`_backends/dask.py`) and the GPU+dask GDS path (`_backends/gpu.py`).
Per-chunk decode already enforces `max_pixels` against the chunk
window via `_read_to_array` -> `_check_dimensions`; the GPU GDS path
keeps its per-TIFF-tile guard, which is a separate hostile-input
defense. The eager (no-chunks) path is unchanged.

Update the `max_pixels` docstring in `open_geotiff`, `read_geotiff_dask`,
and `read_geotiff_gpu`, and rewrite the tests that asserted the
old up-front contract to lock in the new per-chunk semantics.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 27, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Scope max_pixels to the chunk when chunks= is supplied (#2501)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/_backends/gpu.py:1505-1511 -- the new comment says max_pixels "bounds the per-tile decode buffer ... a single tile is the largest contiguous allocation any one task makes." That isn't quite right. The GDS chunk task at _chunk_task (gpu.py:1616) allocates (ch_h, ch_w, samples) per call, which is the chunk shape, not the tile shape. With max_pixels=10, tile=2, and chunks=20 the chunk buffer is 400 pixels, the per-tile check at line 1509 passes (4 < 10), and the old up-front chunk check is gone, so the cap doesn't actually bound the chunk on the GDS path. Either add _check_dimensions(ch_w, ch_h, samples, max_pixels) after the chunks are resolved at gpu.py:1551 to mirror the CPU dask path, or rewrite the comment to describe what's actually being bounded (the per-TIFF-tile dimensions, as a hostile-input guard against forged tile widths) rather than implying dask chunks are bounded.

Nits (optional improvements)

  • xrspatial/geotiff/tests/parity/test_pixel_equality.py:482-501 -- the plan called for multi-band coverage of the eff_bands removal, but the new test only uses a single-band fixture. The deleted eff_bands = (1 if band is not None else (n_bands if n_bands > 0 else 1)) computation isn't directly exercised. Worth adding a 3-band case (or extending small_multiband_tiff_path coverage) where chunks*samples fits under max_pixels but full_image*samples does not.
  • xrspatial/geotiff/tests/unit/test_signatures.py:2095-2099 -- both the read_geotiff_gpu call and .compute() are inside the pytest.raises block. The docstring justifies that (either branch can raise depending on KvikIO availability), but the conventional shape is da = read_geotiff_gpu(...) above the block, then with pytest.raises(...): da.compute() inside. Either form is fine.

What looks good

  • The CPU dask path change is correct: removing the up-front guard delegates to the per-chunk _read_to_array -> _check_dimensions enforcement that was already wired through _delayed_read_window.
  • Docstring updates are precise about what each entry point now bounds.
  • Old tests asserting up-front rejection got rewritten to assert the new contract end-to-end (graph builds, compute raises) rather than deleted.
  • The _MAX_DASK_CHUNKS task-count guard at _backends/dask.py:547 still prevents a forged multi-billion-pixel image from materialising a graph with billions of tasks, so dropping the full-extent guard does not open a DoS hole here.

Checklist

  • Algorithm matches reference/paper (no algorithm changes; pure plumbing)
  • All implemented backends produce consistent results (CPU dask + GPU dask CPU-fallback now share per-chunk semantics; GPU GDS retains per-tile guard, see suggestion)
  • NaN handling is correct (unchanged)
  • Edge cases are covered by tests (single-band, multi-chunk; multi-band gap noted as a nit)
  • Dask chunk boundaries handled correctly (per-chunk read_to_array already correct)
  • No premature materialization or unnecessary copies (removed lines simplify)
  • Benchmark exists or is not needed (not needed; no perf delta)
  • README feature matrix updated (not applicable; no new functions)
  • Docstrings present and accurate

… test (#2501)

* Add `_check_dimensions(ch_w, ch_h, samples, max_pixels)` after the
  chunk shape is resolved in `_read_geotiff_gpu_chunked_gds`. The
  prior comment claimed the per-TIFF-tile check bounded chunk decode;
  it does not, since each `_chunk_task` allocates a chunk-shaped GPU
  buffer. Now the GPU GDS path matches the CPU dask path's per-chunk
  semantics. Update the per-tile comment to describe what it actually
  guards (forged-tile-dim defense).
* Add a multi-band parity test
  (`test_read_geotiff_dask_max_pixels_chunk_includes_band_count`) that
  exercises the per-chunk `chunk_h * chunk_w * samples` arithmetic
  against the 3-band fixture.
* Polish the GDS-path docstring on the GPU chunks max_pixels test to
  acknowledge the new chunk-extent guard.

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review (follow-up on commit 238cbb2): #2501

Blockers

None.

Suggestions

None -- all three points from the previous review are addressed in this push.

Nits

None.

What looks good

  • _backends/gpu.py:1561 adds the chunk-extent guard exactly where the suggestion landed. The comment at gpu.py:1505-1510 is now accurate about what the per-tile check actually defends (forged tile dimensions, not chunk memory). Net effect: the GPU GDS path matches the CPU dask path's per-chunk semantics, so chunk-scoped max_pixels is uniform across CPU dask, GPU GDS, and GPU CPU-fallback.
  • tests/parity/test_pixel_equality.py:505-526 exercises the multi-band arithmetic against the 4x6x3 fixture: chunks=2 gives 12 px per chunk (under cap 20) and a full image of 72 px (above cap), confirming the up-front guard is gone; chunks=4 gives 48 px and trips the per-chunk guard.
  • The compound pytest.raises block in test_signatures.py:2095-2099 is left as-is with the docstring updated to reflect that the GDS path now also raises at build time. Either shape was fine in the prior review.

Checklist

  • Algorithm matches reference/paper (no algorithm changes)
  • All implemented backends produce consistent results (CPU dask + GPU GDS + GPU CPU-fallback all chunk-scoped now)
  • NaN handling is correct (unchanged)
  • Edge cases are covered by tests (single-band and multi-band per-chunk; eager unchanged)
  • Dask chunk boundaries handled correctly
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (not needed)
  • README feature matrix updated (not applicable)
  • Docstrings present and accurate

@brendancol brendancol merged commit 0d60fe9 into main May 27, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

open_geotiff: max_pixels should bound the chunk, not the full image, when chunks= is supplied

1 participant