Rework save system to use ZarrV3#260
Open
JoeyBF wants to merge 13 commits into
Open
Conversation
Rebases the zarrs-save-rework branch (6 save-system commits) onto master. Master had since split ext::secondary into per-primary submodules (SpectralSequences#254) and made the secondary lift entry points return Result (SpectralSequences#241, SpectralSequences#242), so the conflicts were confined to ext/src/secondary.rs. Resolution: - Adopt the store-based save API (store.read/write/exists/delete) in the shared SecondaryHomotopy / SecondaryLift code while preserving master's Result-returning try_compute_homotopy_step. - Drop the stale monolithic duplicates of SecondaryResolution, SecondaryResolutionHomomorphism, and SecondaryChainHomotopy; those types now live in submodules under resolution.rs / chain_homotopy.rs / resolution_homomorphism.rs. - Remove the per-new() SaveKind::secondary_data() create_dir loops from the moved constructors: the reworked store creates its shard arrays lazily on first write, and create_dir no longer exists on SaveKind. Dropped from the original branch: the throwaway "experimental rayon build" commit (patched rayon to a personal fork) and the earlier "tracing to parallel guard" commit (master ships a superior version). All 7 save_load_resolution tests and the ext lib unit tests pass.
Two hot-path improvements to ZarrSaveStore that remove overhead the rebase inherited, with no change to the on-disk format or public API. 1. Cache opened Array handles (`arrays: DashMap<SaveKind, Arc<ShardArray>>`). Previously every read/write/delete called `zarrs::array::Array::open`, which re-reads and re-parses the array's `zarr.json` through the storage backend on each call. The handle holds no chunk data and its methods take `&self`, so a cached `Arc<Array>` is safe to share for concurrent reads and (shard-serialized) writes. Now the metadata is parsed at most once per kind per store. Read/delete of a not-yet-created kind still returns None/Ok as before. 2. Key the write lock by `(kind, shard coords)` instead of by kind alone. The zarrs read-modify-write contract only requires serializing writes that share a shard; the old per-kind lock needlessly serialized every write of a kind across the whole store. Writes to different shards touch disjoint chunk files, so per-shard locking restores the cross-shard parallelism a parallel resolution depends on while still honouring the contract. Kept `with_concurrent_target(1)` so the sharding codec stays sequential and the rayon-join deadlock the original guarded against cannot occur. Verified on S_2 through-stem (n=70, s=40), release + concurrent, 4 cores: best-of-3 wall time 15.1s -> 13.0s (~14% faster) with identical Ext output. Save+resume roundtrip re-checked for both the standard and nassau backends; all lib and save_load_resolution tests pass.
…_LEVEL The rebased branch hardcoded zstd level 19 for the stream tier (res_qi, nassau_qi), which is on the hot write path — every differential quasi-inverse is compressed there. Benchmarked against master's uncompressed old format (S_2 through-stem n=70 s=40, release + concurrent, 4 cores, best of 3): master (old, uncompressed): 2.55s write, 51M on disk (real bytes) zarr, zstd level 19: 13.0s write, 23M zarr, zstd level 3: 3.65s write, 24M Level 19 was ~5x slower than master for a 4% smaller store than level 3 on this dense, high-entropy data. Level 3 (zstd's own default) keeps nearly all the compression — still less than half of master's on-disk size — at a fraction of the write cost, cutting the regression from ~5x to ~1.4x. The read/resume path is unchanged and already at parity with master (~0.10s). Compression still matters for very large runs, so the level is read once from the EXT_SAVE_ZSTD_LEVEL environment variable (clamped to zstd's [1, 22]; unparseable values warn and fall back to the default). Set e.g. EXT_SAVE_ZSTD_LEVEL=19 when the on-disk footprint, not save time, is the binding constraint.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates persistence to a Zarr-backed store, updates save-directory construction and binding, rewires resolution/homotopy/secondary/Nassau load-save paths to store APIs, and revises tests and dependencies for the new storage format. ChangesZarr save store migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MuResolution
participant ZarrSaveStore
participant Bitcode
MuResolution->>ZarrSaveStore: read(SaveKind::Differential, b)
ZarrSaveStore-->>MuResolution: bytes
MuResolution->>Bitcode: deserialize DifferentialPayload
MuResolution->>ZarrSaveStore: write(SaveKind::Kernel, ...)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A line beginning with "+ serde_json parse" was parsed as a markdown "+" list marker, so clippy (-D warnings in CI's lint job) flagged the following lines as list items without indentation. Reword to "and a serde_json parse". No code change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/src/chain_complex/chain_homotopy.rs`:
- Around line 52-59: The subgroup path built in chain_homotopy::save_dir
currently interpolates raw values from left.name() and right.name(), which can
alter the Store layout or collide with other pairs. Update the path construction
in the homotopy save logic to encode/sanitize both names before passing them
into parent.subgroup, and keep the change localized around the save_dir creation
so the homotopy lookup and persistence use the same safe naming scheme.
- Around line 181-187: The cached homotopy payload handling in `ChainHomotopy`
is unsafe because `save_dir.store()` reads raw bytes without validating the
encoded shape. Update the `read` path to store and verify `num_gens` and
`target_dim` alongside the payload, then in the decode block reject/recompute on
any mismatch before constructing `outputs`. Also replace the `unwrap()`-based
decoding in this `SaveKind::ChainHomotopy` branch with explicit error handling
and ensure the cursor has no trailing bytes after `FpVector::from_bytes`
completes.
In `@ext/src/resolution_homomorphism.rs`:
- Around line 237-257: The chain-map cache serialization in the load/save path
is missing the saved row width, so `FpVector::to_bytes` data is being read back
with the current `fx_dimension` instead of the original one. Update the
`save_dir.store()` logic in the chain-map handling to persist a small header
with the saved `fx_dimension` before the row bytes, and in the corresponding
read branch deserialize that header first and use it to parse each `FpVector`;
if the stored width differs from the current target, skip or invalidate the
cached entry. Apply the same header format in both write locations around the
`SaveKind::ChainMap` calls so `add_generators_from_rows_ooo` always sees
correctly framed rows.
In `@ext/src/resolution.rs`:
- Around line 140-142: The cache binding in resolution setup only uses
bind_to_algebra, so a store can be reused across different ChainComplex
instances with the same algebra metadata. Update the save_dir.store() path to
bind and validate against the resolved complex as well, using a
ChainComplex/resolution fingerprint (or equivalent per-complex subgroup key)
before any reads or writes, and keep the existing algebra metadata check in
place.
- Around line 413-426: The differential shard handling in
resolution::add_generators_from_rows currently only checks target_cc_dimension
after deserializing DifferentialPayload, so stale or corrupt data can still be
applied. In the logic that reads DifferentialPayload, add validation for the
full payload before mutating state: verify target_res_dimension matches the
expected resolution dimension, and ensure the differential and chain_map row
vectors are consistent with num_gens and the block size before calling
self.add_generators, current_differential.add_generators_from_rows, and
current_chain_map.add_generators_from_rows. Use the existing DifferentialPayload
and add_generators_from_rows flow to locate the checks.
In `@ext/src/save.rs`:
- Around line 377-386: The subgroup path construction in subgroup() is using raw
name components, which can allow "/" or ".." to alter the intended Zarr
hierarchy. Update subgroup() to encode or strictly validate the supplied name
before building group and group_path, and make sure any downstream use in
ensure_intermediate_groups() and GroupBuilder::new().build() operates on the
sanitized component rather than the raw input.
- Around line 1228-1232: Reject malformed signature payloads before decoding in
the signature parsing path. In the code that builds sig from payload using
chunks_exact(2), add an explicit payload length validation for even length and
return an error if payload.len() % 2 != 0, so a corrupt Signature command cannot
be partially decoded. Keep the fix local to the parsing logic in save.rs around
the signature payload handling.
- Around line 1047-1048: The ResQi row handling in `apply()` and
`into_quasi_inverse()` is still using `assert!` on the result of `next_row`,
which can panic on truncated or partially written data. Replace the panic path
with `anyhow::ensure!` (or equivalent fallible error propagation) at the `got`
check so these APIs return a proper error through the existing bidegree/load
flow instead of aborting. Keep the change localized to the `next_row`/pivot-row
validation logic in the affected ResQi reader paths.
- Around line 1401-1405: The SaveDirectory conversion currently panics in the
From<Option<PathBuf>> implementation by calling
ZarrSaveStore::create(...).expect(...), which prevents new_with_save and related
anyhow::Result flows from reporting store creation failures. Replace this with a
fallible conversion path such as TryFrom<Option<PathBuf>> or a dedicated
constructor, and update the SaveDirectory / new_with_save call site to propagate
the ZarrSaveStore::create error instead of unwrapping it.
- Around line 574-578: The stream-kind guard currently uses debug_assert! in the
write/read/delete paths, so misuse of SaveKind::ResQi or SaveKind::NassauQi is
only caught in debug builds. Replace these with runtime validation in the
affected save methods (including write, read, delete, and the exists probe) so
fallible operations return an error instead of silently targeting the wrong
shard-tier layout. Use the existing SaveKind checks in the relevant methods to
reject stream kinds explicitly and keep exists() from probing the shard layout
when given a stream kind.
- Around line 337-356: The store validation in the save/load path only checks
algebra_magic before reusing cached data, so it can accept metadata created for
a different prime. Update the logic in the algebra_magic handling block to also
compare the stored prime from attrs against the current prime before returning
Ok(()), and include the prime mismatch in the same bailout path used for the
existing algebra mismatch; use the existing symbols algebra_magic,
algebra_prefix, prime, and the attrs lookups in ext/src/save.rs to locate the
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3bd243b-fb2c-425d-a092-fc42f1917e6c
📒 Files selected for processing (11)
ext/Cargo.tomlext/crates/fp/src/matrix/quasi_inverse.rsext/crates/fp/src/vector/fp_wrapper/mod.rsext/src/chain_complex/chain_homotopy.rsext/src/chain_complex/mod.rsext/src/nassau.rsext/src/resolution.rsext/src/resolution_homomorphism.rsext/src/save.rsext/src/secondary.rsext/tests/save_load_resolution.rs
💤 Files with no reviewable changes (1)
- ext/src/chain_complex/mod.rs
Address the low-risk, verified subset of CodeRabbit's review on SpectralSequences#260: - ResQiReader::apply / into_quasi_inverse: replace assert!(got, ...) with anyhow::ensure!(...). Both are already anyhow::Result-returning, so a truncated stream now propagates an error instead of aborting the caller. - NassauQiReader::parse: reject odd-length Signature payloads before chunks_exact(2) silently drops a trailing byte. - Stream-kind guards in write/read/exists/delete: promote debug_assert! to assert! so misusing a stream kind (ResQi/NassauQi) on the shard-tier API is caught in release too, not just debug. Deliberately not applied from the same review: - "Also assert target_res_dimension in the differential load path" — this is a false positive: test_load_smaller legitimately resumes into a smaller target range, so the read-time resolution dimension differs from the saved one (the rows carry their own widths). The original code asserts only target_cc_dimension for exactly this reason. - Prime check in bind_to_algebra — redundant: magic() already encodes the prime (p << 16), so a prime mismatch already fails the existing magic check.
…From) Addresses the remaining actionable review finding on SpectralSequences#260. Store creation can fail (bad path, permissions, unreadable existing metadata), but the old From<Option<PathBuf>> swallowed that with .expect(), so new_with_save's anyhow::Result could never surface it. Replace the impl with TryFrom<Option<PathBuf>> (Error = anyhow::Error) and thread the fallible conversion through the entry points that take a save dir: Resolution / nassau::Resolution::new_with_save and utils::construct{,_nassau, _standard} now take `impl TryInto<SaveDirectory, Error = anyhow::Error>` and `?`-propagate. Every existing call site passes Option<PathBuf>, so this is source-compatible; a bad save path now returns an Err instead of panicking. Not applied from the same review (left for the author's call): name sanitization for subgroup paths, dimension-framing headers for chain-map/homotopy payloads, and per-complex store fingerprinting.
CI's lint job runs clippy on nightly, whose chunks_exact_to_as_chunks lint (not yet in the stable clippy I checked locally) rejects chunks_exact(2) with a constant size. Switch the NassauQi signature decode to as_chunks::<2>(); the even-length check just above guarantees an empty remainder, so this is equivalent. as_chunks is stable since 1.88, well under the toolchains CI tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/src/nassau.rs (1)
1127-1131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard against stored QI images wider than the current input.
Line 1129 only handles
inputbeing longer thanscratch1. If a stored QI was written with a largertarget_dim, this panics inside the fallible replay path instead of returning an error.Fail before slicing past the input
- input - .slice_mut(0, scratch1.len()) - .add(scratch1.as_slice(), 1); + anyhow::ensure!( + scratch1.len() <= input.len(), + "Stored Nassau QI image dimension {} exceeds current input dimension {} at {b}", + scratch1.len(), + input.len(), + ); + input + .slice_mut(0, scratch1.len()) + .add(scratch1.as_slice(), 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/src/nassau.rs` around lines 1127 - 1131, Guard the replay path in the resolve_through_stem logic so it fails gracefully when a stored QI image is wider than the current input. In the code that slices and adds `scratch1` into `input`, check both length directions before calling `slice_mut`/`add`, and return an error if `scratch1.len()` exceeds `input.len()` instead of panicking. Update the fallback handling around `resolve_through_stem` to use the existing fallible error path in `nassau.rs` rather than assuming `input` is always at least as wide as `scratch1`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/src/utils.rs`:
- Line 143: The save_dir parameter bounds in the affected constructors are too
strict and reject callers that already have a SaveDirectory because the
reflexive TryInto implementation uses Infallible rather than anyhow::Error.
Update the save_dir generic constraint in the relevant utility constructors in
ext/src/utils.rs and the matching new_with_save signatures in ext/src/nassau.rs
and ext/src/resolution.rs so they accept any TryInto<SaveDirectory> without
forcing Error = anyhow::Error, keeping the existing API behavior while allowing
existing SaveDirectory values.
---
Outside diff comments:
In `@ext/src/nassau.rs`:
- Around line 1127-1131: Guard the replay path in the resolve_through_stem logic
so it fails gracefully when a stored QI image is wider than the current input.
In the code that slices and adds `scratch1` into `input`, check both length
directions before calling `slice_mut`/`add`, and return an error if
`scratch1.len()` exceeds `input.len()` instead of panicking. Update the fallback
handling around `resolve_through_stem` to use the existing fallible error path
in `nassau.rs` rather than assuming `input` is always at least as wide as
`scratch1`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a7ae8d6-31e1-46b9-8330-1d030eb7f438
📒 Files selected for processing (4)
ext/src/nassau.rsext/src/resolution.rsext/src/save.rsext/src/utils.rs
Follow-up to the TryFrom change: the bound `TryInto<SaveDirectory, Error = anyhow::Error>` was stricter than the original `Into<SaveDirectory>` because the reflexive `TryInto<SaveDirectory>` for `SaveDirectory` has `Error = Infallible`, so a caller passing an already-built `SaveDirectory` was rejected. Widen the bound to `Error: Into<anyhow::Error>` (accepts both `Option<PathBuf>` with `anyhow::Error` and `SaveDirectory` with `Infallible`) and convert via `.map_err(Into::into)`, restoring the original API surface while keeping the fallible path for store creation.
hoodmane
reviewed
Jul 8, 2026
hoodmane
reviewed
Jul 8, 2026
- chain_homotopy.rs: restore the original ChainHomotopy field order (homotopies before save_dir). The rework had swapped them for no functional reason (field order here doesn't affect drop semantics or the constructor); revert to minimize churn. - save.rs: the wasm rationale said the filesystem store "doesn't compile for WASM", but that's specific to wasm32-unknown-unknown — wasm32-unknown-emscripten is `unix` and unaffected. Reword the module doc and the parallel inline comment to name the target precisely.
hoodmane
reviewed
Jul 8, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/src/resolution.rs (1)
865-871: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate store errors from
existsIfreadfails, this returnsfalseand the caller treats the differential as missing. Return aResult<bool>here so transient store errors don’t trigger redundant recomputation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/src/resolution.rs` around lines 865 - 871, The differential cache check in `Resolution::has_computed_bidegree` is swallowing store failures by treating `save_dir.store().exists(...)` as a plain boolean, so transient `read` errors look like “missing data.” Update this check to propagate storage failures instead of collapsing them into `false`, likely by making the `exists` path return a `Result<bool>` and threading that through the caller in `resolution.rs`. Keep the logic around `check_b`, `has_computed_bidegree`, and the `save_dir.store()` access intact, but ensure any error from `exists` is surfaced so the caller can distinguish “not present” from “store unavailable.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ext/src/resolution.rs`:
- Around line 865-871: The differential cache check in
`Resolution::has_computed_bidegree` is swallowing store failures by treating
`save_dir.store().exists(...)` as a plain boolean, so transient `read` errors
look like “missing data.” Update this check to propagate storage failures
instead of collapsing them into `false`, likely by making the `exists` path
return a `Result<bool>` and threading that through the caller in
`resolution.rs`. Keep the logic around `check_b`, `has_computed_bidegree`, and
the `save_dir.store()` access intact, but ensure any error from `exists` is
surfaced so the caller can distinguish “not present” from “store unavailable.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7c0c592-a06a-47a9-9607-e769dad83ec0
📒 Files selected for processing (5)
ext/src/chain_complex/chain_homotopy.rsext/src/nassau.rsext/src/resolution.rsext/src/save.rsext/src/utils.rs
Addresses @hoodmane's review: gate on what the dependencies actually require rather than target_arch = "wasm32", and stop repeating the cfg everywhere. The zarr `filesystem` feature pulls in `positioned-io::RandomAccessFile`, which is itself `cfg(any(windows, unix))`; the `zstd` feature's `zstd-sys` C build needs the same POSIX platform. So `any(windows, unix)` — not `target_arch = "wasm32"` — is the authoritative condition: `wasm32-unknown- emscripten` is `unix` and keeps the filesystem+zstd path, while `wasm32-unknown-unknown` (the web frontend) and `wasm32-wasi` fall back to the in-memory store with CRC32C-only codecs. - Cargo.toml: select the `zarrs` feature set on `cfg(any(windows, unix))` / `cfg(not(any(windows, unix)))`. - save.rs: collapse the ~14 scattered `#[cfg(target_arch = "wasm32")]` sites into two `#[cfg]`-gated `platform` submodules exposing `open_store`, `root_group_missing`, and `stream_tier_codecs` (plus the zstd-level machinery and the wasm `Send`/`Sync` impls). The rest of the file is now cfg-free. Behaviour on the two built targets (native, wasm32-unknown-unknown) is unchanged; only the previously-untargeted emscripten/wasi cases move. Verified: native check + nightly clippy/fmt, wasm32-unknown-unknown build of sseq_gui, and the save_load + lib test suites.
Addresses CodeRabbit's "bind to the complex" finding. Previously a save directory only recorded the algebra (magic + prime), so reusing one directory for a *different* module over the same algebra — e.g. resolve S_2, then point the same dir at C2 — would silently load the first module's cached differentials/kernels/quasi-inverses for the second: structurally valid, semantically wrong, and CRC-clean. - Add `ChainComplex::fingerprint()`: a stable content hash of the complex's structure (each bounded module's per-degree dimensions and the algebra action on every basis element, plus each differential's action — basis-sensitive, exactly what the resolution algorithm consumes). It walks `s` until the cached zero module (since `next_homological_degree` is `i32::MAX` for a `FiniteChainComplex`) under a safety cap, and skips unbounded modules. Hashing uses a fixed FNV-1a so the value is stable across runs and toolchains, unlike `std::hash::DefaultHasher`. - Rename `ZarrSaveStore::bind_to_algebra` -> `bind_to_complex`, taking the fingerprint and storing it (as a hex string, to survive the JSON round-trip exactly). On resume the stored fingerprint must match; a mismatch — including a store predating this attribute — fails loudly instead of mixing data. - New `test_wrong_complex`: S_2 then C2 in the same dir errors with "different complex". Existing save/load + resume tests still pass (same complex hashes equal, so resume is unaffected).
Supersedes the previous complex-fingerprint approach (commit f436040), which bound a save directory to its complex by storing an opaque 64-bit content hash. Storing the module's JSON spec instead does the same identity job while being readable, inspectable, and reusable for reconstruction — one artifact serving three purposes. - Revert `ChainComplex::fingerprint()`/`Fnv` and restore `bind_to_algebra` (algebra magic/prime/prefix). The complex-identity gate now lives in the new `ZarrSaveStore::bind_module_spec`, called from `construct`/`construct_nassau` where the spec is actually known: on a fresh store it records `module_spec`, on an existing one it must match exactly, else it bails. `test_wrong_complex` (S_2 then C2 in one dir) still fails loudly, now via the spec comparison. - `construct_from_save(dir)`: rebuild and resume a resolution from a save directory alone — reads the recorded `module_spec` + `algebra_prefix`, rebuilds the `Config`, and resolves back into the same dir. No need to re-supply what the directory already knows it holds. - `set_complex_name`: record the short human-readable label (the module spec the user typed, e.g. "S_2") as a `complex_name` attribute, a concise companion to the full `module_spec`, so `zarr.json` says what it resolves. - Fix a broken rustdoc intra-doc link (`FilesystemStore`) that failed the docs build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AJt3nb5Fp6okurqLzcQbCa
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/src/resolution.rs`:
- Around line 162-174: The best-effort metadata write in set_name currently
panics via expect, which contradicts the intended non-fatal behavior. Update
set_name so the save_dir.store().set_complex_name(&name) failure is handled
without aborting, ideally by logging a warning with context and then continuing
to assign self.name; keep the change localized to set_name and preserve the
existing behavior that bind_module_spec remains the real load guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25f3f7ec-4a3c-4936-8d56-dd4183c1828e
📒 Files selected for processing (5)
ext/src/nassau.rsext/src/resolution.rsext/src/save.rsext/src/utils.rsext/tests/save_load_resolution.rs
The `set_name` doc comment calls the `set_complex_name` store write "best-effort and purely informational", but it used `.expect()`, so a transient store failure would abort the whole resolution over a label that doesn't guard anything (the real load guard is `bind_module_spec`, which still propagates its errors). Log a warning and continue instead. Applies to both the standard and Nassau `set_name`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AJt3nb5Fp6okurqLzcQbCa
JoeyBF
pushed a commit
to JoeyBF/sseq
that referenced
this pull request
Jul 18, 2026
Port recompute-on-demand to the current (pre-zarr) save format. The quasi-inverse of d_s at (s, t) is re-derivable from the saved differential alone -- it needs only differentials[s], the module bases, and the deterministically-chosen subalgebra, never the rest of the resolution. - EXT_NASSAU_NO_SAVE_QI: resolution persists only the differentials (the qis are ~260-460x larger) and skips writing the quasi-inverse. - RecomputeReader: a streaming io::Read that regenerates the qi in exactly the byte format write_qi produced, so apply_quasi_inverse reads a recomputed qi through the unchanged apply loop -- only the source of bytes differs. It advances one signature at a time (reusing write_qi), so peak memory matches resolve-time, never the whole qi (hundreds of GB at record stems). - apply_quasi_inverse falls back to recomputation whenever no saved qi is available: no store, EXT_NASSAU_NO_SAVE_QI, or the top-of-region bidegree that nassau never saves (qi(max_s, t)). EXT_NASSAU_RECOMPUTE_QI forces it. This is independent of the zarr save rework (PR SpectralSequences#260): it targets the old Magic-bytecode format directly, so it can land first and let SpectralSequences#260 drop its qi stream tier. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiaVy4c9APWdmFeFoPCbdo
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
3 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.
Summary
Replaces the ad-hoc file-per-value save format with a ZarrV3-backed layout via the
zarrscrate, rebased onto currentmaster.Motivations for the format change:
to_bytes/from_bytesplumbing is replaced by zarr arrays whose shape matches the data.This is a clean break — no migration from the old format.
Layout
One zarr v3 store on a
FilesystemStore(native) /MemoryStore(wasm), with two tiers:kernel,differential,augmentation_qi, secondary data, chain maps/homotopies): one shardedvlen-bytesarray per kind, shard shape(8, 8[, 8]), inner chunk[1,1[,1]], CRC32C, no zstd. Tens of thousands of elements collapse into hundreds of shard files per kind.res_qi,nassau_qi): per-bidegree zarr groups with kind-specific sub-arrays, read/written a chunk at a time so peak memory is bounded regardless of payload size. Afinishedgroup attribute is the atomicity source of truth — a writer dropped beforefinish()is treated as missing on read.Coordinates are
(n, s[, idx])(MultiDegree<2/3>::coords()), with a fixed internalN_MIN = -1024offset so negative stems map into zarr's unsigned index space. Named homs and chain homotopies get per-name subgroups (products/{name}/,homotopies/{l}__{r}/) sharing the underlying store viaArc.bitcode(serde) replaces hand-rolled framing for the shard tier; thefptypes gainedSerialize/Deserializein #229.QuasiInverse::stream_quasi_inverse,MilnorSubalgebra::{to,from}_bytes, theMagicenum, andSaveDirectory::Splitare removed.Rebase onto master
Rebased from the original merge-base onto current
master. Since master splitext::secondaryinto per-primary submodules (#254) and made the secondary lifts returnResult(#241/#242), conflicts were confined tosecondary.rs: the store-based save API is applied to the shared secondary machinery while preserving theResult-returning entry points, the stale monolithic type duplicates are dropped, and the per-new()create_dirloops are removed (shard arrays are now created lazily). Two throwaway commits from the original branch (an experimental rayon fork patch and an early tracing tweak that master supersedes) were dropped.Performance
Benchmarked against master's old (uncompressed) format — S_2 through-stem n=70 s=40, release +
concurrent, 4 cores, best of 3:Three fixes over the initial rework, all confined to
save.rswith no format/API change:Arrayhandles perSaveKind— the initial version re-parsed each array'szarr.jsonon every read/write/delete.with_concurrent_target(1)to avoid the sharding-codec rayon-join deadlock.EXT_SAVE_ZSTD_LEVEL(clamped to[1, 22]) so very large runs can trade write time for maximum compression, e.g.EXT_SAVE_ZSTD_LEVEL=19.Net: writes are ~1.4× master (down from ~5×), the read/resume path is at parity, and the store is less than half master's real on-disk size.
Test plan
tests/save_load_resolution.rstests pass (debug + release, incl.--features concurrent,nassau)cargo test --libgreenEXT_SAVE_ZSTD_LEVELoverride verified end-to-endKnown follow-ups
zarrs'FilesystemStorerewrites chunk files in place (no temp+rename), and reads are unlocked, so a read concurrent with a same-shard write could observe a torn shard — CRC32C turns this into a detectable error, not silent corruption, and it hasn't surfaced in practice. A per-shardRwLockwould close it if it ever does.🤖 Generated with Claude Code
https://claude.ai/code/session_01AJt3nb5Fp6okurqLzcQbCa
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests