Skip to content

Privatize build_vrt; route to_geotiff's VRT path through it#2979

Merged
brendancol merged 4 commits into
mainfrom
issue-2974
Jun 5, 2026
Merged

Privatize build_vrt; route to_geotiff's VRT path through it#2979
brendancol merged 4 commits into
mainfrom
issue-2974

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

Closes #2974.

  • Drops build_vrt from the public GeoTIFF API and renames the wrapper to _build_vrt (out of __all__ and the package docstring).
  • Routes to_geotiff's .vrt write path (_write_vrt_tiled) through _build_vrt, so it's the single internal entry point for VRT-index emission. That path used to call _vrt.write_vrt directly, which left the public build_vrt with no internal caller at all.
  • Updates the docs (GeoTIFF reference, safe-IO guide, release-gate table, internal notes), the README feature matrix, and the user-guide notebook's VRT section to write a mosaic through to_geotiff('.vrt') instead of the removed helper.
  • Updates tests to import _build_vrt; the public-surface assertions now expect only open_geotiff and to_geotiff.

build_vrt is advanced-tier, not stable, so it's allowed to change between releases. Anyone who called xrspatial.geotiff.build_vrt either writes to a .vrt path through to_geotiff or imports _build_vrt.

Backend coverage

No behavior change. VRT writing already ran on the eager and dask paths; this is an API-surface refactor. Round-trip checked: to_geotiff(da_chunked, 'mosaic.vrt') tiles the array and open_geotiff reads it back matching the original.

Test plan

  • Public-surface assertions flipped to {open_geotiff, to_geotiff}, plus a new test that build_vrt is not importable while _build_vrt is
  • Every test file that imported the public name now uses _build_vrt
  • VRT round-trip through to_geotiff('.vrt') passes on numpy and dask
  • flake8 + isort clean on changed files
  • User-guide notebook VRT cell re-run; output regenerated from a real execution

…h it (#2974)

build_vrt is dropped from the public API (__all__, package docstring) and
renamed to _build_vrt. to_geotiff's .vrt write path (_write_vrt_tiled) now
routes through _build_vrt instead of calling _vrt.write_vrt directly, so
_build_vrt is the single internal VRT-index entry point.

Tests updated to import the private name; the public-surface assertions now
expect only open_geotiff and to_geotiff.
Remove the public build_vrt entry from the GeoTIFF reference, safe-IO
user guide, release-gate contract table, README feature matrix, and the
internal-dev notes. The user-guide notebook's VRT section now writes a
mosaic through to_geotiff's .vrt path (chunked DataArray -> tiled
GeoTIFFs + index) instead of calling the removed public helper; its
output cell was regenerated from a real run.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 5, 2026
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: Privatize build_vrt; route to_geotiff's VRT path through it

Blockers

None.

Suggestions

None blocking.

Nits

  • _build_vrt's docstring opening now reads [internal-only], but the five per-parameter tier tags below it still say [advanced] (_writers/vrt.py:58-90). Minor internal inconsistency now that the function is no longer a public entry point; worth normalizing to [internal-only].

What looks good

  • The routing change is behavior-preserving. _build_vrt(vrt_path, tile_paths, relative=True, nodata=nodata) resolves to write_vrt(vrt_path, tile_paths, relative=True, crs_wkt=None, nodata=nodata), the same call the path made before. No deprecation warning fires (path positional, crs unset), and the local import sidesteps a circular dependency.
  • Public-surface assertions are flipped correctly, and the new test_build_vrt_is_not_public covers both directions: the public name is gone and _build_vrt is still importable.
  • The pre-existing local helper _build_vrt(tmp_path) in test_finalization.py would have collided with the imported _build_vrt after the rename; it was renamed to _make_one_source_vrt, and the imported call inside it was left intact.
  • The notebook's VRT output cell was regenerated from a real run rather than hand-written, so the printed tile sizes and round-trip result are accurate.

Checklist

  • No algorithm change (API-surface refactor)
  • Backends unaffected; VRT write still runs eager + dask
  • No NaN / edge-case concerns
  • Routing change verified behavior-preserving
  • No premature materialization or extra copies
  • Benchmark not applicable
  • README feature matrix updated
  • Docstrings present (one tier-tag nit above)

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

The one nit from the first pass is fixed in d04a252: the five per-parameter tier tags in _build_vrt's docstring now read [internal-only], matching the function summary. test_polish.py::TestC5WriteVrtKwargs still passes (the docstring keeps relative / crs_wkt / nodata).

No new findings. The change is docstring-only; no code paths moved.

@brendancol brendancol merged commit a3e5d77 into main Jun 5, 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.

Make build_vrt private; route to_geotiff's VRT path through it

1 participant