Fix IDSelectorBitmap conversion to cuVS bitset (#5211)#5211
Closed
mnorris11 wants to merge 1 commit into
Closed
Conversation
Contributor
|
@mnorris11 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D104935985. |
mnorris11
force-pushed
the
export-D104935985
branch
from
307982d to
fd9b1c4
Compare
meta-codesync
Bot
changed the title
mnorris11
force-pushed
the
export-D104935985
branch
from
fd9b1c4 to
130b667
Compare
mnorris11
pushed a commit
to mnorris11/faiss
that referenced
this pull request
May 14, 2026
Summary: `convert_to_bitset_bitmap` in `CuvsFilterConvert.cu` assumed that `IDSelectorBitmap.n` was the bit/vector count, but it can also be the byte count of the bitmap array (which is how the Python wrapper constructs it via `faiss.IDSelectorBitmap(numpy_array)`). This caused two bugs: 1. The `RAFT_EXPECTS(bitset.size() == n)` check failed when `n` was the byte count (e.g., 6.25M bytes vs 50M bits), throwing a `raft::logic_error` that propagated up and caused filtered search to silently fall back to unfiltered results. 2. `n_elements = (selector.n + 7) / 8` divided the byte count by 8 again, copying only 1/8 of the bitmap data when `n` was already the byte count. The fix handles both conventions by computing the actual number of bytes to copy as `min(n, ceil(bitset_size / 8))`. This works regardless of whether `n` represents bytes or bits: - Python wrapper (n = byte count): copies all n bytes - C++ convention (n = bit count): copies ceil(bit_count / 8) bytes (the minimum needed) Differential Revision: D104935985
mnorris11
pushed a commit
to mnorris11/faiss
that referenced
this pull request
May 14, 2026
Summary: `convert_to_bitset_bitmap` in `CuvsFilterConvert.cu` assumed that `IDSelectorBitmap.n` was the bit/vector count, but it can also be the byte count of the bitmap array (which is how the Python wrapper constructs it via `faiss.IDSelectorBitmap(numpy_array)`). This caused two bugs: 1. The `RAFT_EXPECTS(bitset.size() == n)` check failed when `n` was the byte count (e.g., 6.25M bytes vs 50M bits), throwing a `raft::logic_error` that propagated up and caused filtered search to silently fall back to unfiltered results. 2. `n_elements = (selector.n + 7) / 8` divided the byte count by 8 again, copying only 1/8 of the bitmap data when `n` was already the byte count. The fix handles both conventions by computing the actual number of bytes to copy as `min(n, ceil(bitset_size / 8))`. This works regardless of whether `n` represents bytes or bits: - Python wrapper (n = byte count): copies all n bytes - C++ convention (n = bit count): copies ceil(bit_count / 8) bytes (the minimum needed) Differential Revision: D104935985
mnorris11
force-pushed
the
export-D104935985
branch
from
130b667 to
20e1a9b
Compare
Contributor
Author
|
@tarang-jain @lowener Does this change look right? |
Summary: `convert_to_bitset_bitmap` in `CuvsFilterConvert.cu` assumed that `IDSelectorBitmap.n` was the bit/vector count, but it can also be the byte count of the bitmap array (which is how the Python wrapper constructs it via `faiss.IDSelectorBitmap(numpy_array)`). This caused two bugs: 1. The `RAFT_EXPECTS(bitset.size() == n)` check failed when `n` was the byte count (e.g., 6.25M bytes vs 50M bits), throwing a `raft::logic_error` that propagated up and caused filtered search to silently fall back to unfiltered results. 2. `n_elements = (selector.n + 7) / 8` divided the byte count by 8 again, copying only 1/8 of the bitmap data when `n` was already the byte count. The fix handles both conventions by computing the actual number of bytes to copy as `min(n, ceil(bitset_size / 8))`. This works regardless of whether `n` represents bytes or bits: - Python wrapper (n = byte count): copies all n bytes - C++ convention (n = bit count): copies ceil(bit_count / 8) bytes (the minimum needed) Reviewed By: pankajsingh88 Differential Revision: D104935985
mnorris11
force-pushed
the
export-D104935985
branch
from
20e1a9b to
477f22a
Compare
Contributor
|
This pull request has been merged in 172324a. |
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
1 participant
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.
Summary:
convert_to_bitset_bitmapinCuvsFilterConvert.cuassumed thatIDSelectorBitmap.nwas the bit/vector count, but it can also be the byte count of the bitmap array (which is how the Python wrapper constructs it viafaiss.IDSelectorBitmap(numpy_array)). This caused two bugs:The
RAFT_EXPECTS(bitset.size() == n)check failed whennwas the byte count (e.g., 6.25M bytes vs 50M bits), throwing araft::logic_errorthat propagated up and caused filtered search to silently fall back to unfiltered results.n_elements = (selector.n + 7) / 8divided the byte count by 8 again, copying only 1/8 of the bitmap data whennwas already the byte count.The fix handles both conventions by computing the actual number of bytes to copy as
min(n, ceil(bitset_size / 8)). This works regardless of whethernrepresents bytes or bits:Reviewed By: pankajsingh88
Differential Revision: D104935985