Improve multi-CTA algorithm #492
Conversation
tfeher
added
improvement
|
/ok to test |
|
@anaruse there are some unsigned commits, that blocks CI from testing the changes automatically. To fix this issue, could you rebase the PR? |
|
/ok to test |
…when the number of results is large Fix some issues Fix lower recall issue with new multi-cta algo Removing redundant code and changing some parameters Update cpp/src/neighbors/detail/cagra/search_plan.cuh Co-authored-by: Tamas Bela Feher <[email protected]> Remove an unnecessary line and satisfy clang-format
anaruse
force-pushed
the
improved_multi_cta_algo
branch
from
3dce160 to
6223fd2
Compare
|
/merge |
|
/ok to test |
|
/ok to test |
|
/merge |
|
/ok to test |
|
/merge |
|
We are on the brink of missing code freeze for this PR. Please anyone reading this, don't click the "update" button. It inserts a merge commit, which reruns CI in its entirety and this is not needed to merge the PR. We can re-run individual flaky tests that fail without having to rerun the entire CI (the former takes minutes and the latter can take several hours). |
|
@anaruse @tfeher CI seems to be running successfully for other PRs but the gtests seem to be consistently timing out for this PR. As far as I can tell, there's no updates to any of the tests, in this PR, but the timeouts don't seem flaky, they seem isolated to these changes, somehow. We are pushing back code freeze by 1 day. Do you guys think we can still make this in time for 24.12? |
Handle the case when the search result contains invalid indices when building the updated graph in add_nodes. For debugging purposes, fail if any invalid indices found; in future, we can replace RAFT_FAIL with RAFT_LOG_WARN to make the add_nodes routine more robust.
|
I took the liberty to add a workaround to add_nodes, which handles the case when CAGRA search doesn't return enough valid indices. With this, the tests should fail with a descriptive message in place of the segfault. |
|
/ok to test |
There was a problem hiding this comment.
@anaruse thank you for investigating this.
It's not new that our ANN algorithms may return invalid values under some circumstances. One example of this is IVF-PQ with a small number of probes (especially with filtering), so CAGRA multi-cta implementation won't be the first. I think it's reasonable to add a GTEST_SKIP() with an explanation comment for the case of multi-cta and dim = 1 and get this PR merged.
Co-authored-by: Artem M. Chirkin <[email protected]>
|
/ok to test |
|
/ok to test |
|
Could you run the test? At least the issues related to data type have been fixed. |
|
/ok to test |
|
Although some CI tests failed, it seems that all of the tests that have failed are not related to this PR, or more accurately, CAGRA. What would you think? |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
@anaruse all of the C++ test checkers seem to be failing here, which indicates the test failures are likely relared to your changes (and not just flaky tests). I also don't see these tests failing in other PRs. |
|
I can't tell from the logs which tests are failing, can you? |
|
/ok to test |
|
It appears that an infinite loop was occurring in CAGRA_C_TEST and time was running out. I then found that there is a problem where an infinite loop can occur when the graph degree is small. I fixed it and now CAGRA_C_TEST should be able to run. |
|
/ok to test |
|
/ok to test |
This PR is based on #492. The new multi-CTA algorithm proposed in #492 can be used to obtain good recall even with high filtering rates. However, good recall cannot be obtained unless the number of search iterations, or itopk size, one of CAGRA's search parameters, is appropriately increased according to the filtering rate. Therefore, users need to find the appropriate itopk size according to the filtering rate by trial and error, which is a pain. This PR is intended to alleviate this problem by internally calculating the filtering rate and automatically adjusting the itopk size accordingly. Authors: - Akira Naruse (https://github.com/anaruse) - Tamas Bela Feher (https://github.com/tfeher) - Artem M. Chirkin (https://github.com/achirkin) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #509
It has been reported that when the number of search results is large, for example 100, using the multi-CTA algorithm can cause a decrease in recall. This PR is intended to alleviate this low recall issue.
close #208