Skip to content

Correct stable_only docstring to state HTTP/fsspec rejection#2822

Merged
brendancol merged 3 commits into
mainfrom
issue-2820
Jun 2, 2026
Merged

Correct stable_only docstring to state HTTP/fsspec rejection#2822
brendancol merged 3 commits into
mainfrom
issue-2820

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #2820.

Docs-only fix. The stable_only docstring for open_geotiff claimed non-VRT sources "already ride the stable reader.local_file path... so the flag is a no-op for them." That is wrong: the non-VRT read path also accepts HTTP and fsspec sources, and reader.http / reader.fsspec are advanced tier in SUPPORTED_FEATURES (xrspatial/geotiff/_attrs.py:292-293). The old wording hid that those sources are not on the stable path.

Changes:

  • xrspatial/geotiff/__init__.py: rewrite the stable_only param docstring to say it restricts reads to the stable-tier local-file path and rejects advanced-tier sources (VRT, HTTP, fsspec).
  • xrspatial/geotiff/_backends/dask.py: same correction on the duplicated wording in the dask backend docstring.

No behavior change. The actual enforcement of stable_only=True on the eager/dask non-VRT HTTP/fsspec paths is a separate behavior fix; this PR only makes the documented contract match what it should describe once that lands. The VRT rejection is already enforced (#2447, issue #2443) and the docstring stays consistent with it.

Backend coverage: documentation only, no code path affected (numpy / cupy / dask+numpy / dask+cupy unchanged).

Test plan:

  • flake8 clean on both changed files
  • pytest xrspatial/geotiff/tests/release_gates/test_stable_features.py xrspatial/geotiff/tests/unit/test_signatures.py (355 passed, 1 xfailed)
  • Imported xrspatial.geotiff and confirmed the rendered open_geotiff.__doc__ reads correctly

@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: Correct stable_only docstring to state HTTP/fsspec rejection

Docs-only change. Two stable_only docstrings get rewritten so they stop claiming the flag is a no-op for all non-VRT sources. The new wording says stable_only=True restricts the read to the stable-tier local-file path and rejects advanced-tier sources (VRT, HTTP, fsspec). That matches the tiering in _attrs.py:292-293, where reader.http and reader.fsspec are advanced. The correction is accurate and the diff stays inside the two docstrings.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • The docstring now describes a contract that is only partly enforced today. I checked both backends: _validate_stable_only_vrt (_validation.py:668) gates VRT only, and stable_only is never forwarded to read_to_array on the eager non-VRT path (__init__.py:959) or used to reject HTTP/fsspec on the dask path (_backends/dask.py:237-240). So as of this PR, stable_only=True does not actually reject an http(s):// or s3:// source. The docstring leads the enforcement. That is the intended split (this PR is docs-only, the behavior fix is tracked separately in #2820's scope), but the gap should be visible to a reader. Suggest one short sentence in the PR body, or a .. note::, pointing at the enforcement issue so nobody reads the docstring as a description of current runtime behavior. Not blocking, since the brief explicitly calls for documenting the contract as it should read once enforced.

Nits (optional improvements)

None.

What looks good

  • Both copies of the wording were caught (__init__.py and _backends/dask.py), so the two docstrings stay consistent.
  • The new text names the concrete URI schemes (http(s)://, s3://) and the exact feature keys, which is more useful than the old vague "non-VRT sources" phrasing.
  • flake8 is clean and the release-gate / signature tests still pass.

Checklist

  • Wording matches the SUPPORTED_FEATURES tiering
  • Both duplicated docstrings updated
  • No code path changed (docs only)
  • Docstring renders (verified via open_geotiff.__doc__)
  • Documented contract is fully enforced in code (out of scope here; tracked separately)

@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.

Follow-up review (after 096b097)

The one Suggestion from the first pass is addressed. Both stable_only docstrings now carry a short note that the VRT rejection is enforced today while the HTTP / fsspec rejection is the documented contract still being rolled out, with a pointer to issue #2820. A reader can no longer mistake the docstring for a description of current runtime behavior on the HTTP/fsspec paths.

No new findings. flake8 clean, xrspatial.geotiff imports, and the release-gate plus signature tests pass (355 passed, 1 xfailed). Still docs-only, no code path touched.

Nothing left open except the enforcement work itself, which is out of scope for this PR by design.

@brendancol brendancol merged commit 125dedb 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 docstring overstates the stable-path guarantee for non-VRT sources

1 participant