Skip to content

Commit d91d78c

Browse files
committed
Address Copilot review feedback on #1653
- Mirror to_geotiff's cog=True + file-like rejection verbatim (same error string), so callers see identical messages from either entry point. Previously write_geotiff_gpu raised a different message that added "...on the GPU writer..." and dropped the BytesIO hint. - Add a TypeError gate for non-str, non-file-like path arguments, so passing e.g. an int falls through to a clear TypeError instead of an os.path / unicode error deep in the writer. Mirrors to_geotiff's existing TypeError verbatim. - Harden the BytesIO + write_geotiff_gpu tests with the repo's standard _gpu_available() helper (cupy.cuda.is_available() + ImportError guard) instead of pytest.importorskip('cupy'), so CI hosts where CuPy imports but CUDA is unavailable skip cleanly rather than hard-failing in cupy.asarray(). - Add two new regression tests: one pinning byte-for-byte parity of the cog/file-like error message between the two writers, and one pinning the new TypeError on invalid path types. Skip: the low-confidence type annotation suggestion (path: str|os.PathLike|SupportsWrite[bytes]). The other path-accepting functions in this module (to_geotiff, write_vrt, etc.) deliberately leave path untyped; adding a precise union here would diverge from the local convention for marginal benefit. All 17 tests in test_bytesio_source.py pass.
1 parent 1ec50c7 commit d91d78c

2 files changed

Lines changed: 85 additions & 9 deletions

File tree

xrspatial/geotiff/__init__.py

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2841,17 +2841,23 @@ def write_geotiff_gpu(data: xr.DataArray | cupy.ndarray | np.ndarray,
28412841
"max_z_error is not supported on the GPU writer "
28422842
"(nvCOMP has no LERC backend). Use to_geotiff(..., gpu=False) "
28432843
"or omit max_z_error.")
2844-
# Mirror to_geotiff's file-like + cog=True rejection. The auto-dispatch
2844+
# Mirror to_geotiff's path-type + cog=True gating verbatim so callers
2845+
# see identical errors from the two entry points. The auto-dispatch
28452846
# path through ``to_geotiff(gpu=True, cog=True, path=BytesIO)`` raises
2846-
# before reaching here; the explicit GPU writer mirrors the gate so
2847-
# callers cannot bypass it (issue #1652). Non-cog file-like writes
2847+
# before reaching here; the explicit GPU writer mirrors the same gate
2848+
# so callers cannot bypass it (issue #1652). Non-cog file-like writes
28482849
# remain supported on this entry point.
28492850
_path_is_file_like = (
28502851
not isinstance(path, str)) and hasattr(path, 'write')
2851-
if _path_is_file_like and cog:
2852-
raise ValueError(
2853-
"cog=True is not supported for file-like destinations on the "
2854-
"GPU writer. Pass a string path or set cog=False.")
2852+
if _path_is_file_like:
2853+
if cog:
2854+
raise ValueError(
2855+
"cog=True is not supported for file-like destinations. "
2856+
"Pass a string path or write to BytesIO without cog=True.")
2857+
elif not isinstance(path, str):
2858+
raise TypeError(
2859+
f"path must be a str or a binary file-like with a write() "
2860+
f"method, got {type(path).__name__}")
28552861
# streaming_buffer_bytes is intentionally a no-op on the GPU path;
28562862
# the kwarg exists for API parity with to_geotiff so callers can pass
28572863
# the same kwargs to both entry points without filtering.

