Skip to content

Make focal kernel memory guard dtype-aware (#3223)#3232

Merged
brendancol merged 6 commits into
mainfrom
deep-sweep-security-focal-2026-06-10-02
Jun 11, 2026
Merged

Make focal kernel memory guard dtype-aware (#3223)#3232
brendancol merged 6 commits into
mainfrom
deep-sweep-security-focal-2026-06-10-02

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3223

  • _check_kernel_vs_raster_memory budgeted a flat 4 bytes per cell ("focal internals cast to float32"). That stopped being true when Preserve input float dtype in apply() and focal_stats() (#2769) #2805 made apply() and focal_stats() preserve float64, so the guard underestimated float64 allocations by 2x and a kernel + raster combo could pass the check and then use up to ~100% of available memory, the OOM the guard exists to stop (focal apply()/focal_stats()/hotspots() accept unbounded user kernels #1284).
  • The guard now takes an itemsize argument. apply() and focal_stats() pass np.dtype(_promote_float(agg.dtype)).itemsize (8 for float64 input, 4 otherwise). hotspots() keeps the default 4 since it computes in float32 on every backend.
  • The state CSV commit is from the security sweep that found this.

Backend coverage: the guard runs at the public entry points before dispatch, so all four backends get the same check. No backend code changed.

Test plan:

  • test_apply_oversize_kernel_accounts_for_float64_3223: float64 combo sized to pass the old 4-byte budget now raises MemoryError; the same combo as float32 still runs
  • test_focal_stats_oversize_kernel_accounts_for_float64_3223
  • Existing focal apply()/focal_stats()/hotspots() accept unbounded user kernels #1284 guard tests unchanged and passing
  • Full test_focal.py suite: 235 passed

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 10, 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: Make focal kernel memory guard dtype-aware (#3223)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • A companion test asserting that hotspots() with float64 input is still budgeted at 4 bytes/cell (i.e. not over-rejected) would pin down the asymmetry the comment at focal.py:1758 describes. The existing #1284 hotspots test plus the comment already make the intent clear, so this is optional.

What looks good

  • The itemsize is derived from _promote_float(agg.dtype), the same function the internals use to pick their compute dtype, so the budget and the actual allocations cannot drift apart again the way the hardcoded 4 did after #2805.
  • hotspots() keeps the default 4 with a comment explaining why (it computes in float32 on every backend); passing the promoted itemsize there would have over-rejected float64 input.
  • The new tests are deterministic (patched _available_memory_bytes, fixed 1 MB budget) and test both sides: float64 rejected at exactly the sizes that used to slip through, float32 still allowed.
  • The guard runs at the public entry points before backend dispatch, so the 3D per-band recursion and all four backends inherit the corrected budget.
  • np.dtype(...) works for numpy, cupy, and dask-backed DataArrays alike since .dtype is a numpy dtype in all cases.

Checklist

  • Algorithm matches reference (budget formula unchanged; only bytes/cell corrected)
  • All implemented backends produce consistent results (guard is backend-independent)
  • NaN handling is correct (not touched)
  • Edge cases are covered by tests (pass/reject boundary on both dtypes)
  • Dask chunk boundaries handled correctly (not touched)
  • No premature materialization or unnecessary copies (dtype lookup only, no data access)
  • Benchmark exists or is not needed (validation-path change, no kernel work)
  • README feature matrix: no change needed
  • Docstrings present and accurate (guard docstring updated with the #2805 history)

@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.

Follow-up after the nit: added test_hotspots_float64_keeps_float32_budget_3223, which confirms float64 input to hotspots() stays on the 4 bytes/cell budget and is not over-rejected. Full focal suite: 236 passed. Nothing further from me.

…ocal-2026-06-10-02

Conflicts: xrspatial/focal.py, xrspatial/tests/test_focal.py.
Combined the dtype-aware itemsize budget (#3223) with main's
chunk-aware budgeting (#3228); kept both sides' new tests and bumped
the _promote_float spy count in the #3231 test by one for the guard's
dtype-only call.
@brendancol brendancol merged commit 377c35e into main Jun 11, 2026
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.

focal kernel memory guard still assumes float32 internals after #2805

1 participant