imbajin commented on code in PR #359:
URL:
https://github.com/apache/hugegraph-computer/pull/359#discussion_r3775867476
##########
.github/workflows/vermeer-ci.yml:
##########
@@ -75,6 +75,9 @@ jobs:
- name: Build
run: CGO_ENABLED=0 go build -o vermeer
+ - name: Run Go compute tests
Review Comment:
‼️ The workflow containing this new test step is still unexecutable on the
exact head: actionlint rejects the existing push branch filter `/^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.
##########
computer-rust/src/fixtures/tolerance.rs:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * 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 may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+pub struct DifferentialTolerance;
+
+impl DifferentialTolerance {
+ pub fn l1_distance(actual: &[f64], expected: &[f64]) -> Result<f64,
String> {
+ if actual.len() != expected.len() {
+ return Err(format!(
+ "Vector length mismatch: actual len {}, expected len {}",
+ actual.len(),
+ expected.len()
+ ));
+ }
+
+ let mut l1 = 0.0;
+ for i in 0..actual.len() {
+ if !actual[i].is_finite() || !expected[i].is_finite() {
+ return Err(format!(
+ "Non-finite value detected at index {}: actual = {},
expected = {}",
+ i, actual[i], expected[i]
+ ));
+ }
+ l1 += (actual[i] - expected[i]).abs();
+ }
+
+ if !l1.is_finite() {
+ return Err("Calculated L1 distance is non-finite".to_string());
+ }
+
+ Ok(l1)
+ }
+
+ pub fn assert_parity(actual: &[f64], expected: &[f64], epsilon: f64) ->
Result<(), String> {
+ if actual.len() != expected.len() {
+ return Err(format!(
+ "Vector length mismatch: actual len {}, expected len {}",
+ actual.len(),
+ expected.len()
+ ));
+ }
+
+ for i in 0..actual.len() {
+ if !actual[i].is_finite() || !expected[i].is_finite() {
+ return Err(format!(
+ "Non-finite value detected at index {}: actual = {},
expected = {}",
+ i, actual[i], expected[i]
+ ));
+ }
+ let diff = (actual[i] - expected[i]).abs();
+ if !diff.is_finite() || diff > epsilon {
Review 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.
##########
vermeer/apps/compute/rust_bridge.go:
##########
@@ -0,0 +1,107 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with 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 may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+License for the specific language governing permissions and limitations
+under the License.
+*/
+
+package compute
+
+import (
+ "fmt"
+ "math"
+)
+
+// RustKernelBridge manages interaction with high-performance Rust computing
kernels.
+type RustKernelBridge struct {
+ available bool
+ version string
+}
+
+func NewRustKernelBridge() *RustKernelBridge {
+ return &RustKernelBridge{
+ available: false,
+ version: "1.5.0-go-fallback",
+ }
+}
+
+func (b *RustKernelBridge) IsAvailable() bool {
+ return b.available
+}
+
+func (b *RustKernelBridge) Version() string {
+ return b.version
+}
+
+// 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) {
Review Comment:
⚠️ This fallback only rejects numVertices == 0; dampingFactor, tolerance
(including NaN/Inf/negative/out-of-range) and invalid endpoints are otherwise
accepted or ignored, while the Rust C-ABI returns -4/-1 for those inputs and
the roadmap promises identical validation. Please validate and return errors
consistently, or revise the contract, and add regression tests.
##########
computer-rust/src/fixtures/dataset.rs:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * 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 may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+use crate::kernel::csr::CsrGraph;
+
+pub struct GraphFixture {
+ pub name: String,
+ pub num_vertices: u32,
+ pub edges: Vec<(u32, u32, f64)>,
+}
+
+impl GraphFixture {
+ /// Returns the Zachary's Karate Club representative graph dataset fixture.
+ pub fn karate_club() -> Self {
+ let edges = vec![
+ (0, 1, 1.0), (0, 2, 1.0), (0, 3, 1.0), (0, 4, 1.0), (0, 5, 1.0),
+ (0, 6, 1.0), (0, 7, 1.0), (0, 8, 1.0), (0, 10, 1.0), (0, 11, 1.0),
+ (0, 12, 1.0), (0, 13, 1.0), (0, 17, 1.0), (0, 19, 1.0), (0, 21,
1.0),
+ (0, 31, 1.0), (1, 2, 1.0), (1, 3, 1.0), (1, 7, 1.0), (1, 13, 1.0),
+ (1, 17, 1.0), (1, 19, 1.0), (1, 21, 1.0), (1, 30, 1.0), (2, 3,
1.0),
+ (2, 7, 1.0), (2, 8, 1.0), (2, 9, 1.0), (2, 13, 1.0), (2, 27, 1.0),
+ (2, 28, 1.0), (2, 32, 1.0), (3, 7, 1.0), (3, 12, 1.0), (3, 13,
1.0),
+ ];
+ Self {
+ name: "karate_club".to_string(),
+ num_vertices: 34,
+ edges,
+ }
+ }
+
+ /// Generates a synthetic power-law graph dataset fixture for baseline
testing.
+ pub fn synthetic_powerlaw(num_vertices: u32, avg_degree: u32) -> Self {
Review Comment:
⚠️ Despite the `powerlaw` name, this generator gives every vertex an
out-degree of only `avg_degree + (src % 5)`, i.e. 10-14 for the benchmark
input, with no heavy tail. The benchmark therefore does not exercise power-law
hotspots or memory behavior. Please generate a reproducible heavy-tailed
distribution or rename the fixture to match its regular topology.
##########
computer-rust/src/fixtures/dataset.rs:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * 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 may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+use crate::kernel::csr::CsrGraph;
+
+pub struct GraphFixture {
+ pub name: String,
+ pub num_vertices: u32,
+ pub edges: Vec<(u32, u32, f64)>,
+}
+
+impl GraphFixture {
+ /// Returns the Zachary's Karate Club representative graph dataset fixture.
+ pub fn karate_club() -> Self {
+ let edges = vec![
Review Comment:
⚠️ This is labeled as the standard Karate Club fixture, but it contains only
35 directed edges, all sourced from vertices 0-3; vertices 4-33 have no
outgoing edges. The current test only checks non-empty data, so benchmarks and
parity inputs are materially truncated. Please add the complete dataset and
assert edge count/key adjacency, or rename and document this as a reduced
fixture.
##########
computer-rust/src/kernel/pagerank.rs:
##########
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * 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 may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+use crate::kernel::csr::CsrGraph;
+
+#[derive(Debug, Clone)]
+pub struct PageRankKernel {
+ damping_factor: f64,
+ max_iterations: u32,
+ tolerance: f64,
+}
+
+impl PageRankKernel {
+ pub fn try_new(damping_factor: f64, max_iterations: u32, tolerance: f64)
-> Result<Self, String> {
+ if !damping_factor.is_finite() || damping_factor < 0.0 ||
damping_factor > 1.0 {
+ return Err(format!(
+ "Invalid damping_factor: {}. Must be finite and in range [0.0,
1.0]",
+ damping_factor
+ ));
+ }
+ if !tolerance.is_finite() || tolerance < 0.0 {
+ return Err(format!(
+ "Invalid tolerance: {}. Must be finite non-negative number",
+ tolerance
+ ));
+ }
+ Ok(Self {
+ damping_factor,
+ max_iterations,
+ tolerance,
+ })
+ }
+
+ pub fn new(damping_factor: f64, max_iterations: u32, tolerance: f64) ->
Self {
+ Self::try_new(damping_factor, max_iterations, tolerance)
+ .expect("Failed to initialize PageRankKernel due to invalid
parameters")
+ }
+
+ pub fn compute(&self, graph: &CsrGraph) -> Vec<f64> {
+ let num_vertices = graph.num_vertices() as usize;
+ if num_vertices == 0 {
+ return Vec::new();
+ }
+
+ let initial_rank = 1.0 / (num_vertices as f64);
+ let mut ranks = vec![initial_rank; num_vertices];
+ let mut next_ranks = vec![0.0; num_vertices];
+
+ let teleport = (1.0 - self.damping_factor) / (num_vertices as f64);
+
+ for _iter in 0..self.max_iterations {
+ next_ranks.fill(0.0);
+ let mut dangling_sum = 0.0;
+
+ for v in 0..num_vertices {
+ let out_degree = graph.out_degree(v as u32);
+ if out_degree == 0 {
+ dangling_sum += ranks[v];
+ } else {
+ let share = ranks[v] / (out_degree as f64);
+ let (neighbors, _) = graph.out_edges(v as u32);
+ for &target in neighbors {
+ next_ranks[target as usize] += share;
+ }
+ }
+ }
+
+ let dangling_share = self.damping_factor * (dangling_sum /
(num_vertices as f64));
+ let mut max_diff = 0.0f64;
+
+ for v in 0..num_vertices {
+ let new_rank = teleport + dangling_share + self.damping_factor
* next_ranks[v];
+ let diff = (new_rank - ranks[v]).abs();
+ if diff > max_diff {
+ max_diff = diff;
+ }
+ ranks[v] = new_rank;
+ }
+
+ if max_diff < self.tolerance {
Review Comment:
‼️ The loop terminates on the maximum single-vertex difference (`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.
##########
computer/computer-core/src/main/java/org/apache/hugegraph/computer/core/rust/RustKernelBridge.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * 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 may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.computer.core.rust;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RustKernelBridge {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RustKernelBridge.class);
+ private static final boolean NATIVE_AVAILABLE;
+ private static final String LIB_NAME = "hugegraph_computer_rust";
+
+ static {
+ boolean loaded = false;
+ try {
+ System.loadLibrary(LIB_NAME);
+ loaded = true;
+ LOG.info("Successfully loaded Rust graph computing native library:
{}", LIB_NAME);
+ } catch (UnsatisfiedLinkError e) {
+ LOG.info("Native library '{}' not available on system PATH; using
pure Java fallback",
+ LIB_NAME);
+ } catch (Throwable t) {
+ LOG.warn("Failed to load native Rust graph computing library: {}",
t.getMessage());
+ }
+ NATIVE_AVAILABLE = loaded;
+ }
+
+ public static boolean isAvailable() {
+ return NATIVE_AVAILABLE;
+ }
+
+ public static String getVersion() {
+ if (NATIVE_AVAILABLE) {
+ try {
+ return nativeGetVersion();
+ } catch (Throwable t) {
+ LOG.warn("Error calling nativeGetVersion: {}", t.getMessage());
+ }
+ }
+ return "1.5.0-java-fallback";
+ }
+
+ public static double[] computePageRank(double[][] adjMatrix, double
dampingFactor,
+ int maxIterations, double
tolerance) {
+ if (adjMatrix == null || adjMatrix.length == 0) {
Review Comment:
⚠️ This fallback has no validation for dampingFactor or tolerance; NaN or
out-of-range values flow into arithmetic and can return NaN or invalid ranks,
while the Rust C-ABI rejects them with -4 and the roadmap promises parity.
Please validate finite damping in [0,1] and finite non-negative tolerance,
define the error behavior, and add regression tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
