Skip to content

geotiff tests: consolidate GPU codec cluster (Sub-PR C)#2472

Merged
brendancol merged 1 commit into
mainfrom
2438-sub-pr-c-gpu-codec
May 27, 2026
Merged

geotiff tests: consolidate GPU codec cluster (Sub-PR C)#2472
brendancol merged 1 commit into
mainfrom
2438-sub-pr-c-gpu-codec

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes part of #2438 (Sub-PR C). Cluster 14 of long-tail epic #2424.

Summary

Fold 11 GPU codec test files into a new
xrspatial/geotiff/tests/gpu/test_codec.py:

  • test_nvcomp_batch_compress_batched_1712.py (batched compress)
  • test_nvcomp_batch_upload_p3.py (batched H2D upload)
  • test_nvcomp_decompress_cumsum_offsets_1950.py (cumsum offsets)
  • test_nvcomp_from_device_bufs_single_alloc_1659.py (single alloc)
  • test_nvjpeg_encode_stream_sync_2212.py (Stream.null sync)
  • test_nvjpeg2k_single_alloc_2107.py (pool layout)
  • test_jpeg_gpu_1549.py (nvJPEG output-format constants)
  • test_lerc_valid_mask_gpu.py (GPU LERC valid-mask)
  • test_predictor2_big_endian_gpu_1517.py (BE predictor + byte swap)
  • test_predictor3_int_dtype_gpu_1933.py (predictor=3 rejection)
  • test_gpu_jpeg_interop_reject_issue_D_1845.py (JPEG opt-in)

Tests-only. The shared requires_gpu marker from _helpers/markers.py
replaces the per-file _gpu_available / _gpu_only helpers. Module-
level helpers carry the source issue number suffix
(e.g. _write_jpeg_rgb_tiff_1549) so sibling sections stay
collision-free. 71 tests collected, matching the baseline sum.

Sibling Sub-PRs A/B/D land gpu/test_reader.py, gpu/test_writer.py,
and gpu/test_kernels_and_kwargs.py against the same gpu/
directory; gpu/__init__.py is created here and is empty, so a merge
with the sibling branches resolves trivially.

Cross-references updated

  • docs/source/reference/release_gate_geotiff.rst -- the jpeg codec
    row now cites gpu/test_codec.py instead of the deleted
    test_gpu_jpeg_interop_reject_issue_D_1845.py.
  • xrspatial/geotiff/tests/unit/test_predictor.py -- the GPU
    predictor file pointers in the module docstring now point at
    gpu/test_codec.py.

Audit

xrspatial/geotiff/tests/CLUSTER_AUDIT_GPU_C.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_codec.py: 68 passed,
    3 skipped (no nvCOMP / nvJPEG / GDS on this host).
  • Baseline (originals before deletion): 68 passed, 3 skipped --
    identical count.
  • pytest xrspatial/geotiff/tests/release_gates/test_stable_features.py:
    158 passed, 1 xfailed.

Test plan

  • New consolidated gpu/test_codec.py passes on this checkout.
  • Release-gate checklist-parity test passes.
  • unit/test_predictor.py still passes after the docstring update.

Cluster 14 of long-tail epic #2424. Fold 11 GPU codec test files into
a new xrspatial/geotiff/tests/gpu/test_codec.py:

- test_nvcomp_batch_compress_batched_1712.py
- test_nvcomp_batch_upload_p3.py
- test_nvcomp_decompress_cumsum_offsets_1950.py
- test_nvcomp_from_device_bufs_single_alloc_1659.py
- test_nvjpeg_encode_stream_sync_2212.py
- test_nvjpeg2k_single_alloc_2107.py
- test_jpeg_gpu_1549.py
- test_lerc_valid_mask_gpu.py
- test_predictor2_big_endian_gpu_1517.py
- test_predictor3_int_dtype_gpu_1933.py
- test_gpu_jpeg_interop_reject_issue_D_1845.py

Tests-only. The shared requires_gpu marker from _helpers/markers.py
replaces the per-file _gpu_available / _gpu_only helpers. Module-level
helpers carry the source issue number suffix to keep sibling sections
collision-free. 71 tests collected, matching the baseline sum.

The release-gate row for the jpeg codec and the unit/test_predictor
docstring now cite gpu/test_codec.py.

Refs #2438.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 27, 2026
@brendancol brendancol merged commit 81a5b78 into main May 27, 2026
8 of 9 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: geotiff tests: consolidate GPU codec cluster (Sub-PR C)

Blockers (must fix before merge)

  • xrspatial/geotiff/tests/gpu/test_codec.py:1125 - module-level lerc_lerc = pytest.importorskip("lerc", ...). In the original test_lerc_valid_mask_gpu.py that call skipped just that file. In the consolidated module it skips the entire gpu/test_codec.py (71 tests, including nvCOMP, nvJPEG, predictor and JPEG sections that have no LERC dependency) on any host without the lerc package. Replace the module-level skip with a section-local import plus LERC_AVAILABLE gate, or wrap the lerc import inside the fixture / tests that need it.

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/tests/gpu/test_codec.py:425 - import cupy inside test_no_nvcomp_lib_returns_none_1659 is unused after consolidation (the body only uses _gpu_decode and _try_nvcomp_from_device_bufs). Carried over from the original; safe to drop.
  • xrspatial/geotiff/tests/gpu/test_codec.py:1741 - pytest.importorskip("kvikio") raises a PytestDeprecationWarning because kvikio imports but fails to load libkvikio.so. The deprecation suggests pytest.importorskip("kvikio", exc_type=ImportError). Inherited from the original file but easy to silence here.