xrspatial/geotiff/tests/test_bytesio_source.py

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
"""
77
from __future__ import annotations
88

9+
import importlib.util
910
import io
1011
from concurrent.futures import ThreadPoolExecutor
1112

@@ -17,6 +18,26 @@
1718
from xrspatial.geotiff._reader import _BytesIOSource, read_to_array
1819

1920

21+
def _gpu_available() -> bool:
22+
"""True when cupy imports AND a CUDA runtime is initialised.
23+
24+
Mirrors the helper used in other geotiff GPU tests so the BytesIO
25+
GPU-writer tests skip cleanly on hosts where CuPy is installed but
26+
CUDA is unavailable (Copilot review on #1653).
27+
"""
28+
if importlib.util.find_spec("cupy") is None:
29+
return False
30+
try:
31+
import cupy
32+
return bool(cupy.cuda.is_available())
33+
except Exception:
34+
return False
35+
36+
37+
_HAS_GPU = _gpu_available()
38+
_gpu_only = pytest.mark.skipif(not _HAS_GPU, reason="cupy + CUDA required")
39+
40+
2041
def _make_da(height=32, width=40, dtype=np.float32):
2142
arr = np.arange(height * width, dtype=dtype).reshape(height, width)
2243
# Simple geotransform with negative pixel_height (north-up)
@@ -281,8 +302,9 @@ class TestWriteGeotiffGpuBytesIO:
281302
by issue #1652 and confirm the non-cog file-like path still works.
282303
"""
283304

305+
@_gpu_only
284306
def test_cog_with_bytesio_rejected_1652(self):
285-
cupy = pytest.importorskip("cupy")
307+
import cupy
286308
da = xr.DataArray(
287309
cupy.asarray(np.random.rand(64, 64).astype(np.float32)),
288310
dims=['y', 'x'],
@@ -295,8 +317,56 @@ def test_cog_with_bytesio_rejected_1652(self):
295317
with pytest.raises(ValueError, match='cog=True'):
296318
write_geotiff_gpu(da, buf, cog=True)
297319

320+
@_gpu_only
321+
def test_cog_with_bytesio_error_matches_to_geotiff_1652(self):
322+
"""The error string must match ``to_geotiff``'s gate verbatim so
323+
downstream callers can rely on a single message (Copilot review
324+
on #1653)."""
325+
import cupy
326+
da = xr.DataArray(
327+
cupy.asarray(np.random.rand(64, 64).astype(np.float32)),
328+
dims=['y', 'x'],
329+
coords={'y': np.arange(64.0), 'x': np.arange(64.0)},
330+
attrs={'crs': 4326},
331+
)
332+
from xrspatial.geotiff import write_geotiff_gpu
333+
334+
# to_geotiff's canonical message; mirrored verbatim in
335+
# write_geotiff_gpu's gate.
336+
expected = (
337+
"cog=True is not supported for file-like destinations. "
338+
"Pass a string path or write to BytesIO without cog=True."
339+
)
340+
341+
buf = io.BytesIO()
342+
with pytest.raises(ValueError) as exc_info:
343+
write_geotiff_gpu(da, buf, cog=True)
344+
assert str(exc_info.value) == expected
345+
346+
# And the CPU writer raises the same string for parity.
347+
with pytest.raises(ValueError) as exc_info_cpu:
348+
to_geotiff(_make_da(), io.BytesIO(), cog=True)
349+
assert str(exc_info_cpu.value) == expected
350+
351+
@_gpu_only
352+
def test_invalid_path_type_raises_typeerror_1652(self):
353+
"""Mirror to_geotiff's TypeError for non-str, non-file-like paths
354+
so callers see identical behaviour from both entry points."""
355+
import cupy
356+
da = xr.DataArray(
357+
cupy.asarray(np.random.rand(64, 64).astype(np.float32)),
358+
dims=['y', 'x'],
359+
coords={'y': np.arange(64.0), 'x': np.arange(64.0)},
360+
attrs={'crs': 4326},
361+
)
362+
from xrspatial.geotiff import write_geotiff_gpu
363+
364+
with pytest.raises(TypeError, match="path must be a str"):
365+
write_geotiff_gpu(da, 42) # int is neither str nor file-like
366+
367+
@_gpu_only
298368
def test_non_cog_bytesio_still_works_1652(self):
299-
cupy = pytest.importorskip("cupy")
369+
import cupy
300370
arr_cpu = np.random.rand(64, 64).astype(np.float32)
301371
da = xr.DataArray(
302372
cupy.asarray(arr_cpu),

0 commit comments

Comments
 (0)