Use new cardinal hnsw and diskann config to relax checking (zilliztec…

…h#51) * Use new cardinal hnsw and diskann config to relax checking Signed-off-by: chasingegg <[email protected]> * remove diskann pramaters check (zilliztech#169) Signed-off-by: cqy123456 <[email protected]> --------- Signed-off-by: chasingegg <[email protected]> Signed-off-by: cqy123456 <[email protected]> Co-authored-by: cqy123456 <[email protected]>
foxspy · Dec 7, 2023 · 4ddd2ef · 4ddd2ef
1 parent 1b7b809
commit 4ddd2ef
Show file tree

Hide file tree

Showing 8 changed files with 37 additions and 39 deletions.
diff --git a/cmake/libs/libcardinal.cmake b/cmake/libs/libcardinal.cmake
@@ -24,9 +24,9 @@ include_directories(${folly_INCLUDE_DIRS})
 
 if (APPLE)
     include_directories(thirdparty/cardinal/src/apple_patch/*/*.h)
-    target_link_libraries(cardinal PUBLIC glog::glog Folly::folly)
+    target_link_libraries(cardinal PUBLIC glog::glog Folly::folly nlohmann_json::nlohmann_json)
 else()
     find_package(aio REQUIRED)
     include_directories(${AIO_INCLUDE})
-    target_link_libraries(cardinal PUBLIC ${AIO_LIBRARIES} glog::glog Folly::folly)
+    target_link_libraries(cardinal PUBLIC ${AIO_LIBRARIES} glog::glog Folly::folly nlohmann_json::nlohmann_json)
 endif()
diff --git a/conanfile.py b/conanfile.py
@@ -91,6 +91,8 @@ def requirements(self):
         self.requires("xz_utils/5.2.5")
         self.requires("fmt/9.1.0")
         self.requires("folly/2023.07.12@milvus/dev")
+        if self.settings.os != "Macos":
+            self.requires("libunwind/1.7.2")
         if self.options.with_ut:
             self.requires("catch2/3.3.1")
         if self.options.with_benchmark:

diff --git a/src/index/cardinal/cardinal.cc b/src/index/cardinal/cardinal.cc
@@ -384,10 +384,10 @@ class CardinalIndexNode : public IndexNode {
     std::unique_ptr<BaseConfig>
     CreateConfig() const override {
         if constexpr (IndexType == CardinalIndexType::IndexHNSW) {
-            return std::make_unique<HnswConfig>();
+            return std::make_unique<CardinalHNSWConfig>();
         }
         if constexpr (IndexType == CardinalIndexType::IndexDiskANN) {
-            return std::make_unique<DiskANNConfig>();
+            return std::make_unique<CardinalDiskANNConfig>();
         }
         if constexpr (IndexType == CardinalIndexType::IndexCardinal) {
             return std::make_unique<CardinalConfig>();

diff --git a/src/index/cardinal/cardinal_adapter.h b/src/index/cardinal/cardinal_adapter.h
@@ -352,22 +352,6 @@ class CardinalAdapter {
                 config.topK = diskAnnConfig.k.value();
                 config.search_candidates_limit = diskAnnConfig.search_list_size.value();
                 config.beamwidth = diskAnnConfig.beamwidth.value();
-
-                if (config.search_candidates_limit < config.topK) {
-                    return expected<cardinal::VecIndexConfig>::Err(
-                        Status::out_of_range_in_json, "search_candidates_limit should be larger than max_k");
-                }
-                int kSearchListSizeMaxValue = 200;
-                auto max_search_list_size = std::max(kSearchListSizeMaxValue, diskAnnConfig.k.value() * 10);
-                if (diskAnnConfig.search_list_size.value() > max_search_list_size ||
-                    diskAnnConfig.search_list_size.value() < diskAnnConfig.k.value()) {
-                    auto msg = fmt::format(
-                        "search_list_size should be in range: [topk, max(200, topk * 10)], topk = {}, search_list_size "
-                        "= {}",
-                        diskAnnConfig.k.value(), diskAnnConfig.search_list_size.value());
-                    LOG_KNOWHERE_ERROR_ << msg;
-                    return expected<cardinal::VecIndexConfig>::Err(Status::out_of_range_in_json, msg);
-                }
             }
 
             if constexpr (stage == IndexStage::RangeSearch) {

diff --git a/src/index/cardinal/cardinal_config.h b/src/index/cardinal/cardinal_config.h
@@ -16,6 +16,28 @@
 
 namespace knowhere {
 
+class CardinalHNSWConfig : public HnswConfig {
+ public:
+    inline Status
+    CheckAndAdjustForSearch(std::string* err_msg) override {
+        if (!ef.has_value()) {
+            ef = std::max(k.value(), kEfMinValue);
+        }
+        return Status::success;
+    }
+};
+
+class CardinalDiskANNConfig : public DiskANNConfig {
+ public:
+    inline Status
+    CheckAndAdjustForSearch(std::string* err_msg) override {
+        if (!search_list_size.has_value()) {
+            search_list_size = std::max(k.value(), kSearchListSizeMinValue);
+        }
+        return Status::success;
+    }
+};
+
 class CardinalConfig : public BaseConfig {
  public:
     CFG_STRING index_type;

diff --git a/src/index/diskann/diskann.cc b/src/index/diskann/diskann.cc
@@ -176,7 +176,6 @@ class DiskANNIndexNode : public IndexNode {
 namespace knowhere {
 namespace {
 static constexpr float kCacheExpansionRate = 1.2;
-static constexpr int kSearchListSizeMaxValue = 200;
 
 Status
 TryDiskANNCall(std::function<void()>&& diskann_call) {
@@ -522,15 +521,6 @@ DiskANNIndexNode<T>::Search(const DataSet& dataset, const Config& cfg, const Bit
     if (!CheckMetric(search_conf.metric_type.value())) {
         return expected<DataSetPtr>::Err(Status::invalid_metric_type, "unsupported metric type");
     }
-    auto max_search_list_size = std::max(kSearchListSizeMaxValue, search_conf.k.value() * 10);
-    if (search_conf.search_list_size.value() > max_search_list_size ||
-        search_conf.search_list_size.value() < search_conf.k.value()) {
-        auto msg = fmt::format(
-            "search_list_size should be in range: [topk, max(200, topk * 10)], topk = {}, search_list_size = {}",
-            search_conf.k.value(), search_conf.search_list_size.value());
-        LOG_KNOWHERE_ERROR_ << msg;
-        return expected<DataSetPtr>::Err(Status::out_of_range_in_json, msg);
-    }
     auto k = static_cast<uint64_t>(search_conf.k.value());
     auto lsearch = static_cast<uint64_t>(search_conf.search_list_size.value());
     auto beamwidth = static_cast<uint64_t>(search_conf.beamwidth.value());

diff --git a/tests/ut/test_diskann.cc b/tests/ut/test_diskann.cc
@@ -123,14 +123,14 @@ TEST_CASE("Invalid diskann params test", "[diskann]") {
         knowhere::Json test_json;
         auto query_ds = GenDataSet(kNumQueries, kDim, 42);
 
-        // search list size < topk
-        {
-            test_json = test_gen();
-            test_json["search_list_size"] = 1;
-            auto res = diskann.Search(*query_ds, test_json, nullptr);
-            REQUIRE_FALSE(res.has_value());
-            REQUIRE(res.error() == knowhere::Status::out_of_range_in_json);
-        }
+        // search list size < topk works now
+        // {
+        //     test_json = test_gen();
+        //     test_json["search_list_size"] = 1;
+        //     auto res = diskann.Search(*query_ds, test_json, nullptr);
+        //     REQUIRE_FALSE(res.has_value());
+        //     REQUIRE(res.error() == knowhere::Status::out_of_range_in_json);
+        // }
         // min_k > max_k
         {
             test_json = test_gen();

diff --git a/thirdparty/cardinal b/thirdparty/cardinal