geotiff: default _vrt.read_vrt missing_sources to 'raise' (#1843)#1850
Merged
Conversation
Flip the internal ``read_vrt`` default in ``_vrt.py`` from ``'warn'`` to ``'raise'`` so an unreadable VRT source halts the call instead of leaving a silent zero-fill hole. On an integer raster without a nodata sentinel the old lenient default produced data indistinguishable from real zero pixels unless the caller checked ``attrs['vrt_holes']``. The change matches the up-front rejection pattern the rest of the module uses (#1697, #1737, #1784, band validation). Callers that want the previous behaviour pass ``missing_sources='warn'`` explicitly, which still emits ``GeoTIFFFallbackWarning`` and records the skipped source on ``vrt.holes``. ``XRSPATIAL_GEOTIFF_STRICT=1`` keeps its module-wide force-raise semantics. One existing test (``test_missing_source_still_takes_lenient_warning_path`` from #1784) relied on the lenient default; it now passes ``missing_sources='warn'`` explicitly to exercise the opt-in branch. The public ``read_vrt`` / ``open_geotiff`` wrappers in ``__init__.py`` still default to ``'warn'`` and forward the kwarg explicitly, so this commit changes the internal contract only; aligning the public default is a separate follow-up.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens the internal GeoTIFF VRT reader by changing _vrt.read_vrt to fail fast on unreadable sources by default, while preserving explicit lenient behavior through missing_sources='warn'.
Changes:
- Changes internal
_vrt.read_vrtdefaultmissing_sourcesfrom'warn'to'raise'. - Updates related documentation/changelog text and the existing lenient-path test.
- Adds regression coverage for the new default, explicit warn behavior, and strict-mode precedence.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
xrspatial/geotiff/_vrt.py |
Updates the internal VRT reader default and parameter documentation. |
xrspatial/geotiff/tests/test_vrt_missing_sources_default_raise_1843.py |
Adds regression tests for default raise, explicit warn, and strict env behavior. |
xrspatial/geotiff/tests/test_geotiff_vrt_srcrect_validation_1784.py |
Updates the lenient missing-source test to opt in with missing_sources='warn'. |
CHANGELOG.md |
Documents the internal default change and migration path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| band: int | None = None, | ||
| max_pixels: int | None = None, | ||
| missing_sources: str = 'warn', | ||
| missing_sources: str = 'raise', |
Address copilot review on #1850: the inline notes around the source-read fallback still described "Default mode warns" and the vrt_holes inspection path, which no longer match the new raise-by-default behavior. Reword them to describe the current default and the explicit missing_sources='warn' opt-in.
Contributor
Author
|
Updated the fallback comment block in _vrt.py to describe the new raise-by-default behavior and the explicit missing_sources='warn' opt-in. Thanks for catching the drift. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1843.
Summary
_vrt.read_vrtdefaultmissing_sourcesfrom'warn'to'raise'so an unreadable VRT source halts the call instead of silently leaving a zero-fill hole.test_missing_source_still_takes_lenient_warning_pathfrom geotiff: malformed VRT SrcRect is swallowed by the lenient source-read fallback #1784) that relied on the lenient default; it now passesmissing_sources='warn'explicitly to exercise the opt-in path.test_vrt_missing_sources_default_raise_1843.pycovering the new default, the explicit-warn escape hatch, andXRSPATIAL_GEOTIFF_STRICT=1precedence.Why
The lenient default produced a zero-fill hole on integer rasters without a nodata sentinel, indistinguishable from real zero pixels unless the caller checked
attrs['vrt_holes']. The rest of the module already rejects malformed input up front (#1697, #1737, #1784, band validation). Before stable, the safer contract should be the default; lenience should be opt-in.Migration
Callers that want partial mosaics:
The public
read_vrtandopen_geotiffwrappers in__init__.pystill default to'warn'and forward the kwarg explicitly, so user-facing behaviour does not change in this PR. Aligning the public default is a separate follow-up.XRSPATIAL_GEOTIFF_STRICT=1keeps its module-wide force-raise semantics regardless of this kwarg.Test plan
pytest xrspatial/geotiff/tests/test_vrt_missing_sources_default_raise_1843.py(3 new tests)pytest xrspatial/geotiff/tests/test_geotiff_vrt_srcrect_validation_1784.py(updated lenient-path test)pytest xrspatial/geotiff/tests/test_vrt_missing_sources_policy_1799.py(existing explicit-kwarg tests still green)pytest xrspatial/geotiff/tests/test_open_geotiff_missing_sources_1810.py(public dispatcher unchanged)test_features.py, GPU predictor tests, GPU tile size test)