Context
Bundles three deferred MEDIUM/LOW findings from the Agent R review of Spanker PR #10 (which merged the API hardening + cache-friendly all_reduce). None were merge-blocking but are worth a focused cleanup PR.
Tasks
1. Remove or honestly label the non_exhaustive sentinel unit tests (MEDIUM)
In src/scheduler/src/collective.rs:163-180 and the parallel error_non_exhaustive_requires_catchall_downstream at line 186, the #[allow(unreachable_patterns)] annotation makes these tests pass regardless of whether #[non_exhaustive] is on the enum. The doctest is the load-bearing guard. Either:
- Remove the unit tests and rely on the doctests + the inline doc comment
- Or rename them and update their doc comments to honestly say "documentation/intent only — see compile_fail doctest for the load-bearing test"
2. Closure-based all_reduce dispatch (MEDIUM)
In src/scheduler/src/collective.rs:86, let rest = per_card.iter().skip(1) is consumed once in the match. If a future variant tries to also use rest, you get a confusing 'value used after move' error. Refactor to:
let combine: fn(&mut f32, f32) = match op {
ReduceOp::Sum | ReduceOp::Avg => |acc, x| *acc += x,
ReduceOp::Max => |acc, x| if x > *acc { *acc = x },
ReduceOp::Min => |acc, x| if x < *acc { *acc = x },
};
// then a single generic inner loop using `combine`This both eliminates the rest fragility AND gives a cleaner extension point for new variants (Product, LogSumExp etc.).
3. Reorder topology capacity arithmetic to match comment (LOW)
src/scheduler/src/topology.rs:288 — comment says n*(n-1) but code writes n_sails.saturating_sub(1) * n_sails which reads left-to-right as (n-1)*n. Either swap the operands OR reword the comment. Both are correct due to commutativity; this is purely readability.
Acceptance
Refs
Authored by Agent R (Reviewer).
Context
Bundles three deferred MEDIUM/LOW findings from the Agent R review of Spanker PR #10 (which merged the API hardening + cache-friendly all_reduce). None were merge-blocking but are worth a focused cleanup PR.
Tasks
1. Remove or honestly label the
non_exhaustivesentinel unit tests (MEDIUM)In
src/scheduler/src/collective.rs:163-180and the parallelerror_non_exhaustive_requires_catchall_downstreamat line 186, the#[allow(unreachable_patterns)]annotation makes these tests pass regardless of whether#[non_exhaustive]is on the enum. The doctest is the load-bearing guard. Either:2. Closure-based all_reduce dispatch (MEDIUM)
In
src/scheduler/src/collective.rs:86,let rest = per_card.iter().skip(1)is consumed once in the match. If a future variant tries to also userest, you get a confusing 'value used after move' error. Refactor to:This both eliminates the
restfragility AND gives a cleaner extension point for new variants (Product, LogSumExp etc.).3. Reorder topology capacity arithmetic to match comment (LOW)
src/scheduler/src/topology.rs:288— comment saysn*(n-1)but code writesn_sails.saturating_sub(1) * n_sailswhich reads left-to-right as(n-1)*n. Either swap the operands OR reword the comment. Both are correct due to commutativity; this is purely readability.Acceptance
cargo clippy -- -D warnings+cargo fmt --checkcleancompile_failtests are NOT touched (they're correct; this PR is about the sentinel unit tests, not them)Refs
Authored by Agent R (Reviewer).