-
Notifications
You must be signed in to change notification settings - Fork 8
Add mmap based storage of vectors, while keeping buffered storage as default #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
PR is pending resolution of the discussion here: facebookresearch/faiss#4274 |
Based on discussion in facebookresearch/faiss#4274, we determined that the reason the memory spikes when storing vectors in CPU memory is because of the CUDA version. This issue is observed with CUDA version >= 12. I've updated this PR to make memory based storage the default, while still giving the caller the option to use disk/mmap based storage as another option. I'll update the |
@rchitale7 fix the commit message and also the PR description to correctly point out what the PR is doing |
…default Signed-off-by: Rohan Chitale <rchital@amazon.com>
missed this, fixed it now. |
@rchitale7 Based on the response here: facebookresearch/faiss#4274 (comment) I think we might not even need this PR. So, I will wait for the bug to be fixed. |
Description
This issue enables the vector and doc id binary downloaded from Remote Store to be saved to disk, and read using mmap. The caller of
run_tasks
can download the vectors to disk if they setstorage_mode
= "disk", but the default storage mode is stillmemory
. I refactored the code so that we can support other storage modes if necessary. Key to the refactoring is theBinarySource
object that wraps the storage mechanism;FileSource
wraps the file object used fornumpy.mmap
, whileBufferSource
wraps the buffer object used fornumpy.frombuffer
.I verified manually that both storage modes work, and create the expected faiss graph.
Issues Resolved
Partially resolves for #49. I need to still update the
USER_GUIDE.md
to specify thatmemory
is the default approach, but with CUDA versions >= 12, we see memory spike. So to avoid memory spike, user can usedisk
(at the expense of slower index builds)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.