Add index feature manager & static config check #796
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: foxspy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
|
@foxspy 🔍 Important: PR Classification Needed! For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:
For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”. Thanks for your efforts and contribution to the community!. |
foxspy
force-pushed
the
add_vector_index_mgr
branch
from
19b5c83
to
43b51ae
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #796 +/- ##
=========================================
+ Coverage 0 79.64% +79.64%
=========================================
Files 0 80 +80
Lines 0 6259 +6259
=========================================
+ Hits 0 4985 +4985
- Misses 0 1274 +1274 |
| // This flag indicates that there is no need to create any index structure (build stage can be skipped) | ||
| constexpr uint64_t BF = 1UL << 16; | ||
| // This flag indicates that the index defaults to KNN search, meaning the recall rate is 100% | ||
| constexpr uint64_t KNN = 1UL << 17; |
There was a problem hiding this comment.
the comment is confusing. Bruteforce with fp16 does not have a 100% recall rate.
I'd use the This flag indicates that the index defaults to brute force search, meaning that the recall rate is maximum possible for the given data precision
| // This flag indicates that the index defaults to KNN search, meaning the recall rate is 100% | ||
| constexpr uint64_t KNN = 1UL << 17; | ||
| // This flag indicates that the index is deployed on GPU (need GPU devices) | ||
| constexpr uint64_t GPU = 1UL << 18; |
There was a problem hiding this comment.
training or search? or both?
| constexpr uint64_t KNN = 1UL << 17; | ||
| // This flag indicates that the index is deployed on GPU (need GPU devices) | ||
| constexpr uint64_t GPU = 1UL << 18; | ||
| // This flag indicates that the index support using mmap manage its mainly memory, which can significant improve the |
| // This flag indicates that the index support using mmap manage its mainly memory, which can significant improve the | ||
| // capacity | ||
| constexpr uint64_t MMAP = 1UL << 19; | ||
| // This flag indicates that the index support using materialized view to accelerate filtering search |
include/knowhere/feature.h
Outdated
| constexpr uint64_t MMAP = 1UL << 19; | ||
| // This flag indicates that the index support using materialized view to accelerate filtering search | ||
| constexpr uint64_t MV = 1UL << 20; | ||
| // This flag indicates that the index need disk during search |
There was a problem hiding this comment.
the comment is unclear
| #define FEATURE_H | ||
|
|
||
| // these features have been report to outside (milvus); pls sync the feature code when it needs to be changed. | ||
| namespace knowhere::feature { |
There was a problem hiding this comment.
why uint64_t instead of a formal struct Features {};, which can be extendable and more appropriate for further changes and more clear code?
| IndexFactory::GlobalIndexTable& | ||
| IndexFactory::StaticIndexTableInstance() { | ||
| static GlobalIndexTable static_index_table; | ||
| return static_index_table; | ||
| } | ||
|
|
||
| bool | ||
| IndexFactory::FeatureCheck(const std::string& name, uint64_t feature) const { |
There was a problem hiding this comment.
maybe, return std::optional<bool>, which returns nullopt if name is not known?
foxspy
force-pushed
the
add_vector_index_mgr
branch
2 times, most recently
from
8a599a6
to
6b011e1
Compare
foxspy
force-pushed
the
add_vector_index_mgr
branch
from
6b011e1
to
e0b027e
Compare
|
@foxspy is it good to review this PR? |
foxspy
force-pushed
the
add_vector_index_mgr
branch
2 times, most recently
from
cfd1285
to
3322656
Compare
foxspy
force-pushed
the
add_vector_index_mgr
branch
5 times, most recently
from
a619291
to
75caa52
Compare
|
/hold |
Thanks, please help with the review. The code won’t change much, but this commit contains a lot of mixed content. I plan to break it into several commits. |
foxspy
force-pushed
the
add_vector_index_mgr
branch
2 times, most recently
from
dd8e57a
to
4f23127
Compare
foxspy
force-pushed
the
add_vector_index_mgr
branch
from
4f23127
to
ba93968
Compare
foxspy
force-pushed
the
add_vector_index_mgr
branch
2 times, most recently
from
afb84da
to
0f281f7
Compare
Signed-off-by: xianliang.li <[email protected]>
foxspy
force-pushed
the
add_vector_index_mgr
branch
from
0f281f7
to
9e1257e
Compare
issue: #795