Open
Conversation
hailangx
changed the title
…to 'cosine' The default distance metric was changed from L2 to Cosine in RespServerSessionVectors.cs, but the VINFO test assertions were not updated to match. This caused all Garnet.test CI jobs to fail.
hailangx
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request bumps the diskann-garnet package from version 1.0.20 to 1.0.23 and includes a crash fix along with changes to default vector set parameters. The crash fix adds validation to prevent using an invalid index when CreateIndex fails, while the default parameter changes update the quantization type from Q8 to NoQuant and the distance metric from L2 to Cosine.
Changes:
- Bumped diskann-garnet package version from 1.0.20 to 1.0.23
- Added validation check for CreateIndex failure (returns 0) with proper cleanup and error handling
- Changed default vector quantization type from Q8 to NoQuant and default distance metric from L2 to Cosine
- Updated test assertions to reflect the new default distance metric (from "l2" to "cosine")
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Directory.Packages.props | Bumped diskann-garnet package from version 1.0.20 to 1.0.23 |
| libs/server/Resp/Vector/VectorManager.Locking.cs | Added validation check for CreateIndex failure with cleanup logic and BADSTATE error return |
| libs/server/Resp/Vector/RespServerSessionVectors.cs | Changed default quantType from Q8 to NoQuant and default distanceMetric from L2 to Cosine |
| test/Garnet.test/RespVectorSetTests.cs | Updated test assertions to expect "cosine" instead of "l2" for distance metric |
Comments suppressed due to low confidence (1)
libs/server/Resp/Vector/VectorManager.Locking.cs:356
- The condition check at line 353 is redundant. At this point in the code, we are inside the else block that starts at line 325, which means needsRecreate is guaranteed to be false. The check if (!needsRecreate) will always evaluate to true and can be removed. The CleanupDroppedIndex call should always execute in this error path.
if (!needsRecreate)
{
CleanupDroppedIndex(ref ActiveThreadSession.vectorContext, indexContext);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When the DiskANN native CreateIndex returns 0 (null pointer), the server should return an error to the client rather than crashing. This adds: - ExceptionInjectionType.VectorSet_CreateIndex_NullReturn to simulate the failure in DEBUG builds - An injection point in VectorManager.Locking.cs after CreateIndex that drops the real index and forces a 0 return when the condition is set - CreateIndexNullReturnIsHandledGracefully test that verifies: 1. VADD returns an error (not a crash) when CreateIndex fails 2. The key is properly cleaned up (doesn't exist after failure) 3. The server remains healthy (subsequent VADD succeeds)
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.
bumps the diskann-garnet package from version 1.0.20 to 1.0.23 and includes a crash fix along with changes to default vector set parameters.
The crash fix adds validation to prevent using an invalid index when CreateIndex fails, while the default parameter changes update the quantization type from Q8 to NoQuant and the distance metric from L2 to Cosine.