Declare dependency version bounds and test them in CI - #3702
Conversation
install_requires listed six bare package names with no bounds. numba caps numpy in its own metadata and re-checks it at import, so a bare numpy let pip install a version where import xrspatial raises ImportError. - add lower bounds derived from the first CPython 3.12 wheel of each compiled dependency, plus a numpy ceiling mirroring numba - raise the xarray floor to 2025.11: earlier releases drop attrs across DataArray binary ops, which breaks morph_gradient, the tophats, and the mcda and corridor metadata tests - declare pandas, which three modules import at top level and which zonal_stats returns - move zstandard to the geotiff and tests extras, matching the guarded import in geotiff/_compression.py - add a min-deps job to test.yml that pins every floor and runs the suite
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Declare dependency version bounds and test them in CI
No compute code changed, so most of the usual correctness checks (algorithm, NaN semantics, dask chunk boundaries, CUDA bounds) do not apply. This is a packaging and CI review.
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
xrspatial/tests/test_dependency_metadata_3701.py:11importspackaging.requirements, butpackagingis not in thetestsextra. It arrives only because xarray declarespackaging>=24.1. That is the exact pattern this PR is fixing for pandas, so the test that enforces the rule should not break it. Addpackagingto thetestsextra insetup.cfg:111. -
setup.cfg:35setsurllib3 >=2.7, which is the tightest bound in the set and the only one motivated by advisories rather than by an API the code needs. 2.7.0 is what pip resolves today so it costs nothing on PyPI, but conda-forge and distro packagers lag, and a user on urllib3 2.6.x will now get a resolver error rather than a working install with a stale HTTP stack. That may still be the right call given_sources.py:1507streams under a byte cap; it is worth a maintainer's explicit sign-off rather than sliding in with the wheel-availability floors. -
Dropping
zstandardfrominstall_requires(setup.cfg:98) is user-visible: anyone on a plainpip install xarray-spatialwho reads a ZSTD-compressed TIFF goes from working toImportError, and the error only appears at decode time, not at import. The message is actionable and the extras convention is right, but this belongs in the release notes at tag time.
Nits (optional improvements)
-
.github/workflows/test.yml:75givesmin-depsnoif:guard, so it runs identically on pull_request, push-to-main, the nightly cron, andworkflow_dispatch, always on the fast lane. Therunjob distinguishes those (fast lane on PRs, full suite otherwise). Keeping min-deps cheap everywhere is defensible; just note it is a deliberate divergence rather than an oversight. -
setup.cfg:32pinsnumpy <2.5to mirror numba, andtest_numpy_has_an_upper_boundonly checks that some ceiling exists. When numba widens to<2.6there is nothing that surfaces the stale cap, so xarray-spatial silently blocks a numpy it would otherwise support. Not worth building tooling for, but the comment atsetup.cfg:26should probably say it needs a look on every numba bump. -
test_dependency_metadata_3701.py:22re-reads and re-parsessetup.cfgon each helper call (five times per run). Harmless at this size.
What looks good
-
The floors are derived rather than rounded. First-cp312-wheel for the compiled packages is a rule that regenerates the same answer next year, and the xarray bound comes from an actual bisect rather than from picking a recent-looking date.
-
The xarray finding is the substantive one.
morph_white_tophatisagg - opened, so on any xarray before 2025.11 the output silently losesresandcrs. That is a real metadata bug reachable through today's declared metadata, and the seven failing tests are the evidence. -
test_numpy_has_an_upper_boundfails against the pre-change barenumpydeclaration, so the guard is not vacuous. Readingsetup.cfgfrom the source tree instead ofimportlib.metadataavoids reporting on a stale editable install, and the skip whensetup.cfgis absent keeps it from failing inside a wheel. -
Adding
zstandardto thetestsextra alongside thegeotiffmove is the part that is easy to forget; without it the ZSTD codec tests would have skipped silently and nobody would have noticed for months.
Checklist
- Algorithm matches reference/paper (n/a, no compute code)
- All implemented backends produce consistent results (n/a; numpy and dask+numpy exercised by the floor run)
- NaN handling is correct (n/a)
- Edge cases are covered by tests (metadata guards cover the pre-change declaration)
- Dask chunk boundaries handled correctly (n/a)
- No premature materialization or unnecessary copies (n/a)
- Benchmark exists or is not needed (not needed)
- README feature matrix updated (not applicable, no new functions)
- Docstrings present and accurate (test module documents why it reads setup.cfg rather than installed metadata)
- declare packaging in the tests extra; the metadata guards import it and were leaning on xarray to supply it, which is the pattern this PR exists to remove - note in setup.cfg that the numpy ceiling needs re-checking on every numba bump, since nothing flags a stale cap - say why min-deps stays on the fast lane for every trigger while `run` does not - cache the parsed setup.cfg instead of re-reading it per helper call
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Declare dependency version bounds and test them in CI (follow-up pass)
Second pass over 62e3300b, which addresses the first review.
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None outstanding. Disposition of the first pass:
packagingmissing from thetestsextra: fixed atsetup.cfg:120, with a comment saying why a test file should not lean on a transitive dependency.urllib3 >=2.7wanting a maintainer decision: not changed. The advisory class (GHSA-2xpw-w6gg-jr37 and friends, streaming decompression) lands on the byte-capped streaming reads at_sources.py:1507, and 2.7.0 is what pip resolves today, so the bound is free on PyPI. Surfaced explicitly in the PR body under "For the maintainer" rather than decided unilaterally.- ZSTD leaving the default install: not changed, and cannot be, since
CHANGELOG.mdis off limits on a feature branch. Written into the PR body so it reaches the release notes at tag time.
Nits (optional improvements)
-
xrspatial/tests/test_dependency_metadata_3701.py:15caches the parsed config withlru_cache, so a test that editssetup.cfgmid-session would read stale values. Nothing does that today and these tests are read-only, so this is only worth remembering if someone later adds a test that writes the file.
What looks good
-
The
packagingfix is the right shape. A guard against undeclared transitive imports that itself relied on an undeclared transitive import would have been an easy thing to shrug at. -
setup.cfg:29-31now says the numpy ceiling needs re-checking on every numba bump. That is the failure mode a mirrored bound actually has: it goes stale quietly and blocks a numpy that works fine. -
The min-deps comment at
.github/workflows/test.yml:78-80explains the fast-lane-everywhere choice instead of leaving it looking like a copy-paste omission.
Checklist
- Algorithm matches reference/paper (n/a, no compute code)
- All implemented backends produce consistent results (n/a; numpy and dask+numpy exercised by the floor run)
- NaN handling is correct (n/a)
- Edge cases are covered by tests
- Dask chunk boundaries handled correctly (n/a)
- No premature materialization or unnecessary copies (n/a)
- Benchmark exists or is not needed (not needed)
- README feature matrix updated (not applicable)
- Docstrings present and accurate
# Conflicts: # .claude/sweep-dependencies-state.csv # .github/workflows/test.yml
|
CI note: everything in the The one red check is Both install steps exit 0 under the new bounds, so the metadata resolves fine on RTD. The sphinx step has no exit code and no captured output, and the build length is 902 seconds against RTD's 900-second cap, so it was killed on the time limit. For comparison, #3700's builds came in at 595s and 569s, already close to the ceiling; this branch merges #3698 and #3700 on top, and the merged tree tips over. The diff here is |
Closes #3701.
install_requireslisted six bare package names with no version bound, and nothing in CI ever installed an old one. This declares bounds, adds the dependency that was missing, moves out the one that was never required, and adds a job that installs the floors so the numbers stay honest.What changed
install_requires = numba >=0.59 scipy >=1.11.2 xarray >=2025.11 numpy >=1.26,<2.5 pandas >=2.2 urllib3 >=2.7zstandard >=0.22moves to thegeotiffextra (besidedeflateandpyproj) and totests, so the ZSTD codec tests keep running in CI instead of silently skipping.Where each bound comes from
numba >=0.59, scipy >=1.11.2, numpy >=1.26 are the first releases of each that ship a CPython 3.12 wheel, and
python_requires = >=3.12already rules out anything older. Checked against the PyPI JSON API rather than guessed:numpy <2.5 mirrors numba. Every current numba declares a numpy ceiling in its own metadata, and re-checks it at import in
numba._ensure_critical_deps:numpy<2.5,>=1.22numpy<2.5,>=1.22numpy<2.5,>=1.22numpy<2.4,>=1.22numpy 2.5.1 is on PyPI now and no numba supports it, so the old bare
numpyclaimed support for a version where the package cannot import:xarray >=2025.11 is the one bound that is not a wheel-availability artifact, and it is the interesting one. Running the suite in a Python 3.12 virtualenv pinned to the older candidate floors produced 7 failures, all the same assertion (
output_agg.attrs == input_agg.attrs):Those functions build their result with a DataArray binary op (
morph_white_tophatisagg - opened), and older xarray drops attrs across those, soresandcrsnever reach the output. Bisecting with everything else held fixed:Ruled out a co-installed package:
xarray==2025.10.0 + pandas==3.0.5still fails,xarray==2025.12.0 + pandas==2.2.3passes.pandas >=2.2 follows from that, since xarray 2025.11.0 declares
pandas>=2.2. pandas needed declaring regardless:xrspatial/zonal.py:26,xrspatial/dasymetric.py:35andxrspatial/focal.py:11import it unguarded at module scope, andzonal_statsandcrosstabare annotated-> pd.DataFrame. It was resolving purely because xarray drags it in.urllib3 >=2.7 is the first release OSV reports with no open advisory against the streaming read path. Everything earlier carries at least one of GHSA-2xpw-w6gg-jr37, GHSA-gm62-xv2j-4w53 or GHSA-38jv-5279-wg99, all decompression handling in the streaming API, and
xrspatial/geotiff/_sources.py:1507streams responses withpreload_content=Falseunder a byte cap. The redirect-related urllib3 CVEs do not apply here:_sources.py:445builds the pool withredirect=Falseand validates eachLocationby hand.zstandard was never required.
xrspatial/geotiff/_compression.py:1402guards the import behindZSTD_AVAILABLE, andzstd_compress/zstd_decompressraise a pip-install hint when it is missing. Blocking it withsys.modules['zstandard'] = Noneleavesimport xrspatialworking, unlike the other five:Verification
Fast lane in a Python 3.12 virtualenv pinned to exactly the declared floors (
numba==0.59.0 scipy==1.11.2 xarray==2025.11.0 numpy==1.26.0 pandas==2.2.0 urllib3==2.7.0 zstandard==0.22.0):The new
min-depsjob in.github/workflows/test.ymlruns that same install and suite on every PR, so a floor that stops being true shows up as a red check rather than as a user bug report.xrspatial/tests/test_dependency_metadata_3701.pyreadssetup.cfgfrom the source checkout (skipped when absent, e.g. from an installed wheel) and asserts pandas is declared, zstandard is not, every requirement carries a lower bound, numpy carries an upper bound, and the installed numpy satisfies both setup.cfg and numba. The upper-bound test fails against the old barenumpydeclaration.Backend coverage: metadata and CI only, no compute paths touched. The floor run above exercised numpy and dask+numpy; the cupy paths skip in that virtualenv and are unaffected by the change.
Test plan
pytest -m "not slow"at the declared floors on Python 3.12: 10797 passed, 0 failedpytest xrspatial/tests/test_dependency_metadata_3701.pyon both Python 3.12 at the floors and Python 3.14 at current versionssetup.cfgparses viasetuptools.config.setupcfg.read_configuration.github/workflows/test.ymlparses as YAML with both jobs presentFor the maintainer
Two calls in here want an explicit yes rather than a silent merge:
zstandardleaves the default install. A plainpip install xarray-spatialno longer reads ZSTD-compressed TIFFs; the failure surfaces at decode time asImportError: zstandard is required to read ZSTD-compressed TIFFs. Install it with: pip install zstandard, not at import.pip install 'xarray-spatial[geotiff]'restores it. This needs a line in the release notes at tag time (not touched here, since parallel branches all editing CHANGELOG.md conflict).urllib3 >=2.7is the tightest bound in the set and the only one driven by advisories rather than by an API the code calls. It is free on PyPI (2.7.0 is what pip resolves today), but conda-forge and distro packagers lag, so anyone on 2.6.x gets a resolver error instead of a working install with an older HTTP stack. Happy to relax it to a functionality-only floor if you would rather carry the advisories.