diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index 0af9307d..467ee96a 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -26,8 +26,10 @@ write_geotiff_gpu(data, path, ...) GPU-only writer using nvCOMP. ``to_geotiff(..., gpu=True)`` calls this internally. -write_vrt(vrt_path, source_files, ...) - Generate a VRT mosaic XML from a list of GeoTIFF files. +write_vrt(path, source_files, ...) + Generate a VRT mosaic XML from a list of GeoTIFF files. ``vrt_path`` + is kept as a deprecated alias for ``path``; passing both ``path`` and + ``vrt_path`` raises ``TypeError`` (#1946). """ from __future__ import annotations diff --git a/xrspatial/geotiff/_runtime.py b/xrspatial/geotiff/_runtime.py index aaed453c..402558c8 100644 --- a/xrspatial/geotiff/_runtime.py +++ b/xrspatial/geotiff/_runtime.py @@ -39,6 +39,21 @@ # (forward verbatim to read_vrt). Mirrors the on_gpu_failure pattern. See # issue #1810. _MISSING_SOURCES_SENTINEL = object() +# ``write_vrt`` historically named its first positional kwarg ``vrt_path`` +# while ``to_geotiff`` / ``write_geotiff_gpu`` use ``path``. The deprecation +# shim adds ``path`` as the new name and accepts ``vrt_path`` with a +# DeprecationWarning. The sentinel pattern distinguishes "user passed +# vrt_path= explicitly" from "user passed nothing", which is the same +# rationale ``_CRS_WKT_DEPRECATED_SENTINEL`` documents above. See +# issue #1946. +_VRT_PATH_DEPRECATED_SENTINEL = object() +# ``write_vrt`` also needs to distinguish "user passed path= explicitly" +# (including an explicit ``path=None``, which is an error) from "user +# passed nothing" (fall through to the ``vrt_path`` shim). Without this +# sentinel, ``write_vrt(None, sources)`` silently fell through to the +# ``path is None`` branch and raised a "missing required argument" +# TypeError for the wrong reason. See PR #1962 review. +_VRT_PATH_MISSING_SENTINEL = object() # Spatial dim names recognised on 3D writer inputs. ``y``/``x`` are the diff --git a/xrspatial/geotiff/_writers/vrt.py b/xrspatial/geotiff/_writers/vrt.py index cbaf6480..a2411cf3 100644 --- a/xrspatial/geotiff/_writers/vrt.py +++ b/xrspatial/geotiff/_writers/vrt.py @@ -1,19 +1,25 @@ """VRT writer entry point. Wraps ``_vrt.write_vrt`` with the public ``write_vrt`` surface: -deprecation handling for ``crs_wkt`` (#1715), normalisation of the -``crs`` kwarg to WKT via ``_resolve_crs_to_wkt``, and the parity -docstring vs ``to_geotiff`` / ``write_geotiff_gpu``. +deprecation handling for ``crs_wkt`` (#1715) and ``vrt_path`` (#1946), +normalisation of the ``crs`` kwarg to WKT via ``_resolve_crs_to_wkt``, +and the parity docstring vs ``to_geotiff`` / ``write_geotiff_gpu``. """ from __future__ import annotations import warnings from .._crs import _resolve_crs_to_wkt -from .._runtime import _CRS_WKT_DEPRECATED_SENTINEL +from .._runtime import ( + _CRS_WKT_DEPRECATED_SENTINEL, + _VRT_PATH_DEPRECATED_SENTINEL, + _VRT_PATH_MISSING_SENTINEL, +) -def write_vrt(vrt_path: str, source_files: list[str], *, +def write_vrt(path: str = _VRT_PATH_MISSING_SENTINEL, + source_files: list[str] | None = None, *, + vrt_path: str | None = _VRT_PATH_DEPRECATED_SENTINEL, relative: bool = True, crs: int | str | None = None, crs_wkt: str | None = _CRS_WKT_DEPRECATED_SENTINEL, @@ -22,10 +28,19 @@ def write_vrt(vrt_path: str, source_files: list[str], *, Parameters ---------- - vrt_path : str - Output .vrt file path. + path : str + Output .vrt file path. Mirrors the ``path`` kwarg on + ``to_geotiff`` and ``write_geotiff_gpu`` so the writer trio + shares a single destination-arg name (issue #1946). source_files : list of str Paths to the source GeoTIFF files. + vrt_path : str, optional + Deprecated alias for ``path``. Emits ``DeprecationWarning`` when + supplied; passing both ``path`` and ``vrt_path`` raises + ``TypeError``. Kept so existing callers (``write_vrt(vrt_path, + sources)`` positional or ``write_vrt(vrt_path=...)`` keyword) + keep working through the deprecation window. New code should + use ``path``. See issue #1946. relative : bool, optional Store source paths relative to the VRT file (default True). crs : int, str, or None, optional @@ -60,6 +75,53 @@ def write_vrt(vrt_path: str, source_files: list[str], *, # historic ``crs_wkt`` path; the new ``crs`` path normalises through # ``_resolve_crs_to_wkt`` before forwarding because the internal # writer still only speaks WKT. + # + # The ``path`` / ``vrt_path`` shim resolves the destination kwarg + # before any other processing so the rest of the function works + # uniformly against a single ``vrt_path`` local. ``path`` is the + # new name (parity with to_geotiff / write_geotiff_gpu); ``vrt_path`` + # is kept as a deprecated alias to preserve back-compat for callers + # using either positional ``write_vrt(vrt_path, sources)`` or + # keyword ``write_vrt(vrt_path=...)``. + path_passed = path is not _VRT_PATH_MISSING_SENTINEL + vrt_path_passed = vrt_path is not _VRT_PATH_DEPRECATED_SENTINEL + if path_passed and vrt_path_passed: + # Both supplied is ambiguous regardless of whether the two values + # happen to be the same string. Refuse rather than silently + # picking one. Mirrors the same rule the ``crs`` / ``crs_wkt`` + # shim below applies. + raise TypeError( + "write_vrt: pass either 'path' or the deprecated 'vrt_path' " + "alias, not both.") + if vrt_path_passed: + warnings.warn( + "write_vrt(..., vrt_path=...) is deprecated; use path=... " + "instead. The kwarg was renamed for parity with to_geotiff " + "and write_geotiff_gpu, which already accept 'path' as the " + "destination kwarg.", + DeprecationWarning, + stacklevel=2, + ) + path = vrt_path + elif not path_passed: + # Neither name supplied. Match the previous ``TypeError: missing + # required positional argument`` semantics by raising rather than + # forwarding the sentinel into ``_write_vrt_internal``. + raise TypeError( + "write_vrt: missing required argument 'path'") + if path is None: + # Explicit ``path=None`` (including positional ``write_vrt(None, + # sources)``) is rejected up front so the error message names the + # offending kwarg instead of crashing deep in + # ``os.path.dirname(os.path.abspath(None))``. The sentinel default + # on ``path`` is what lets us distinguish this case from "caller + # passed nothing" above. + raise TypeError( + "write_vrt: 'path' must be a str, got None") + if source_files is None: + raise TypeError( + "write_vrt: missing required argument 'source_files'") + crs_wkt_passed = crs_wkt is not _CRS_WKT_DEPRECATED_SENTINEL if crs is not None and crs_wkt_passed: # Both supplied is ambiguous regardless of whether the WKT happens @@ -83,7 +145,7 @@ def write_vrt(vrt_path: str, source_files: list[str], *, from .._vrt import write_vrt as _write_vrt_internal return _write_vrt_internal( - vrt_path, source_files, + path, source_files, relative=relative, crs_wkt=resolved_wkt, nodata=nodata, diff --git a/xrspatial/geotiff/tests/test_signature_annotations_1654.py b/xrspatial/geotiff/tests/test_signature_annotations_1654.py index 585fb77f..246a14f7 100644 --- a/xrspatial/geotiff/tests/test_signature_annotations_1654.py +++ b/xrspatial/geotiff/tests/test_signature_annotations_1654.py @@ -83,10 +83,24 @@ def test_write_geotiff_gpu_path_annotated(): assert 'BinaryIO' in ann +def test_write_vrt_path_annotated(): + """``write_vrt(path, ...)`` is str-only (VRT writes are path-only by + design; no file-like buffer support). After #1946 the canonical name + is ``path`` (parity with ``to_geotiff`` / ``write_geotiff_gpu``). + The annotation is plain ``str``: the default value is a private + sentinel (not ``None``) so the deprecation shim can distinguish + ``write_vrt(path=None, ...)`` (rejected with TypeError) from a + caller who omitted ``path`` entirely (routed through the ``vrt_path`` + alias). See PR #1962 review.""" + assert _annotation(write_vrt, 'path') == 'str' + + def test_write_vrt_vrt_path_annotated(): - """``write_vrt(vrt_path, ...)`` stays str-only (VRT writes are - path-only by design; no file-like buffer support).""" - assert _annotation(write_vrt, 'vrt_path') == 'str' + """The deprecated ``vrt_path`` alias keeps the same ``str | None`` + annotation as ``path`` (str-only at the type level; ``None`` only + appears because the sentinel default lets the shim detect omission). + Pinned so a future re-rename does not silently widen the alias.""" + assert _annotation(write_vrt, 'vrt_path') == 'str | None' # --- source: str or BinaryIO (open_geotiff is the public dispatch) --- diff --git a/xrspatial/geotiff/tests/test_write_vrt_path_kwarg_1946.py b/xrspatial/geotiff/tests/test_write_vrt_path_kwarg_1946.py new file mode 100644 index 00000000..b1add2ce --- /dev/null +++ b/xrspatial/geotiff/tests/test_write_vrt_path_kwarg_1946.py @@ -0,0 +1,221 @@ +"""Regression test for #1946: write_vrt accepts ``path`` for parity +with ``to_geotiff`` / ``write_geotiff_gpu``. + +The api-consistency sweep on 2026-05-15 flagged that ``write_vrt`` was +the only writer in ``xrspatial.geotiff`` whose destination kwarg was +named ``vrt_path`` while the sibling writers use ``path``. The fix adds +``path`` as the canonical kwarg and keeps ``vrt_path`` as a deprecated +alias. + +This module pins: + +* Positional ``write_vrt(path, sources)`` works (back-compat with the + previous ``write_vrt(vrt_path, sources)`` positional form). +* Keyword ``write_vrt(path=..., source_files=...)`` works (the new + canonical form). +* Keyword ``write_vrt(vrt_path=...)`` still works and emits + ``DeprecationWarning``. +* Passing both ``path`` and ``vrt_path`` raises ``TypeError``. +* The signature exposes ``path`` as the first positional, matching + ``to_geotiff`` / ``write_geotiff_gpu``. +* The deprecation shim does NOT warn when ``path`` is used. +* Omitting both names raises ``TypeError`` (preserves the pre-#1946 + required-argument semantics). +""" +from __future__ import annotations + +import inspect +import os +import warnings + +import numpy as np +import pytest +import xarray as xr + +from xrspatial.geotiff import ( + read_vrt, + to_geotiff, + write_geotiff_gpu, + write_vrt, +) + + +def _build_source_tif(tmp_path, name='src.tif'): + """Create a small GeoTIFF used as the VRT's source file.""" + arr = np.arange(8 * 8, dtype=np.float32).reshape(8, 8) + da = xr.DataArray( + arr, dims=['y', 'x'], + coords={'y': np.arange(8.0, 0, -1), 'x': np.arange(8.0)}, + attrs={'crs': 4326, 'transform': (1.0, 0, 0.0, 0, -1.0, 8.0)}, + ) + p = str(tmp_path / name) + to_geotiff(da, p) + return p + + +def test_write_vrt_signature_first_arg_is_path(): + """Signature parity with to_geotiff / write_geotiff_gpu. + + The api-consistency sweep cares specifically about + ``inspect.signature``: IDE autocomplete, mypy, and Sphinx-rendered + docs all read the same source. Pinning the first param name here + catches any future re-rename that re-introduces the drift. + """ + sig = inspect.signature(write_vrt) + params = list(sig.parameters) + # ``path`` is the new canonical name, ``source_files`` follows. + # ``vrt_path`` is kept as a keyword-only deprecated alias. + assert params[0] == 'path' + assert params[1] == 'source_files' + assert 'vrt_path' in params + # ``vrt_path`` is keyword-only (the alias should never be used + # positionally going forward). + assert sig.parameters['vrt_path'].kind == inspect.Parameter.KEYWORD_ONLY + + +def test_write_vrt_positional_path_works(tmp_path): + """Positional ``write_vrt(path, sources)`` is unchanged. + + Existing callers ``write_vrt(some_path, sources)`` keep working + after the rename because the new ``path`` parameter sits where + ``vrt_path`` used to be. No deprecation warning should fire. + """ + src = _build_source_tif(tmp_path) + out = str(tmp_path / 'out.vrt') + with warnings.catch_warnings(): + warnings.simplefilter('error', DeprecationWarning) + result = write_vrt(out, [src]) + assert result == out + assert os.path.exists(out) + + +def test_write_vrt_path_kwarg_works(tmp_path): + """Keyword ``write_vrt(path=..., source_files=...)`` works. + + A caller who passes everything by keyword (no positional args) + cannot reach the function before #1946 because ``path`` did not + exist; this is the path-symmetric counterpart to the existing + ``write_vrt(vrt_path=...)`` test below. + """ + src = _build_source_tif(tmp_path) + out = str(tmp_path / 'out.vrt') + with warnings.catch_warnings(): + warnings.simplefilter('error', DeprecationWarning) + result = write_vrt(path=out, source_files=[src]) + assert result == out + assert os.path.exists(out) + + +def test_write_vrt_vrt_path_kwarg_emits_deprecation_warning(tmp_path): + """``vrt_path=...`` works but emits ``DeprecationWarning``. + + Mirrors the existing ``crs_wkt`` deprecation in the same writer + (#1715): old name still works, but caller sees a clear migration + hint via the warning. + """ + src = _build_source_tif(tmp_path) + out = str(tmp_path / 'out.vrt') + with pytest.warns(DeprecationWarning, match='vrt_path'): + result = write_vrt(vrt_path=out, source_files=[src]) + assert result == out + assert os.path.exists(out) + + +def test_write_vrt_path_and_vrt_path_together_raises(tmp_path): + """Both names supplied is ambiguous; refuse to pick one. + + Mirrors the ``crs`` / ``crs_wkt`` rule documented in the existing + write_vrt source: passing both is rejected with TypeError + regardless of whether the two values happen to match. + """ + src = _build_source_tif(tmp_path) + out = str(tmp_path / 'out.vrt') + with pytest.raises(TypeError, match="path.*vrt_path"): + write_vrt(path=out, vrt_path=out, source_files=[src]) + + +def test_write_vrt_no_path_raises(tmp_path): + """Neither ``path`` nor ``vrt_path`` -> TypeError. + + Before the shim, omitting the first positional argument raised + ``TypeError: missing 1 required positional argument`` from CPython. + The shim adds a sentinel default so the kwarg-only positional no + longer triggers that automatic check; the explicit raise inside + the shim preserves the pre-#1946 error semantics. + """ + src = _build_source_tif(tmp_path) + with pytest.raises(TypeError, match='path'): + write_vrt(source_files=[src]) + + +def test_write_vrt_explicit_path_none_raises(tmp_path): + """``write_vrt(path=None, ...)`` is rejected with TypeError. + + The sentinel-default pattern (#1962 review) distinguishes "caller + passed nothing" (sentinel) from "caller passed None explicitly". + Explicit ``None`` is a bug in the caller's code, not a request to + fall through to the deprecated ``vrt_path`` alias, so the shim + raises with a clear message that names the offending kwarg. + """ + src = _build_source_tif(tmp_path) + with pytest.raises(TypeError, match="'path'.*None"): + write_vrt(path=None, source_files=[src]) + + +def test_write_vrt_positional_none_raises(tmp_path): + """Positional ``write_vrt(None, sources)`` is rejected with TypeError. + + Same rationale as the keyword case: an explicit positional ``None`` + is rejected up front instead of crashing deep in + ``os.path.dirname(os.path.abspath(None))``. Pinned because the + pre-#1962 code accepted positional ``None`` and raised the wrong + "missing required argument" error. + """ + src = _build_source_tif(tmp_path) + with pytest.raises(TypeError, match="'path'.*None"): + write_vrt(None, [src]) + + +def test_write_vrt_first_arg_name_matches_writer_trio(): + """Cross-sibling consistency: all three writers use the same + destination kwarg name. + + The deep-sweep-api-consistency sweep keeps adding to the writer + trio's parity contract. Pin the rule here so future re-renames + that split the trio again will trip a test. + """ + eager_first = list( + inspect.signature(to_geotiff).parameters + )[1] # data, path -> index 1 + gpu_first = list( + inspect.signature(write_geotiff_gpu).parameters + )[1] + vrt_first = list( + inspect.signature(write_vrt).parameters + )[0] # path, source_files -> index 0 + assert eager_first == 'path' + assert gpu_first == 'path' + assert vrt_first == 'path' + + +def test_write_vrt_path_round_trip_matches_old(tmp_path): + """The written VRT decodes the same regardless of which kwarg name + the caller used. + + Smoke test that the shim does not silently drop or re-route any of + the other kwargs while resolving ``path`` vs ``vrt_path``. + """ + src = _build_source_tif(tmp_path) + out_new = str(tmp_path / 'out_new.vrt') + out_old = str(tmp_path / 'out_old.vrt') + + write_vrt(out_new, [src]) + with warnings.catch_warnings(): + # ignore the deprecation; we still need the legacy path to + # produce a byte-identical mosaic. + warnings.simplefilter('ignore', DeprecationWarning) + write_vrt(vrt_path=out_old, source_files=[src]) + + a_new = read_vrt(out_new) + a_old = read_vrt(out_old) + np.testing.assert_array_equal(np.asarray(a_new), np.asarray(a_old))