Repository navigation
feat(lance-fold): consume upstream graph-hop kernel on Rust 1.99 - #3
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd4dc4bfce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,8 @@ | |||
| [toolchain] | |||
| channel = "1.99.0" | |||
There was a problem hiding this comment.
The Lints job in .github/workflows/ci.yml installs nightly but then invokes unqualified cargo through just fmt, just clippy, and cargo doc. The checked-in toolchain file takes precedence over rustup's default toolchain, while dtolnay/rust-toolchain only installs and sets nightly as the default, so this job will select or attempt to install stable 1.99 instead. In particular, stable rustfmt ignores this repository's nightly-only rustfmt.toml options, weakening the formatting check; remove the pin for this job or invoke the commands with +nightly. This precedence is documented in the rustup override documentation.
Useful? React with 👍 / 👎.
| let mut frontier = vec![0; words_for(snapshot.node_rows)]; | ||
| frontier[seed / 64] |= 1u64 << (seed % 64); | ||
| let mut targets = vec![0u64; words_for(snapshot.node_rows)]; | ||
|
|
||
| if edges.sources.is_empty() { | ||
| return Ok(targets); | ||
| } | ||
|
|
||
| let planes_masks = [snapshot.live_edges]; | ||
| let lanes = [ | ||
| LaneRef::U32(edges.sources), | ||
| LaneRef::U32(edges.targets), | ||
| ]; | ||
| let planes = Planes { | ||
| n_rows: edges.sources.len(), | ||
| masks: &planes_masks, | ||
| lanes: &lanes, | ||
| }; | ||
| let foreign_planes = [ | ||
| ForeignPlane { words: &frontier, rows: snapshot.node_rows }, | ||
| ForeignPlane { words: snapshot.visible_nodes, rows: snapshot.node_rows }, | ||
| ]; | ||
| let foreign = Foreign { planes: &foreign_planes, lanes: &[] }; | ||
| let program = Program::new( | ||
| vec![ | ||
| MaskOp::Gather { | ||
| lane: 0, | ||
| foreign: 0, | ||
| under: Some(Operand::Plane(0)), | ||
| dst: 0, | ||
| }, | ||
| MaskOp::Gather { | ||
| lane: 1, | ||
| foreign: 1, | ||
| under: Some(Operand::Scratch(0)), | ||
| dst: 1, | ||
| }, | ||
| ], | ||
| Terminal::ScatterOrU32 { | ||
| mask: Operand::Scratch(1), | ||
| lane: 1, | ||
| out_rows: snapshot.node_rows as u32, | ||
| }, | ||
| ); | ||
| let mut scratch = Scratch::for_program(&program, edges.sources.len()) | ||
| .map_err(|error| HopError::Executor(format!("{error:?}")))?; | ||
| execute_into( | ||
| &program, &planes, &foreign, &mut scratch, Out::Mask(&mut targets), | ||
| ) | ||
| .map_err(|error| HopError::Executor(format!("{error:?}")))?; | ||
|
|
||
| Ok(targets) | ||
| let mut out = vec![0; words_for(snapshot.node_rows)]; |
There was a problem hiding this comment.
For malformed input whose claimed node_rows is large but whose visibility mask contains the selected seed word, these two allocations happen before kernel.run can validate the snapshot masks and edge lanes. A caller can therefore make one_hop reserve up to roughly 1 GiB for a u32-addressable graph and potentially abort on allocation, whereas the removed implementation returned InvalidShape before allocating. Validate the full upstream snapshot/edge shape before constructing frontier and out.
Useful? React with 👍 / 👎.
1 participant
Why
Follow up to merged petgraph #2 and dependent on AdaWorldAPI/lance-graph#1616. The first fork-side experiment established that Mask-RISC executes a directed BFS one-hop parity query. Move the reusable execution out of the fork, leaving a minor petgraph bridge, exactly as intended by the fork-first plan.
Change
lance::one_hopnow delegates tolance-graph-mask-risc::OutgoingHop; the local Gather/Gather/Scatter lowering and visibility-shape implementation are removed.66636da46107edcc3280237db088172f652cacd5.rust-toolchain.tomlat 1.99.0 (rustfmt + clippy), explicit Rust 1.99 for the experiment crate, and set its GitHub Actions runner to 1.99.0.Cargo/MSRV contract
Pinning the compiler also pins the Cargo toolchain. The fork retains petgraph's upstream
rust-version = 1.85compatibility declaration; only the isolated experiment requires 1.99. Blanketcargo upgradeof dependency ranges is intentionally out of scope, as it would conflate toolchain migration with library dependency changes and the repo has no tracked root Cargo.lock.Validation
Lance fold hop probetest green under 1.99.0Next
Explore a properly witnessed continuation contract for multiple graph folds (BFS/Gremlin repeat) without sneaking a materialized projection back into the fold algebra. Extract
lance-graph-coreonly after actual consumers establish the minimal contract.