diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index 3949d219..85691396 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,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 @@ -1809,7 +1819,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 @@ -1835,7 +1846,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 +1866,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: 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..cdd0229c --- /dev/null +++ b/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) 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()