Release-gate negative test for ambiguous GeoTIFF metadata (#2361)#2366
Conversation
Pins the release promise from epic #2341 that "unsupported or ambiguous metadata fails loudly instead of flattening or guessing". Parametrized four cases: * Conflicting CRS between the GeoTIFF header and a sibling ``.aux.xml`` PAM sidecar -- xfail until PAM sidecar support lands. * Integer nodata sentinel that cannot be honoured on a float-promoted raster -- xfail until issue #1774's no-op gets upgraded to a typed rejection. * Rotated affine transform without ``allow_rotated=True`` -- actively passes across the eager numpy, dask, and windowed read entry points. * Mixed-tier VRT children when stable-only is requested -- xfail against epic #2342 which owns the stable-only VRT knob. Updates ``_geotags.py`` and ``_validation.py`` so the rotated-transform error message names the ``reader.allow_rotated`` experimental tier opt-in and points the caller at ``docs/source/reference/release_gate_geotiff.rst``. Adds a row to that checklist under "Cross-cutting CI gates". Assertions inlined per case; no shared helper module so parallel sibling PRs of epic #2341 cannot collide on a cross-PR symbol.
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Release-gate negative test for ambiguous GeoTIFF metadata (#2361)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
xrspatial/geotiff/tests/test_release_gate_negative_2341.py:386-389-- the comment above the tier check claims "The tier wordadvancedis the live tier for this opt-in in SUPPORTED_FEATURES", but_attrs.py:298tagsreader.allow_rotatedasexperimental, notadvanced. The runtime check itself is fine because it accepts any of("advanced", "experimental", "stable"), but the comment is inaccurate and will mislead the next person who tries to tighten the check. Update the comment to match the live tier. -
xrspatial/geotiff/tests/test_release_gate_negative_2341.py:451-- the case-4 trick_tmp(...).replace(".tif", ".vrt")is brittle if thetmp_pathever happens to embed.tifin a directory segment. Construct the.vrtpath directly viatmp_path / f"...uuid....vrt"rather than string-replacing the.tifsuffix. Same outcome, less surface area for surprise. -
The case-3 release promise per the issue spec also names "every promised read entry point" -- GPU read is
experimentalbut it is a promised entry point inSUPPORTED_FEATURES. Adding a fourth sub-casetest_release_gate_negative_rotated_gpu(skipped on hosts without cupy + CUDA via the existingrequires_gpufixture fromconftest.py) would close the matrix without changing scope. Optional; the issue explicitly listed eager / dask / windowed, so leaving GPU out is defensible.
Nits (optional improvements)
-
xrspatial/geotiff/tests/test_release_gate_negative_2341.py:107-- the_TAG_MODEL_TRANSFORMATIONand rotation constants are duplicated fromtest_allow_rotated_geotiff_2115.py. The PR description says this is intentional (no shared helper module across the four sibling PRs). Keep the inline copy; consider adding a one-line comment pointing back at the source file so a future grep finds the canonical location. -
xrspatial/geotiff/_geotags.py:684-690andxrspatial/geotiff/_validation.py:1117-1122-- the two rewritten messages share the trailing pointer prose. A short module-level constant in_errors.py(e.g._RELEASE_CONTRACT_POINTER) would keep both raise sites in lockstep if the docs path changes again. Not in this PR's scope; flag for follow-up if the message gets edited a third time. -
The case-4 xfail body calls
open_geotiff(path, stable_only=True)and relies onTypeError: unexpected keyword argumentto satisfy the xfail. The assertions on the message inside thewith pytest.raisesblock never run today. When epic #2342 lands andstable_only=becomes a real kwarg, the test will flip to XPASS even if the new code raises a different exception type or with a message that does not mention the opt-in. Either tighten the xfail toraises=GeoTIFFAmbiguousMetadataError(so it stays xfail until the correct shape lands) or add a short note in the docstring that the xfail-to-pass transition needs a manual review of the message contract.
What looks good
- Cases 1, 2, 4 follow the spec's xfail policy:
strict=False, distinct reason strings, each pointing at the production-side blocker that flips the gate. - Case 3 actually exercises three entry points (eager, dask, windowed) with the shared fixture, matching the issue spec wording.
- Production-side message edits in
_geotags.pyand_validation.pyare minimal and the existingtest_allow_rotated_geotiff_2115.pyregexmatch="rotation"keeps matching, so the edit does not regress sibling tests. - The
_msg_cites_release_contracthelper is intentionally loose (multiple acceptable pointers) so the gate does not break when the docs file changes extension. - The new row in
release_gate_geotiff.rstlands under "Cross-cutting CI gates" exactly where the issue spec asked. - Unique tmp file naming via the per-call UUID matches the project memo about parallel sibling PRs.
Checklist
- Algorithm matches reference/paper (n/a -- this is a test-only / message-edit PR)
- All implemented backends produce consistent results (eager / dask / windowed locked; GPU optional, see Suggestion above)
- NaN handling is correct (case 2 covers the non-finite nodata sentinel as xfail)
- Edge cases are covered by tests
- Dask chunk boundaries handled correctly
- No premature materialization or unnecessary copies
- Benchmark exists or is not needed (not needed for a release-gate test)
- README feature matrix updated (n/a -- no new public function)
- Docstrings present and accurate
* Fix comment drift: ``reader.allow_rotated`` is ``experimental`` in
SUPPORTED_FEATURES, not ``advanced``. Update the comment above the
tier-string check to match the live tier and document the upgrade
path if the tier ever moves.
* Replace brittle ``_tmp(...).replace(".tif", ".vrt")`` with an
explicit ``suffix=`` kwarg on the ``_tmp`` helper so the case-4 VRT
path is constructed directly.
* Add a GPU sub-case to case 3. The validator fires on header read
before any pixel buffer reaches the GPU, so the same typed error
surfaces uniformly with the CPU paths. Skipped on hosts without
cupy + CUDA via the existing ``requires_gpu`` marker.
* Add a docstring pointer from the inlined rotated-matrix constants
back to ``test_allow_rotated_geotiff_2115.py`` so a future grep
finds the canonical copy.
* Document the XFAIL-to-PASS transition for case 4: today's
``TypeError`` from the unknown ``stable_only`` kwarg trips the
``strict=False`` xfail; when epic #2342 lands, the new code path
must satisfy both inline message assertions before the xfail
marker comes off.
brendancol
left a comment
There was a problem hiding this comment.
PR Review (follow-up): Release-gate negative test for ambiguous GeoTIFF metadata (#2361)
Re-reviewed after commit 1f3c49ca.
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None remaining.
Nits (optional improvements)
xrspatial/geotiff/tests/test_release_gate_negative_2341.py:485-487-- the inline comment above the case-4_tmpcall still says "this call fails for an unrelated reason (unknown kwarg), which the strict=False xfail swallows". The same point now lives in the docstring's "XFAIL-to-PASS transition note" block. Either trim the inline comment or remove it -- the longer docstring is the canonical home and the duplication can drift. Pure tidy-up, not blocking.
Disposition of the prior review
- Suggestion 1 (tier-string comment drift): fixed in
1f3c49ca. The comment now matches_attrs.py:298(reader.allow_rotatedisexperimental). - Suggestion 2 (brittle
.replace(".tif", ".vrt")): fixed in1f3c49caby adding asuffix=kwarg to_tmp. - Suggestion 3 (GPU sub-case): fixed in
1f3c49ca.test_release_gate_negative_rotated_gpuwas added, gated by the existingrequires_gpumarker fromconftest.py. The case 3 release promise now covers all four read entry points: eager, dask, windowed, and GPU. - Nit 1 (point-back comment to canonical source): fixed in
1f3c49ca. - Nit 2 (shared release-contract pointer constant in
_errors.py): deferred. The PR description was explicit that this is a follow-up if the message ever gets edited a third time; the two raise sites stayed in sync in this PR. - Nit 3 (case-4 xfail tightening): fixed via a docstring "XFAIL-to-PASS transition note" rather than
raises=. Tightening toraises=GeoTIFFAmbiguousMetadataErrorwould break the test today because the current call site raisesTypeError(unknown kwarg). The docstring documents the manual verification step required before the xfail marker comes off, which preserves the gate value without flipping the test red right now.
What looks good
- All four case-3 entry points (eager / dask / windowed / GPU) now pass locally and share the
_assert_rotated_messagehelper, so message drift in any one path fails the others uniformly. - The
_tmphelper signature is cleaner with the explicitsuffix=kwarg; the case-4 path no longer depends on string replacement. - The xfail markers all stay
strict=Falseand carry distinct reason strings pointing at the production-side blockers (#1774, PAM sidecar epic,#2342). - The production-side message edits in
_geotags.pyand_validation.pydid not regress sibling tests;test_allow_rotated_geotiff_2115.py,test_allow_rotated_crs_drop_2126.py,test_allow_rotated_no_crs_2122.py, andtest_ambiguous_metadata_hooks_1987.pyall pass.
Checklist
- Algorithm matches reference/paper (n/a -- test-only / message edit)
- All implemented backends produce consistent results (eager / dask / windowed / GPU locked for case 3)
- 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 (n/a -- no new public function)
- Docstrings present and accurate
CI statusThe new test file passes on every platform in CI: ( The 3.14
Verification: checking out |
Closes #2361.
Summary
Adds
xrspatial/geotiff/tests/test_release_gate_negative_2341.py, thefifth and final PR of epic #2341. The file pins the release promise
that ambiguous GeoTIFF metadata fails closed at every promised read
entry point, with an actionable typed error.
Four parametrized cases:
.aux.xmlPAM sidecar. PAM sidecar reads are not implementedtoday, so this case is
xfail(strict=False)and flips to a passthe moment the production fix lands.
raster. Issue geotiff: int(nan) crash on integer TIFF with GDAL_NODATA="nan" #1774 currently treats this as a silent no-op; the
release promise upgrades the no-op to a typed rejection. Tracked
as
xfail(strict=False)against the geotiff: int(nan) crash on integer TIFF with GDAL_NODATA="nan" #1774 follow-up.allow_rotated=True. Activelypasses across the eager numpy, dask, and windowed read entry
points. The production-side message in
_geotags.pyand_validation.pywas updated to name thereader.allow_rotatedexperimental tier opt-in and point the caller at
docs/source/reference/release_gate_geotiff.rst.stable-only knob is owned by epic Epic: Conservative VRT support contract for GeoTIFF release #2342; tracked as
xfail(strict=False)until that knob lands.Adds a row to
docs/source/reference/release_gate_geotiff.rstunder"Cross-cutting CI gates" citing the new file.
Test plan
pytest xrspatial/geotiff/tests/test_release_gate_negative_2341.py -v(3 pass, 3 xfail).
pytest xrspatial/geotiff/tests/test_allow_rotated_geotiff_2115.py xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py xrspatial/geotiff/tests/test_allow_rotated_no_crs_2122.py xrspatial/geotiff/tests/test_ambiguous_metadata_hooks_1987.py--confirms the error-message edits did not regress existing rotated
or ambiguous-metadata coverage.
pytest xrspatial/geotiff/tests/test_release_gate_2321.py--meta-gate that checks every cited file in the checklist exists.
Assertions inlined per case; no shared helper module so the four
sibling PRs of epic #2341 cannot collide on a cross-PR symbol.