Align SUPPORTED_FEATURES with epic #2340 tiering#2350
Merged
Conversation
Reconcile xrspatial.geotiff.SUPPORTED_FEATURES with the release-contract tiering proposed in epic #2340. Additions at stable tier: - reader.windowed (release-gate covered by the existing window-read suite). - reader.dask (cross-backend parity gate in test_backend_parity_matrix.py and test_backend_full_parity_2211.py). Demotions from advanced to experimental: - reader.allow_rotated (read-side escape hatch around the rotated-transform check; the epic places rotated writes as Unsupported and lumps the read-side opt-in with the permissive escape hatches). - reader.allow_unparseable_crs (explicit-CRS permissive escape hatch). New test test_supported_features_shape_2348.py pins the structural invariants of the mapping so future drift fails CI: - every entry carries a tier label, - the tier set is closed at {stable, advanced, experimental, internal_only}, - the dict literal carries no duplicate keys (parsed from source so the silent dedup of duplicate keys in a Python dict literal cannot hide a copy/paste typo), - the wave-1 reconciliation lands the expected tiers. Metadata only: runtime behaviour of every read and write path is unchanged.
brendancol
commented
May 24, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Align SUPPORTED_FEATURES with epic #2340 tiering
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
xrspatial/geotiff/_attrs.py:231-- the older block comment listing things "outside the stable contract" still flagsreader.allow_rotatedwithout noting that wave-1 also moved its tier fromadvancedtoexperimental. The statement is still factually true (experimental is outside stable), but a reader has to jump to the wave-1 block to learn the tier itself changed. A one-line nudge would make the two comment blocks tell the same story.
Nits (optional improvements)
xrspatial/geotiff/_attrs.py:279-283-- the inline comment hard-codes the names of seven test files that gatereader.windowed. The names aren't load-bearing (no runtime parser reads them), but a future rename will silently stale the comment. Either trim to "covered by the window-read suite inxrspatial/geotiff/tests/" or accept the maintenance cost.
What looks good
- Scope is narrow:
_attrs.py, one new shape test, one CHANGELOG line. Nothing else touched. - The shape test parses the dict literal via
astto catch Python's silent dedup of duplicate dict-literal keys. That's a real footgun for a single-source-of-truth constant. test_epic_2340_wave_1_reconciliationpins the four specific tier moves so any future revert needs a justifying commit.test_every_tier_label_is_usedcatches the case where a label is documented in the package docstring but no entry references it.- The CHANGELOG entry names every concrete change, the public-API break, and the parent epic.
Checklist
- Algorithm matches reference -- N/A (metadata-only).
- All implemented backends produce consistent results -- N/A (no backend code).
- NaN handling is correct -- N/A.
- Edge cases are covered by tests -- empty/closed tier set, duplicate keys, missing tier.
- Dask chunk boundaries handled correctly -- N/A.
- No premature materialization or unnecessary copies -- N/A.
- Benchmark exists or is not needed -- not needed.
- README feature matrix updated -- N/A (no new function).
- Docstrings present and accurate -- new test file has module + per-test docstrings; the constant's tier semantics block covers the new entries.
…2348) Apply review findings on PR #2350: - The "stays outside the stable contract" block comment now flags that wave-1 of epic #2340 also demoted ``reader.allow_rotated`` from ``advanced`` to ``experimental``, so the two comment blocks tell the same story instead of leaving a reader to reconcile them. - The ``reader.windowed`` inline comment no longer hard-codes seven specific test filenames; it now names the test directory and the branches the window-read suite covers (eager numpy, dask, GPU, HTTP, VRT). A future rename of any single test file no longer stales the comment. Metadata-only; the tier mapping itself is unchanged.
brendancol
commented
May 24, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review pass
Both findings from the first review pass have been addressed in 1be3b11:
- Suggestion (
_attrs.py:231) fixed: the outside-stable-contract bullet now flags the wave-1 tier demotion inline, so a reader doesn't have to reconcile the two comment blocks. - Nit (
_attrs.py:279-283) fixed: the windowed-reads comment now describes the coverage by backend ("eager numpy, dask, GPU, HTTP, VRT") instead of hard-coding seven filenames.
No new findings on the re-pass. Shape and tier tests still pass (30/30).
The wave-1 reconciliation under epic #2340 promoted reader.windowed and reader.dask to stable in SUPPORTED_FEATURES, but the release-gate checklist never grew matching rows. test_release_gate_2321.py's "every stable key shows up in the checklist" assertion caught the gap on the 3.14 matrix run. Each new row cites an existing regression test on disk so the gate-1 "cited files exist" check stays green.
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
1 participant
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 #2348. Wave 1 of 6 under epic #2340.
Summary
xrspatial.geotiff.SUPPORTED_FEATURESwith the GeoTIFF release-contract tiering proposed in epic Epic: GeoTIFF release contract and feature tiering #2340.reader.windowedandreader.daskatstable(both have release-gate coverage).reader.allow_rotatedandreader.allow_unparseable_crsfromadvancedtoexperimentalto match the epic's placement of permissive read-side escape hatches.What ships
SUPPORTED_FEATURESis metadata-only. Runtime behaviour of every read and write path is unchanged. The only break is for callers that gate on the exact string value of the two demoted entries (reader.allow_rotated,reader.allow_unparseable_crs).Out of scope under epic #2340 (sibling PRs handle these):
docs/source/.Test plan
pytest xrspatial/geotiff/tests/test_supported_features_shape_2348.py.pytest xrspatial/geotiff/tests/test_supported_features_tiers_2137.py.