Skip to content

geotiff tests: consolidate GPU reader cluster (Sub-PR A of #2438)#2471

Merged
brendancol merged 2 commits into
mainfrom
worktree-agent-aff71ce25e27f468b
May 27, 2026
Merged

geotiff tests: consolidate GPU reader cluster (Sub-PR A of #2438)#2471
brendancol merged 2 commits into
mainfrom
worktree-agent-aff71ce25e27f468b

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Part of #2438 (Sub-PR A). Cluster 14 of long-tail epic #2424.

Summary

Fold 10 GPU read-side regression files into a new xrspatial/geotiff/tests/gpu/test_reader.py:

  • test_gpu_window_band_1605.py -- window / band kwarg forwarding
  • test_gpu_stripped_multiband.py -- 3-D (y, x, band) via stripped fallback
  • test_gpu_stripped_no_georef_window_1753.py -- stripped no-georef windowed coord parity
  • test_gpu_stripped_forwarding_1732.py -- stripped fallback forwards max_pixels/window/band
  • test_gpu_nodata_1542.py -- GPU read nodata promotion + attrs['nodata']
  • test_gpu_miniswhite_band_first_2097.py -- band-first MinIsWhite guard
  • test_gpu_chunks_out_of_core_1876.py -- chunks= out-of-core dask graph
  • test_gpu_sidecar_georef_parity_2324.py -- sidecar overview-inheritance georef parity
  • test_read_geotiff_gpu_url_eager_2161.py -- HTTP / fsspec URL eager path
  • test_chunked_gpu_declared_dtype_1909.py -- GDS chunked declared-dtype cast

Tests-only. Each file's local _gpu_available / _gpu_only helper is replaced by the shared requires_gpu marker from _helpers/markers.py (aliased _gpu_only for ergonomic decoration). Test IDs that were not already issue-suffixed are renamed with the source issue number so sibling sections do not collide.

66 tests collected (matches the baseline sum of the 10 deleted files).

The relative import in test_gpu_sidecar_georef_parity_2324.py (.integration.test_sidecar._write_pair) was re-anchored to ..integration.test_sidecar because the consolidated module sits one directory deeper.

Cross-cutting touches

  • docs/source/reference/release_gate_geotiff.rst -- GPU nodata row now cites gpu/test_reader.py.
  • docs/source/reference/geotiff.rst -- prose pointer updated.
  • read/test_nodata.py -- docstring pointer updated.

Audit

xrspatial/geotiff/tests/CLUSTER_AUDIT_GPU_A.md maps every old file::test to its new file::test_id. The audit file is deleted on a final commit on this branch before merge.

Verification

  • pytest xrspatial/geotiff/tests/gpu/test_reader.py: 66 passed (on a CUDA host).
  • pytest xrspatial/geotiff/tests/release_gates/: 158 passed, 1 xfailed.
  • Full xrspatial/geotiff/tests/ collect: 5890 tests collected (no broken imports).

Test plan

  • New consolidated gpu/test_reader.py collects + passes locally.
  • Release-gate checklist-parity test passes (no stale citations).
  • No remaining grep hit for any of the 10 deleted filenames in source / docs.

Sub-PR layout

Sibling sub-PRs (B / C / D) for the writer / codec / kernels-and-kwargs clusters are independent and land on parallel branches under the same gpu/ subdirectory.

Fold 10 GPU read-side regression files into a new
``xrspatial/geotiff/tests/gpu/test_reader.py``. Sections are grouped
by the upstream issue number they pin:

- #1605 -- window / band kwarg forwarding on GPU read
- stripped multiband (#1582) -- 3-D (y, x, band) reads via stripped fallback
- #1753 -- stripped no-georef windowed coord parity
- #1732 -- stripped fallback forwards max_pixels/window/band
- #1542 -- GPU read nodata promotion + attrs['nodata']
- #2097 -- write_geotiff_gpu rejects MinIsWhite (band-first guard)
- #1876 -- chunks= is a real out-of-core dask graph
- #2324 -- sidecar overview-inheritance georef parity
- #2161 -- HTTP / fsspec URLs on the eager GPU path
- #1909 -- GDS chunked path casts each chunk to declared dtype

Tests-only. Each file's local ``_gpu_available`` / ``_gpu_only`` helper
is replaced by the shared ``requires_gpu`` marker from
``_helpers/markers.py`` (aliased ``_gpu_only`` for ergonomic
decoration). 66 tests collected (matches the baseline sum of the 10
deleted files).

Cross-cutting touches:
- ``docs/source/reference/release_gate_geotiff.rst`` -- GPU nodata
  row now cites ``gpu/test_reader.py``.
- ``docs/source/reference/geotiff.rst`` -- prose pointer updated.
- ``read/test_nodata.py`` -- docstring pointer updated.

``CLUSTER_AUDIT_GPU_A.md`` maps every old ``file::test`` to its new
``test_id`` and is deleted on a final commit on this branch before
merge.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 27, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: geotiff tests: consolidate GPU reader cluster (Sub-PR A of #2438)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/tests/gpu/test_reader.py:852 -- the _gds_only_1876 skip predicate dropped the runtime CUDA-availability probe from the original test_gpu_chunks_out_of_core_1876.py. The original _gds_only gated on _HAS_GPU and _HAS_KVIKIO, where _HAS_GPU was cupy.cuda.is_available(). The replacement only checks importlib.util.find_spec("cupy") is not None and _HAS_KVIKIO_1876. On a host with cupy installed but no working CUDA runtime, the GDS-path test will now try to run instead of skipping. Drop the manual skipif and stack @_gpu_only + @pytest.mark.skipif(not _HAS_KVIKIO_1876, reason=...) on test_read_geotiff_gpu_chunks_uses_gds_path_when_available_1876.

Nits (optional improvements)

  • xrspatial/geotiff/tests/gpu/test_reader.py:286-339 -- the stripped-multiband section is the only group with no _<issue> test-id suffix. The source filename had no issue number, but other consolidated PRs (e.g. #2469) suffix every renamed test. Consider tagging these _stripped_multiband for grep-friendliness.
  • xrspatial/geotiff/tests/gpu/test_reader.py:825 -- test_samples_hint_band_first_without_gpu_2097 is intentionally CPU-runnable (matches the original) but the section header reads as if the whole #2097 block is GPU-only. A one-line comment noting that the last test in the section is CPU-only by design would save a reader the round-trip.
  • xrspatial/geotiff/tests/CLUSTER_AUDIT_GPU_A.md -- the audit lists TestStrippedGpuWindowedBackendParity::test_dtype_parity[*] with a single [*] wildcard. The full parametrize list (three windows) is short enough to spell out and would make the audit grep-able per-case. Not load-bearing since the file is deleted pre-merge.

What looks good

  • Test count matches baseline: 66 before, 66 after. Verified via --collect-only against both states.
  • Shared requires_gpu marker from _helpers/markers.py replaces ten per-file _gpu_available / _gpu_only reimplementations; the alias-as-import pattern matches PR #2469 / #2470.
  • The relative import from ..integration.test_sidecar import _write_pair correctly accounts for the extra directory level (tests/gpu/ vs the old top-level location).
  • The three CPU-only originals (test_samples_hint_band_first_without_gpu_2097, test_chunked_gpu_eager_paths_keep_source_dtype_1909, test_sidecar_without_geokeys_attrs_match_cpu_vs_dask_2324) keep their no-gate disposition rather than being forced behind requires_gpu. A literal reading of the issue's "apply @requires_gpu on every test" rule would weaken these three; preserving the original CPU-runnable behaviour matches the precedent in PR #2469.
  • Three docs touchpoints (release_gate_geotiff.rst, geotiff.rst, read/test_nodata.py docstring) repointed off the deleted test_gpu_nodata_1542.py to gpu/test_reader.py. Release-gate checklist-parity test passes.
  • Fixture and helper class names are issue-suffixed (single_band_tiff_1605, _FakeArray2097, _RangeHandler2161, etc.) so sibling sub-PRs B/C/D sharing this module's namespace later cannot collide.

Checklist

  • Algorithm matches reference/paper -- N/A, tests-only consolidation
  • All implemented backends produce consistent results -- preserved from originals
  • NaN handling is correct -- preserved from originals
  • Edge cases are covered by tests -- preserved from originals
  • Dask chunk boundaries handled correctly -- preserved from originals
  • No premature materialization or unnecessary copies -- N/A
  • Benchmark exists or is not needed -- N/A
  • README feature matrix updated (if applicable) -- N/A
  • Docstrings present and accurate -- module-level + per-section

- ``gpu/test_reader.py``: restore the ``cupy.cuda.is_available()`` runtime
  probe on the GDS path test by composing ``@_gpu_only`` (which uses the
  shared ``gpu_available()`` from ``_helpers/markers.py``) with a
  kvikio-only skipif. The original ``_gds_only`` checked both gates; the
  consolidated ``_gds_only_1876`` had dropped the CUDA check, so a host
  with cupy installed but no working CUDA runtime would have attempted
  to run the GDS test instead of skipping cleanly.
- ``gpu/test_reader.py``: suffix the three stripped-multiband tests with
  ``_stripped_multiband`` so every test id in the module is
  grep-friendly (the source filename had no issue number).
- ``gpu/test_reader.py``: note in the ``#2097`` section header that the
  last test in the section is intentionally CPU-only.
- ``CLUSTER_AUDIT_GPU_A.md``: spell out individual parametrize ids for
  the ``TestStrippedGpuWindowedBackendParity`` cases and update the
  stripped-multiband entries to the new suffixed names.
@brendancol brendancol merged commit fa8d5f9 into main May 27, 2026
6 of 7 checks passed
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review (round 2): geotiff tests: consolidate GPU reader cluster (Sub-PR A of #2438)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

None.

What looks good

  • Round-1 GDS skip predicate fix: _gds_only_1876 is gone. The GDS-path test now stacks @_gpu_only + @_requires_kvikio_1876, so the cupy + CUDA runtime probe lives in one place (_helpers/markers.py) and the kvikio probe stays local to its only consumer.
  • Round-1 stripped-multiband renames: the three tests carry the _stripped_multiband suffix, matching the issue-suffix convention used everywhere else in the module.
  • Round-1 #2097 section header: the new comment block explicitly calls out the CPU-only test_samples_hint_band_first_without_gpu_2097 so a future hand reading the section header doesn't try to "fix" its missing @_gpu_only.
  • Round-1 audit parametrize: TestStrippedGpuWindowedBackendParity1753::test_{dtype,value}_parity now lists [win0] / [win1] / [win2] with the window tuple beside each, so the audit is grep-able per-case.
  • 66 tests still collect and pass on the consolidated module.

Checklist

  • Algorithm matches reference/paper -- N/A
  • All implemented backends produce consistent results
  • NaN handling is correct
  • Edge cases are covered by tests
  • Dask chunk boundaries handled correctly
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed -- N/A
  • README feature matrix updated (if applicable) -- N/A
  • Docstrings present and accurate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant