Skip to content

Remove stale external-overview skips on dask/fsspec/GPU golden-corpus backends#3026

Merged
brendancol merged 3 commits into
mainfrom
issue-3024
Jun 7, 2026
Merged

Remove stale external-overview skips on dask/fsspec/GPU golden-corpus backends#3026
brendancol merged 3 commits into
mainfrom
issue-3024

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #3024

What

The golden-corpus dask, fsspec, GPU, and dask+GPU parity modules listed overview_external_ovr_uint16 in _OVERVIEW_READER_GAPS, which switched off the overview-level oracle comparison and ran only the base-IFD check. The reader has external .ovr sidecar discovery (_sidecar.py) shared across backends, so these gates were stale.

  • Removed the overview_external_ovr_uint16 entry from _OVERVIEW_READER_GAPS in all four modules. The overview-level comparison now runs and passes.
  • For fsspec, also pushed the sibling .tif.ovr into the memory:// filesystem so the reader's fsspec sidecar discovery (_probe_fsspec -> load_sidecar) can resolve the external overview levels. Without it the memory filesystem held only the base .tif and overview_level>=1 failed as out of range. That was a test-harness gap, not a reader limitation.

Backends

Test-only change. All four backends (dask+numpy, fsspec, GPU/cupy, dask+GPU) run and pass the overview comparison in the dev environment used here, which has a working CUDA device. Eager numpy already had its gap cleared.

Test plan

  • test_dask_numpy_parity[overview_external_ovr_uint16] passes
  • test_fsspec_parity[overview_external_ovr_uint16] passes
  • test_gpu_parity[overview_external_ovr_uint16] passes
  • test_dask_gpu_parity[overview_external_ovr_uint16] passes
  • Full golden-corpus suite for the four modules: 127 passed, 9 skipped
  • test_taxonomy_ids_are_in_manifest passes in each module

The golden-corpus dask, fsspec, GPU, and dask+GPU modules listed
overview_external_ovr_uint16 in _OVERVIEW_READER_GAPS, which skipped
the overview-level oracle comparison. The reader gained external .ovr
sidecar discovery (_sidecar.py) shared across backends, so these gates
were stale.

Drop the entry from all four modules. For fsspec, also push the sibling
.tif.ovr into the memory filesystem so the reader's fsspec sidecar
discovery can resolve the external overview levels; without it the
memory filesystem held only the base .tif and the test failed as a
harness gap, not a reader limitation.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 7, 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: Remove stale external-overview skips on dask/fsspec/GPU golden-corpus backends

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • test_fsspec.py: the sidecar is served inside test_fsspec_parity via the memory_fs_clean fixture handle, while the base .tif goes through the module-level _serve_via_memory helper. Both write to the same process-global memory filesystem so it works, but a reader scanning the test sees two write paths for related files. Folding the sidecar write into _serve_via_memory (or a small sibling helper) keeps the memory-FS setup in one place. Cosmetic; the current form is correct.

What looks good

  • The four skip removals are verified rather than assumed. Each backend's overview-level comparison was run and passes, and the taxonomy guard tests still pass because the entries were removed cleanly, not left dangling.
  • The fsspec change is the substantive one and is right. That skip was masking a test-harness gap (only the base .tif was in the memory filesystem), not a reader limitation. Serving the sibling .tif.ovr lets the real fsspec sidecar discovery path (_probe_fsspec -> load_sidecar) resolve the external overview levels, so the test exercises production code instead of routing around it.
  • The sidecar path construction (path.name + .ovr) matches find_sidecar's source + ".ovr" convention, so the served URL lines up with what the reader probes.
  • Comments left in place of the removed entries explain why the gap is gone, matching the eager numpy module's existing note.

Checklist

  • Algorithm matches reference/paper (n/a, test-only)
  • All implemented backends produce consistent results (all four verified passing)
  • NaN handling is correct (n/a)
  • Edge cases covered by tests (this PR adds the previously-skipped overview coverage)
  • Dask chunk boundaries handled correctly (n/a, no production change)
  • No premature materialization or unnecessary copies (n/a)
  • Benchmark exists or is not needed (not needed, test-only)
  • README feature matrix updated (n/a, no new function)
  • Docstrings present and accurate (skip-list comments updated)

#3024)

Keep all memory-FS writes in one helper instead of splitting the base
.tif and sibling .tif.ovr across the helper and the test body.
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 (after edcf7bb)

The single nit from the first pass is resolved. The fsspec sidecar write now lives in _serve_via_memory alongside the base .tif write, so all memory-filesystem setup is in one helper and the test body no longer carries a parallel write path. Full test_fsspec.py module passes (32 passed, 2 skipped).

No new findings. Nothing outstanding.

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

Stale golden-corpus skips hide external-overview validation on dask/fsspec/GPU backends

1 participant