Rename read_geotiff_gpu gpu kwarg to on_gpu_failure (#1560) (#1590)

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

* 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).

Author: brendancol

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,16 @@ def _apply_nodata_mask_gpu(arr_gpu, nodata):
 )
 
 
+# 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.
 _TIFF_BYTE = 1
 _TIFF_ASCII = 2
@@ -1813,7 +1823,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=_ON_GPU_FAILURE_SENTINEL,
+                     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
@@ -1839,7 +1850,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``
@@ -1859,18 +1870,47 @@ 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.
     """
+    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 "
+            "'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
+    elif not new_passed:
+        on_gpu_failure = 'auto'
+    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:

xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py

@@ -0,0 +1,120 @@
+"""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 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, ImportError)
+        ) as exc_info:
+            read_geotiff_gpu("/nonexistent.tif", gpu='strict')
+
+    # The validation ValueError carries our exact message; a generic
+    # 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)
+
+
+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',
+        )
+
+
+@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``.
+    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)

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()