Backport PR #2751 to release/v1.7 for Add rust-agent-qbg #2790
Conversation
* add rust-agent-qbg * add unit-test-rust.yaml --------- Signed-off-by: datelier <[email protected]>
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive implementation of a Rust-based QBG (Quantized Binary Graph) library with supporting infrastructure. The changes include adding a new GitHub Actions workflow for Rust unit testing, creating a Rust package for QBG algorithms, implementing C++ interoperability through the Changes
Sequence DiagramsequenceDiagram
participant Rust
participant CxxBridge
participant CPP
participant NGT
Rust->>CxxBridge: Create Property
CxxBridge->>CPP: Initialize QBG Construction Parameters
Rust->>CxxBridge: Configure Property Settings
CxxBridge->>CPP: Set Dimensions, Subvectors
Rust->>CxxBridge: Create Index
CxxBridge->>CPP: Open/Build Index
CPP->>NGT: Interact with NGT Library
Rust->>CxxBridge: Perform Search
CxxBridge->>CPP: Execute Search Operation
CPP->>NGT: Query Index
NGT-->>CPP: Return Search Results
CPP-->>CxxBridge: Provide Search Results
CxxBridge-->>Rust: Return Search Results
Possibly related PRs
Suggested Labels
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying vald with
|
| Latest commit: |
3c8b68c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://18eb5ea8.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-featur-f4u5.vald.pages.dev |
|
[CHATOPS:HELP] ChatOps commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
rust/libs/algorithms/qbg/src/input.cpp (2)
210-225: Parameter validation is missing for path correctness.While opening or creating an index, user-provided file paths may be invalid or malicious. Currently, the code only handles library-level or file-level errors. Consider adding param checks (e.g., preventing empty strings) or sanitizing path inputs to prevent unexpected behaviors.
390-404: Potential dangling pointer fromget_object.
get_objectreturns a raw pointer that references data owned by the underlying C++ library. Storing or modifying this pointer beyond its immediate use can be risky. Consider copying the data or returning a safe wrapper object to avoid unexpected invalidation.rust/libs/algorithms/qbg/src/lib.rs (1)
382-388: Be cautious with slice lifetime in get_object.The returned slice is built from a raw pointer and dimension. If the underlying memory is freed or changed by C++ after this call, the slice can become invalid. Usually, copying into a Rust-owned buffer is safer.
rust/libs/algorithms/qbg/build.rs (1)
16-35: Consider environment detection for library paths.Hardcoding
/usr/local/libmight be limiting on some systems. If possible, consider letting library search paths be set via environment variables or build arguments. This will make the build more portable across diverse environments.rust/libs/algorithms/qbg/src/input.h (1)
92-114: Index class manages raw pointer effectively, but consider RAII-friendly design.The class uses a
void*for the index and frees it using specific library calls. If the code grows more complex or requires safe concurrency, adopting a wrapper or more modern RAII approach can reduce risk of leaks and simplify error handling.rust/libs/algorithms/qbg/Cargo.toml (1)
26-27: Review build dependency configuration.The
miettecrate withfancyfeature seems unnecessary as a build dependency since it's typically used for runtime error reporting. Consider:
- Moving it to regular dependencies if needed for runtime
- Removing it if not actively used in build.rs
.github/workflows/unit-test-rust.yaml (1)
45-51: Enhance the workflow configuration
- Add a newline at the end of file
- Consider adding these improvements:
- Cache Rust dependencies using
actions/cache- Upload test results as artifacts
- Add timeout limits for the test job
Apply this diff to implement the suggestions:
- name: Set Git config run: | git config --global --add safe.directory ${GITHUB_WORKSPACE} + - uses: actions/cache@v3 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + rust/target + key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} - name: Run tests for Rust / gotestfmt run: | TEST_RESULT_DIR=${GITHUB_WORKSPACE} make test/rust/qbg + timeout-minutes: 10 + - name: Upload test results + if: always() + uses: actions/upload-artifact@v3 + with: + name: rust-test-results + path: ${{ github.workspace }}/TEST_RESULT_DIR +🧰 Tools
🪛 yamllint (1.35.1)
[error] 51-51: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/unit-test-rust.yaml(1 hunks)Makefile.d/test.mk(1 hunks)rust/Cargo.toml(1 hunks)rust/libs/algorithm/Cargo.toml(1 hunks)rust/libs/algorithms/qbg/Cargo.toml(1 hunks)rust/libs/algorithms/qbg/build.rs(1 hunks)rust/libs/algorithms/qbg/src/input.cpp(1 hunks)rust/libs/algorithms/qbg/src/input.h(1 hunks)rust/libs/algorithms/qbg/src/lib.rs(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/unit-test-rust.yaml
23-23: could not parse as YAML: yaml: line 23: did not find expected '-' indicator
(syntax-check)
🪛 yamllint (1.35.1)
.github/workflows/unit-test-rust.yaml
[warning] 24-24: wrong indentation: expected 6 but found 5
(indentation)
[error] 25-25: syntax error: expected , but found ''
(syntax)
[error] 51-51: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (14)
rust/libs/algorithms/qbg/src/input.cpp (3)
27-31: Destructor logic looks correct for cleanup.
The destructor consistently releases both qbg_construction_parameters and qbg_build_parameters. Ensure no further usage of these pointers happens outside the destructor to avoid undefined behavior.
43-60: Repeated re-initialization might overwrite prior settings.
Calling qbg_initialize_construction_parameters again in this setter resets all previously set fields, which might be intentional. If partial updates are needed, consider removing the re-initialization or offering a separate method to preserve previously set values.
336-388: Zero search results trigger a runtime error.
Returning zero results is not always an error case. Some queries might not match anything. Verify whether rsize == 0 truly indicates an error or if returning an empty result set is more appropriate. Throwing an exception may block valid use cases where an empty result is acceptable.
rust/libs/algorithms/qbg/src/lib.rs (3)
16-106: FFI declarations look consistent with the C++ header.
The bridging definitions correctly match the C++ method signatures. The usage of Pin<&mut ...> and UniquePtr is appropriate for safe FFI interactions, preventing accidental moves of pinned data.
109-126: Property struct wraps the C++ resource effectively.
The Property struct is storing UniquePtr<ffi::Property> responsibly. Methods like get_property() use pin_mut() correctly to avoid accidental reallocation or moves. Good practice for bridging C++ and Rust.
374-380: Efficient search result mapping.
Mapping the FFI’s search results into a native Rust Vec<(u32, f32)> is a clear approach. This code is easy to maintain and leverages Rust's iteration safely. Good job.
rust/libs/algorithms/qbg/src/input.h (1)
23-30: Struct SearchResult is straightforward and safe.
Returning a simple struct with an ID and distance is understandable. The inline getters are trivial, but effective. This structure is well-defined for its purpose.
Makefile.d/test.mk (1)
287-296: Rust test target clarifies QBG tests well.
The added .PHONY: test/rust/qbg target is explicit about which tests to run for QBG. The removal of the index directory ensures a clean slate for each test phase.
rust/Cargo.toml (1)
24-24: LGTM! Workspace member addition follows conventions.
The new QBG algorithm package is properly integrated into the workspace following the established pattern for algorithm libraries.
rust/libs/algorithm/Cargo.toml (1)
25-25: LGTM! Dependency addition follows conventions.
The QBG dependency is properly configured with consistent versioning and path structure matching other algorithm dependencies.
rust/libs/algorithms/qbg/Cargo.toml (2)
29-29: Add test dependencies for unimplemented functions.
The empty [dev-dependencies] section and PR objectives mentioning unimplemented functions (_as_uint8 and _as_float16) suggest a need for test coverage. Consider adding test dependencies to support implementation of these functions.
Would you like me to suggest appropriate test dependencies and example test cases for the unimplemented functions?
23-23: Verify C++20 compatibility with NGT.
Since this is a backport to release/v1.7 and integrates with NGT v2.3.4, please verify that C++20 feature requirement is compatible with the target environments.
✅ Verification successful
Let me gather more information about NGT's C++ standard requirements.
Let me try a different approach to check NGT's C++ requirements.
Let me check the NGT version requirements and compatibility.
Let me check the NGT version requirements in the codebase.
Let me check the NGT version in the codebase.
Based on the findings, I can now provide a response about C++20 compatibility with NGT v2.3.4:
C++20 requirement is compatible with NGT v2.3.4 target environment
The codebase shows NGT v2.3.5 is used in the main branch, while this PR targets v2.3.4. Both versions are compatible with C++20 as:
- The build scripts explicitly set C++20 flags for NGT compilation
- NGT versions 2.3.x series support modern C++ standards
- The rust-ngt bindings are already configured to use C++20 features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check NGT source for C++ standard requirements
rg -i "c\+\+|standard" $(fd -t f "CMakeLists.txt|\.cmake$")
Length of output: 71796
Script:
#!/bin/bash
# Check NGT source for C++ standard requirements in CMake files
fd -t f "CMakeLists.txt|\.cmake$" | xargs cat | grep -i "c++\|standard"
Length of output: 73
Script:
#!/bin/bash
# Check build files and source for C++ standard requirements
rg -i "std=c\+\+|CMAKE_CXX_STANDARD|CXX_STANDARD"
Length of output: 201
Script:
#!/bin/bash
# Check NGT version requirements
rg -i "ngt.*version|version.*ngt"
Length of output: 66975
Script:
#!/bin/bash
# Check NGT version requirements in code
rg -i "ngt.*version|version.*ngt|NGT_VERSION"
Length of output: 66987
Script:
#!/bin/bash
# Check NGT version requirements in code
fd -t f "NGT_VERSION" | xargs cat
Length of output: 39
.github/workflows/unit-test-rust.yaml (2)
1-16: LGTM!
The license header and workflow name are properly formatted and follow best practices.
38-44: Verify test coverage for Rust QBG implementation
The workflow runs tests via the test/rust/qbg make target. Let's verify the test coverage configuration.
✅ Verification successful
Let me gather more information about the test coverage configuration.
Let me check one more thing about the test coverage configuration.
Test coverage appears adequate but could benefit from documentation
The Rust QBG implementation has comprehensive test coverage with four distinct test cases:
test_ffi_qbg: Tests FFI functionalitytest_ffi_qbg_prebuilt: Tests pre-built FFI functionalitytest_property: Tests property managementtest_index: Tests index operations
The test execution is properly configured in the Makefile target test/rust/qbg with appropriate test filters and output settings. While there's no explicit coverage measurement tool configured (like tarpaulin), the test suite covers the main functional areas:
- FFI interface
- Property management
- Index operations
- Pre-built functionality
Suggestion: Consider adding test coverage measurement and documenting the test strategy in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the make target and test configuration
# Check the make target definition
fd -t f "test.mk" -x cat {} \; | grep -A 5 "test/rust/qbg"
# Check for test files in the Rust QBG implementation
fd -e rs -t f . rust/libs/algorithms/qbg/src -x echo "Test file: {}"
Length of output: 719
Script:
#!/bin/bash
# Check the test configuration in lib.rs
cat rust/libs/algorithms/qbg/src/lib.rs | grep -A 20 "#\[cfg(test)\]"
# Check Cargo.toml for test dependencies and coverage configuration
fd Cargo.toml -x cat {} \; | grep -A 10 "\[dev-dependencies\]"
Length of output: 1500
Script:
#!/bin/bash
# Check for test modules and test cases in lib.rs
rg "fn test_" rust/libs/algorithms/qbg/src/lib.rs -A 1
# Check if there's any tarpaulin or other coverage configuration
fd -t f ".*\.toml$" -x grep -l "coverage\|tarpaulin" {} \;
Length of output: 371
| Property::Property() | ||
| { | ||
| qbg_construction_parameters = new QBGConstructionParameters(); | ||
| qbg_build_parameters = new QBGBuildParameters(); | ||
| qbg_initialize_construction_parameters(qbg_construction_parameters); | ||
| qbg_initialize_build_parameters(qbg_build_parameters); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safer memory ownership or smart pointers for better exception safety.
Currently, raw pointers are allocated and deallocated manually. If an exception occurs during construction, e.g., if new throws before finishing, the destructor may not be called, potentially leaking resources. Using smart pointers (e.g., std::unique_ptr) can improve exception safety and simplify memory management.
kpango
deleted the
backport/release/v1.7/feature/agent/rust-agent-qbg
branch
Description
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Summary by CodeRabbit
qbglibrary.