Vamana build improvement and added docs #558
Merged
Conversation
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
7 tasks
bkarsin
added
non-breaking
Member
|
/ok to test |
bkarsin
force-pushed
the
vamana_cleanup
branch
from
e4b557d to
f125b6d
Compare
bkarsin
force-pushed
the
vamana_cleanup
branch
from
f125b6d to
61819c5
Compare
cjnolet
reviewed
Jan 28, 2025
docs/source/indexes/vamana.rst
Outdated
| Vamana builds a graph that is stored in device memory. However, in order to serialize the index and write it to a file for later use, it must be moved into host memory. If the `include_dataset` parameter is also set, then the dataset must be resident in host memory when calling serialize as well. | ||
|
|
||
| Device memory usage | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
Member
There was a problem hiding this comment.
I'm surprised the docs built without error. I'm sure there's a warning in there about having the underlining for headings be longer the text.
Contributor
Author
There was a problem hiding this comment.
Fixed all the DiskANN/vamana docs.
cjnolet
reviewed
Jan 28, 2025
docs/source/indexes/vamana.rst
Outdated
|
|
||
| The built index represents the graph as fixed degree, storing a total of :math:`graph_degree * n_index_vectors` edges. Graph construction also requires the dataset be in device memory (or it copies it to device during build). In addition, device memory is used during construction to sort and create the reverse edges. Thus, the amount of device memory needed depends on the dataset itself, but it is bounded by a maximum sum of: | ||
|
|
||
| - vector dataset: :math:`n_index_vectors * n__dims * sizeof(T)` |
Member
There was a problem hiding this comment.
I think you might need to escape all of the _'s in here. Have you checked to see how this formats?
Contributor
Author
There was a problem hiding this comment.
Did a quick scan but didn't look at all the formatting details. Thanks for checking, I'll do a pass over the format of the final built docs.
tfeher
requested changes
Jan 28, 2025
| T* s_coords = reinterpret_cast<T*>(&smem[sort_smem_size]); | ||
| DistPair<IdxT, accT>* new_nbh_list = | ||
| reinterpret_cast<DistPair<IdxT, accT>*>(&smem[dim * sizeof(T) + sort_smem_size]); | ||
| int align_padding = (((dim - 1) / alignof(ShmemLayout)) + 1) * alignof(ShmemLayout) - dim; |
Contributor
There was a problem hiding this comment.
Suggested change
| int align_padding = (((dim - 1) / alignof(ShmemLayout)) + 1) * alignof(ShmemLayout) - dim; | |
| // #include <raft/util/cuda_dev_essentials.cuh> | |
| int align_padding = raft::alignTo<int>(dim, alignof(ShmemLayout)) - dim; |
bkarsin
force-pushed
the
vamana_cleanup
branch
from
8d00af5 to
c14e002
Compare
Member
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Includes several fixes and improvements to Vamana, primarily:
The edge case fix adds padding to all shared memory size and offset calculations so any dataset dimension is supported (tests added that verify this). A bug was also fixed with the L2 distance metric causing incorrect results in some rare cases.
This PR addresses the most pressing items in #393 and stabilize the index construction sufficiently to remove the experimental namespace.