Skip to content

Reject VRT sources under stable_only=True (#2443)#2447

Merged
brendancol merged 2 commits into
mainfrom
issue-2443
May 26, 2026
Merged

Reject VRT sources under stable_only=True (#2443)#2447
brendancol merged 2 commits into
mainfrom
issue-2443

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • Add stable_only: bool = False to open_geotiff, read_geotiff_dask, read_geotiff_gpu, and read_vrt. When True on a VRT source, the read raises VRTStableSourcesOnlyError (a GeoTIFFAmbiguousMetadataError subclass) at graph-build / eager-read setup time, before any pixel decode.
  • The rejection message names the offending VRT path, the stable_only flag, and the allow_experimental_codecs unlock, and cites docs/source/reference/release_gate_geotiff.rst plus epic Epic: Conservative VRT support contract for GeoTIFF release #2342.
  • Flips test_release_gate_negative_mixed_tier_vrt_children from xfail to a regular pass.

Backend coverage

Numpy, cupy, dask+numpy, dask+cupy. VRT routing happens in open_geotiff and read_geotiff_dask; both entry points forward the kwarg to read_vrt, which carries the validator call. The GPU eager path does not open VRT sources directly, so the flag is a no-op there (kept on the signature for cross-backend symmetry, per the canonical-order test).

Test plan

  • pytest xrspatial/geotiff/tests/test_vrt_stable_only_2443.py (7 new tests: default rejection on each entry point, default-false no-op, allow_experimental_codecs unlock, error subclass invariant)
  • pytest xrspatial/geotiff/tests/release_gates/test_stable_features.py::test_release_gate_negative_mixed_tier_vrt_children (xfail flipped to pass)
  • pytest xrspatial/geotiff -n auto (5811 passed, 4 unrelated xfailed)
  • pytest xrspatial -n auto (10675 passed; two *_dask_temp_cleanup failures are pre-existing flakes unrelated to this change)

Closes #2443.

Add a `stable_only: bool = False` kwarg to `open_geotiff`,
`read_geotiff_dask`, `read_geotiff_gpu`, and `read_vrt`. When True,
the VRT path raises a typed `VRTStableSourcesOnlyError` (a
`GeoTIFFAmbiguousMetadataError` subclass) before any pixel decode.
The message names the offending VRT path and the
`allow_experimental_codecs` unlock so callers can opt into the
broader tier set explicitly. Flips the
`test_release_gate_negative_mixed_tier_vrt_children` xfail from epic
#2342.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 26, 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: Reject VRT sources under stable_only=True (#2443)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None substantive. The PR mirrors the #2442 precedent closely.

Nits (optional improvements)

  • xrspatial/geotiff/_validation.py:715-737: the validator does not verify the source is actually a VRT. read_vrt is the only caller and is contractually VRT-only, so this is defensive at worst -- but the message says "VRT source '{source}'" unconditionally, which would mislabel a non-VRT path if someone called _validate_stable_only_vrt directly from a future code path. Optional: tighten the docstring to say "the caller must have already classified source as a VRT," or sniff source.lower().endswith('.vrt') and pass through silently otherwise.
  • xrspatial/geotiff/tests/test_vrt_stable_only_2443.py:65-79: the "unlock" test asserts that with allow_experimental_codecs=True we do not get VRTStableSourcesOnlyError, but does not pin the downstream error (still VRTUnsupportedError for the no-bands case). Slight risk that a future refactor silently broadens the unlock past intent. Optional: assert pytest.raises(VRTUnsupportedError) instead of Exception.

What looks good

  • Matches the typed-error-plus-opt-in-kwarg shape from PR #2442 / #2441: subclass of GeoTIFFAmbiguousMetadataError, kwarg threaded through every public reader, canonical-order test updated alongside.
  • Error message satisfies both release-gate assertions (names the opt-in flag and cites release_gate_geotiff plus #2342).
  • Gate fires at the top of read_vrt, before _validate_dispatch_kwargs, so the typed error is the first raise the caller sees.
  • New test file covers all four entry points (eager VRT, direct VRT, dask VRT, the unlock path) plus the subclass invariant.

Checklist

  • Algorithm matches reference: matches the release-gate test contract.
  • All implemented backends produce consistent results: validator runs in read_vrt; both routing paths (open_geotiff and read_geotiff_dask) forward the kwarg.
  • NaN handling is correct: N/A (validation-only).
  • Edge cases are covered by tests: default-false no-op, unlock path, subclass invariant.
  • Dask chunk boundaries handled correctly: N/A.
  • No premature materialization or unnecessary copies: validator raises before any decode.
  • Benchmark exists or is not needed: not needed.
  • README feature matrix updated (if applicable): not applicable.
  • Docstrings present and accurate: every entry point's docstring carries the new kwarg.

…rts (#2443)

- ``_validate_stable_only_vrt`` now passes through silently when the
  source is not a ``.vrt`` path, so a future call site that forwards a
  non-VRT path cannot trip a mislabeled "VRT source" rejection. The
  docstring spells out the pass-through contract.
- ``test_vrt_stable_only_2443.py`` pins the downstream
  ``VRTUnsupportedError`` on the default-false and ``allow_experimental_codecs``-unlock
  paths so a future refactor cannot silently broaden either branch past
  intent.
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.

Follow-up Review: nits addressed (commit 2a642dd)

Disposition of original findings

  • Nit 1 (validator does not check for VRT extension): Fixed. _validate_stable_only_vrt now passes through silently when source is not a string ending in .vrt (case-insensitive). Docstring updated to spell out the pass-through.
  • Nit 2 (unlock test asserts only Exception): Fixed. Both the default-false and allow_experimental_codecs unlock tests now assert pytest.raises(VRTUnsupportedError) so a future refactor cannot silently broaden either branch.

Verification

  • pytest xrspatial/geotiff/tests/test_vrt_stable_only_2443.py xrspatial/geotiff/tests/release_gates/test_stable_features.py::test_release_gate_negative_mixed_tier_vrt_children xrspatial/geotiff/tests/test_features.py::TestPublicAPI xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py -- 16 passed, 0 failed.

No further blockers, suggestions, or nits.

@brendancol brendancol merged commit 8fcb89e into main May 26, 2026
6 of 7 checks passed
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.

Reject mixed-tier VRT child sources under stable_only=True (epic #2342)

1 participant