Skip to content

read_vrt: chunked missing_sources='raise' fails at build, not compute#2271

Merged
brendancol merged 3 commits into
mainfrom
issue-2265
May 21, 2026
Merged

read_vrt: chunked missing_sources='raise' fails at build, not compute#2271
brendancol merged 3 commits into
mainfrom
issue-2265

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • Closes Chunked VRT: missing_sources='raise' defers raise to compute, breaking 'fails immediately' contract #2265.
  • read_vrt(chunks=N, missing_sources='raise') now fails at construction time, not at compute time. The docstring already promised this; the chunked path didn't honor it.
  • The static os.path.exists sweep that already runs to populate vrt_holes is the place the raise hooks in. No source decoding at build.
  • Scoped to the requested window= and band=: a windowed or band-restricted read that doesn't touch a missing source still builds and computes, matching the eager path.
  • XRSPATIAL_GEOTIFF_STRICT=1 forces the raise regardless of the kwarg.

Backend coverage

Build-time validation change in the dispatcher. All four backends (numpy / cupy / dask+numpy / dask+cupy) flow through the same _read_vrt_chunked static sweep and pick up the new behavior.

Test plan

  • New test module test_vrt_chunked_missing_raise_at_build_2265.py:
    • Build raises immediately on 'raise' (and on the default kwarg)
    • Error message names the 'warn' opt-in
    • Window past missing source still builds and computes
    • Window intersecting missing source still raises
    • band= restriction scopes the raise (missing source on an unselected band is ignored)
    • XRSPATIAL_GEOTIFF_STRICT=1 overrides 'warn' kwarg
    • 'warn' path still records vrt_holes and warns at compute time
  • Existing tests updated for the new contract (test_vrt_chunked_missing_sources_1799.py, test_vrt_lazy_chunks_1814.py, test_read_vrt_lazy_chunks_1798.py)
  • Full VRT test suite passes locally (659 tests via pytest -k vrt)

The chunked VRT path used to defer the 'raise' policy to compute time:
the static os.path.exists sweep populated attrs['vrt_holes'] but
construction returned a delayed graph that only blew up when a chunk
touching a hole was computed. A downstream pipeline that windowed past
the missing tile could ship a partial mosaic silently, contradicting
the public docstring's "fails immediately" promise.

Hook the raise into the same static sweep that already runs to populate
vrt_holes. The check is scoped to the requested window= and band= so
a windowed or band-restricted read that does not depend on a missing
source still succeeds (eager-path parity). XRSPATIAL_GEOTIFF_STRICT=1
forces the raise regardless of the kwarg.

