Fix IVF PQ build metric for CAGRA #862
Conversation
Signed-off-by: Mickael Ide <[email protected]>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
cjnolet
added
improvement
|
/ok to test 57aa341 |
|
@lowener does this PR impacts the integration of Cuvs with Faiss library? |
|
@navneet1v Faiss is currently tied to libcuvs=25.04. Whatever goes in after 25.04 is not reflected in faiss until it upgrades to the latest cuVS version. |
|
@tarang-jain let me clarify what I am asking, question is: |
|
Yes this was already present in libcuvs 25.04 @navneet1v |
|
@lowener should we create a PR in Faiss to upgrade Cuvs version in Faiss to fix the issue? |
|
Since Faiss just depends on a pre-installed version of cuVS and does not build cuVS from source, you should be able to simply install 25.06 after this PR is merged (or even build this PR branch from source) in your environment. So long as any of the public API signatures have not changed between 25.04 and 25.06, it will work just fine while building Faiss from source. |
Signed-off-by: Mickael Ide <[email protected]>
| search_params.lut_dtype = CUDA_R_16F; | ||
| search_params.internal_distance_dtype = CUDA_R_16F; | ||
| search_params.coarse_search_dtype = CUDA_R_16F; | ||
| search_params.lut_dtype = CUDA_R_32F; |
There was a problem hiding this comment.
In the context of data already normalized (for cosine computation for example) using FP16 as data type leads to terrible recall due to the loss of precision on small floats. Defaulting to FP32 could prevent those extreme cases, at the expense of index build time. This is important since CAGRA-IVFPQ build params are not yet exposed through python API.
There was a problem hiding this comment.
- I don't think we should be changing defaults. This can lead to massive confusion for users when their perf suffers as a result.
- We need to get the ivfpq params expoed through CAGRA's Python API. I didn't realize they weren't already exposed.
There was a problem hiding this comment.
@lowener I agree with Corey we should better keep fp16, at least for now, as it has a significant impact on performance (I saw tens of percents of build time difference hence the default is fp16).
However, I'm very curious about the cases where these three internal types significantly affect the recall being fp16 against fp32. Could you please point me to these for a follow up study?
There was a problem hiding this comment.
#841 With the Flickr dataset. The user also gave some reproducer code.
Signed-off-by: Mickael Ide <[email protected]>
|
/ok to test c620cc1 |
|
@cjnolet by when we can expect this change to be merged? We are looking to test this with Faiss to see if the issue of recall is fixed. |
|
@navneet1v it should be merged for the 25.06 release (burndown is in 2 weeks). It'll likely be merged well before burndown, but it needs to pass CI first. |
|
/merge |
github-project-automation
bot
moved this from In Progress
to Done
in Vector Search, ML, & Data Mining Release Board
6 participants
Related to #841.
The IVF-PQ build metric is not properly initialized in the case of InnerProduct, so I am adding here correct initialization on the C layer, as well as some checks on the C++ side so that the CAGRA metric match the knn metric.