Relax the dependency graph in Nassau's compute_through_stem#272
Open
JoeyBF wants to merge 7 commits into
Open
Conversation
Previously `compute_through_stem` computed a bidegree `(s, t)` only after
both `(s - 1, t)` and `(s, t - 1)`. A comment noted that in theory `(s, t)`
only needs `(s, t - 1)` and `(s - 1, t - 1)`, but that "having the
dimensions of the modules change halfway through the computation is
annoying to do correctly".
This implements that relaxed dependency graph while preserving the
wavefront scheduler, so that many t-diagonals (n = t - s fixed) stay in
flight at once — which is where the parallelism comes from.
The key observation is that computing `(s, t)` only reads generators of
degree strictly less than `t` of the target modules: by minimality the
differentials we lift land in the radical, so the degree-`t` generators
that `(s - 1, t)` adds to `C_{s-1}` (and the degree-`(t-1)` generators of
`C_{s-2}`) contribute neither to the kernel we quotient by nor to those
differentials. `step_resolution_with_subalgebra` is therefore made to read
its target through a generator-degree cutoff (`signature_mask` /
`restricted_dimension` / `restricted_partial_matrix`), so it never touches
the data that `(s - 1, t)` is concurrently appending. This is race-free
against those append-only writes without any locking. The differentials are
stored in this restricted (radical) basis, and the quasi-inverse save files
always take the existing "incomplete information" (`Magic::Fix`) path, which
the secondary machinery already supports.
The wavefront in `compute_through_stem` is rewritten with the relaxed
triggers `(s, t) <- (s, t - 1), (s - 1, t - 1)` for `s >= 2`. Rows `s = 0`
and `s = 1` are kept on the strict schedule (they are cheap and read their
targets through full matrices).
`ModuleHomomorphism::apply_to_basis_element` now permits a result buffer
shorter than the full target dimension (a prefix of the target basis); the
matching truncated differential means `act` never writes past it.
Verified with `--features concurrent` (the parallel path):
- test_restart_stem and milnor_vs_nassau still pass;
- a new wide stem cross-check (compare_stem) against the standard
resolution (S_2, C2 to n=40/s=30; Joker to n=30/s=20), run repeatedly at
16 threads to shake out nondeterminism;
- a new save-backed secondary (d2) cross-check whose chart matches the
standard resolution, exercising the quasi-inverse `Magic::Fix` path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MCUtWj6P6suSZqvATCdg6d
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesThe PR adds truncated basis and generator-degree support to Nassau matrix computation, rewrites stem scheduling around relaxed dependencies for concurrent execution, changes parallel-depth tracking to thread-local state, and adds comparisons against standard resolutions. Nassau bounded matrix computation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant compute_through_stem
participant step_resolution_with_subalgebra
participant restricted_partial_matrix
participant signature_matrix
compute_through_stem->>step_resolution_with_subalgebra: process ready bidegree
step_resolution_with_subalgebra->>restricted_partial_matrix: build truncated target matrix
restricted_partial_matrix->>signature_matrix: construct bounded matrix
signature_matrix-->>step_resolution_with_subalgebra: return signature matrix
step_resolution_with_subalgebra-->>compute_through_stem: complete bidegree and enable successors
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 |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCUtWj6P6suSZqvATCdg6d
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/crates/algebra/src/module/homomorphism/free_module_homomorphism.rs`:
- Around line 65-71: Enforce the truncated result bound in the homomorphism
action method containing the result-length assertion: ensure both
output_on_generator and target.act write only within result’s provided prefix,
using a bounded action API or a dedicated restricted method that validates the
invariant. Keep the generic full-dimension behavior unchanged and prevent
shorter buffers from causing out-of-bounds writes.
In `@ext/src/nassau.rs`:
- Around line 1052-1055: Update the row-1 readiness branch in the progress check
so the stem-boundary case `(1, max_n + 1)` does not bypass row 0. Compute the
required one-bidegree row-0 dependency halo before this check, then retain
strict readiness validation against the corresponding row-0 progress rather than
treating `t > max_n` as automatically satisfied.
🪄 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: 6b01d781-3d1c-4c33-ab32-35cc51bf1ca3
📒 Files selected for processing (3)
ext/crates/algebra/src/module/homomorphism/free_module_homomorphism.rsext/src/nassau.rsext/tests/milnor_vs_nassau.rs
- Instead of weakening the shared `ModuleHomomorphism::apply_to_basis_element` assertion, keep it strict and add `apply_to_basis_element_restricted` (with a documented prefix-buffer contract) for Nassau's restricted matrix builders. - Add `compare_stem` cases with `max_n = 2^i - 1`, placing a nonzero class (`h_i`) on the `s = 1` stem edge `(1, max_n + 1)`, confirming the row-1 boundary computes correct generators against the standard resolution. - Fix rustdoc intra-doc link warnings under `-D warnings`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCUtWj6P6suSZqvATCdg6d
Use the same ungated `use maybe_rayon::prelude::*` with `#[allow(unused_imports)]` that `algebra::module::homomorphism` uses. The code path is identical in both feature configurations; the prelude's `enumerate`/`for_each` resolve via rayon's `IndexedParallelIterator` when `concurrent` is on and via `std::iter::Iterator` when it is off, which left the import formally unused (hence the allow) in the sequential build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCUtWj6P6suSZqvATCdg6d
JoeyBF
added a commit
to JoeyBF/sseq
that referenced
this pull request
Jul 18, 2026
…Sequences#272's restricted graph PR SpectralSequences#272's restricted_partial_matrix (degree-bounded columns) had displaced PR SpectralSequences#271's GPU partial-matrix offload and cross-signature row-reuse. Add a restricted GPU variant (get_partial_matrix_restricted[_verified]) that keeps the batched Milnor multiply but sizes output to the frozen prefix and masks bits beyond it, dispatch to it from restricted_partial_matrix_maybe_gpu, and restore row-reuse (one full restricted matrix per bidegree, select_rows per signature). Verified GPU==CPU on H200 NVL (CUDA 12.4): nassau_gpu (14230 rows / 527 bidegrees) and milnor_vs_nassau (9 tests) pass with NASSAU_GPU_VERIFY=1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JoeyBF
added a commit
to JoeyBF/sseq
that referenced
this pull request
Jul 18, 2026
Under SpectralSequences#272's relaxed dependency graph, many more step jobs are ready at once; each checked is_in_parallel() and re-queued itself (send_retry) whenever a guarded rayon region was active, busy-retry-storming the CPU. The heavy nassau matrix builds are now GPU-offloaded, so the guard's priority-inversion avoidance no longer pays for itself. The mechanism is a pure scheduling optimization with no effect on computed results, so removal is safe. Deletes utils::parallel (ParallelGuard/is_in_parallel/PARALLEL_DEPTH) and the retry plumbing in both the nassau and generic resolution schedulers. Verified GPU==CPU still holds on H200 (milnor_vs_nassau 9/9; nassau_gpu 14230 rows/527 bidegrees) with NASSAU_GPU_VERIFY=1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The relaxed wavefront keeps many bidegrees in flight at once, so at any instant it is likely that some job is inside a linear-algebra critical section (ParallelGuard). The scheduler re-spawned a bounced job immediately, which just re-checked is_in_parallel, found it still busy, and bounced again — spawning a whole rayon job per re-check and pegging every core on a retry storm that does no useful work. Instead the receiver checks the flag itself (a cheap atomic load) and parks a bidegree only when the section is genuinely busy. A job acquires and releases its guards many times and spends most of its time outside them, so the section frees far more often than jobs complete; parked bidegrees are therefore re-checked via a short recv_timeout while anything is parked, and re-spawned as soon as the section frees. Incoming messages are still handled the instant they arrive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCUtWj6P6suSZqvATCdg6d
is_in_parallel was a global count of active par_iter critical sections, so a step_resolution job bounced whenever *any* thread was in one. Under the relaxed wavefront many bidegrees are in flight, so that flag is almost always set and nearly every job bounced, producing the retry churn the parking mitigation only softened. The priority inversion the guard exists to prevent is narrower: a worker that initiated a par_iter blocks in the join and work-steals, and if it steals another (heavy, nested-parallel) resolution step, that step stalls the section the worker is blocked on. A stolen job runs on the stealer's own OS thread, so a thread-local depth counter reports exactly whether *this* worker is a blocked guard holder. Jobs picked up by a free worker read zero and run, letting independent bidegrees resolve concurrently instead of serializing behind any single critical section. The scheduler thread never holds a guard, so it can no longer read the flag to sense saturation; park bounced bidegrees and retry them on each completion or a short recv_timeout tick. Bounces are now rare (only a genuine steal-onto-a-blocked-holder), so the parking path barely engages. The classical scheduler shares the guard and benefits the same way, so its immediate-respawn no longer storms. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCUtWj6P6suSZqvATCdg6d
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/utils.rs (1)
598-662: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a small unit test for thread-local independence.
This subtle invariant — a stolen job's
PARALLEL_DEPTHis scoped to its worker thread, not global — has no direct test in this file; it's presently exercised only indirectly through Nassau's concurrent scheduling. A focused test (e.g. spawn twostd::threads, hold aParallelGuardon one, assertis_in_parallel()isfalseon the other) would cheaply pin down this behavior against future regressions (e.g. accidentally reverting to a shared counter).🤖 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/utils.rs` around lines 598 - 662, Add a focused unit test alongside ParallelGuard and is_in_parallel that spawns two OS threads, holds a ParallelGuard on one thread, and verifies is_in_parallel() remains false on the other thread while the guard is active. Ensure the test synchronizes the threads so the assertion specifically validates thread-local independence.
🤖 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/utils.rs`:
- Around line 598-662: Add a focused unit test alongside ParallelGuard and
is_in_parallel that spawns two OS threads, holds a ParallelGuard on one thread,
and verifies is_in_parallel() remains false on the other thread while the guard
is active. Ensure the test synchronizes the threads so the assertion
specifically validates thread-local independence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 679a693a-61e7-4998-a8ba-af03976cc2b8
📒 Files selected for processing (2)
ext/src/nassau.rsext/src/utils.rs
Pins the invariant the previous commit relies on — a ParallelGuard held on one thread reads as absent on another — so a future change that reverts to a shared counter fails loudly instead of silently reintroducing the retry storm. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCUtWj6P6suSZqvATCdg6d
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.
Implements the relaxation flagged by the long-standing comment in
nassau.rs: computing(s, t)only needs(s, t-1)and(s-1, t-1), not(s-1, t)and(s, t-1). The comment noted that "having the dimensions of the modules change halfway through the computation is annoying to do correctly" — this handles that.What
compute_through_stemnow uses the relaxed wavefront(s, t) <- (s, t-1), (s-1, t-1)fors >= 2, keeping manyt-diagonals (n = t - sfixed) in flight at once. Rowss = 0, 1stay strict — they're cheap, andstep0/step1read their targets through full matrices.step_resolution_with_subalgebrareads its targetC_{s-1}using only generators of degree< t(andC_{s-2}of degree< t-1). By minimality the differentials we lift land in the radical, so the degree-tgenerators — which(s-1, t)may be adding concurrently — contribute neither to the kernel we quotient by nor to those differentials. The read is thus race-free against those append-only writes, with no locking. New helpers: a generator-degree bound onsignature_mask, plusrestricted_dimensionandrestricted_partial_matrix.Magic::Fix) path, which the secondary machinery already handles.ModuleHomomorphism::apply_to_basis_elementnow allows a result buffer shorter than the full target dimension (a prefix of the target basis); the matching truncated differential meansactnever writes past it. This is the only change outsidenassau.rs.Notes
The relaxed graph shortens the critical path to
(S, T)from ~S + Tto ~T: a whole internal-degree column can compute in parallel once the previous one is committed. Nassau can afford this because each bidegree already computes the kernel it consumes locally (it reduces the incoming differentiald_{s-1}), so relaxing shares nothing across bidegrees and duplicates no work.Remark: the classical resolution (
resolution.rs)The same relaxation is mathematically valid for
MuResolution—ker(d_{s-1})_tis a column-(t-1)object there too (see the existingget_kernelcomment) — but it is not a free win. The classical algorithm reduces the outgoing differential at each bidegree and getsker(d_{s-1})_tas the byproduct of the single row reduction it does at(s-1, t)to place that row's generators, handing it forward through the kernel cache. That reuse is exactly the(s-1, t) -> (s, t)edge. Relaxing means decoupling "find kernel" (column-(t-1)) from "place generators" (which needs(s-2, t)) and scheduling/caching the kernel independently — theget_kernelstem-edge path is already a special case of this. Done carefully it costs no extra reductions, only holding the reduced matrix live a little longer (memory) plus the scheduling refactor; done naively (cache only theSubspace, recompute the matrix) it doubles the expensive reductions. Left out of this PR since p=2 workloads use Nassau in practice, but noted here for the record.Test plan
All with
--features concurrent(the parallel path;maybe_rayonruns sequentially otherwise):cargo test -p ext --lib nassau::—test_restart_stem, plus a newtest_stem_concurrent_secondary(save-backed d2 cross-check against the standard resolution, exercising theMagic::Fixquasi-inverse path)cargo test -p ext --test milnor_vs_nassau— existingcompare, plus a new widecompare_stemcross-check against the standard resolution (S_2, C2 to n=40/s=30; Joker to n=30/s=20), run 12× atRAYON_NUM_THREADS=16for determinismcargo test -p ext— green except the pre-existingsave_load_resolution::test_tempdir_lock(unrelated: it relies on read-only directory bits that root bypasses)concurrentfeature configurations🤖 Generated with Claude Code
https://claude.ai/code/session_01MCUtWj6P6suSZqvATCdg6d
Generated by Claude Code
Summary by CodeRabbit