From 6ed25beebf7d5cb787e2cba3f22fb61e82713457 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 12 May 2026 10:53:31 -0700 Subject: [PATCH 1/2] Reject overview_level / on_gpu_failure on VRT sources (#1685) open_geotiff documents overview_level and on_gpu_failure as supported, but the VRT dispatch branch routes through read_vrt, whose signature accepts neither. Callers got full-resolution data (overview_level) or CPU-decoded output (on_gpu_failure) with no warning. Issue #1561 fixed the same class of silent kwarg drop for the dask and GPU dispatchers; this commit closes the VRT gap by rejecting both combinations up front with the same wording other VRT-incompatible kwargs already use. Add regressions for the new error path and a smoke test that confirms non-VRT sources still accept overview_level. --- xrspatial/geotiff/__init__.py | 23 +++++ .../test_open_geotiff_vrt_kwarg_drop_1685.py | 98 +++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 xrspatial/geotiff/tests/test_open_geotiff_vrt_kwarg_drop_1685.py diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index 517e9a9d..03829991 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -611,6 +611,29 @@ def open_geotiff(source, *, dtype=None, # VRT files (string paths only -- VRT XML references other files on disk) if isinstance(source, str) and source.lower().endswith('.vrt'): + # ``read_vrt`` does not accept ``overview_level`` (the VRT XML + # references its own source files; overview selection would need + # to apply to each one). Silently dropping the kwarg was the same + # class of bug issue #1561 fixed for the dask and GPU dispatchers, + # so refuse the combination up front rather than handing the + # caller a full-resolution mosaic with no warning. See issue #1685. + if overview_level is not None: + raise ValueError( + "overview_level is not supported for VRT sources. " + "VRT references its own source files; pass overview_level " + "to open_geotiff on a .tif source, or drop the kwarg.") + # ``on_gpu_failure`` only routes through ``read_geotiff_gpu``. + # ``read_vrt`` has no analogous failure policy, so any value the + # caller supplied alongside a VRT source would be silently lost. + # The ``gpu=False`` branch is already rejected above; this catches + # the ``gpu=True, source.endswith('.vrt')`` case the earlier check + # lets through. + if on_gpu_failure is not _ON_GPU_FAILURE_SENTINEL: + raise ValueError( + "on_gpu_failure is not supported for VRT sources. " + "VRT reads do not go through the GPU decoder pipeline; " + "drop the kwarg or call read_geotiff_gpu directly on a " + ".tif source.") return read_vrt(source, dtype=dtype, window=window, band=band, name=name, chunks=chunks, gpu=gpu, max_pixels=max_pixels) diff --git a/xrspatial/geotiff/tests/test_open_geotiff_vrt_kwarg_drop_1685.py b/xrspatial/geotiff/tests/test_open_geotiff_vrt_kwarg_drop_1685.py new file mode 100644 index 00000000..266d2e66 --- /dev/null +++ b/xrspatial/geotiff/tests/test_open_geotiff_vrt_kwarg_drop_1685.py @@ -0,0 +1,98 @@ +"""Regression test for #1685: ``open_geotiff`` silently dropped +``overview_level`` and ``on_gpu_failure`` when the source was a VRT. + +The api-consistency sweep on 2026-05-12 flagged that ``open_geotiff`` +documents both kwargs as supported, but the VRT dispatch branch routes +to ``read_vrt`` whose signature accepts neither. Calls like +``open_geotiff('mosaic.vrt', overview_level=2)`` returned full-resolution +data with no warning. Issue #1561 fixed the same class of bug for the +dask and GPU dispatch branches; this one closes the remaining gap by +refusing the unsupported combinations up front. +""" +from __future__ import annotations + +import numpy as np +import pytest +import xarray as xr + +from xrspatial.geotiff import open_geotiff, to_geotiff, write_vrt + + +@pytest.fixture +def small_vrt(tmp_path): + """Two-tile uint16 VRT we can hand to ``open_geotiff``.""" + arr_a = np.arange(16, dtype=np.uint16).reshape(4, 4) + da_a = xr.DataArray( + arr_a, + dims=["y", "x"], + coords={ + "y": np.array([0.5, 1.5, 2.5, 3.5]), + "x": np.array([0.5, 1.5, 2.5, 3.5]), + }, + attrs={"crs": 4326}, + ) + tile_a = tmp_path / "tile_a.tif" + to_geotiff(da_a, str(tile_a)) + + arr_b = np.arange(16, 32, dtype=np.uint16).reshape(4, 4) + da_b = xr.DataArray( + arr_b, + dims=["y", "x"], + coords={ + "y": np.array([0.5, 1.5, 2.5, 3.5]), + "x": np.array([4.5, 5.5, 6.5, 7.5]), + }, + attrs={"crs": 4326}, + ) + tile_b = tmp_path / "tile_b.tif" + to_geotiff(da_b, str(tile_b)) + + vrt_path = tmp_path / "mosaic.vrt" + write_vrt(str(vrt_path), [str(tile_a), str(tile_b)]) + return str(vrt_path) + + +def test_open_geotiff_vrt_rejects_overview_level(small_vrt): + """``overview_level`` plus ``.vrt`` raises ValueError, not a silent drop.""" + with pytest.raises(ValueError, match="overview_level is not supported"): + open_geotiff(small_vrt, overview_level=1) + + +def test_open_geotiff_vrt_rejects_on_gpu_failure_with_gpu_true(small_vrt): + """``on_gpu_failure='strict'`` plus ``.vrt`` (gpu=True) is refused.""" + # The check fires before any GPU code runs; no CUDA needed. + with pytest.raises(ValueError, match="on_gpu_failure is not supported"): + open_geotiff(small_vrt, gpu=True, on_gpu_failure="strict") + + +def test_open_geotiff_vrt_without_unsupported_kwargs_still_works(small_vrt): + """The previously-accepted kwargs still flow through to ``read_vrt``.""" + da = open_geotiff(small_vrt) + # Two 4x4 tiles side-by-side; result is 4x8. + assert da.shape == (4, 8) + + +def test_open_geotiff_vrt_with_window_still_works(small_vrt): + """``window`` was already forwarded; this regression should not break it.""" + da = open_geotiff(small_vrt, window=(0, 1, 4, 5)) + assert da.shape == (4, 4) + + +def test_open_geotiff_non_vrt_still_accepts_overview_level(tmp_path): + """The fix is VRT-specific; ``.tif`` sources keep accepting overview_level.""" + # Build a single COG with one overview so overview_level=0 round-trips. + arr = np.arange(64, dtype=np.uint16).reshape(8, 8) + da = xr.DataArray( + arr, + dims=["y", "x"], + coords={ + "y": np.arange(8, dtype=np.float64), + "x": np.arange(8, dtype=np.float64), + }, + attrs={"crs": 4326}, + ) + tif_path = tmp_path / "with_ovr.tif" + to_geotiff(da, str(tif_path), cog=True, tile_size=4, overview_levels=[2]) + # Either overview_level value must be accepted without raising. + open_geotiff(str(tif_path), overview_level=0) + open_geotiff(str(tif_path), overview_level=1) From 93cee55fa150a03f8324484d89e1a8bd0de6b419 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 12 May 2026 11:15:06 -0700 Subject: [PATCH 2/2] Treat overview_level=0 as a no-op for VRT sources Addresses Copilot review on PR #1689. ``overview_level=0`` is documented as "full resolution" (the default), so passing it on a VRT is semantically equivalent to omitting the kwarg and should not raise. The guard now rejects only non-zero overview levels for VRT sources. The regression test now also covers the ``overview_level=0`` case to prevent the guard from regressing back to ``is not None``. --- xrspatial/geotiff/__init__.py | 5 ++++- .../tests/test_open_geotiff_vrt_kwarg_drop_1685.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index 03829991..70f35984 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -617,7 +617,10 @@ def open_geotiff(source, *, dtype=None, # class of bug issue #1561 fixed for the dask and GPU dispatchers, # so refuse the combination up front rather than handing the # caller a full-resolution mosaic with no warning. See issue #1685. - if overview_level is not None: + # ``overview_level=0`` is documented as "full resolution" (the + # default), so treat it as a no-op the same as ``None`` rather + # than rejecting a kwarg value the caller could have omitted. + if overview_level not in (None, 0): raise ValueError( "overview_level is not supported for VRT sources. " "VRT references its own source files; pass overview_level " diff --git a/xrspatial/geotiff/tests/test_open_geotiff_vrt_kwarg_drop_1685.py b/xrspatial/geotiff/tests/test_open_geotiff_vrt_kwarg_drop_1685.py index 266d2e66..10bf9ba0 100644 --- a/xrspatial/geotiff/tests/test_open_geotiff_vrt_kwarg_drop_1685.py +++ b/xrspatial/geotiff/tests/test_open_geotiff_vrt_kwarg_drop_1685.py @@ -58,6 +58,16 @@ def test_open_geotiff_vrt_rejects_overview_level(small_vrt): open_geotiff(small_vrt, overview_level=1) +def test_open_geotiff_vrt_accepts_overview_level_zero(small_vrt): + """``overview_level=0`` is documented as full resolution (the default), + so passing it on a VRT is semantically equivalent to omitting the kwarg + and must not raise. Only non-zero overview levels are rejected. + """ + da = open_geotiff(small_vrt, overview_level=0) + # Same shape as the no-kwarg case: two 4x4 tiles side-by-side. + assert da.shape == (4, 8) + + def test_open_geotiff_vrt_rejects_on_gpu_failure_with_gpu_true(small_vrt): """``on_gpu_failure='strict'`` plus ``.vrt`` (gpu=True) is refused.""" # The check fires before any GPU code runs; no CUDA needed.