Revert pruning PRs#331
Draft
ethanglaser wants to merge 2 commits into
Draft
Conversation
This reverts commit 629a79c.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revert #282 and #327 to restore green CI for VecSim tests (via Claude)
Problem
Private CI
build-share-lib.ymlworkflow'stest-shared-libsjobs (VecSim tests) — reduced, gcc-11, and gcc-14 on rockylinux8 — started failing between Apr 29 (last green:0ce34e1) and May 6 (first red:8050bec).Failing tests
SVSTest.quant_modesSVSTest.save_loadfrom
VectorSimilarity/tests/unit/test_svs.cpp:2975, with assertions like:Root cause
The
test-shared-libs(reduced) job clonesRedisAI/VectorSimilarityand runs its test suite against the SVS shared library. The failure window coincided with SVS commit629a79c— "Enhance heuristic pruning to handle duplicate clusters (#282)" — which added a post-pruning step to bothIterativePruneStrategyandProgressivePruneStrategyininclude/svs/index/vamana/prune.h:On the VectorSimilarity test fixture (
v[i] = (i,i,i,i), dim=4, n=100), every node has symmetric pairs of equidistant neighbors (e.g. nodesi-1andi+1both atd²=4from nodei). These look like "duplicate clusters" to the heuristic, but geometrically they are symmetric pairs in opposite directions, not redundant edges.The post-prune step:
Result: the constructed Vamana graph is no longer traversable to certain neighbors for certain queries. For
query = (50,50,50,50), the exact top-10 by ID ([45..54]) can't all be reached; search returns55in place of54.Attempted fixes
Attempt 1 —
9cee0f1(#327, "Fix duplicate-cluster trigger to require >=2 kept candidates")Tightened the guard from
!result.empty()toresult.size() >= 2. Correctly prevented the branch from firing when only one neighbor was retained, but did not address the core bug: the heuristic still overwrites a genuine nearest edge with a farther candidate when 2+ retained neighbors tie in distance.Result: the
test-shared-libsfailures persisted.Attempt 2 — append-only variant
Branch:
dev/eglaser-sharedci-fixChanged both strategies in
include/svs/index/vamana/prune.h:result.back() = ...→result.push_back(...)(append instead of overwrite).result.size() < max_result_sizeguard (only fire when there's room).Rationale: if every kept neighbor ties at one distance, each one is a legitimate nearest edge and must not be replaced. Append a diversity edge only when capacity allows.
Result: the failure pattern changed but the test still failed:
[45..53, 55][46..55]The same graph-asymmetry problem reappeared on the opposite side because the appended "diversity" candidate is still systematically biased (always the first pool entry past the tie band, which stably falls on one side of the symmetric ring). The heuristic gains an asymmetric edge toward lower IDs instead of losing an asymmetric edge toward higher IDs.
Conclusion
Both #327 and the append-only variant confirm that the diversity heuristic's detection criterion is fundamentally wrong: it uses distance equality to infer directional redundancy, but same-distance neighbors can be (and in symmetric data, typically are) in opposite directions.
No local tweak to the guard or the insertion method fixes this — the criterion itself needs redesign to detect directional collapse rather than distance collapse. That's a nontrivial design task and out of scope for an immediate unblock.
This PR
Reverts:
629a79c) — Enhance heuristic pruning to handle duplicate clusters9cee0f1) — Fix duplicate-cluster trigger to require >=2 kept candidates (only exists to patch Enhance heuristic pruning to handle duplicate clusters #282)This restores the pre-April pruning behavior, which restores green CI for VecSim tests.
Follow-up
Issue #80 (the original motivation for #282) should be reopened with a note that the fix needs to detect directional rather than distance redundancy.