bitflicker64 commented on code in PR #360: URL: https://github.com/apache/hugegraph-computer/pull/360#discussion_r3952041892
########## computer-rust/Cargo.toml: ########## @@ -0,0 +1,43 @@ +# 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] +name = "hugegraph-computer-rust" Review Comment: ‼️ This is two PRs wearing one coat. The branch carries two commits against `master`: - `ada86aa`, 21 files, **+1508**: this crate, `rust-ci.yml`, `RustKernelBridge.java`, `rust_bridge.go`, the roadmap doc. - `e533ae8`, 4 files, **+206**: `computer/pom.xml` (Spotless and JaCoCo), `commit-check.yml`, `release-notes.yml`, `docs/automation-guide.md`. Issue #320, which this PR says it fixes, is an automation-maturity audit: it recommends code formatting and test coverage as next steps and flags commit validation and release notes among the gaps. It does not mention Rust anywhere. The Rust work has its own issue (#355) and its own open PR (#359). It is not even a clean duplicate of #359. Six files here are absent from #359's head `c1fc10a` (15 files, +1124): ``` computer-rust/src/kernel/aggregator.rs computer-rust/src/kernel/sssp.rs computer/computer-core/.../rust/RustKernelBridge.java computer/computer-test/.../rust/RustKernelBridgeTest.java vermeer/apps/compute/rust_bridge.go vermeer/apps/compute/rust_bridge_test.go ``` That is +529 lines of Rust kernel and native-bridge code with no issue and no PR of its own, arriving on an automation PR. (Correcting myself: my earlier review said the Rust came from the stacked, unmerged #359. Six of these files are not in #359 either.) Please rebase onto `master` and drop `ada86aa`. What is left maps one-to-one onto #320 and can be reviewed in one sitting, and the Rust review stays on #359 where it already has a thread. ########## 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, Review Comment: ‼️ Nothing calls this, and both algorithms already ship here. @imbajin has already noted at line 119 that `computePageRank` never reaches native code. The part worth acting on is what sits underneath: this is a dense adjacency-matrix PageRank in pure Java that rebuilds every vertex's out-degree by scanning all n cells per row on **every** iteration, and `vermeer/apps/compute/rust_bridge.go` writes it again with `available` hard-coded to `false`. The repository already has: - `computer/computer-algorithm/src/main/java/org/apache/hugegraph/computer/algorithm/centrality/pagerank/PageRank.java` - `vermeer/algorithms/pagerank.go` and `vermeer/algorithms/sssp.go` Those two handle dangling mass and converge on an L1 norm; these fallbacks converge on `maxDiff`, which is L-infinity. So the new copies are not a drop-in for the old ones, they are a fourth and fifth implementation with different numerics. And nothing calls them: `RustKernelBridge` and `NewRustKernelBridge` appear nowhere outside `RustKernelBridgeTest.java` and `rust_bridge_test.go`. Nothing builds `libhugegraph_computer_rust` into any artifact either, so `System.loadLibrary(LIB_NAME)` cannot succeed on anything this repo currently ships. Please delete both bridge files and their tests. When the shared library actually exists and is packaged, the bridge is `System.loadLibrary` plus ```java private static native double[] nativePageRank(double[][] adj, double damping, int maxIter, double tol); ``` with no hand-rolled fallback at all. (@imbajin already offered removal as one of two options on the Go side; this is the evidence for taking it on both.) ########## computer-rust/src/lib.rs: ########## @@ -0,0 +1,27 @@ +/* + * 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 mod ffi; +pub mod fixtures; Review Comment: ⚠️ 177 lines of this crate are reachable only from their own `#[cfg(test)]` blocks, and this line puts the test helpers in the library's public API. - `kernel/aggregator.rs` (100 lines): a CAS-loop lock-free `f64` accumulator. `AtomicAggregator` has no caller. The only references outside the `pub use` below are in its own test module, and no kernel in this crate is parallel: the single `thread::spawn` in the whole diff is in that test. A lock-free accumulator for concurrency that does not exist yet is scaffolding. - `fixtures/tolerance.rs` (77 lines): `DifferentialTolerance` is only ever compared against hand-written literals in its own unit test. Please delete `aggregator.rs` until a parallel kernel needs it, and `tolerance.rs` until something actually runs a differential. That is roughly 180 lines with no behaviour change. `fixtures/dataset.rs` has to stay public as it is, since `benches/kernel_bench.rs:19` imports `GraphFixture` from the library, and `cargo clippy --all-targets` in `rust-ci.yml` builds that bench. One related note: `docs/rust-modernization-roadmap.md` states the parity guardrail as differential correctness testing enforcing an L1 distance of at most 1e-6 against ground-truth outputs. Nothing compares these kernels to the Java or Go baselines, so the helper that guardrail is named after has no differential to run. -- 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]
