Drop the numpy<2.0 runtime pin from the Python bindings - #56
Conversation
The bindings' metadata had NumPy's compatibility rule inverted. requirements.txt asks for numpy>=2.0 headers at build time, but pyproject.toml pinned the runtime to numpy<2.0. NumPy's C API is backward compatible the other way round: a module compiled against 2.x headers loads on any runtime from its target level up, 1.x and 2.x alike, while one compiled against 1.x refuses to import under 2.x. The pin bit hardest on Python 3.13+, where numpy<2.0 has no wheel and pip built numpy 1.26 from source against an interpreter it never supported, corrupting arrays silently. The benchmark skill, the seismic_mmap demo and the CI job all carried workarounds for this (install numpy>=2.1 over the package; stay on Python 3.12). Fix the direction instead of working around it: - pyproject.toml: require numpy>=1.26 with no upper bound. 1.26 is the floor because loader.py imports numpy._core, which older 1.x lacks. - CMakeLists.txt: refuse to configure against numpy<2.0 headers, so the open-ended runtime requirement cannot be undermined by a 1.x build. - swignsparse.swig: set NPY_TARGET_VERSION to NPY_1_25_API_VERSION, the C-API level numpy 1.26 ships, so the runtime floor is explicit rather than whatever the numpy 2.x providing the headers defaults to. - conftest.py: drop the guard that rejected numpy>=2; numpy's own import_array() already fails loudly on a real mismatch. - CI: build against numpy 2.x on Python 3.12 and 3.13, then on the 3.12 leg downgrade to numpy<2.0 and run the tests again to cover the floor. - Docs: remove the workaround text from DEVELOPER_GUIDE.md, the benchmark skill and the seismic_mmap docstring. Verified locally on Amazon Linux 2023 / Python 3.12: built against numpy 2.5.1, python_tests pass on numpy 2.5.1 and on numpy 1.26.4 (140/140 each), and configuring against a numpy 1.26 environment fails with the new CMake error. Signed-off-by: Heng Qian <qianheng@amazon.com>
…project#36 The benchmark skill and DISK_SEISMIC_BENCH.md still said the serialized header has no version and that an old binary silently misreads a new file. IndexHeader has carried a per-type kFormatVersion since opensearch-project#36, and read_index rejects an unknown version. Restate the gotcha as what it now is: a layout change shipped without a version bump is still misread silently, so cached .dat files must be regenerated on layout changes. Signed-off-by: Heng Qian <qianheng@amazon.com>
chishui
left a comment
There was a problem hiding this comment.
loader.py:33—is_sve_supported()returns False on numpy>=2.0, so SVE dies on aarch64 for exactly the numpy this PR steers to. line 42's getauxval needs no distutils, use it?loader.py:56— legacy numpy<1.19 branch is dead, remove? (no need in this PR)- Comments are excessive
| - name: Run Python tests on numpy 1.x | ||
| if: matrix.python-version == '3.12' | ||
| run: | | ||
| python -m pip install "numpy<2.0" |
There was a problem hiding this comment.
numpy==1.26.4? <2.0 tests whatever pip resolves, not the floor pyproject declares.
There was a problem hiding this comment.
Changed to numpy==1.26.0, the exact floor pyproject declares.
| # --no-deps keeps the numpy installed above, so the first test run below | ||
| # uses the very numpy the extension was compiled against; the 1.x run then | ||
| # swaps it deliberately. | ||
| - name: Install the built package |
There was a problem hiding this comment.
FYI, with --no-deps everywhere, numpy>=1.26 is never actually resolved by pip in CI.
| description = "Python bindings for NSPARSE sparse vector search library" | ||
| requires-python = ">=3.8" | ||
| dependencies = ["numpy>=1.20.0,<2.0", "packaging"] | ||
| requires-python = ">=3.9" |
There was a problem hiding this comment.
why? not in the description's change list. And CI only runs 3.12/3.13, so the declared floor is untested.
There was a problem hiding this comment.
No numpy >= 1.26 has a release for Python 3.8, so increase the floor version to 3.9.
Do we need CI test for 3.9, since we it was untested for 3.8 before?
| @@ -1 +1,5 @@ | |||
| # Build-time requirement for the SWIG extension: compile against numpy 2.x | |||
There was a problem hiding this comment.
3 lines of comment on a 1-line file. same in swignsparse.swig, pyproject.toml and the CI step. these comments are huge, make them precise and concise.
| // (CMakeLists.txt enforces this), the module runs on any numpy from this C-API | ||
| // level up, 1.x and 2.x alike; numpy's import_array() rejects an older runtime | ||
| // with a clear error rather than misbehaving. 1.25 is the C-API level numpy | ||
| // 1.26 ships, matching the floor in pyproject.toml. Without this the floor |
There was a problem hiding this comment.
IIRC 1.25 API level is also what numpy 1.25 ships, so it would load on 1.25 too — the real 1.26 floor is numpy._core, not this. Reword?
| surface only. One harness guard runs before any test, because its failure mode | ||
| is silent: a shadowed import gives an incomplete module rather than an error. | ||
|
|
||
| No numpy guard is needed: the extension is built against numpy 2.x headers |
There was a problem hiding this comment.
remove — a docstring paragraph explaining why a check isn't there.
Review follow-ups: - loader.py: detect SVE through getauxval(AT_HWCAP) alone. The numpy.distutils path returned False on numpy >= 2, the very numpy this change steers to, so SVE silently stopped being selected on aarch64. - loader.py: fall back to numpy.core when numpy._core is absent. The _core shim only appeared in 1.26.1, so the declared floor of 1.26 admitted a release (1.26.0) on which the package failed to import. - CI: install the package without --no-deps so pip resolves pyproject's requirement for real, and run the 1.x leg on numpy==1.26.0, the exact floor, instead of whatever numpy<2.0 happens to resolve to. - Cut the comments down to what the code does: one or two lines in CMakeLists.txt and swignsparse.swig, none in pyproject.toml or the CI steps, no paragraphs explaining removed checks or absent flags in conftest.py, the demo docstring or the benchmark skill. State only what NPY_TARGET_VERSION pins (the C-API level) and leave the package floor to pyproject.toml. Signed-off-by: Heng Qian <qianheng@amazon.com>
872363a to
3e681ee
Compare
Description
The Python bindings' metadata had NumPy's compatibility rule inverted.
nsparse/python/requirements.txtasks fornumpy>=2.0headers at build time, butpyproject.tomlpinned the runtime tonumpy<2.0. NumPy's C API is backward compatible the other way round: a module compiled against 2.x headers loads on any runtime from its target level up (1.x and 2.x alike), while one compiled against 1.x refuses to import under 2.x.The pin bit hardest on Python 3.13+, where
numpy<2.0has no wheel and pip built numpy 1.26 from source against an interpreter it never supported, corrupting arrays silently. The benchmark skill,demos/seismic_mmap.pyand the CI job all carried workarounds for this (installnumpy>=2.1over the package; stay on Python 3.12). This PR fixes the direction instead of working around it.Changes
pyproject.toml: requirenumpy>=1.26with no upper bound. 1.26 is the last 1.x line and the oldest with a Python 3.12 wheel, so it is the oldest floor CI can actually exercise.requires-pythonmoves from 3.8 to 3.9 because no numpy >= 1.26 supports 3.8; keeping 3.8 would be a promise pip could never satisfy.loader.py: import__cpu_features__fromnumpy._corewith a fallback tonumpy.core. The_coreshim only appeared in 1.26.1, so without the fallback 1.26.0 (inside the declared floor) failed to import. Detect SVE throughgetauxval(AT_HWCAP)alone; thenumpy.distutilspath returned False on numpy >= 2, the very numpy this change steers to.CMakeLists.txt: refuse to configure against numpy < 2.0 headers, so a build cannot produce a package that claims numpy 2 support but cannot import under it.swignsparse.swig: setNPY_TARGET_VERSIONtoNPY_1_25_API_VERSIONso the C-API floor (the 1.25/1.26 level) is pinned rather than inherited from whichever numpy 2.x provides the headers. This is the C-API floor only; the package floor ispyproject.toml's.python_tests/conftest.py: drop the guard that rejected numpy >= 2; numpy's ownimport_array()already fails loudly on a real mismatch..github/workflows/CI.yml: build against numpy 2.x on Python 3.12 and 3.13; install the package without--no-depsso pip resolves the declared requirement; on the 3.12 leg, switch tonumpy==1.26.0(the exact floor) and run the tests again.DEVELOPER_GUIDE.md, the benchmark skill and theseismic_mmap.pydocstring.benchmarks/DISK_SEISMIC_BENCH.mdstill claimed the serialized header carries no format version; it has since Add a format version to the index file header #36. Restated the gotcha as what it now is (a layout change without akFormatVersionbump is still misread silently).Verification
Local, Amazon Linux 2023, Python 3.12, built against numpy 2.5.1:
pip install build/nsparse/python(no--no-deps) into an env with numpy 2.5.1 keeps 2.5.1;pytest python_tests: 140 passednumpy==1.26.0(nonumpy._core): import succeeds via the fallback; 140 passed-DPython_EXECUTABLEpointing at a numpy 1.26 environment fails with the new CMake errorNote for maintainers
The Python CI job now runs as a matrix, so its check names become
Build and Test nsparse Python bindings on Linux (3.12)and(3.13). If branch protection lists the old un-suffixed name as a required check, it will need updating.🤖 Generated with Claude Code