Skip to content

Hide read_to_array from xrspatial.geotiff public namespace (#1708)#1711

Merged
brendancol merged 2 commits into
mainfrom
deep-sweep-api-consistency-geotiff-2026-05-12-1778611459-2954-02
May 12, 2026
Merged

Hide read_to_array from xrspatial.geotiff public namespace (#1708)#1711
brendancol merged 2 commits into
mainfrom
deep-sweep-api-consistency-geotiff-2026-05-12-1778611459-2954-02

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

The api-consistency sweep on 2026-05-12 flagged an orphan-API leak. xrspatial/geotiff/__init__.py imports read_to_array from ._reader so the eager open_geotiff body can call it, but as a side-effect that bound the name in the public xrspatial.geotiff namespace. __all__ does not list it, the module-level "Public API" docstring does not document it, and the Sphinx pages do not advertise it -- yet from xrspatial.geotiff import read_to_array worked anyway.

The library's own test_band_validation_1673.py imported it that way six times, proving the leak was being used as if it were public. External users who pattern-matched against the test would also be relying on a name the maintainer never committed to support. A future cleanup that removed the top-level import would have broken them with no deprecation path.

This PR:

  • Binds the helper under a leading-underscore name in __init__.py: from ._reader import read_to_array as _read_to_array.
  • Updates six internal call sites (one in open_geotiff, one in the dask delayed window reader, four GPU-fallback paths in read_geotiff_gpu).
  • Updates test_band_validation_1673.py to import from xrspatial.geotiff._reader directly, which is the canonical private path.
  • Adds test_namespace_no_leak_1708.py pinning the namespace contract.

No runtime behaviour change in the supported public API.

Closes #1708.

Test plan

  • pytest xrspatial/geotiff/tests/test_namespace_no_leak_1708.py covers the no-leak contract (4 tests) and the canonical private import.
  • pytest xrspatial/geotiff/tests/test_band_validation_1673.py still passes (the updated imports).
  • pytest xrspatial/geotiff/tests/test_signature_annotations_1654.py xrspatial/geotiff/tests/test_signature_parity_1631.py xrspatial/geotiff/tests/test_features.py -k "all or contract or signature" passes (__all__ and namespace contracts intact).
  • Smoke: open_geotiff (eager + windowed) and read_geotiff_dask produce the same shapes and data as before.

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 12, 2026
@brendancol brendancol requested a review from Copilot May 12, 2026 18:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens xrspatial.geotiff’s public namespace by preventing the internal helper read_to_array (defined in xrspatial.geotiff._reader) from being inadvertently re-exported via xrspatial.geotiff.__init__, while keeping all internal call paths working.

Changes:

  • Rebinds _reader.read_to_array as a private name (_read_to_array) inside xrspatial/geotiff/__init__.py and updates internal call sites accordingly.
  • Updates test_band_validation_1673.py to import read_to_array from the canonical private module path (xrspatial.geotiff._reader).
  • Adds a regression test module to ensure read_to_array does not reappear in the top-level xrspatial.geotiff namespace.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
xrspatial/geotiff/__init__.py Stops leaking read_to_array into the public namespace while preserving internal usage via _read_to_array.
xrspatial/geotiff/tests/test_band_validation_1673.py Switches tests to import read_to_array directly from the private _reader module.
xrspatial/geotiff/tests/test_namespace_no_leak_1708.py Adds regression coverage that enforces the “no public read_to_array export” namespace contract.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The api-consistency sweep on 2026-05-12 flagged an orphan-API leak:
xrspatial/geotiff/__init__.py imports ``read_to_array`` from ``._reader``
so the eager ``open_geotiff`` body can call it, but as a side-effect
that bound the name in the public ``xrspatial.geotiff`` namespace.
``__all__`` does not list it, the module-level "Public API" docstring
does not document it, and the Sphinx pages do not advertise it -- but
``from xrspatial.geotiff import read_to_array`` worked anyway.

The library's own ``test_band_validation_1673.py`` imported it that way
six times, proving the leak was being used as if it were public.
External users who pattern-matched against the test would also be
relying on a name the maintainer never committed to support. A future
cleanup that removed the top-level import would have broken them with
no deprecation path.

This change binds the helper under a leading-underscore name in
``__init__.py``:

    from ._reader import UnsafeURLError
    from ._reader import read_to_array as _read_to_array

and updates each internal call site (six call sites: ``open_geotiff``,
the dask delayed window reader, and four GPU-fallback paths in
``read_geotiff_gpu``). The single test file that imported the leaked
name is updated to import from ``xrspatial.geotiff._reader`` directly,
which is the canonical private path.

``test_namespace_no_leak_1708.py`` pins the namespace contract:
``read_to_array`` must not be an attribute of ``xrspatial.geotiff``,
``__all__`` must not list it, and the canonical
``xrspatial.geotiff._reader.read_to_array`` import must still work.
This guards against a future maintainer re-adding a bare
``from ._reader import read_to_array`` line without realising it would
re-leak the name.

No runtime behaviour change in the supported public API. The deprecated
``plot_geotiff`` continues to be exposed at the top level for backward
compatibility because it has a documented deprecation path; this commit
takes no position on ``plot_geotiff``.

Closes #1708.
@brendancol brendancol force-pushed the deep-sweep-api-consistency-geotiff-2026-05-12-1778611459-2954-02 branch from 143f9d4 to 9b9eb82 Compare May 12, 2026 19:29
The #1708 cleanup hid ``read_to_array`` from ``xrspatial.geotiff`` and
bound the internal alias as ``_read_to_array``. The existing #1624
trace test still set ``xrspatial.geotiff.read_to_array`` via
monkeypatch, which now raises ``AttributeError`` at the setattr call.
Patch the alias the dask worker actually reads instead.
@brendancol brendancol merged commit 8adb749 into main May 12, 2026
10 of 11 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.

geotiff: read_to_array leaks into public namespace but is not in __all__ or docs

2 participants