Updates the two existing tests that previously locked in the
deferred-raise behavior (test_vrt_chunked_missing_sources_1799 and
test_vrt_lazy_chunks_1814) and adds a focused test module for the new
contract.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 21, 2026
Self-review catch: the band-scope gate compared
``vrt_band.band_num`` (the 1-based GDAL attribute from the XML) to
``band + 1`` (the 0-based public kwarg shifted up by one). For
well-formed VRTs that emit sequential ``band="1"``, ``band="2"``,
... attributes this is equivalent to comparing list positions, but
the public ``selected_bands = [vrt.bands[band]]`` slice a few lines
above already treats ``band`` as a list index, so the gate should
match that convention. Switching to ``enumerate`` makes the two
agree even if a hand-rolled VRT emits non-sequential ``band=``
attributes.
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: read_vrt: chunked missing_sources='raise' fails at build, not compute

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/_backends/vrt.py:989 — the band-scope gate compared vrt_band.band_num (the XML's 1-based band= attribute) against band + 1, but the slice a few lines up (selected_bands = [vrt.bands[band]] at line 813) uses band as a list index. They line up for sequential VRTs, but a hand-rolled VRT with non-sequential band= attributes could route a hole to the wrong scope. Switched to enumerate so both gates use list position. Fixed in a follow-up commit.

Nits (optional improvements)

  • xrspatial/geotiff/_backends/vrt.py:1042 — the raise message names only chunked_holes[0] and the total count. With several missing sources, listing a few paths instead of one would save the caller a 'warn'-mode re-read. Not high priority; the count is enough to know to investigate.
  • xrspatial/geotiff/tests/test_vrt_chunked_missing_raise_at_build_2265.py — no test covers multiple missing sources. Could assert the count token in the error message for that case.

What looks good

  • The fix piggybacks on the existing static os.path.exists sweep, so build still doesn't decode any source.
  • Window-scoping (lines 1001-1008) and band-scoping (lines 988-991) mirror the eager path. A windowed or band-restricted read that misses the bad tile still builds.
  • XRSPATIAL_GEOTIFF_STRICT=1 is honored alongside the kwarg.
  • Docstring and docs/source/reference/geotiff.rst updated. The old RST still claimed 'warn' was the default (stale since #1860); cleaned up here as a side effect.
  • The three pre-existing tests that locked in the deferred-raise behavior were rewritten under the new contract instead of deleted, so the underlying invariants are still pinned.
  • New test module covers build-time raise, error-message content, window scoping, band scoping, strict-mode override, and 'warn' preservation.

Checklist

  • Algorithm matches reference (eager-path contract from #1860)
  • Backends share the dispatcher path, so the build-time raise applies to numpy / cupy / dask+numpy / dask+cupy
  • NaN handling unaffected (build-time validation only)
  • Edge cases covered (empty window, band restriction, strict mode)
  • Dask chunk boundaries unaffected
  • No premature materialization (sweep only calls os.path.exists)
  • No benchmark needed
  • README feature matrix unchanged
  • Docstrings and RST docs updated

Address review nits: when the static sweep finds multiple missing
sources, the raise message now lists the first 3 paths plus an "and
N more" tail, so the caller can act on them without having to flip
to ``missing_sources='warn'`` and re-parse ``attrs['vrt_holes']``.
The total count is still appended so the caller knows the full
magnitude even when the preview is truncated.

Adds two tests against a new multi-missing-source VRT helper:
* n=2 lists both paths and the count
* n=5 truncates to the first 3 and appends "and 2 more"
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 (after #2265 fixes)

Disposition of prior findings

  • Suggestion (band-scope vrt_band.band_num vs list index, vrt.py:989): fixed in commit aec6422f. The gate now uses enumerate on vrt.bands and compares band_idx == band so it matches the selected_bands = [vrt.bands[band]] slice convention.
  • Nit (raise message lists only the first hole, vrt.py:1042): fixed in commit e57e646e. The message now previews up to 3 missing paths and appends an "and N more" tail when truncated. Total count still reported.
  • Nit (no test for multiple missing sources): added in commit e57e646e. Two new tests under TestMultipleMissingSources: one for n=2 (both paths listed) and one for n=5 (first 3 listed, "and 2 more" suffix, total count reported).

New findings

None.

What looks good (additions)

  • New tests use a dedicated _make_multi_missing_vrt helper rather than monkey-patching the existing single-hole helper, so the multi-source schema stays explicit.
  • Preview cap is bounded (3) and the truncation suffix includes the remainder count, so the error message size is predictable even on mosaics with hundreds of holes.
  • All 24 tests in the new module pass plus the rewritten existing suites; broader pytest -k 'vrt or strict' is green (696 passed).

Final checklist

  • Build-time raise honors missing_sources='raise', the default kwarg, and XRSPATIAL_GEOTIFF_STRICT=1
  • Window-scoping and band-scoping mirror the eager path
  • 'warn' semantics preserved (records vrt_holes, warns at compute time)
  • Error message previews up to 3 paths and reports the total
  • All four backends share the dispatcher path
  • Tests cover single-hole, multi-hole, window-past, band-restricted, and strict-mode cases
  • Docstring and RST docs updated to match the new contract

@brendancol brendancol merged commit 48fc5db into main May 21, 2026
5 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.

Chunked VRT: missing_sources='raise' defers raise to compute, breaking 'fails immediately' contract

1 participant