From b0ae2f3ec09519cba6e2a0a2ca388f36dc65bd0a Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Fri, 15 May 2026 07:33:49 -0700 Subject: [PATCH] geotiff: reader kwarg order matches open_geotiff (#1935) read_geotiff_gpu and read_geotiff_dask listed shared keyword-only params in different orders than open_geotiff. The kwargs are all keyword-only so callers were not broken, but inspect.signature, IDE autocomplete, and Sphinx docs all showed the drift. Pick open_geotiff as the canonical reference (the public dispatcher). Swap window and overview_level in read_geotiff_gpu. Reorder read_geotiff_dask so window, overview_level, band, name, chunks, max_pixels match the canonical position. read_vrt already conformed and is covered by the new regression test. Regression test (test_reader_kwarg_order_1935.py) pins each reader's kw-only param order via inspect.signature and asserts no pairwise inversions across the four readers, so future kwargs cannot be added in arbitrary positions. Mirrors the writer-side fix in #1922 / #1925. --- xrspatial/geotiff/_backends/dask.py | 8 +- xrspatial/geotiff/_backends/gpu.py | 2 +- .../tests/test_reader_kwarg_order_1935.py | 150 ++++++++++++++++++ 3 files changed, 155 insertions(+), 5 deletions(-) create mode 100644 xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py diff --git a/xrspatial/geotiff/_backends/dask.py b/xrspatial/geotiff/_backends/dask.py index da930cbd5..9b14a9ddc 100644 --- a/xrspatial/geotiff/_backends/dask.py +++ b/xrspatial/geotiff/_backends/dask.py @@ -29,12 +29,12 @@ def read_geotiff_dask(source: str, *, dtype: str | np.dtype | None = None, - chunks: int | tuple = 512, - overview_level: int | None = None, window: tuple | None = None, + overview_level: int | None = None, band: int | None = None, - max_pixels: int | None = None, - name: str | None = None) -> xr.DataArray: + name: str | None = None, + chunks: int | tuple = 512, + max_pixels: int | None = None) -> xr.DataArray: """Read a GeoTIFF as a dask-backed DataArray for out-of-core processing. Each chunk is loaded lazily via windowed reads. diff --git a/xrspatial/geotiff/_backends/gpu.py b/xrspatial/geotiff/_backends/gpu.py index 30a61f28c..4c2db013c 100644 --- a/xrspatial/geotiff/_backends/gpu.py +++ b/xrspatial/geotiff/_backends/gpu.py @@ -69,8 +69,8 @@ def _preflight_cuda_runtime(cupy) -> None: def read_geotiff_gpu(source: str, *, dtype: str | np.dtype | None = None, - overview_level: int | None = None, window: tuple | None = None, + overview_level: int | None = None, band: int | None = None, name: str | None = None, chunks: int | tuple | None = None, diff --git a/xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py b/xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py new file mode 100644 index 000000000..d099652c1 --- /dev/null +++ b/xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py @@ -0,0 +1,150 @@ +"""Regression test for #1935: public reader entry points share a canonical +keyword-only parameter order. + +``open_geotiff`` is the canonical surface. The three backend readers +(``read_geotiff_gpu``, ``read_geotiff_dask``, ``read_vrt``) must list the +shared kwargs in the same relative order so ``inspect.signature``, IDE +autocomplete, and Sphinx-rendered docs do not drift. + +Each per-backend signature carries its own subset of the canonical +parameter list (``read_vrt`` does not take ``overview_level``, +``read_geotiff_dask`` does not take ``gpu``/``on_gpu_failure``, etc.). +The test compares each reader's params with the slice of the canonical +order it actually accepts; backend-specific extras (``read_geotiff_gpu``'s +deprecated ``gpu`` alias) are checked at the tail. + +Prior to #1935: ``read_geotiff_gpu`` had ``overview_level`` before +``window``, ``read_geotiff_dask`` placed ``chunks`` and ``name`` out of +the canonical position. +""" +from __future__ import annotations + +import inspect + +from xrspatial.geotiff import ( + open_geotiff, + read_geotiff_dask, + read_geotiff_gpu, + read_vrt, +) + + +# Canonical order taken from ``open_geotiff``'s public signature. +_CANONICAL_ORDER = ( + "dtype", + "window", + "overview_level", + "band", + "name", + "chunks", + "gpu", + "max_pixels", + "on_gpu_failure", + "missing_sources", +) + + +def _kwonly_params(fn): + """Return the keyword-only parameter names of *fn* in declaration order.""" + sig = inspect.signature(fn) + return [ + name + for name, param in sig.parameters.items() + if param.kind is inspect.Parameter.KEYWORD_ONLY + ] + + +def _assert_canonical(fn, allowed_tail=()): + """Assert *fn*'s kw-only params follow the canonical order. + + Parameters that appear in ``_CANONICAL_ORDER`` must show up in the + same relative order. Extras (e.g. the deprecated ``gpu`` alias on + ``read_geotiff_gpu``) are accepted at the tail when listed in + ``allowed_tail`` and otherwise rejected so new kwargs cannot be + quietly added in arbitrary positions. + """ + params = _kwonly_params(fn) + canonical = [p for p in params if p in _CANONICAL_ORDER] + expected = [p for p in _CANONICAL_ORDER if p in canonical] + assert canonical == expected, ( + f"{fn.__name__} kwarg order {canonical!r} does not match the " + f"canonical subset {expected!r}" + ) + tail = [p for p in params if p not in _CANONICAL_ORDER] + unexpected = set(tail) - set(allowed_tail) + assert not unexpected, ( + f"{fn.__name__} has unexpected kw-only params {sorted(unexpected)!r}; " + f"add them to _CANONICAL_ORDER or to the test's allowed_tail." + ) + + +def test_open_geotiff_defines_canonical_order(): + """``open_geotiff``'s signature is the canonical reference.""" + params = _kwonly_params(open_geotiff) + expected = list(_CANONICAL_ORDER) + assert params == expected, ( + f"open_geotiff kw-only params {params!r} no longer match the " + f"canonical order {expected!r}. Update both the function and the " + f"_CANONICAL_ORDER constant together." + ) + + +def test_read_geotiff_gpu_matches_canonical_order(): + """``read_geotiff_gpu`` must list shared params in the canonical order.""" + # ``gpu`` here is the deprecated alias for ``on_gpu_failure`` (see + # ``read_geotiff_gpu``'s docstring). It is not the boolean backend + # selector that lives on ``open_geotiff`` / ``read_vrt``, so it sits + # at the tail rather than in its canonical-order slot. + params = _kwonly_params(read_geotiff_gpu) + # ``gpu`` is the deprecated alias, intentionally last. + assert params[-1] == "gpu", ( + f"read_geotiff_gpu must keep the deprecated 'gpu' alias as the last " + f"kwarg; got {params!r}" + ) + # Drop the alias and run the canonical-subset check on the rest. + head = params[:-1] + canonical_head = [p for p in _CANONICAL_ORDER if p in head] + assert head == canonical_head, ( + f"read_geotiff_gpu kwarg order {head!r} does not match the canonical " + f"subset {canonical_head!r}" + ) + + +def test_read_geotiff_dask_matches_canonical_order(): + """``read_geotiff_dask`` must list shared params in the canonical order.""" + _assert_canonical(read_geotiff_dask) + + +def test_read_vrt_matches_canonical_order(): + """``read_vrt`` must list shared params in the canonical order.""" + _assert_canonical(read_vrt) + + +def test_no_pairwise_order_inversions(): + """For any pair of params shared by two readers, the order is consistent. + + ``read_geotiff_gpu``'s ``gpu`` kwarg is a deprecated alias for + ``on_gpu_failure`` rather than the boolean backend selector that + ``open_geotiff`` / ``read_vrt`` expose, so it is excluded from the + cross-reader pair check. + """ + readers = (open_geotiff, read_geotiff_gpu, read_geotiff_dask, read_vrt) + orders = {} + for fn in readers: + params = _kwonly_params(fn) + if fn is read_geotiff_gpu: + # Drop the deprecated alias before cross-comparing with the other + # readers' boolean ``gpu`` kwarg (different meaning, same name). + params = [p for p in params if p != "gpu"] + orders[fn.__name__] = params + canonical_pairs = [] + for i, a in enumerate(_CANONICAL_ORDER): + for b in _CANONICAL_ORDER[i + 1:]: + canonical_pairs.append((a, b)) + for name, params in orders.items(): + for a, b in canonical_pairs: + if a in params and b in params: + assert params.index(a) < params.index(b), ( + f"{name}: {a!r} must appear before {b!r}; got " + f"{params!r}" + )