Repository navigation
Conversation
LHT129
left a comment
There was a problem hiding this comment.
PR Review Summary
This PR backports PiPNN parallel graph construction and adaptive alpha pruning from main to the 1.0 branch (4620 additions, 109 deletions across 45 files).
What Changed
- PiPNN graph builder (
src/impl/pipnn/): New batch graph construction pipeline using direction-hash partitioning, BLAS-based distance computation (Sgemm/Ssyrk), and reservoir-based candidate management with adaptive pruning. This enables significantly faster index construction at high thread counts compared to ODescent. - HGraph integration (
src/algorithm/hgraph/): Addedbuild_by_pipnnandbuild_by_batch_graphentry points, parameter parsing/validation, and batch insert optimization for contiguous IDs. - ODescent parallelization (
src/impl/odescent/): Parallelizedadd_reverse_edgesandcount_in_edgesusing SafeThreadPool. - Infrastructure: Added
SsyrkBLAS wrapper,ParallelBatchInsertVectorhelper, batch scalar quantization encoding,IsFiniteFloatBitsutility, andIsNumber()JSON method. - Eval tooling: Rewrote
MemoryPeakMonitorwith background sampling thread for accurate peak RSS tracking; added CPU time tracking toDurationMonitor. - Tests: New
hgraph_pipnn_test.cpp(448 lines),pipnn_graph_builder_test.cpp(534 lines),pipnn_adaptive_pruning_test.cpp(280 lines), plus extensive additions totest_hgraph.cppandtest_hgraph_rabitq_split.cpp. - Docs: PiPNN parameter documentation in English and Chinese, including the new
pipnn_parallel.yamlbenchmark config.
Overall Assessment
The code quality is high. The PiPNN implementation is well-structured with clear separation between the pipeline, partition strategy, and pruning policy. Thread safety is handled carefully with SafeThreadPool, per-point locking, and proper atomic/barrier usage. The test coverage is thorough across unit, integration, and end-to-end levels.
Issues Found (see inline comments for details)
-
std::isfiniteunder-Ofast(3 locations inpipnn_graph_builder.cpp):sanitize_distance,robust_prune/reservoir insertion, andprepare_sketchescosine normalization all usestd::isfinite, which is unreliable when compiled with-ffast-math. Should useIsFiniteFloatBitsfromfloat_utils.hinstead. -
Implicit coupling in
build_by_batch_graph: Theall_rows_are_validguard and batch insertion lambdas are implicitly coupled — a comment or assertion would help future readers. -
[=]capture inParallelBatchInsertVector: The default-by-value capture copies theFlattenInterfacePtrshared_ptr for every batch. Explicit captures would be clearer. -
MemoryPeakMonitorthread lifecycle: TheStart()/Stop()pair is not exception-safe by itself; callers must ensureStop()runs on all paths. Acceptable for eval tooling but worth noting. -
Parallel
add_reverse_edgesthread safety: Relies on graph implementation internal locking — a brief comment would clarify the contract.
Recommendation
The issues are minor and none are blocking. The std::isfinite → IsFiniteFloatBits change is the most impactful fix, as it affects correctness under common -Ofast builds. Everything else is code clarity / maintainability. This PR is ready to merge after addressing the std::isfinite issue.
a6b5559 to
154a30f
Compare
LHT129
left a comment
There was a problem hiding this comment.
PR Review Summary
This is a comprehensive backport of parallel PiPNN graph construction and adaptive alpha pruning from main to the 1.0 branch. The PR has been thoroughly reviewed (43 inline comments, all resolved) and is in good shape.
What this PR does
- Adds PiPNN as a new graph construction algorithm for HGraph indexes (
graph_type: "pipnn") - Backports adaptive alpha pruning (PR #2933) for PiPNN final reservoir pruning
- Adds parallel batch insertion utilities for flatten datacells
- Fixes entry point duplicate label expansion in both basic and parallel searchers
- Adds BLAS
Ssyrkwrapper for symmetric rank-k operations - Adds
IsFiniteFloatBitsutility for NaN/Inf detection under-Ofast
Review dimensions
Correctness: All -Ofast hazards have been addressed — IsFiniteFloatBits is used consistently throughout the PiPNN pipeline. The searcher fix for entry point duplicate label expansion is correct and symmetric between BasicSearcher and ParallelSearcher. The entry_point_id_ fallback to inner_ids.front() is properly guarded.
Style: Consistent with the codebase conventions — 4-space indent, vsag namespace, .cpp suffix, uint64_t over size_t. New files follow the Apache 2.0 license header pattern.
Tests: Comprehensive test coverage including:
- Unit tests for the PiPNN graph builder (determinism, invariants, duplicate handling, submission failure recovery)
- Unit tests for adaptive pruning (relaxation, tightening, zero-step equivalence)
- Integration tests for HGraph PiPNN (all metrics, RaBitQ+SQ8, serialization, extra info, attribute filtering)
- Existing HGraph tests parametrized for PiPNN via
HGRAPH_PR_DAILY_CASE_WITH_PIPNN
Performance: The parallel batch insertion avoids per-vector EncodeOne + SetVector overhead. The route graph ODescent construction copies parameters per iteration (faithful backport, author confirmed this is intentional for diff-comparability with main).
Security: No new security concerns. Input validation is thorough (parameter bounds, metric compatibility, data type checks).
Maintainability: Clear backport comments throughout explain where 1.0 diverges from main (no fused RaBitQ datacell, no validate_add_data(), no deduplicate_storage). The build_by_batch_graph refactoring cleanly shares orchestration between ODescent and PiPNN paths.
Verdict
The PR is well-structured, thoroughly tested, and all previously raised issues have been addressed. Ready to merge.
bd8e9a0 to
c32579f
Compare
|
Jev-driven classification Applied: Option probabilities
Model: |
LHT129
left a comment
There was a problem hiding this comment.
I have completed a thorough review of this backport PR. The code quality is high, and the 30 existing review comments from the earlier review round have addressed all the substantive issues I would have raised.
Summary of what was reviewed:
src/impl/pipnn/— PiPNN graph builder, adaptive pruning policy, and their unit tests (1523 + 125 + 87 + 49 lines of new implementation code)src/algorithm/hgraph/hgraph_build.cpp— HGraph integration: PiPNN dispatch, batch insert paths, route-graph ODescent constructionsrc/algorithm/hgraph/hgraph_parameter.cpp/.h— Parameter parsing, validation, JSON round-trip, compatibility checkssrc/impl/searcher/basic_searcher.cpp/parallel_searcher.cpp— Entry-point duplicate label expansion fixsrc/utils/float_utils.h—IsFiniteFloatBitsfor-Ofastsafetysrc/datacell/flatten_datacell.h— Contiguous batch insertion fast pathsrc/datacell/flatten_build_utils.h—parallel_batch_insert_vectortemplatesrc/impl/odescent/odescent_graph_builder.cpp— Parallelizedadd_reverse_edgesandcount_in_edgessrc/impl/blas/—SsyrkBLAS wrapper and testsrc/quantization/scalar_quantization/—EncodeBatchrefactortools/eval/monitor/— Background-sampling memory peak monitor and CPU-aware duration monitor- Documentation (EN/ZH), eval configs, and test files
Key observations (all already addressed in existing threads):
IsFiniteFloatBitscorrectly replacesstd::isfinitethroughout — critical for-Ofastbuilds- The
insert_candidatehash-bucket early-return is intentional and documented add_reverse_edgesparallelization uses atomicfetch_addfor disjoint slot allocation — thread-safeMemoryPeakMonitordestructor lifetime contract is documentedbatch_insert_flatten_codescoupling betweentotalandinner_idssize is explicitly asserted- Route-graph ODescent parameter copy is a known tradeoff (small struct, few iterations)
entry_point_id_fallback toinner_ids.front()is guarded by thenot inner_ids.empty()check
No new blocking issues found. The backport is clean, well-documented, and the 1.0 adaptations are clearly explained in both code comments and the PR description. The test coverage is thorough (19 PiPNN unit test cases, 32 HGraph unit test cases, 4 rabitq_split cases, all passing).
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
I have completed a thorough review of this PR which backports parallel PiPNN graph construction and adaptive alpha pruning from main to the 1.0 release branch (46 files, +4844/-109).
Overall Assessment
The code quality is high. The PiPNN implementation is well-structured with clear separation of concerns (pipeline stages, partitioning, leaf building, pruning). The parallelization patterns (atomic work-stealing, pre-sized reverse edges with disjoint slots) are correct and well-documented. The -Ofast safety issue with std::isfinite has been properly addressed via IsFiniteFloatBits.
Key Observations
-
sanitize_distanceis the single choke point for distance validation — all distances round-trip through BF16 order keys, so fixingIsFiniteFloatBitshere covers the reservoir insertion and pruning paths. Confirmed no remainingstd::isfinitecalls inpipnn_graph_builder.cpp. -
batch_insert_flatten_codesnow has aCHECK_ARGUMENTthat tiesbatch_counttoinner_ids.size(), and all callers are guarded byall_rows_are_valid. The coupling is explicit rather than implicit. -
add_reverse_edgesparallelization uses atomicfetch_addto hand each writer a disjoint slot in pre-sized rows — no per-node locking required. The safety argument is documented in a comment. -
EncodeBatchImplallocates thenormalizedvector only whenmetric == COSINE(guarded byif constexpr), avoiding unnecessary allocation for L2/IP metrics. -
entry_point_id_assignment is guarded bynot inner_ids.empty(). -
Test coverage is comprehensive: unit tests for PiPNN build, continue-add, factor, remove, duplicate-vector search, serialization, and RaBitQ split datacell integration.
Prior Review Comments
LHT129 has already provided extensive review feedback. All correctness issues have been addressed by the author. The remaining open items are performance suggestions (route graph make_shared hoisting, work-stealing batch size, power-of-2 assertion) that do not affect correctness.
No new substantive issues found.
Backport main PR antgroup#2858, landed commit 65e3ba3, onto 1.0. Add the opt-in PiPNN batch graph builder and wire it into HGraph as `graph_type: "pipnn"`, including the `pipnn_*` build parameters, the `GRAPH_TYPE_PIPNN` constant, parameter mapping and validation, and the `build_by_pipnn` dispatch in the build path. PiPNN builds the whole graph in one batch pass instead of incremental insertion; `Add()` delegates the first batch to `Build()` under a dedicated mutex. Also expand the entry point's duplicate labels in both searchers. The entry point is marked visited before its neighbours are expanded, so its own duplicates were unreachable from the traversal loop. 1.0 adaptations: - Reuse the existing `external_mapping` parameter table instead of main's later `build_default_hgraph_inner_param` refactor. - Keep 1.0's cache, MCI and Training control flow; drop main-only `try_optimized_build`, `deduplicate_storage`, and fused-RaBitQ publish hooks that do not exist on 1.0. - Write the contiguous batch-insert run through 1.0's flat `io_->Write` instead of main's layout-aware `WriteRange`. - Add `JsonWrapper::IsNumber()` (main parity) and keep 1.0's single `is_result_distance_eligible` overload. - Drop tests that depend on 1.0-absent engine support: fused RaBitQ datacell, deduplicated storage, and query-computer accounting. Pyramid PiPNN from the same upstream commit is intentionally not included: on 1.0 its prerequisites (path store, build cache, IndexNode routing graphs, source-id persistence) are absent. HGraph PiPNN only. Verified on the 1.0 baseline: the library builds clean, and the PiPNN unit tests (18 cases, 31412 assertions) and HGraph tests (30 cases, 1186 assertions) all pass. The full suite goes from 610 to 629 cases with the same two pre-existing `non-finite-distance bridge` searcher failures present on an unmodified 1.0 tree. Signed-off-by: 庄叶 <zhuangye.yxw@antgroup.com>
Backport main PR antgroup#3049, landed commit dee3bd7, onto 1.0 after the PiPNN graph construction backport. Add `pipnn_adaptive_pruning` and `pipnn_adaptive_pruning_adjust_step`, off by default. When enabled, the per-point count-based alpha schedule from antgroup#2933 is applied to PiPNN's final reservoir pruning: accepted and scanned-reject counts from one pass select an alpha adjustment for the next, so no rejected neighbor is appended unconditionally. The policy and step are serialized and compared on restore; missing fields mean disabled and the disabled path keeps legacy adjacency. L2 only, initial `Build` only. 1.0 adaptations: - Extend 1.0's `external_mapping` table with the two new keys rather than adopting main's refactored parameter router. - Keep the PiPNN compatibility checks; drop the main-only `rabitq_fused_datacell` field and the HGraph conjugate-graph guard, neither of which exists on 1.0. - Add `src/utils/float_utils.h` (main parity) so the non-finite checks survive `-Ofast`, where `std::isfinite` would fold away. - Restore `tests/test_hgraph_rabitq_split.cpp` to 1.0's three tests and append only the PiPNN case. The main branch had grown that file to 22 cases of unrelated drift (fused RaBitQ, MRLE, rerank statistics) that 1.0 cannot run; keeping them was a real regression. Verified on the 1.0 baseline: the library and both test binaries build clean. PiPNN unit tests pass (19 cases, 33469 assertions), HGraph unit tests pass (32 cases, 1663 assertions), and `[rabitq_split][hgraph]` passes 4/4 including the PiPNN case. The full unit suite is 640 cases with only the same two pre-existing `non-finite-distance bridge` failures that an unmodified 1.0 tree also has. Signed-off-by: 庄叶 <zhuangye.yxw@antgroup.com>
- Replace std::isfinite with IsFiniteFloatBits in the PiPNN builder: under -Ofast (-ffast-math) std::isfinite can fold to true, silently dropping the non-finite guards in sanitize_distance, the cosine normalization in prepare_sketches, and the leader_sample_rate/alpha parameter checks. - Make the all_rows_are_valid -> inner_ids coupling explicit with a documented invariant check in batch_insert_flatten_codes. - Capture the batch-insert lambda explicitly instead of [=]. - Document the intentional early return in the reservoir hash-bucket lookup, the parallel reverse-edge fill safety argument, and the MemoryPeakMonitor Start/Stop lifetime contract. - Apply clang-format-15 to the touched files. Signed-off-by: 庄叶 <zhuangye.yxw@antgroup.com>
…l buffer The destructor deallocated data_ unconditionally. A failed Allocate already throws from the constructor, so the object never finishes construction and the destructor cannot observe a null data_ today; the guard makes the class invariant explicit and keeps the destructor safe if construction is ever restructured. Signed-off-by: 庄叶 <zhuangye.yxw@antgroup.com>
- Note that the IP branch of distance_from_dot must return before the norm lookups because norms_ is never allocated for IP. - Document that group_duplicate_rows deduplicates bitwise on raw row bytes, so -0.0f and +0.0f are intentionally distinct. - Document that odescent_param_->max_degree is mutated in place by the bottom graph build, which is pre-existing 1.0 behaviour. No behaviour change. Signed-off-by: 庄叶 <zhuangye.yxw@antgroup.com>
The ASan Build X86 job lints the changed .cpp files with clang-tidy 15 and treats diagnostics as errors. main carries these identifiers in CamelCase, but this branch enforces FunctionCase=lower_case and StructCase=lower_case, so the ported code failed the gate with 4 struct and 2 function/constant diagnostics. Rename the file-local helpers and types to match the branch conventions: ReservoirEntry/ReservoirState/WorkItem/SplitResult -> lower_case ParallelBatchInsertVector -> parallel_batch_insert_vector BATCH_SIZE -> batch_size No behaviour change. All changed .cpp files now pass clang-tidy 15 with the same -p=<builddir> invocation the ASan job uses. Signed-off-by: 庄叶 <zhuangye.yxw@antgroup.com>
c32579f to
5ea2dfc
Compare
| } | ||
| if (graph_type == GRAPH_TYPE_VALUE_PIPNN) { | ||
| pipnn_param.FromJson(graph_json); | ||
| pipnn_param.alpha = this->alpha; |
There was a problem hiding this comment.
[suggestion] The pipnn_param.alpha = this->alpha; assignment in FromJson is overwritten at build time: build_by_batch_graph copies pipnn_param_ into a local pipnn_parameter and reassigns pipnn_parameter.alpha = this->alpha_; before use. The FromJson assignment only matters for ToJson() serialization. Consider either removing the FromJson assignment and letting build_by_batch_graph be the sole source of truth, or adding a comment explaining the dual-assignment intent.
| in_edges_count[neighbor.id]++; | ||
| if (thread_pool_ != nullptr and data_num_ >= 4096) { | ||
| Vector<std::atomic<int>> parallel_counts(static_cast<uint64_t>(data_num_), allocator_); | ||
| auto initialize_counts = [&](int64_t start, int64_t end) { |
There was a problem hiding this comment.
[note] count_in_edges uses Vector<std::atomic<int>> for the parallel count accumulator, while add_reverse_edges uses Vector<std::atomic<uint32_t>>. Both are functionally correct for practical graph sizes, but the signed int could theoretically overflow (UB) if data_num_ * max_degree exceeds INT_MAX. Using uint32_t would be consistent with add_reverse_edges and avoids the theoretical signed-overflow concern.
|
|
||
| auto allocate_rows = [&](int64_t start, int64_t end) { | ||
| for (int64_t i = start; i < end; ++i) { | ||
| reverse_graph[i].neighbors.resize( |
There was a problem hiding this comment.
[note] In the allocate_rows phase, reverse_counts[i].exchange(0, std::memory_order_relaxed) is used to reset the counter and size the reverse adjacency row. The return value of exchange is discarded — store(0, std::memory_order_relaxed) followed by load() (or just using the value already in resize) would be semantically clearer. The current form works correctly but reads as if the old value is needed when it is not.
| reverse_graph[node.id].neighbors.emplace_back(i, node.distance); | ||
| if (thread_pool_ != nullptr and data_num_ >= 4096) { | ||
| Vector<std::atomic<uint32_t>> reverse_counts(static_cast<uint64_t>(data_num_), allocator_); | ||
| auto initialize_counts = [&](int64_t start, int64_t end) { |
There was a problem hiding this comment.
[note] The Vector<std::atomic<uint32_t>> reverse_counts(data_num_, allocator_) constructor value-initializes all elements to 0, so the subsequent initialize_counts parallel task (lines 184-189) writes 0 to already-zero atomics. This is a minor double-initialization. If the intent is to ensure a known state before the parallel count_edges phase, a comment would clarify; otherwise the initialize_counts phase could be removed. The same pattern appears in count_in_edges (line 333).
| } | ||
| }; | ||
| parallelize_task(initialize_counts); | ||
|
|
There was a problem hiding this comment.
[suggestion] The initialize_counts phase in both count_in_edges() and add_reverse_edges() is redundant: std::vector<std::atomic<T>>(n, alloc) value-initializes every element to zero (atomic value-initialization performs zero-initialization per [atomics.types.operations]/2). The explicit store(0, memory_order_relaxed) loop adds a full parallel phase with no effect. Removing it saves one parallelize_task barrier per call.
In add_reverse_edges, the three remaining phases (count → allocate → fill) are still correctly ordered because each parallelize_task drains all futures before returning.
| parallel_counts[i].store(0, std::memory_order_relaxed); | ||
| } | ||
| }; | ||
| parallelize_task(initialize_counts); |
There was a problem hiding this comment.
[suggestion] Same redundant initialize_counts as in add_reverse_edges: Vector<std::atomic<int>>(n, alloc) already zero-initializes all elements. The explicit store(0, memory_order_relaxed) loop is unnecessary.
| CHECK_FIELD_EQ(*this, *p, support_force_remove); | ||
| CHECK_FIELD_EQ(*this, *p, pipnn_param.adaptive_pruning); | ||
| if (this->pipnn_param.adaptive_pruning) { | ||
| CHECK_FIELD_EQ(*this, *p, graph_type); |
There was a problem hiding this comment.
[suggestion] CheckCompatibility only validates graph_type when adaptive_pruning is enabled. When PiPNN is used without adaptive pruning, a PiPNN-built index could pass compatibility checks against a non-PiPNN config. The graph_type check should be unconditional for any PiPNN index, since the graph structure is fundamentally different from NSW/ODescent.
Suggested fix: move CHECK_FIELD_EQ(*this, *p, graph_type) outside the if (this->pipnn_param.adaptive_pruning) block, or add a separate unconditional check for graph_type.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This is a large backport (+4733/-109, 46 files) that brings PiPNN parallel graph construction and adaptive alpha pruning from main to the 1.0 release branch. Overall the implementation quality is high — the -Ofast/std::isfinite issue has been addressed with IsFiniteFloatBits, thread safety is well-considered, and the existing review comments have been resolved.
Key Findings
1. flatten_datacell.h — EncodeBatch return value ignored
In the contiguous-ID fast path of BatchInsertVector, quantizer_->EncodeBatch(...) returns bool but the return value is discarded. If encoding fails (e.g., due to invalid input data), invalid codes are silently written to IO. The fallback path (per-row InsertVector) also ignores the return value, so this is a pre-existing pattern, but worth noting since the batch path writes a larger block of data in one shot.
2. hgraph_parameter.cpp — bottom_graph_param null safety
When graph_type == GRAPH_TYPE_VALUE_PIPNN, the code calls pipnn_param.Validate(this->bottom_graph_param->max_degree_) without an explicit null check on bottom_graph_param. In practice it is always initialized by this point, but a debug assertion would make the invariant explicit.
3. hgraph_build.cpp — Route graph ODescentParameter copy per iteration
When use_pipnn is true, current_route_odescent_param is created via std::make_shared<ODescentParameter>(*route_odescent_param) on every iteration of the route-graph loop. The only per-iteration mutation is block_size (and max_degree, which is set identically each time). The allocation could be hoisted outside the loop. (Already flagged by LHT129, acknowledged.)
4. Test coverage is adequate for the backport scope
hgraph_pipnn_test.cpp covers: all supported metrics (L2/IP/Cosine), parameter parsing/serialization, adaptive pruning validation, duplicate handling, and incremental Add after Build. The existing test_hgraph.cpp diff adds PiPNN variants of existing test cases. This is reasonable coverage for a backport.
5. No security concerns — the changes are purely algorithmic and data-structure additions with no new external input parsing beyond the existing JSON parameter framework.
Verdict
Requesting changes for the EncodeBatch return value issue (item 1). The other items are suggestions that can be addressed at the author's discretion.
| allocator_); | ||
| quantizer_->EncodeBatch(static_cast<const float*>(vectors), codes.data, count); | ||
| std::lock_guard lock(mutex_); | ||
| total_count_ = std::max(total_count_, idx_vec[0] + count); |
There was a problem hiding this comment.
[note] quantizer_->EncodeBatch() returns bool but the return value is discarded here. If encoding fails, invalid codes are silently written to IO via io_->Write() on the next line. The per-row fallback path (InsertVector loop below) has the same pattern, so this is consistent with existing code, but the batch path writes a larger block in one shot — a failed encode is more impactful. Consider at least logging a warning on failure, or propagating the error.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This is a large backport (+4733/-109, 46 files) that brings PiPNN parallel graph construction and adaptive alpha pruning from main to the 1.0 release branch. Overall the implementation quality is high — the -Ofast/std::isfinite issue has been addressed with IsFiniteFloatBits, thread safety is well-considered, and the existing review comments have been resolved.
Key Findings
1. flatten_datacell.h — EncodeBatch return value ignored
In the contiguous-ID fast path of BatchInsertVector, quantizer_->EncodeBatch(...) returns bool but the return value is discarded. If encoding fails (e.g., due to invalid input data), invalid codes are silently written to IO. The fallback path (per-row InsertVector) also ignores the return value, so this is a pre-existing pattern, but worth noting since the batch path writes a larger block of data in one shot.
2. hgraph_parameter.cpp — bottom_graph_param null safety
When graph_type == GRAPH_TYPE_VALUE_PIPNN, the code calls pipnn_param.Validate(this->bottom_graph_param->max_degree_) without an explicit null check on bottom_graph_param. In practice it is always initialized by this point, but a debug assertion would make the invariant explicit.
3. hgraph_build.cpp — Route graph ODescentParameter copy per iteration
When use_pipnn is true, current_route_odescent_param is created via std::make_shared<ODescentParameter>(*route_odescent_param) on every iteration of the route-graph loop. The only per-iteration mutation is block_size (and max_degree, which is set identically each time). The allocation could be hoisted outside the loop. (Already flagged by LHT129, acknowledged.)
4. Test coverage is adequate for the backport scope
hgraph_pipnn_test.cpp covers: all supported metrics (L2/IP/Cosine), parameter parsing/serialization, adaptive pruning validation, duplicate handling, and incremental Add after Build. The existing test_hgraph.cpp diff adds PiPNN variants of existing test cases. This is reasonable coverage for a backport.
5. No security concerns — the changes are purely algorithmic and data-structure additions with no new external input parsing beyond the existing JSON parameter framework.
Verdict
Requesting changes for the EncodeBatch return value issue (item 1). The other items are suggestions that can be addressed at the author's discretion.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This is a large backport (+4733/-109, 46 files) that brings PiPNN parallel graph construction and adaptive alpha pruning from main to the 1.0 release branch. Overall the implementation quality is high.
Key Findings
1. flatten_datacell.h — EncodeBatch return value ignored
In the contiguous-ID fast path of BatchInsertVector, quantizer_->EncodeBatch(...) returns bool but the return value is discarded. If encoding fails, invalid codes are silently written to IO. The fallback path also ignores the return value, so this is a pre-existing pattern, but the batch path writes a larger block in one shot.
2. hgraph_parameter.cpp — bottom_graph_param null safety
When graph_type == GRAPH_TYPE_VALUE_PIPNN, the code calls pipnn_param.Validate(this->bottom_graph_param->max_degree_) without an explicit null check on bottom_graph_param. A debug assertion would make the invariant explicit.
3. hgraph_build.cpp — Route graph ODescentParameter copy per iteration
When use_pipnn is true, current_route_odescent_param is created via std::make_shared<ODescentParameter>(*route_odescent_param) on every iteration. The allocation could be hoisted outside the loop. (Already flagged by LHT129.)
4. Test coverage is adequate — hgraph_pipnn_test.cpp covers all supported metrics, parameter parsing, adaptive pruning, duplicates, and incremental Add after Build.
5. No security concerns.
Verdict
Requesting changes for the EncodeBatch return value issue (item 1). The other items are suggestions.
Summary
Backports the PiPNN feature from
mainonto the1.0release branch, following the established backport pattern on this branch (e.g. #3079, #3076).Two upstream commits are ported:
65e3ba36graph_type: "pipnn")dee3bd71Scope: HGraph only
Upstream PiPNN supports both HGraph and Pyramid. This backport intentionally covers HGraph only.
On
1.0the Pyramid PiPNN path depends on infrastructure that does not exist on this branch —PyramidPathStore(multi-path storage),PyramidBuildCache,IndexNoderouting graphs /GraphBuildFunc, source-ID persistence, andoptimized_build_codes_. Porting Pyramid PiPNN would mean backporting roughly six additional, later feature commits first. The docs state the HGraph-only boundary explicitly in both English and Chinese.What is included
src/impl/pipnn/— the PiPNN batch graph builder plus the adaptive-pruning policy and their unit tests.GRAPH_TYPE_PIPNN,build_by_pipnndispatch, thepipnn_*build parameters, parameter mapping/validation/JSON round-trip, and theAdd()hand-off that routes the first batch throughBuild().alphaon HGraph;JsonWrapper::IsNumber()(main parity);src/utils/float_utils.hso the non-finite checks survive-Ofast, wherestd::isfinitefolds away.tools/eval/pipnn_parallel.yamleval config, and build/monitor output tweaks.1.0 adaptations (differences from upstream)
external_mappingparameter table instead of upstream's laterbuild_default_hgraph_inner_paramrefactor.1.0's cache/MCI/Training control flow; drops upstream-onlytry_optimized_build(),deduplicate_storage, and the fused-RaBitQ publish hooks, none of which exist here.1.0's flatio_->Writeinstead of upstream's layout-awareWriteRange.1.0's single non-templateis_result_distance_eligible; drops upstream'suse_custom_distanceconjunct.CheckCompatibility(drops the upstream-onlyrabitq_fused_datacellfield and the HGraph conjugate-graph guard).Tests that depend on engine support absent from
1.0are dropped rather than stubbed — specifically the fused RaBitQ datacell tests, deduplicated-storage tests, and query-computer accounting tests.tests/test_hgraph_rabitq_split.cppis restored to1.0's three tests with only the PiPNN case appended.mainhad grown that file to 22 cases of unrelated drift (fused RaBitQ, MRLE, rerank statistics) that cannot run on1.0; carrying them over produced four real failures locally, so they are excluded.Testing
Built and tested on the
1.0baseline (6c4d297d) with GCC 11.4, Release,-Ofast:libvsag_static,unittests,functests).[rabitq_split][hgraph]: 4/4 pass, including the PiPNN case over the split RaBitQ datacell.Regression baseline — the two failures below are pre-existing on an unmodified
1.0tree and are not caused by this backport. I verified this by buildingorigin/1.0at6c4d297din a separate worktree and running it:1.0basic_searcher_test.cpp— non-finite-distance bridgeparallel_searcher_test.cpp— non-finite-distance bridgeBoth pre-existing failures come from
maincommit1527d13c(threshold filtering), which added those tests but was never backported to1.0— the tests are present on1.0without the accompanying implementation fix.