geotiff: extend backend parity matrix to 8 backends and 7 fixtures#2142
Conversation
…2132) Builds on the matrix scaffold from #1985. Adds gpu, dask+gpu, vrt-eager, vrt-dask, http-cog, and fsspec-memory backend rows, plus uint16 multiband tiled, float32 with nodata, int8 unmasked, COG, VRT mosaic, and MinIsWhite fixtures. Adds an error-fixture sub-matrix for the rotated ModelTransformationTag case. Backend / fixture compatibility lives on the backend descriptor's compat set, so incompatible (backend, source) pairs skip cleanly. HTTP and fsspec rows reuse the patterns already in test_cog_http_concurrent.py and test_features.py.
brendancol
left a comment
There was a problem hiding this comment.
PR Review: geotiff: extend backend parity matrix to 8 backends and 7 fixtures
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
Dead
labelvariable in the error test (xrspatial/geotiff/tests/test_backend_parity_matrix.py:1003-1011): the error matrix builds alabelstring and immediatelydels it. The pytest parametrize id already carries that info. Drop the assignment, drop thedel, drop the comment about tracebackhide-friendly messages. There is no assertion that useslabelin this block;pytest.raisesis doing the work. -
Two of the VRT cells are duplicates (
test_backend_parity_matrix.py:154-186):dask+numpyhas_SRC_LOCAL_VRTin its compat set, sodask+numpy-vrt-mosaicruns withkwargs={"chunks": 16}against the VRT path.vrt-daskruns with the same kwargs against the same path. Same story fornumpy-vrt-mosaicvsvrt-eager-vrt-mosaic(bothkwargs={}). These cells exercise the same code. The issue's plan namesvrt-eagerandvrt-daskas the dedicated VRT rows, so the cleanest fix is to trim_SRC_LOCAL_VRTout of thenumpyanddask+numpycompat sets and let the VRT-named backends own VRT.
Nits (optional improvements)
-
Type hints on
_deliver_via_http/_deliver_via_fsspec(test_backend_parity_matrix.py:606-632): both functions are annotated as taking_FixtureSpec, but the error matrix passes_ErrorFixtureSpecinstances. Duck typing onfix_idcovers it. Widen the annotation to_FixtureSpec | _ErrorFixtureSpecor drop the constraint. -
Session-scoped HTTP server keeps payloads forever (
test_backend_parity_matrix.py:583-603): thepayload_by_pathdict on the shared handler is appended to per cell and never trimmed. Memory cost is tiny (each fixture is a few KB), but a per-test clear would be tidier. Skip if it never shows up as overhead.
What looks good
- Backend and fixture descriptors are separated. Adding a new backend or fixture is a single row plus an optional builder, which is what the issue asked for.
- The per-backend
compatset is a good way to express (backend, source) incompatibility. Skip messages name the axis that cut the cell. - Pixel comparison is NaN-aware for float and byte-equal for int. dtype is checked against the spec rather than just the reference, so a uniform silent upcast across every backend still fails the cell.
masked_nodatais asserted on both theTrue(float32 with sentinel) andFalse(int8 withmask_nodata=False) sides. That locks both branches of the #2092 / #2127 contract.- The error sub-matrix mirrors the success matrix, so a new error fixture is one entry.
- 46 cells pass and 18 skip cleanly (incompatible pairs). Existing per-bug parity files stay green.
Checklist
- Algorithm matches the issue spec (#2132)
- All implemented backends produce consistent results
- NaN handling is correct (NaN-aware float comparison)
- Edge cases covered (nodata sentinel, multiband, COG, MinIsWhite, rotated error)
- N/A: no dask chunk-boundary logic in this PR (test harness only)
- No premature materialization
- N/A: no benchmark needed for a test harness
- N/A: README feature matrix unchanged (no new public function)
- Docstrings present and accurate
- Trim VRT compat from numpy / dask+numpy backends so vrt-eager and vrt-dask own the VRT row (drops two duplicate cells). - Drop the dead ``label`` variable from the error sub-matrix. - Widen the type hint on ``_deliver_via_http`` / ``_deliver_via_fsspec`` so both ``_FixtureSpec`` and ``_ErrorFixtureSpec`` callers match. - Clear the session HTTP server's payload dict between tests so the shared handler does not accumulate stale entries.
brendancol
left a comment
There was a problem hiding this comment.
Follow-up: review findings addressed
All four items from the previous review pass have been applied in df3b8eb:
- Dead
labelvariable: removed. The block now contains onlypytest.raisesand the inner read. - Duplicate VRT cells:
_SRC_LOCAL_VRTis no longer in the compat sets fornumpyanddask+numpy. The VRT fixture is now routed only throughvrt-eagerandvrt-dask, matching the issue's named-row layout. Cell count moves from 46 passed / 18 skipped to 44 passed / 20 skipped. - Type hints:
_deliver_via_httpand_deliver_via_fsspecare annotated as_FixtureSpec | _ErrorFixtureSpecso the error matrix call sites are covered by the signature. - HTTP payload accumulation: split the session-scoped server into
_matrix_http_server_session(owns the socket + thread) and a function-scoped_matrix_http_serverwrapper that clearspayload_by_pathbefore and after each test.
Local test run: 44 passed, 20 skipped on the matrix file; the per-bug regression markers (test_backend_pixel_parity_matrix_1813.py, test_attrs_parity_1548.py, test_backend_kwarg_parity_1561.py, test_miniswhite_backend_parity_1797.py) stay at 212 passed.
Closes #2132.
Summary
test_backend_parity_matrix.pyfrom a 2-backend / 1-fixture scaffold to the full 8-backend / 7-fixture matrix described in geotiff: add formal backend parity test matrix #2132.ModelTransformationTagwithoutallow_rotated) so the contract for raised exceptions has one home._BackendSpec.compatset, so incompatible cells skip cleanly without taking the matrix down.Backends covered
numpyopen_geotiff(path)dask+numpyopen_geotiff(path, chunks=16)gpuopen_geotiff(path, gpu=True)dask+gpuopen_geotiff(path, gpu=True, chunks=16)vrt-eageropen_geotiff(path.vrt)vrt-daskopen_geotiff(path.vrt, chunks=16)http-cogopen_geotiff('http://...')fsspec-memoryopen_geotiff('memory://...')GPU rows skip when cupy or CUDA are missing. fsspec rows skip when fsspec is absent. HTTP rows reuse the loopback
http.serverpattern fromtest_cog_http_concurrent.py.Fixtures covered
-9999.0nodata sentinel (locks the Bug: attrs['masked_nodata'] reports True when masking was disabled #2092masked_nodata=Truecontract)mask_nodata=False(locks the Bug: attrs['masked_nodata'] reports True when masking was disabled #2092 / geotiff: thread masked decision through _set_nodata_attrs (#2092) #2127masked_nodata=Falsecontract)ModelTransformationTagwithoutallow_rotatedOut of scope
test_writer_matrix.py).memory://row already covers the same code path for the matrix, and the existingtest_bytesio_source.pykeeps the explicit BytesIO behaviour pinned.Test plan
pytest xrspatial/geotiff/tests/test_backend_parity_matrix.py -vreports 46 passed and 18 skipped (incompatible cells).test_backend_pixel_parity_matrix_1813.py,test_attrs_parity_1548.py,test_backend_kwarg_parity_1561.py,test_miniswhite_backend_parity_1797.py) stay green.ruffandflake8clean on the changed file.