From 3a9e573cc4996a66e9bd80375a3fe084932b44c0 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Mon, 11 May 2026 07:56:24 -0700 Subject: [PATCH 1/2] Rename read_geotiff_gpu gpu kwarg to on_gpu_failure (#1560) The kwarg shared a name with the boolean gpu= on open_geotiff / to_geotiff / read_vrt but carried a different value space ('auto' or 'strict'). Calling read_geotiff_gpu(path, gpu=True) -- the natural mental model after using open_geotiff(path, gpu=True) -- raised "gpu must be 'auto' or 'strict', got True", obscuring the rename and forcing readers to dig through the source to learn the correct values. Rename to on_gpu_failure and keep gpu= as a deprecation shim: - on_gpu_failure: canonical name, same {'auto', 'strict'} value space. - gpu: still accepted; emits DeprecationWarning that points to the new name and explains why the rename happened. - Passing both raises TypeError to avoid silently picking one. - Validation error message names on_gpu_failure so the rename is discoverable from the traceback. Existing test_gpu_strict_fallback_1516.py keeps using gpu='strict' via the shim (with DeprecationWarning, as expected). One assertion that hardcoded the old error string is updated to the new wording. Module docstring updated to reference on_gpu_failure='strict' as the example. --- xrspatial/geotiff/__init__.py | 46 ++++++++-- .../tests/test_gpu_kwarg_rename_1560.py | 92 +++++++++++++++++++ .../tests/test_gpu_strict_fallback_1516.py | 11 ++- 3 files changed, 140 insertions(+), 9 deletions(-) create mode 100644 xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index 3949d219..6bed409c 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -11,8 +11,8 @@ read_geotiff_gpu(source, ...) GPU-only read returning a CuPy-backed DataArray. ``open_geotiff(..., gpu=True)`` calls this internally; use the explicit name when you want - the strict-mode failure semantics (``gpu='strict'``) or want to bypass - auto-dispatch. + the strict-mode failure semantics (``on_gpu_failure='strict'``) or want + to bypass auto-dispatch. read_geotiff_dask(source, ...) Dask-only read returning a windowed lazy DataArray. ``open_geotiff(..., chunks=N)`` calls this internally. @@ -660,6 +660,12 @@ def _apply_nodata_mask_gpu(arr_gpu, nodata): ) +# Sentinel distinguishing "user passed gpu= explicitly" from "user passed +# nothing". A plain default of None would not work because None is itself +# a value a caller could supply. +_GPU_DEPRECATED_SENTINEL = object() + + # TIFF type ids needed when synthesizing extra_tags entries from attrs. _TIFF_BYTE = 1 _TIFF_ASCII = 2 @@ -1809,7 +1815,8 @@ def read_geotiff_gpu(source: str, *, name: str | None = None, chunks: int | tuple | None = None, max_pixels: int | None = None, - gpu: str = 'auto') -> xr.DataArray: + on_gpu_failure: str = 'auto', + gpu=_GPU_DEPRECATED_SENTINEL) -> xr.DataArray: """Read a GeoTIFF with GPU-accelerated decompression via Numba CUDA. Decompresses all tiles in parallel on the GPU and returns a @@ -1835,7 +1842,7 @@ def read_geotiff_gpu(source: str, *, max_pixels : int or None Maximum allowed pixel count (width * height * samples). None uses the default (~1 billion). - gpu : {'auto', 'strict'}, default 'auto' + on_gpu_failure : {'auto', 'strict'}, default 'auto' Behaviour when any GPU decode stage raises an exception. The GPU pipeline has two stages: first ``gpu_decode_tiles_from_file`` @@ -1855,18 +1862,43 @@ def read_geotiff_gpu(source: str, *, GPU fast path. Stripped layouts and sparse-tile files route directly to the CPU - reader before either GPU decode stage runs, so the ``gpu`` kwarg - does not affect them. A failure inside the subsequent + reader before either GPU decode stage runs, so the ``on_gpu_failure`` + kwarg does not affect them. A failure inside the subsequent ``cupy.asarray(...)`` upload propagates unchanged in both modes. + gpu : str, optional + Deprecated alias for ``on_gpu_failure``. Emits ``DeprecationWarning`` + when used. Passing both ``gpu`` and ``on_gpu_failure`` raises + ``TypeError``. The old name shipped with values ``'auto'`` / + ``'strict'`` and was easy to confuse with the boolean ``gpu=`` + kwarg on ``open_geotiff`` / ``to_geotiff`` / ``read_vrt``. Returns ------- xr.DataArray CuPy-backed DataArray on GPU device. """ + if gpu is not _GPU_DEPRECATED_SENTINEL: + # Caller passed gpu= explicitly. Forward to on_gpu_failure unless + # both were supplied; in that case the intent is ambiguous and we + # refuse rather than silently picking one. + if on_gpu_failure != 'auto': + raise TypeError( + "read_geotiff_gpu: pass either 'on_gpu_failure' or the " + "deprecated 'gpu' alias, not both.") + warnings.warn( + "read_geotiff_gpu(..., gpu=...) is deprecated; use " + "on_gpu_failure=... instead. The kwarg was renamed because " + "'gpu' on open_geotiff/to_geotiff/read_vrt is a bool that " + "selects the GPU backend, while here it selects the failure " + "policy when the GPU path raises.", + DeprecationWarning, + stacklevel=2, + ) + on_gpu_failure = gpu + gpu = on_gpu_failure if gpu not in ('auto', 'strict'): raise ValueError( - f"gpu must be 'auto' or 'strict', got {gpu!r}") + f"on_gpu_failure must be 'auto' or 'strict', got {gpu!r}") try: import cupy except ImportError: diff --git a/xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py b/xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py new file mode 100644 index 00000000..2f8f5f69 --- /dev/null +++ b/xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py @@ -0,0 +1,92 @@ +"""Regression tests for issue #1560. + +``read_geotiff_gpu`` previously took a ``gpu={'auto','strict'}`` kwarg +that controlled GPU-failure policy, sharing a name with the boolean +``gpu=`` kwarg on ``open_geotiff`` / ``to_geotiff`` / ``read_vrt``. +Calling ``read_geotiff_gpu(path, gpu=True)`` -- the mental model after +using ``open_geotiff(path, gpu=True)`` -- raised the unhelpful +``ValueError: gpu must be 'auto' or 'strict', got True``. + +The fix renames the kwarg to ``on_gpu_failure`` and keeps ``gpu=`` as a +deprecation shim: + +* ``on_gpu_failure`` alone behaves like the old ``gpu`` kwarg. +* ``gpu`` alone still works, but emits ``DeprecationWarning``. +* Passing both raises ``TypeError``. + +These tests exercise the validation path only, which fires before the +``cupy`` import inside ``read_geotiff_gpu``. No GPU runtime needed. +""" +from __future__ import annotations + +import warnings + +import pytest + + +def test_on_gpu_failure_invalid_value_raises_value_error(): + """Bad ``on_gpu_failure`` value still raises ``ValueError``.""" + from xrspatial.geotiff import read_geotiff_gpu + + with pytest.raises(ValueError, match="on_gpu_failure must be"): + read_geotiff_gpu("/nonexistent.tif", on_gpu_failure='loose') + + +def test_gpu_alias_emits_deprecation_warning(): + """Old ``gpu=`` kwarg still routes through, with a DeprecationWarning.""" + from xrspatial.geotiff import read_geotiff_gpu + + with warnings.catch_warnings(record=True) as records: + warnings.simplefilter("always") + # Pass an invalid sentinel so we don't have to mock the full GPU + # pipeline; ValueError fires after the deprecation handler runs. + with pytest.raises(ValueError, match="on_gpu_failure must be"): + read_geotiff_gpu("/nonexistent.tif", gpu='loose') + + deprecations = [r for r in records if issubclass(r.category, DeprecationWarning)] + assert deprecations, "expected DeprecationWarning when gpu= is used" + assert "on_gpu_failure" in str(deprecations[0].message) + + +def test_gpu_alias_accepts_old_values_without_validation_error(): + """``gpu='strict'`` was the legacy spelling; should still validate.""" + from xrspatial.geotiff import read_geotiff_gpu + + with warnings.catch_warnings(): + # Suppress the deprecation noise; we only care that the value + # passes validation and the call proceeds to the file-read stage + # (which will fail with a missing-file error, not a ValueError). + warnings.simplefilter("ignore", DeprecationWarning) + with pytest.raises((FileNotFoundError, OSError, ValueError)) as exc_info: + read_geotiff_gpu("/nonexistent.tif", gpu='strict') + + # The validation ValueError carries our exact message; a generic + # file-read failure is fine because it means validation passed. + if isinstance(exc_info.value, ValueError): + assert "on_gpu_failure must be" not in str(exc_info.value) + + +def test_passing_both_raises_type_error(): + """Mixing the new and deprecated names is ambiguous; refuse.""" + from xrspatial.geotiff import read_geotiff_gpu + + with pytest.raises(TypeError, match="pass either 'on_gpu_failure' or"): + read_geotiff_gpu( + "/nonexistent.tif", + on_gpu_failure='strict', + gpu='auto', + ) + + +def test_gpu_alias_bool_no_longer_misleading_value_error(): + """Calling with ``gpu=True`` -- the documented bool from the dispatchers -- + used to raise ``ValueError: gpu must be 'auto' or 'strict', got True``. + The new error explicitly names ``on_gpu_failure`` so the rename is + discoverable from the traceback. + """ + from xrspatial.geotiff import read_geotiff_gpu + + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + with pytest.raises(ValueError, match="on_gpu_failure must be"): + read_geotiff_gpu("/nonexistent.tif", gpu=True) diff --git a/xrspatial/geotiff/tests/test_gpu_strict_fallback_1516.py b/xrspatial/geotiff/tests/test_gpu_strict_fallback_1516.py index 6b06c4a2..ed547a5b 100644 --- a/xrspatial/geotiff/tests/test_gpu_strict_fallback_1516.py +++ b/xrspatial/geotiff/tests/test_gpu_strict_fallback_1516.py @@ -25,6 +25,7 @@ import importlib import sys import types +import warnings import numpy as np import pytest @@ -260,8 +261,14 @@ def test_invalid_gpu_kwarg_rejected(tiled_tiff_path): path, _ = tiled_tiff_path - with pytest.raises(ValueError, match="gpu must be 'auto' or 'strict'"): - read_geotiff_gpu(path, gpu='loose') + # The kwarg was renamed to ``on_gpu_failure`` in #1560; the + # validation error now names the new kwarg even when callers + # supply the deprecated ``gpu=`` alias. + with pytest.raises(ValueError, + match="on_gpu_failure must be 'auto' or 'strict'"): + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + read_geotiff_gpu(path, gpu='loose') finally: if inserted_stub: _restore_cupy() From f5d6d970445808267862afe321c4c5501528eb6f Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Mon, 11 May 2026 09:15:17 -0700 Subject: [PATCH 2/2] Detect both-kwargs via sentinel in read_geotiff_gpu Copilot caught a hole in the both-supplied check: the old guard ``if on_gpu_failure != 'auto'`` only fired when the new kwarg was non-default, so calls like ``on_gpu_failure='auto', gpu='strict'`` (or ``on_gpu_failure='auto', gpu='auto'``) silently accepted the deprecated value, contradicting the documented contract. Use a sentinel for both kwargs so the both-supplied case is rejected regardless of values. Add parametrized regression tests for the three previously-silent value combinations. Also widen the accepted-exceptions tuple in test_gpu_alias_accepts_old_values_without_validation_error to include ImportError, so it stays meaningful in CPU-only CI (where ``import cupy`` raises after validation passes). --- xrspatial/geotiff/__init__.py | 32 ++++++++++------- .../tests/test_gpu_kwarg_rename_1560.py | 36 ++++++++++++++++--- 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index 6bed409c..85691396 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -660,10 +660,14 @@ def _apply_nodata_mask_gpu(arr_gpu, nodata): ) -# Sentinel distinguishing "user passed gpu= explicitly" from "user passed -# nothing". A plain default of None would not work because None is itself -# a value a caller could supply. +# Sentinels distinguishing "user passed this kwarg explicitly" from "user +# passed nothing". A plain default of None would not work because None is +# itself a value a caller could supply. ``read_geotiff_gpu`` needs both +# sentinels so it can tell whether the deprecated ``gpu=`` and the new +# ``on_gpu_failure=`` were *each* supplied, and refuse the ambiguous +# both-supplied case regardless of which values were chosen. _GPU_DEPRECATED_SENTINEL = object() +_ON_GPU_FAILURE_SENTINEL = object() # TIFF type ids needed when synthesizing extra_tags entries from attrs. @@ -1815,7 +1819,7 @@ def read_geotiff_gpu(source: str, *, name: str | None = None, chunks: int | tuple | None = None, max_pixels: int | None = None, - on_gpu_failure: str = 'auto', + on_gpu_failure=_ON_GPU_FAILURE_SENTINEL, gpu=_GPU_DEPRECATED_SENTINEL) -> xr.DataArray: """Read a GeoTIFF with GPU-accelerated decompression via Numba CUDA. @@ -1877,14 +1881,16 @@ def read_geotiff_gpu(source: str, *, xr.DataArray CuPy-backed DataArray on GPU device. """ - if gpu is not _GPU_DEPRECATED_SENTINEL: - # Caller passed gpu= explicitly. Forward to on_gpu_failure unless - # both were supplied; in that case the intent is ambiguous and we - # refuse rather than silently picking one. - if on_gpu_failure != 'auto': - raise TypeError( - "read_geotiff_gpu: pass either 'on_gpu_failure' or the " - "deprecated 'gpu' alias, not both.") + new_passed = on_gpu_failure is not _ON_GPU_FAILURE_SENTINEL + old_passed = gpu is not _GPU_DEPRECATED_SENTINEL + if new_passed and old_passed: + # Both supplied is ambiguous regardless of which values were + # chosen (including the matching ``on_gpu_failure='auto', + # gpu='auto'`` pair). Refuse rather than silently picking one. + raise TypeError( + "read_geotiff_gpu: pass either 'on_gpu_failure' or the " + "deprecated 'gpu' alias, not both.") + if old_passed: warnings.warn( "read_geotiff_gpu(..., gpu=...) is deprecated; use " "on_gpu_failure=... instead. The kwarg was renamed because " @@ -1895,6 +1901,8 @@ def read_geotiff_gpu(source: str, *, stacklevel=2, ) on_gpu_failure = gpu + elif not new_passed: + on_gpu_failure = 'auto' gpu = on_gpu_failure if gpu not in ('auto', 'strict'): raise ValueError( diff --git a/xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py b/xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py index 2f8f5f69..cdd0229c 100644 --- a/xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py +++ b/xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py @@ -54,14 +54,20 @@ def test_gpu_alias_accepts_old_values_without_validation_error(): with warnings.catch_warnings(): # Suppress the deprecation noise; we only care that the value - # passes validation and the call proceeds to the file-read stage - # (which will fail with a missing-file error, not a ValueError). + # passes validation and the call proceeds past the value check. + # In CPU-only CI the next step is ``import cupy`` which raises + # ``ImportError`` (cupy is an optional extra); on a GPU host it + # gets to the file-read stage and raises ``FileNotFoundError``. + # Either is fine: both mean validation passed. warnings.simplefilter("ignore", DeprecationWarning) - with pytest.raises((FileNotFoundError, OSError, ValueError)) as exc_info: + with pytest.raises( + (FileNotFoundError, OSError, ValueError, ImportError) + ) as exc_info: read_geotiff_gpu("/nonexistent.tif", gpu='strict') # The validation ValueError carries our exact message; a generic - # file-read failure is fine because it means validation passed. + # file-read or cupy-import failure is fine because it means + # validation passed. if isinstance(exc_info.value, ValueError): assert "on_gpu_failure must be" not in str(exc_info.value) @@ -78,6 +84,28 @@ def test_passing_both_raises_type_error(): ) +@pytest.mark.parametrize("on_gpu_failure_val,gpu_val", [ + ('auto', 'strict'), + ('auto', 'auto'), + ('strict', 'strict'), +]) +def test_passing_both_raises_regardless_of_values(on_gpu_failure_val, gpu_val): + """Both-supplied is rejected even when ``on_gpu_failure='auto'``. + + A sentinel-based detection (rather than ``!= 'auto'``) catches the + case where the caller passes the default value explicitly alongside + the deprecated alias. + """ + from xrspatial.geotiff import read_geotiff_gpu + + with pytest.raises(TypeError, match="pass either 'on_gpu_failure' or"): + read_geotiff_gpu( + "/nonexistent.tif", + on_gpu_failure=on_gpu_failure_val, + gpu=gpu_val, + ) + + def test_gpu_alias_bool_no_longer_misleading_value_error(): """Calling with ``gpu=True`` -- the documented bool from the dispatchers -- used to raise ``ValueError: gpu must be 'auto' or 'strict', got True``.