feat(rust): establish Rust graph computing modernization framework (#355) - #359
Conversation
dosubot
Bot
added
size:XXL
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. The Rust framework currently has correctness and delivery blockers: invalid endpoints can corrupt the CSR, negative-weight SSSP can fail to terminate, and the new Rust CI/license/integration path is not passing or connected; the exact head has failed checks. Evidence: actionlint on .github/workflows/rust-ci.yml; gh run view 31351599313 --log-failed; computer-rust/src/kernel/{csr,sssp}.rs; Java/Go bridge sources.
| push: | ||
| branches: | ||
| - master | ||
| - /^release-.*$/ |
There was a problem hiding this comment.
/^release-.*$/ is rejected as an invalid branch name/pattern (actionlint reports the leading /, ^, and trailing / as invalid); the exact-head Rust CI run 31351599715 ended in startup_failure, so formatting, clippy, tests, and release build never ran. Please use a valid glob such as release-* and rerun the workflow.
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You me obtain a copy of the License at |
There was a problem hiding this comment.
You me obtain a copy, which makes the exact-head check-license-header job fail on this file. Please correct the standard license text to You may obtain a copy and rerun the license check.
| pub fn from_edges(num_vertices: u32, edges: &[(u32, u32, f64)]) -> Self { | ||
| let mut degree = vec![0; num_vertices as usize]; | ||
| for &(src, _dst, _weight) in edges { | ||
| if src < num_vertices { |
There was a problem hiding this comment.
degree counts every edge whose source is in range, but the fill loop skips an out-of-range destination. For example, from_edges(2, &[(0, 99, 1.0)]) allocates one slot and leaves it as the default 0 -> 0 edge, so PageRank/SSSP consume a topology that was never supplied. Please validate both endpoints when counting and filling, and return an error from the C API for invalid vertices.
| let (neighbors, weights) = graph.out_edges(position); | ||
| for i in 0..neighbors.len() { | ||
| let next_target = neighbors[i]; | ||
| let next_cost = cost + weights[i]; |
There was a problem hiding this comment.
0 -> 1 = -1 and 1 -> 0 = -1 keeps lowering both distances and pushing new heap entries, so the exported SSSP call can run without termination and exhaust CPU/memory. Please reject negative/non-finite weights at the API boundary or use an algorithm that detects negative cycles.
|
|
||
| func NewRustKernelBridge() *RustKernelBridge { | ||
| return &RustKernelBridge{ | ||
| available: false, |
There was a problem hiding this comment.
NewRustKernelBridge hard-codes available: false, and ComputePageRank always executes the Go fallback. The Java bridge likewise computes in Java and only declares nativeGetVersion, which does not match Rust's computer_kernel_version export. Please implement and test the JNI/CGO bindings and native-path selection, or document this PR as fallback-only instead of presenting an active Rust integration.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Independent gaps remain in the Go fallback's input validation, the C-ABI graph builder lifecycle, and the new correctness tests' ability to catch invalid output. Evidence: exact-head sources under computer-rust/, vermeer/apps/compute/, and the Maven/Go test wiring; the existing exact-head review already covers the branch filter, license header, CSR corruption, negative SSSP, and native bridge reachability findings.
|
|
||
| outDegree := make([]uint32, numVertices) | ||
| for _, edge := range edges { | ||
| src := edge[0] |
There was a problem hiding this comment.
outDegree when only src is valid, but the propagation loop later requires both endpoints to be valid. With numVertices=2 and an edge (0, 99), vertex 0 divides its rank by an edge that contributes nothing, so the fallback result loses mass and diverges from the Rust path. Please validate both endpoints before counting, or reject invalid edges with an error.
| return -1; | ||
| } | ||
| let builder = unsafe { &mut *handle }; | ||
| builder.edges.push((src, dst, weight)); |
There was a problem hiding this comment.
computer_graph_add_edge() still returns success after computer_graph_finalize() has populated builder.csr. Subsequent edges are appended to edges, but both compute functions keep reading the old CSR, so the C caller silently computes an obsolete graph. Please reject additions after finalization or invalidate/rebuild the CSR before allowing computation.
| return -3; | ||
| } | ||
|
|
||
| let distances = SsspKernel::compute(csr, source_vertex); |
There was a problem hiding this comment.
source_vertex is passed to SsspKernel::compute(), which returns an all-INFINITY vector, and the FFI function still returns 0. This is indistinguishable from a valid graph whose vertices are all unreachable. Please validate the source at the C boundary and return a documented error code.
|
|
||
| for i in 0..actual.len() { | ||
| let diff = (actual[i] - expected[i]).abs(); | ||
| if diff > epsilon { |
There was a problem hiding this comment.
NaN > epsilon is false, so assert_parity([f64::NAN], [0.0], epsilon) returns Ok(()); l1_distance() likewise returns Ok(NaN). A non-finite kernel result can therefore pass the differential fixture. Please reject non-finite inputs/differences and add NaN/Infinity regression cases.
| } | ||
|
|
||
| impl PageRankKernel { | ||
| pub fn new(damping_factor: f64, max_iterations: u32, tolerance: f64) -> Self { |
There was a problem hiding this comment.
PageRankKernel::new() accepts non-finite or out-of-range parameters without validation. A NaN damping factor produces NaN ranks, and a NaN tolerance prevents convergence because every comparison is false; damping values outside [0, 1] also violate the probability contract. Please validate finite damping/tolerance at the API boundary and return an error for invalid input.
|
|
||
| #[test] | ||
| fn test_pagerank_computation() { | ||
| let edges = vec![(0, 1, 1.0), (1, 2, 1.0), (2, 0, 1.0)]; |
There was a problem hiding this comment.
|
|
||
| public class RustKernelBridgeTest { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
computer-test/pom.xml includes only **/UnitTestSuite.java, and UnitTestSuite does not reference RustKernelBridgeTest. The class can compile while its fallback regression never runs in CI. Please add it to the suite or configure an explicit Surefire include, then verify the test count.
| "testing" | ||
| ) | ||
|
|
||
| func TestRustBridgePageRank(t *testing.T) { |
There was a problem hiding this comment.
go test. Please add at least go test ./apps/compute (and a native-path job when bindings exist) so fallback behavior is continuously verified.
| use crate::RUST_KERNEL_VERSION; | ||
| use std::ffi::CString; | ||
| use std::os::raw::c_char; | ||
| use std::ptr; |
There was a problem hiding this comment.
std::ptr is unused in this file, while the new workflow runs cargo clippy --all-targets -- -D warnings. Once the workflow startup issue is fixed, this import will fail the quality gate. Please remove it and rerun Clippy.
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn computer_kernel_version() -> *const c_char { | ||
| thread_local! { |
There was a problem hiding this comment.
computer_kernel_version() returns a pointer into a thread-local CString; that pointer becomes invalid when the calling thread exits, and the header does not document the borrowed lifetime or provide a copy/free contract. A C caller that stores the pointer or passes it across threads can use freed memory. Please return process-lifetime static storage or expose an explicit copy API and document ownership.
- Fix workflow branch filter glob in rust-ci.yml - Fix license header typo and remove unused std::ptr import in c_api.rs - Fix CSR degree counting for out-of-bounds destinations - Validate non-negative finite edge weights and reject post-finalization additions in C-ABI - Fix Go fallback out-degree calculation for invalid edge endpoints - Store version string in process-wide static OnceLock to guarantee pointer lifetime - Validate PageRank parameters (damping, tolerance) and SSSP source vertex bounds - Synchronize AtomicAggregator reset with RwLock - Enhance C-ABI, differential tolerance, PageRank, and bridge test assertions - Wire RustKernelBridgeTest into Maven suite and Go tests into Vermeer CI - Update C-ABI header doc comments and architecture roadmap docs
|
Hi @imbajin, Thanks for the detailed review. I’ve addressed the requested issues in the latest commit The updates include the CI/license fixes, CSR/FFI validation, SSSP/PageRank safety checks, aggregator synchronization, test coverage improvements, CI test wiring, and documentation updates. I also verified the final diff and kept the changes scoped to the review feedback. Could you please take another look when you have a chance? Thanks! |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The exact head still cannot execute Vermeer CI, and the advertised parity contract is not met: PageRank stops on a per-vertex maximum error instead of the documented L1 threshold while the Java/Go fallbacks accept invalid parameters. Evidence: actionlint on .github/workflows/vermeer-ci.yml; gh run view 31675719210 and 31675719706; computer-rust/src/kernel/pagerank.rs:83-95; computer/computer-core/.../RustKernelBridge.java:61-73; vermeer/apps/compute/rust_bridge.go:47-67.
| - name: Build | ||
| run: CGO_ENABLED=0 go build -o vermeer | ||
|
|
||
| - name: Run Go compute tests |
There was a problem hiding this comment.
/^release-.*$/ at line 23, and run 31675719210 finished startup_failure with no jobs. Please replace the filter with a GitHub Actions glob such as release-*, then rerun and require a successful Vermeer CI run so this added test actually executes.
| } | ||
|
|
||
| // ComputePageRank calculates PageRank with fallback to Go execution when native library is inactive. | ||
| func (b *RustKernelBridge) ComputePageRank(numVertices uint32, edges [][2]uint32, dampingFactor float64, maxIterations uint32, tolerance float64) ([]float64, error) { |
There was a problem hiding this comment.
|
|
||
| public static double[] computePageRank(double[][] adjMatrix, double dampingFactor, | ||
| int maxIterations, double tolerance) { | ||
| if (adjMatrix == null || adjMatrix.length == 0) { |
There was a problem hiding this comment.
| ranks[v] = new_rank; | ||
| } | ||
|
|
||
| if max_diff < self.tolerance { |
There was a problem hiding this comment.
max_diff < tolerance), but the roadmap declares an L1 error bound. With N vertices, this permits aggregate L1 error up to N*tolerance, so the advertised parity guarantee is not met. Please accumulate the L1 difference for convergence, or change the contract and tests to match.
| )); | ||
| } | ||
| let diff = (actual[i] - expected[i]).abs(); | ||
| if !diff.is_finite() || diff > epsilon { |
There was a problem hiding this comment.
epsilon itself is never validated. With epsilon = NaN, diff > epsilon is false, so finite mismatched vectors can return Ok; this lets an invalid tolerance bypass the differential check. Please reject non-finite or negative epsilon before the loop and add a NaN regression case.
| impl GraphFixture { | ||
| /// Returns the Zachary's Karate Club representative graph dataset fixture. | ||
| pub fn karate_club() -> Self { | ||
| let edges = vec
|
Please pause further coding for now. This PR has been marked as Draft. Before continuing, please submit and get approval for a complete review plan covering at least the objectives and scope, implementation steps, API and compatibility impact, testing and validation, risks and rollback strategy, and acceptance criteria. Until the plan is approved, the previous review process will remain paused; follow-up review can resume after the complete plan is approved. |
|
Thanks for the detailed review, @imbajin. I’ve paused further implementation as requested. I’ll prepare a complete review/implementation plan covering the scope, implementation steps, API/compatibility impact, testing and validation, risks/rollback, and acceptance criteria, and will wait for approval before making further code changes. |
9e72b0e to
c1fc10a
Compare
|
Hi @imbajin, I’ve updated the PR according to the agreed Phase 1 scope. The premature Java/Go bridges and secondary Rust components have been removed, and the PR is now focused on the isolated Rust crate, C-ABI boundary, correctness fixtures/benchmark, CI, and the roadmap/RFC documentation. The PR is now ready for review. Thank you! |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The exact head still has two non-duplicate contract issues: PageRank ignores supplied edge weights, and the parity helper checks per-element error instead of the documented L1 bound. Evidence: computer-rust/src/kernel/pagerank.rs:79-82 and computer-rust/src/fixtures/tolerance.rs:48-70; exact-head Rust CI is startup_failure and license/CodeQL are action_required.
| dangling_sum += ranks[v]; | ||
| } else { | ||
| let share = ranks[v] / (out_degree as f64); | ||
| let (neighbors, _) = graph.out_edges(v as u32); |
There was a problem hiding this comment.
weight and CsrGraph stores it, but this binding explicitly discards the weights (let (neighbors, _)) and always sends ranks[v] / out_degree to every neighbor. Consequently, edges with weights 1.0 and 100.0 produce the same PageRank result, despite the exported API and CSR carrying edge weights. Please either use normalized outgoing weights in the transition or remove/document the argument as topology-only, and add an asymmetric-weight regression test. Evidence: computer-rust/src/ffi/c_api.rs:45-64, computer-rust/src/kernel/csr.rs:60-73, and this line.
| )); | ||
| } | ||
| let diff = (actual[i] - expected[i]).abs(); | ||
| if !diff.is_finite() || diff > epsilon { |
There was a problem hiding this comment.
assert_parity checks each element against epsilon, but the roadmap promises an L1-distance bound. For two elements whose absolute differences are each 0.75 * epsilon, this function returns Ok while the L1 distance is 1.5 * epsilon; a caller can therefore accept a result outside the advertised contract. Please compare l1_distance(actual, expected) with a validated epsilon, or rename/document this as a per-element check, and add a multi-element regression test. Evidence: the per-element diff > epsilon condition at this line and the L1 contract in docs/rust-modernization-roadmap.md.
2 participants
Summary
This PR establishes the initial Phase 1 foundation for the incremental Rust modernization roadmap described in #355.
The scope is intentionally kept small and isolated. It introduces a standalone Rust crate for graph-computing experiments, together with the initial C-ABI boundary, correctness fixtures, benchmarks, documentation, and CI support.
Included in this PR
computer-rustcratecargo fmt, Clippy, tests, release build, and benchmark compilation to Rust CIIntentionally out of scope
To keep this first step small and reviewable, this PR does not include:
Those areas are intended for dedicated follow-up tasks after the core Rust slice, interoperability boundary, and acceptance criteria are reviewed.
Validation
The implementation is scoped to an isolated Rust slice and does not modify existing Java or Go runtime behavior.
The Rust CI workflow includes:
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo test --all-targets --verbosecargo build --releasecargo bench --no-runRelated Issue
Part of the incremental Rust modernization roadmap tracked in #355.