Address PR #1667 review: __all__, traceback preservation, test strict promotion

Five Copilot review comments on PR #1667:

1. Export GeoTIFFFallbackWarning via __all__ so it surfaces in
   ``from xrspatial.geotiff import *`` and matches the docstring claim
   that it's a user-facing warning type. Update the public-API
   regression test in tests/test_features.py to keep the expected set
   in sync.

2. Preserve the original traceback when ``_warn_or_raise_gpu_fallback``
   would raise in strict mode. The helper used to do ``raise exc`` from
   its own frame, which reset ``__traceback__`` to the helper. It now
   returns ``True`` in strict mode and each call site does a bare
   ``raise`` from its ``except`` block, keeping the original traceback
   intact. All nine call sites in ``_gpu_decode.py`` were updated.

3. Drop the unused ``import os`` from ``test_strict_mode_1662.py``.

4. Tighten the GPU gating in ``test_strict_mode_1662.py`` from a bare
   ``importlib.util.find_spec('cupy')`` check to a ``_gpu_available()``
   helper that also confirms ``cupy.cuda.is_available()`` -- matches
   the pattern used in the other GPU-only geotiff tests.

5. Rewrite ``test_read_geotiff_gpu_env_var_promotes_to_strict`` so it
   actually exercises the env-var promotion. The previous version
   passed a non-existent path which raised regardless of strict mode.
   The new version writes a real on-disk TIF, monkeypatches
   ``gpu_decode_tiles_from_file`` and ``gpu_decode_tiles`` on the
   ``_gpu_decode`` module to raise, then asserts that the default mode
   returns a CuPy DataArray (CPU fallback path) and that
   ``XRSPATIAL_GEOTIFF_STRICT=1`` re-raises the patched error.

Author: brendancol

xrspatial/geotiff/__init__.py

