Skip to content

Enforce stable_only=True for HTTP/fsspec sources on non-VRT read paths#2826

Merged
brendancol merged 4 commits into
mainfrom
issue-2821
Jun 2, 2026
Merged

Enforce stable_only=True for HTTP/fsspec sources on non-VRT read paths#2826
brendancol merged 4 commits into
mainfrom
issue-2821

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #2821

reader.http and reader.fsspec are advanced-tier in SUPPORTED_FEATURES, but stable_only=True was only gated on the VRT dispatch. The eager non-VRT path called read_to_array directly, and the dask path only forwarded stable_only to read_vrt for .vrt sources, so an HTTP/fsspec source read straight through stable_only=True unchecked.

Changes

  • Add RemoteStableSourcesOnlyError (a GeoTIFFAmbiguousMetadataError subclass), the non-VRT sibling of VRTStableSourcesOnlyError, and re-export it from xrspatial.geotiff.
  • Add _validate_stable_only_remote and call it from open_geotiff (covers the eager/dask/GPU dispatch) and from read_geotiff_dask (direct callers). It rejects http(s):// and fsspec URIs under stable_only=True before any fetch or decode.
  • allow_experimental_codecs=True stays the documented unlock; local-file reads still pass.
  • Document the gate in docs/source/reference/geotiff.rst.

Backends

The gate lives in open_geotiff ahead of the eager / dask / GPU dispatch, plus a direct check in read_geotiff_dask. CPU eager and CPU dask are covered by the new tests; the GPU dispatch path shares the same open_geotiff gate (no GPU hardware in CI to assert directly).

Test plan

  • Eager fsspec source under stable_only=True raises RemoteStableSourcesOnlyError
  • Dask fsspec source (via open_geotiff and direct read_geotiff_dask) raises
  • http:// source raises before any network fetch
  • allow_experimental_codecs=True unlocks the read
  • Local-file eager and dask reads still succeed under stable_only=True
  • Existing VRT stable_only and release-gate suites still green

)

reader.http and reader.fsspec are advanced-tier in SUPPORTED_FEATURES, but
stable_only=True was only gated on the VRT dispatch. The eager non-VRT path
called read_to_array directly and the dask path only forwarded stable_only to
read_vrt for .vrt sources, so an HTTP/fsspec source slipped through
stable_only=True unchecked.

- Add RemoteStableSourcesOnlyError (GeoTIFFAmbiguousMetadataError subclass),
  the non-VRT sibling of VRTStableSourcesOnlyError.
- Add _validate_stable_only_remote and call it from open_geotiff (covers
  eager/dask/GPU dispatch) and from read_geotiff_dask (direct callers).
- allow_experimental_codecs=True remains the documented unlock; local-file
  reads still pass.
- Add tests across eager, dask via open_geotiff, and direct read_geotiff_dask;
  document the gate in geotiff.rst.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 2, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR Review: Enforce stable_only=True for HTTP/fsspec sources on non-VRT read paths

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • read_geotiff_gpu (xrspatial/geotiff/_backends/gpu.py:71) takes stable_only but does not gate remote HTTP/fsspec sources on its own. Through open_geotiff the new gate runs ahead of the GPU dispatch, so the public entry point is fine. A direct read_geotiff_gpu(http_url, stable_only=True) call still gets through, the same gap this PR closes for eager/dask. Out of scope for #2821 (eager + dask), but a follow-up would make all three direct entry points match. No GPU in CI to test it here anyway.

Nits (optional improvements)

  • The rejection message in _validate_stable_only_remote (xrspatial/geotiff/_validation.py:798) cites release_gate_geotiff.rst and epic #2342, matching _validate_stable_only_vrt. The doc that actually describes this gate is geotiff.rst, not the release-gate checklist. The sibling validator does the same, so leaving it is fine.

What looks good

  • The gate sits in open_geotiff ahead of the eager/dask/GPU dispatch (__init__.py:917), with a direct check in read_geotiff_dask (_backends/dask.py), so both the routed and direct dask paths are covered.
  • _validate_stable_only_remote mirrors _validate_stable_only_vrt: same early returns, same allow_experimental_codecs unlock, same error family (GeoTIFFAmbiguousMetadataError).
  • source is already normalized by _coerce_path before the gate, so path-like inputs are strings by the time _is_fsspec_uri sees them.
  • Tests cover eager, dask via open_geotiff, direct read_geotiff_dask, the http:// case (gated before any fetch), the experimental-codecs unlock, and both local-file backends still passing.

Checklist

  • Behavior matches the VRT gate it mirrors
  • Targeted backends (eager, dask) gated and tested
  • Error type/message consistent with the existing VRT path
  • Edge cases covered (local file passes, http gated pre-fetch, unlock works)
  • No premature materialization (gate runs before any read)
  • Benchmark not needed (validation-only change)
  • Docstrings present and accurate
  • Doc updated (geotiff.rst)

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review follow-up

No new commits: both findings were resolved without code changes in this PR.

  • Suggestion (direct read_geotiff_gpu does not gate remote sources): deferred to #2830. It is GPU-only and was out of scope for the eager + dask finding in #2821. The public open_geotiff(..., gpu=True) path is already covered by the shared gate in this PR; only the direct GPU entry point remains, which #2830 tracks.
  • Nit (error message cites release_gate_geotiff.rst rather than geotiff.rst): dismissed. The citation matches the sibling _validate_stable_only_vrt validator on purpose. Changing one and not the other would split the convention, and the review itself flagged leaving it as fine.

The new error is exported in xrspatial.geotiff.__all__, so the frozen
public-API contract test (test_all_lists_supported_functions) and the
_errors.__all__ list both have to include it. CI caught the missing
entries on the eager fast-lane job.
@brendancol brendancol merged commit a2a4479 into main Jun 2, 2026
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.

stable_only=True does not reject HTTP/fsspec sources on non-VRT read paths

1 participant