fix(retriever): stream embeddings to the memmap during encode instead of buffering the whole corpus - #238
Open
AmirF194 wants to merge 1 commit into
Open
Conversation
… of buffering the whole corpus Encoder.encode() appends every batch to a python list and only calls np.concatenate() once all batches are done, so the list-of-batches and the freshly concatenated array are simultaneously resident (a transient ~2x peak over the full corpus). Index_Builder.encode_all() then holds that one fully materialized array as the only copy of the corpus embeddings for the rest of the run: _save_embedding() only writes it to a memmap after encode_all() returns, so --save_embedding provides no relief on the run that actually OOMs, since the process is killed inside encode_all() itself. For the single-GPU, non-SentenceTransformer path, encode_all() now writes each batch straight into a pre-allocated on-disk memmap as it is produced, so peak RAM is O(one batch) instead of O(corpus), independent of --save_embedding. The multi-GPU (DataParallel) and CLIP dual-modal paths still buffer the full corpus and are left as follow-up. Fixes RUC-NLPIR#233
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.
Fixes #233.
Root cause
Encoder.encode()(flashrag/retriever/encoder.py:70-75) appends every batch's embedding array to a python list and only callsnp.concatenate(query_emb, axis=0)once all batches are done, so the list of batches and the freshly concatenated array are simultaneously resident: a transient ~2x peak over the full corpus.Index_Builder.encode_all()(index_builder.py:484-493) then holds that single fully materialized array as the only in-memory copy of the corpus embeddings for the rest of the run._save_embedding()(index_builder.py:472-482), which writes to a memmap, only runs afterencode_all()returns, so it gives no memory relief on the run that actually OOMs: the process is killed insideencode_all()itself, before--save_embeddingever gets a chance to run. That workaround only helps a future re-run skip re-encoding, which the reporter never reaches.Fix
For the single-GPU, non-
SentenceTransformerpath (the reporter's case: bge-base, no multi-GPU),encode_all()now pre-allocates the on-disk memmap (self.embedding_save_path) and writes each batch into it directly as it's produced, so peak RAM is O(one batch) instead of O(corpus), independent of--save_embedding.build_dense_index()skips the redundant post-hoc_save_embedding()call for this path and removes the memmap file afterward when--save_embeddingwasn't requested, keeping the on-disk footprint unchanged from before.The multi-GPU (
DataParallel) path and the CLIP dual-modal path (encode_all_clip) still call the same accumulate-then-concatenate pattern and were left as follow-up: the reporter's repro (and every OOM report on this issue) is the single-GPU dense-retriever path, and streaming those two would need separate handling (multi-GPU splits work across processes; CLIP builds two modalities into one array).Verification
Index_Builder.encode_all()through a ~500MB corpus under a 700MB hard memory ceiling. Onmainthis gets OOM-killed (exit 137) insideencode_all(); with this patch, the same corpus and ceiling complete successfully.tests/test_index_builder_streaming.pylocally (a synthetic-encoder unit test asserting the memmap receives every batch in order and is readable on disk afterward) and confirmed it fails onmainand passes on this branch; it isn't included in this diff becausetests/*is in this repo's.gitignore, so I'm stating that plainly rather than force-adding a file the repo excludes. Happy to add it a different way if there's a preferred location.