Repository navigation
Add a format version to the index file header - #36
Merged
zirui-song-18 merged 2 commits intoAug 27, 2026
Merged
Conversation
Serialized indexes carried a fourcc and a dimension, with no way to tell one payload layout from another. A layout change therefore had no mechanism behind it: an older binary reading a newer file would consume whatever its fields happened to align with. Add a uint32 version to the header, between the id and the dimension, and reject anything outside 1..format_version() before parsing a payload -- on the mapped path as well as the copying one, since borrowing arrays at the wrong offsets is worse than copying garbage. Versions are numbered per index type rather than per file: IndexIO::format_version() returns the type's own kFormatVersion, so revising one type's payload leaves the others alone, and an IDMapIndex delegate keeps its own. format_version() is pure rather than defaulted so a payload change cannot ship without one. The header is passed to read_index/mmap_index whole, as IndexHeader, rather than as a loose version: the payload readers are the only code that can act on a version, and threading it now keeps the first real layout change from having to re-plumb five signatures first. For mmap_index this replaces the int dimension parameter, so its arity is unchanged. DEVELOPER_GUIDE.md records the format and the bump procedure. Renames SESQ's write_header/read_header to write_quantizer_header/ read_quantizer_header, now that "header" alone is ambiguous. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
chishui
requested review from
model-collapse,
yuye-aws and
zirui-song-18
as code owners
August 27, 2026 07:45
zirui-song-18
left a comment
zirui-song-18
left a comment
Collaborator
There was a problem hiding this comment.
Excellent change.
Comment on lines
+402
to
+405
| To change a payload layout: | ||
|
|
||
| 1. Bump that type's `kFormatVersion`. | ||
| 2. Branch on `header.version` in the type's `read_index` **and** its `mmap_index`, keeping the older branch so existing files still load. |
Collaborator
There was a problem hiding this comment.
Great to have this documented. One accuracy fix on step 2: "branch on header.version in the type's read_index and its mmap_index" doesn't hold for two of the five IndexIO types. IDMapIndex has no mmap_index at all (it's not in mmap_index_payload's switch — it handles mmap by threading io_flags to the delegate's detail::read_index), and DiskSeismicIndex::read_index intentionally just throws (mmap-only), so its version branch would live solely in mmap_index. Maybe soften to: "branch on header.version wherever that type actually parses its payload — its read_index and/or mmap_index."
Collaborator
Author
There was a problem hiding this comment.
Verified both: mmap_index_payload's switch has no IDMP case and IDMapIndex has zero mmap_index references, and DiskSeismicIndex::read_index only throws. Took your wording, and added a line naming both exceptions so the reader knows which case they are in. 05f70df
|
|
||
| ### Index file format | ||
|
|
||
| Every serialized index starts with a fixed header (`nsparse::IndexHeader` in `nsparse/io/io.h`), written by `write_index` and consumed by `read_index`: |
Collaborator
There was a problem hiding this comment.
"written by write_index and consumed by read_index" reads as the type's members, but the header is actually written/read centrally by write_header/read_header in index_io.cpp — the member read_index just receives the already-parsed header. Worth clarifying so future readers look in the right place.
Collaborator
Author
There was a problem hiding this comment.
Agreed, and the ambiguity was mine — I meant the free nsparse::write_index, but the IndexIO members share the name. Now names write_header/read_header in index_io.cpp and says a type never reads its own header. Fixed the same ambiguity one paragraph down. 05f70df
Two inaccuracies in the new section, both from review: "written by write_index and consumed by read_index" reads as the IndexIO members of those names, but the header is written and parsed centrally by write_header/read_header in index_io.cpp; a type receives the already parsed header and never reads its own. Step 2 told the reader to branch on the version in "read_index and its mmap_index", which holds for neither DiskSeismicIndex (mmap-only, its read_index throws, so the branch lives only in mmap_index) nor IDMapIndex (no mmap_index at all -- it threads io_flags to its delegate). Point at wherever the type actually parses its payload instead, and say which types are the exceptions. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
chishui
pushed a commit
that referenced
this pull request
Sep 16, 2026
* Drop the numpy<2.0 runtime pin from the Python bindings 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> * docs: the index header has carried a format version since #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 #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> * Make the declared numpy floor real, and trim the comments 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> --------- Signed-off-by: Heng Qian <qianheng@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #35.
Option 2 (separate
versionfield), not fourcc bumps: the fourcc is whatread_indexdispatches on, so aliasing it duplicates every case inindex_io.cppand still leaves a version to thread to the payload reader.id (u32) | version (u32) | dimension (i32). No release tags yet, so v1 is baked in with no legacy-sniffing path.format_version()returns that type'skFormatVersion, so revising one payload leaves the others alone; anIDMapIndexdelegate carries its own. Pure virtual, so a change cannot ship unversioned.read_indexrejects versions outside1..format_version()before parsing any payload, and before the mmap path.read_index/mmap_indextakeIndexHeader, replacingint dimension.write_headertowrite_quantizer_header; documents the bump procedure inDEVELOPER_GUIDE.md.595 C++ (7 new) and 99 Python tests pass. Mutation-checked: stubbing the check fails 5 of the 7 new tests.