@@ -50,6 +50,7 @@
 # is intentionally omitted: it is deprecated in favour of ``da.xrs.plot()``
 # and emits a ``DeprecationWarning`` when called.
 __all__ = [
+    'GeoTIFFFallbackWarning',
     'open_geotiff',
     'read_geotiff_gpu',
     'read_geotiff_dask',

xrspatial/geotiff/_gpu_decode.py

@@ -13,24 +13,28 @@
 from numba import cuda
 
 
-def _warn_or_raise_gpu_fallback(stage: str, exc: BaseException) -> None:
+def _warn_or_raise_gpu_fallback(stage: str, exc: BaseException) -> bool:
     """Report a GPU helper falling back to None (issue #1662).
 
-    Under ``XRSPATIAL_GEOTIFF_STRICT=1`` the original exception is
-    re-raised so CI catches silent fast-path regressions. In the default
-    mode a ``GeoTIFFFallbackWarning`` is emitted with the exception type
-    and message; the caller still receives ``None`` and chooses its own
-    next step (typically another decoder or CPU fallback).
+    Returns ``True`` when ``XRSPATIAL_GEOTIFF_STRICT=1`` is set; callers
+    should then re-raise the live exception with a bare ``raise`` so the
+    original traceback is preserved (re-raising ``exc`` here would reset
+    ``__traceback__`` to this helper's frame). Returns ``False`` in the
+    default mode after emitting a ``GeoTIFFFallbackWarning`` with the
+    exception type and message; the caller still returns ``None`` and
+    chooses its own next step (typically another decoder or CPU
+    fallback).
     """
     from . import _geotiff_strict_mode, GeoTIFFFallbackWarning
     if _geotiff_strict_mode():
-        raise exc
+        return True
     warnings.warn(
         f"{stage} fell back to None "
         f"({type(exc).__name__}: {exc}).",
         GeoTIFFFallbackWarning,
         stacklevel=3,
     )
+    return False
 
 #: Fraction of free GPU memory we're willing to allocate in a single call.
 #: Above this, raise MemoryError up-front so the caller gets an actionable
@@ -972,7 +976,8 @@ def _try_kvikio_read_tiles(file_path, tile_offsets, tile_byte_counts, tile_bytes
             cupy.cuda.Device().synchronize()
         except Exception:
             pass
-        _warn_or_raise_gpu_fallback("_try_kvikio_read_tiles", e)
+        if _warn_or_raise_gpu_fallback("_try_kvikio_read_tiles", e):
+            raise
         return None
 
 
@@ -1076,8 +1081,9 @@ def _try_nvcomp_batch_decompress(compressed_tiles, tile_bytes, compression):
             d_decompressed = manager.decompress(d_compressed)
             return cupy.concatenate([d.ravel() for d in d_decompressed])
         except Exception as e:
-            _warn_or_raise_gpu_fallback(
-                "_try_nvcomp_batch_decompress (kvikio DeflateManager)", e)
+            stage = "_try_nvcomp_batch_decompress (kvikio DeflateManager)"
+            if _warn_or_raise_gpu_fallback(stage, e):
+                raise
             return None
 
     # Direct ctypes nvCOMP C API
@@ -1197,8 +1203,9 @@ class _NvcompDeflateDecompOpts(ctypes.Structure):
         return d_decomp
 
     except Exception as e:
-        _warn_or_raise_gpu_fallback(
-            "_try_nvcomp_batch_decompress (ctypes nvCOMP C API)", e)
+        stage = "_try_nvcomp_batch_decompress (ctypes nvCOMP C API)"
+        if _warn_or_raise_gpu_fallback(stage, e):
+            raise
         return None
 
 
@@ -1376,7 +1383,8 @@ class _NvjpegImage(ctypes.Structure):
                 destroy_fn(nvjpeg_handle)
 
     except Exception as e:
-        _warn_or_raise_gpu_fallback("_try_nvjpeg_batch_decode", e)
+        if _warn_or_raise_gpu_fallback("_try_nvjpeg_batch_decode", e):
+            raise
         return None
 
 
@@ -1521,7 +1529,8 @@ class _NvjpegImage(ctypes.Structure):
                 destroy_fn(nvjpeg_handle)
 
     except Exception as e:
-        _warn_or_raise_gpu_fallback("_nvjpeg_batch_encode", e)
+        if _warn_or_raise_gpu_fallback("_nvjpeg_batch_encode", e):
+            raise
         return None
 
 
@@ -1680,7 +1689,8 @@ class _NvcompDecompOpts(ctypes.Structure):
     except MemoryError:
         raise
     except Exception as e:
-        _warn_or_raise_gpu_fallback("_try_nvcomp_from_device_bufs", e)
+        if _warn_or_raise_gpu_fallback("_try_nvcomp_from_device_bufs", e):
+            raise
         return None
 
 
@@ -2467,7 +2477,8 @@ class _DeflateCompOpts(ctypes.Structure):
         return result
 
     except Exception as e:
-        _warn_or_raise_gpu_fallback("_nvcomp_batch_compress", e)
+        if _warn_or_raise_gpu_fallback("_nvcomp_batch_compress", e):
+            raise
         return None
 
 
@@ -2657,7 +2668,8 @@ class _NvJpeg2kImage(ctypes.Structure):
         return d_all_tiles
 
     except Exception as e:
-        _warn_or_raise_gpu_fallback("_try_nvjpeg2k_batch_decode", e)
+        if _warn_or_raise_gpu_fallback("_try_nvjpeg2k_batch_decode", e):
+            raise
         return None
 
 
@@ -2803,7 +2815,8 @@ class _CompInfo(ctypes.Structure):
         return result
 
     except Exception as e:
-        _warn_or_raise_gpu_fallback("_nvjpeg2k_batch_encode", e)
+        if _warn_or_raise_gpu_fallback("_nvjpeg2k_batch_encode", e):
+            raise
         return None
 
 

xrspatial/geotiff/tests/test_features.py

@@ -2642,6 +2642,7 @@ def test_all_lists_supported_functions(self):
         # public API. If any of these gets removed or renamed, that is a
         # breaking change and should go through a deprecation cycle.
         expected = {
+            'GeoTIFFFallbackWarning',
             'open_geotiff',
             'read_geotiff_gpu',
             'read_geotiff_dask',

xrspatial/geotiff/tests/test_strict_mode_1662.py

@@ -13,13 +13,12 @@
   silent fallback.
 
 The GPU helper sites in ``_gpu_decode.py`` are exercised indirectly
-through ``read_geotiff_gpu`` when CuPy is available; tests gated on
-``importlib.util.find_spec('cupy')`` are skipped otherwise.
+through ``read_geotiff_gpu`` when CuPy + CUDA are available; tests
+gated on ``_gpu_available()`` are skipped otherwise.
 """
 from __future__ import annotations
 
 import importlib.util
-import os
 import warnings
 
 import pytest
@@ -141,14 +140,16 @@ def test_vrt_missing_source_default_warns_then_continues(
     from xrspatial.geotiff import read_vrt
 
     vrt_path = tmp_path / 'mosaic_1662_missing.vrt'
+    missing_src = f'{tmp_path}/does_not_exist_1662.tif'
     vrt_path.write_text(
         '<VRTDataset rasterXSize="4" rasterYSize="4">\n'
         '  <SRS></SRS>\n'
         '  <GeoTransform>0, 1, 0, 0, 0, -1</GeoTransform>\n'
         '  <VRTRasterBand dataType="Float32" band="1">\n'
         '    <NoDataValue>-9999</NoDataValue>\n'
         '    <SimpleSource>\n'
-        f'      <SourceFilename relativeToVRT="0">{tmp_path}/does_not_exist_1662.tif</SourceFilename>\n'
+        f'      <SourceFilename relativeToVRT="0">{missing_src}'
+        '</SourceFilename>\n'
         '      <SourceBand>1</SourceBand>\n'
         '      <SrcRect xOff="0" yOff="0" xSize="4" ySize="4"/>\n'
         '      <DstRect xOff="0" yOff="0" xSize="4" ySize="4"/>\n'
@@ -178,14 +179,16 @@ def test_vrt_missing_source_strict_raises(set_strict_env, tmp_path):
     from xrspatial.geotiff import read_vrt
 
     vrt_path = tmp_path / 'mosaic_1662_missing_strict.vrt'
+    missing_src = f'{tmp_path}/does_not_exist_1662_strict.tif'
     vrt_path.write_text(
         '<VRTDataset rasterXSize="4" rasterYSize="4">\n'
         '  <SRS></SRS>\n'
         '  <GeoTransform>0, 1, 0, 0, 0, -1</GeoTransform>\n'
         '  <VRTRasterBand dataType="Float32" band="1">\n'
         '    <NoDataValue>-9999</NoDataValue>\n'
         '    <SimpleSource>\n'
-        f'      <SourceFilename relativeToVRT="0">{tmp_path}/does_not_exist_1662_strict.tif</SourceFilename>\n'
+        f'      <SourceFilename relativeToVRT="0">{missing_src}'
+        '</SourceFilename>\n'
         '      <SourceBand>1</SourceBand>\n'
         '      <SrcRect xOff="0" yOff="0" xSize="4" ySize="4"/>\n'
         '      <DstRect xOff="0" yOff="0" xSize="4" ySize="4"/>\n'
@@ -204,18 +207,18 @@ def test_vrt_missing_source_strict_raises(set_strict_env, tmp_path):
 # not depend on having a working CUDA/GDS/nvCOMP stack.
 # ---------------------------------------------------------------------------
 
-CUPY_AVAILABLE = importlib.util.find_spec('cupy') is not None
-
 
 def test_warn_or_raise_gpu_fallback_default_warns(clear_strict_env):
     """Default mode emits one GeoTIFFFallbackWarning carrying type + msg."""
     from xrspatial.geotiff._gpu_decode import _warn_or_raise_gpu_fallback
 
     with warnings.catch_warnings(record=True) as w:
         warnings.simplefilter('always')
-        _warn_or_raise_gpu_fallback(
+        result = _warn_or_raise_gpu_fallback(
             "_try_nvjpeg_batch_decode", RuntimeError("bogus 1662"))
 
+    # Default mode returns False so the caller falls back to None.
+    assert result is False
     fallback_warnings = [
         x for x in w if issubclass(x.category, GeoTIFFFallbackWarning)
     ]
@@ -226,30 +229,128 @@ def test_warn_or_raise_gpu_fallback_default_warns(clear_strict_env):
     assert 'bogus 1662' in msg
 
 
-def test_warn_or_raise_gpu_fallback_strict_reraises(set_strict_env):
-    """Strict mode re-raises the original exception."""
+def test_warn_or_raise_gpu_fallback_strict_returns_true(set_strict_env):
+    """Strict mode returns True so the caller can ``raise`` itself.
+
+    The helper deliberately does not raise here -- re-raising the
+    captured exception from inside this frame would clobber the
+    original traceback. Returning True lets each call site bubble the
+    live exception up via a bare ``raise`` from its own ``except``
+    block.
+    """
     from xrspatial.geotiff._gpu_decode import _warn_or_raise_gpu_fallback
 
-    with pytest.raises(RuntimeError, match='bogus 1662 strict'):
-        _warn_or_raise_gpu_fallback(
+    with warnings.catch_warnings(record=True) as w:
+        warnings.simplefilter('always')
+        result = _warn_or_raise_gpu_fallback(
             "_try_nvjpeg_batch_decode", RuntimeError("bogus 1662 strict"))
 
+    assert result is True
+    # No warning should be emitted in strict mode.
+    fallback_warnings = [
+        x for x in w if issubclass(x.category, GeoTIFFFallbackWarning)
+    ]
+    assert fallback_warnings == []
+
+
+def test_warn_or_raise_gpu_fallback_preserves_traceback(set_strict_env):
+    """The call-site pattern preserves the original exception traceback.
+
+    The helper returns True in strict mode; the caller re-raises with a
+    bare ``raise``. The resulting traceback should point at the original
+    failure (``raise RuntimeError(...)`` below), not at the helper.
+    """
+    from xrspatial.geotiff._gpu_decode import _warn_or_raise_gpu_fallback
+
+    def site():
+        try:
+            raise RuntimeError("bogus 1662 traceback")
+        except Exception as e:
+            if _warn_or_raise_gpu_fallback("_dummy_stage", e):
+                raise
+            return None
+
+    with pytest.raises(RuntimeError, match='bogus 1662 traceback') as excinfo:
+        site()
+    # The deepest traceback frame should be ``site``'s ``raise
+    # RuntimeError`` line, not ``_warn_or_raise_gpu_fallback``.
+    tb = excinfo.tb
+    while tb.tb_next is not None:
+        tb = tb.tb_next
+    assert tb.tb_frame.f_code.co_name == 'site'
+
 
 # ---------------------------------------------------------------------------
 # read_geotiff_gpu on_gpu_failure='auto' + env var integration
 # ---------------------------------------------------------------------------
 
+def _gpu_available() -> bool:
+    if importlib.util.find_spec("cupy") is None:
+        return False
+    try:
+        import cupy
+        return bool(cupy.cuda.is_available())
+    except Exception:
+        return False
+
+
+_HAS_GPU = _gpu_available()
+
+
 @pytest.mark.skipif(
-    not CUPY_AVAILABLE,
-    reason="cupy required for read_geotiff_gpu fallback test",
+    not _HAS_GPU,
+    reason="cupy + CUDA required for read_geotiff_gpu fallback test",
 )
-def test_read_geotiff_gpu_env_var_promotes_to_strict(set_strict_env, tmp_path):
+def test_read_geotiff_gpu_env_var_promotes_to_strict(monkeypatch, tmp_path):
     """With on_gpu_failure='auto' but XRSPATIAL_GEOTIFF_STRICT=1, a GPU
-    decode failure surfaces instead of falling back to CPU."""
-    from xrspatial.geotiff import read_geotiff_gpu
-
-    # A non-existent path triggers a failure before any decode runs;
-    # the env var should still bubble it up.
-    bogus = str(tmp_path / 'no_such_file_1662_promote.tif')
-    with pytest.raises(Exception):
-        read_geotiff_gpu(bogus)
+    decode failure surfaces instead of falling back to CPU.
+
+    The seam: ``read_geotiff_gpu`` does a local
+    ``from ._gpu_decode import gpu_decode_tiles_from_file`` inside the
+    function body, so rebinding the attribute on the
+    ``xrspatial.geotiff._gpu_decode`` module is picked up on the next
+    call. Stubbing it to raise lets us exercise both branches against
+    a real on-disk TIF without needing a broken GPU stack.
+    """
+    import cupy as cp
+    import numpy as np
+    import xarray as xr
+
+    from xrspatial.geotiff import read_geotiff_gpu, to_geotiff
+    from xrspatial.geotiff import _gpu_decode
+
+    # 1. Write a small valid TIF so the metadata parse succeeds and we
+    # reach the GPU decode stage.
+    h, w = 16, 16
+    arr = np.arange(h * w, dtype=np.float32).reshape(h, w)
+    y = np.arange(h, dtype=np.float64) * -1.0 + 100.0
+    x = np.arange(w, dtype=np.float64) * 1.0 + 0.0
+    da = xr.DataArray(arr, dims=['y', 'x'], coords={'y': y, 'x': x})
+    p = str(tmp_path / 'strict_promote_1662.tif')
+    to_geotiff(da, p, crs=4326)
+
+    sentinel = "bogus 1662 promote"
+
+    def _boom(*args, **kwargs):
+        raise RuntimeError(sentinel)
+
+    monkeypatch.setattr(
+        _gpu_decode, 'gpu_decode_tiles_from_file', _boom)
+    monkeypatch.setattr(
+        _gpu_decode, 'gpu_decode_tiles', _boom)
+
+    # 2. Default mode: XRSPATIAL_GEOTIFF_STRICT unset, the failure
+    # should be absorbed and the CPU fallback should return a
+    # CuPy-backed DataArray.
+    monkeypatch.delenv('XRSPATIAL_GEOTIFF_STRICT', raising=False)
+    with warnings.catch_warnings():
+        warnings.simplefilter('ignore')
+        result = read_geotiff_gpu(p)
+    assert isinstance(result, xr.DataArray)
+    assert isinstance(result.data, cp.ndarray)
+
+    # 3. With XRSPATIAL_GEOTIFF_STRICT=1, the same call should re-raise
+    # the patched RuntimeError instead of falling back.
+    monkeypatch.setenv('XRSPATIAL_GEOTIFF_STRICT', '1')
+    with pytest.raises(RuntimeError, match=sentinel):
+        read_geotiff_gpu(p)