Nits (optional improvements)

  • xrspatial/geotiff/tests/gpu/test_codec.py:64 - needs_cupy = requires_gpu and _gpu_only = requires_gpu aliases sit at module top but the rest of the file references only requires_gpu. They can be dropped now that the consolidation is in.
  • xrspatial/geotiff/tests/gpu/test_codec.py:65 - _HAS_PIL / _HAS_IMAGECODECS are referenced only by the _gpu_only_1549 skipif; keeping them next to the section that uses them would be slightly easier to follow.

What looks good

  • Test count matches baseline (71 collected, 68 passed + 3 skipped on this host); no coverage dropped during the fold.
  • Module-level helpers carry the source issue number suffix, keeping sibling sections collision-free per the model PR convention.
  • requires_gpu from _helpers/markers.py replaces the per-file _gpu_available() / _gpu_only helpers across every section.
  • Cross-reference updates in release_gate_geotiff.rst and unit/test_predictor.py keep the release-gate parity check green (verified by running release_gates/test_stable_features.py).
  • The pathlib.Path(__file__).parent.parent.parent / "_gpu_decode.py" path in the #1950 cumsum-source check was updated for the deeper gpu/ location.
  • The audit file maps every old file::test to the new file::test_id; sibling agents touching the same gpu/ directory will hit only an empty __init__.py conflict that resolves trivially.

Checklist

  • Test count parity (71 == 71)
  • Release-gate file pointers updated
  • All sections gated through shared requires_gpu marker
  • Helper-name suffixes prevent sibling-section collisions

brendancol added a commit that referenced this pull request May 27, 2026
Three review findings on PR #2472:

1. Blocker: ``pytest.importorskip("lerc")`` at module scope would skip
   the entire consolidated ``gpu/test_codec.py`` module (71 tests,
   including nvCOMP, nvJPEG, predictor and JPEG sections that have no
   LERC dependency) on hosts without ``lerc``. The original
   ``test_lerc_valid_mask_gpu.py`` had the same call but only the
   single file skipped. Replace with the existing ``LERC_AVAILABLE``
   per-test gate; move the ``import lerc`` into the fixture that
   needs it.

2. Suggestion: ``pytest.importorskip("kvikio")`` raised a
   ``PytestDeprecationWarning`` because ``kvikio`` imports but
   ``libkvikio.so`` is missing. Pass ``exc_type=ImportError`` to
   silence the deprecation.

3. Nit: drop the unused ``needs_cupy`` / ``_gpu_only`` module-level
   aliases for ``requires_gpu``.

The ``import cupy`` suggestion on ``test_no_nvcomp_lib_returns_none_1659``
was a misread on my part; ``cupy`` is actually used at the bottom of
that function. Leave it.

Refs #2438.
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 codec cluster (Sub-PR C)

Reviewed commit 3256aad3 ("Address review: scope LERC importorskip + silence kvikio deprecation").

Blockers (must fix before merge)

  • None.

Suggestions (should fix, not blocking)

  • None.

Nits (optional improvements)

  • None.

Round-1 disposition

  • Blocker (LERC module-level skip): fixed at xrspatial/geotiff/tests/gpu/test_codec.py:1118-1160. The pytest.importorskip("lerc") at module scope is gone; LERC_AVAILABLE plus the _gpu_only_lerc skipif now gate the LERC section per-test, and the fixture imports lerc lazily.
  • Suggestion (kvikio deprecation): fixed at xrspatial/geotiff/tests/gpu/test_codec.py:1742. exc_type=ImportError is passed; the warning count dropped from 7 to 6 in the local test run.
  • Suggestion (unused import cupy in test_no_nvcomp_lib_returns_none_1659): dismissed in the response commit. The cupy module is in fact used at the bottom of the function (cupy.zeros); the round-1 finding was a misread.
  • Nits (drop needs_cupy / _gpu_only aliases): fixed by dropping the aliases at the top of the file.

What looks good

  • Module-level skip surface reduced to nothing; gpu/test_codec.py collects all 71 tests independent of whether lerc is installed.
  • Tests + release-gate parity still pass on this host: 68 passed, 3 skipped on the consolidated module.
  • One pyflakes warning remains (kvikio.nvcomp imported but unused at line 164) -- inherited from the original file; the import is a deliberate availability probe, intentional.

Checklist

  • Test count parity (71 == 71)
  • Release-gate file pointers updated
  • No module-level optional-library skips
  • kvikio importorskip deprecation silenced
  • All sections gated through shared requires_gpu marker

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