Typed warnings + XRSPATIAL_GEOTIFF_STRICT for silent geotiff fallbacks (#1662)#1667
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1662 by making previously-silent GeoTIFF “fallback” paths visible via a typed warning (GeoTIFFFallbackWarning) and adding an opt-in strict mode (XRSPATIAL_GEOTIFF_STRICT) that converts those fallbacks into raised exceptions (including promoting read_geotiff_gpu(on_gpu_failure='auto') to strict behavior).
Changes:
- Introduces
GeoTIFFFallbackWarningplus_geotiff_strict_mode()and applies warn-vs-raise behavior to_wkt_to_epsg,_epsg_to_wkt, VRT source skips, and multiple GPU helper fallbacks. - Adds
_warn_or_raise_gpu_fallback()to standardize GPU helper fallback reporting and integrates strict mode intoread_geotiff_gpufailure handling. - Adds a new regression test module and updates docs with a “Strict mode” section describing
XRSPATIAL_GEOTIFF_STRICT.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
xrspatial/geotiff/__init__.py |
Adds strict-mode env var helper + new GeoTIFFFallbackWarning; strict mode also forces GPU failure policy to raise. |
xrspatial/geotiff/_geotags.py |
Emits GeoTIFFFallbackWarning (or raises in strict mode) when EPSG→WKT resolution fails. |
xrspatial/geotiff/_vrt.py |
Emits GeoTIFFFallbackWarning (or raises in strict mode) when a VRT source tile can’t be read. |
xrspatial/geotiff/_gpu_decode.py |
Adds _warn_or_raise_gpu_fallback() and uses it in several GPU helper except Exception: return None fallback sites. |
xrspatial/geotiff/tests/test_strict_mode_1662.py |
Adds tests pinning the warn-vs-raise contract for strict mode and fallback warning contents. |
docs/source/reference/geotiff.rst |
Documents strict mode and the XRSPATIAL_GEOTIFF_STRICT env var behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+79
to
+86
| class GeoTIFFFallbackWarning(UserWarning): | ||
| """Warning emitted when a geotiff helper falls back to a slower path. | ||
|
|
||
| Raised in the same call sites that would silently return ``None`` under | ||
| the historic ``except Exception: return None`` pattern. See issue #1662 | ||
| for the audit and the ``XRSPATIAL_GEOTIFF_STRICT=1`` env var that | ||
| promotes these warnings to exceptions. | ||
| """ |
Comment on lines
+16
to
+33
| def _warn_or_raise_gpu_fallback(stage: str, exc: BaseException) -> None: | ||
| """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). | ||
| """ | ||
| from . import _geotiff_strict_mode, GeoTIFFFallbackWarning | ||
| if _geotiff_strict_mode(): | ||
| raise exc | ||
| warnings.warn( | ||
| f"{stage} fell back to None " | ||
| f"({type(exc).__name__}: {exc}).", | ||
| GeoTIFFFallbackWarning, | ||
| stacklevel=3, | ||
| ) |
| from __future__ import annotations | ||
|
|
||
| import importlib.util | ||
| import os |
Comment on lines
+207
to
+246
| 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( | ||
| "_try_nvjpeg_batch_decode", RuntimeError("bogus 1662")) | ||
|
|
||
| fallback_warnings = [ | ||
| x for x in w if issubclass(x.category, GeoTIFFFallbackWarning) | ||
| ] | ||
| assert len(fallback_warnings) == 1 | ||
| msg = str(fallback_warnings[0].message) | ||
| assert '_try_nvjpeg_batch_decode' in msg | ||
| assert 'RuntimeError' in msg | ||
| assert 'bogus 1662' in msg | ||
|
|
||
|
|
||
| def test_warn_or_raise_gpu_fallback_strict_reraises(set_strict_env): | ||
| """Strict mode re-raises the original exception.""" | ||
| from xrspatial.geotiff._gpu_decode import _warn_or_raise_gpu_fallback | ||
|
|
||
| with pytest.raises(RuntimeError, match='bogus 1662 strict'): | ||
| _warn_or_raise_gpu_fallback( | ||
| "_try_nvjpeg_batch_decode", RuntimeError("bogus 1662 strict")) | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # read_geotiff_gpu on_gpu_failure='auto' + env var integration | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| @pytest.mark.skipif( | ||
| not CUPY_AVAILABLE, | ||
| reason="cupy required for read_geotiff_gpu fallback test", | ||
| ) | ||
| def test_read_geotiff_gpu_env_var_promotes_to_strict(set_strict_env, tmp_path): |
Comment on lines
+246
to
+255
| def test_read_geotiff_gpu_env_var_promotes_to_strict(set_strict_env, 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) |
brendancol
added a commit
that referenced
this pull request
May 12, 2026
… 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.
…1662) Each broad `except Exception: return None` site flagged in #1662 now emits a GeoTIFFFallbackWarning carrying the original exception type and message. The new XRSPATIAL_GEOTIFF_STRICT=1 env var (read by _geotiff_strict_mode()) re-raises instead of warning so CI can fail on silent GPU or VRT fallbacks. Sites covered: - _wkt_to_epsg, _epsg_to_wkt: pyproj-missing vs broken-input - _vrt.py source read skip: surface missing-tile holes - _gds_read_tiles, _try_nvcomp_batch_decompress (kvikio + ctypes), _try_nvjpeg_batch_decode, _nvjpeg_batch_encode, _try_nvcomp_from_device_bufs, _nvcomp_batch_compress, _try_nvjpeg2k_batch_decode, _nvjpeg2k_batch_encode - read_geotiff_gpu(on_gpu_failure='auto') stages also honor the env var
21 tests covering the warn-vs-raise contract: - _geotiff_strict_mode() env var parsing (truthy + falsy values) - _wkt_to_epsg / _epsg_to_wkt default-warns vs strict-raises - VRT missing-source default-warns-then-continues vs strict-raises - _warn_or_raise_gpu_fallback default + strict for GPU helper sites - read_geotiff_gpu(on_gpu_failure='auto') honors XRSPATIAL_GEOTIFF_STRICT=1
… 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1662.
except Exception: return Nonesite flagged in the Replace silent except-return-None in geotiff with typed exceptions + strict mode #1662 audit now emits aGeoTIFFFallbackWarningcarrying the original exception type and message. Affected helpers:_wkt_to_epsg,_epsg_to_wkt, the VRT source-skip loop in_vrt.py, and eight GPU helpers in_gpu_decode.py(GDS, nvCOMP kvikio + ctypes, nvJPEG decode + encode, nvCOMP from-device-bufs, nvCOMP batch compress, nvJPEG2000 decode + encode).XRSPATIAL_GEOTIFF_STRICT=1env var (read by_geotiff_strict_mode()) re-raises instead of warning. The same env var also promotesread_geotiff_gpu(on_gpu_failure='auto')to strict so CI can fail on silent GPU fallbacks without touching every call site.test_strict_mode_1662.pypin the warn-vs-raise contract for each entry point.Out of scope:
except Exception: passcleanup in_try_kvikio_read_tiles(a failedcupy.cuda.Device().synchronize()during error recovery) stays broad; there is no useful next step.ImportErrorguards on optional deps still returnNonequietly. Those are real "feature not available" branches, not silent fast-path failures.Test plan
pytest xrspatial/geotiff/tests/test_strict_mode_1662.py -v(21 pass)pytest xrspatial/geotiff/tests/(1364 pass, 3 skipped; one unrelated matplotlib deepcopy recursion intest_features.pyexcluded)XRSPATIAL_GEOTIFF_STRICT=1 pytest xrspatial/geotiff/tests/on a box with GPU + kvikio to confirm no spurious strict-mode failures