Knowhere support multi data type #210

cqy123456 · 2023-11-23T03:55:16Z

issue: #287
/kind feature

sre-ci-robot · 2023-11-23T03:55:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cqy123456

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:

~~OWNERS~~ [cqy123456]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mergify · 2023-11-23T03:55:52Z

@cqy123456 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

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!.

alexanderguzhva · 2023-11-23T11:51:23Z

include/knowhere/dataset.h

@@ -60,6 +60,11 @@ class DataSet {
        }
    }

+    std::shared_ptr<const DataSet>
+    Get() const {


unneeded method, just use shared_from_this();

alexanderguzhva · 2023-11-23T11:55:04Z

include/knowhere/factory.h

@@ -21,23 +21,52 @@
 namespace knowhere {
 class IndexFactory {
 public:
+    template <typename DataType>
    Index<IndexNode>
    Create(const std::string& name, const int32_t& version, const Object& object = nullptr);


what is the advantage of templatizing methods instead of using

class IndexFactoryBase { virtual Index<IndexNode> Create(...) = 0; ... } template <typename DataType> class IndexFactory : public IndexFactoryBase { Index<IndexNode> Create(...) override; ... };

?

Because index factory is the entry of all indices, it should not be a template class.

try to Restricting Template Parameter Types.

alexanderguzhva · 2023-11-23T11:57:54Z

include/knowhere/operands.h

+};  // namespace
+
+namespace knowhere {
+using fp32 = float;


What is the point of adding fp32? Basically, fp16 and bf16 were added, because C++ cannot natively handle them, I understand that. Here, you're literally saying #define fp32 float and I can't imagine that this particular line will be changed ever.

So, please feel free to keep it, if appropriate, just make sure that you're not overcomplicating the code :)

Just to unify the names of various data types.

alexanderguzhva · 2023-11-23T12:08:57Z

include/knowhere/dataset.h

@@ -24,7 +24,7 @@

 namespace knowhere {

-class DataSet {
+class DataSet : public std::enable_shared_from_this<const DataSet> {


There is a simpler solution for this. Just pass std::shared_ptr<DataSet> or std::shared_ptr<DataSet>& only across the code (such as function parameters) and get rid of passing DataSet& ones

alexanderguzhva · 2023-11-23T12:12:48Z

src/index/flat/flat.cc

                        auto cur_i_dis = reinterpret_cast<int32_t*>(cur_dis);

                        faiss::SearchParameters search_params;
                        search_params.sel = id_selector;

-                        index_->search(1, (const uint8_t*)x + index * dim / 8, k, cur_i_dis, cur_ids, &search_params);
+                        index_->search(1, (const DataType*)x + index * dim / 8, k, cur_i_dis, cur_ids, &search_params);


this is pretty confusing piece of code, because it literally implies that DataType is uint8_t because of the following dim / 8, and if DataType is uint32_t, then this code will fail.
Same note for this whole file.
Maybe, put an Index that uses faiss::IndexBinaryFlat into a separate class?

liliu-z

Serialize datatype. (Don't forget to take compatibility into consideration)
Simplify wrapper creation by directly transferring data into fp32. (More comments needed)

sre-ci-robot

@cqy123456: 1 invalid OWNERS file

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sre-ci-robot · 2023-12-12T02:59:59Z

OWNERS

@@ -1,32 +1,12 @@
-filters:


Cannot parse file: error converting YAML to JSON: yaml: mapping values are not allowed in this context.

Presburger · 2023-12-18T08:34:43Z

include/knowhere/operands.h

+    }
+};
+
+struct bf16 {


while c++23 will have std::float16_t and std::bfloat16_t, leave some macro for the future support of this.

liliu-z · 2023-12-26T09:30:19Z

include/knowhere/index_node.h

@@ -18,10 +18,10 @@
 #include "knowhere/dataset.h"
 #include "knowhere/expected.h"
 #include "knowhere/object.h"
+#include "knowhere/operands.h"


redundant change

Used in all index_node.cc

liliu-z · 2023-12-26T10:28:41Z

src/index/hnsw/hnsw.cc

@@ -107,7 +113,7 @@ class HnswIndexNode : public IndexNode {
    expected<DataSetPtr>
    Search(const DataSet& dataset, const Config& cfg, const BitsetView& bitset) const override {
        if (!index_) {
-            LOG_KNOWHERE_WARNING_ << "search on empty index";
+            LOG_KNOWHERE_WARNING_ << "search on empty indefloatx";


liliu-z · 2023-12-26T10:29:34Z

src/index/hnsw/hnsw.cc

 public:
+    using DistType = typename ResultType<DataType>::type;


Any other expected type?

Maybe support uint8, and DistType is uint32 in the future.

Let's don't reserve anything for the future for now.

liliu-z · 2024-01-04T08:33:30Z

src/index/hnsw/hnsw.cc

 public:
+    using DistType = typename ResultType<DataType>::type;


Let's don't reserve anything for the future for now.

liliu-z · 2024-01-04T08:33:47Z

src/index/gpu_raft/gpu_raft_ivf_flat.cc

-    return Index<IndexNodeThreadPoolWrapper>::Create(std::make_unique<GpuRaftIvfFlatIndexNode>(version, object),
-                                                     cuda_concurrent_size);
-});
+KNOWHERE_REGISTER_GLOBAL(


Why not make a macro for this

Added a macro function for IndexNodeThreadPoolWrapper

liliu-z · 2024-01-04T08:34:33Z

tests/ut/test_bruteforce.cc

@@ -38,7 +38,7 @@ TEST_CASE("Test Brute Force", "[float vector]") {
    };

    SECTION("Test Search") {
-        auto res = knowhere::BruteForce::Search(train_ds, query_ds, conf, nullptr);
+        auto res = knowhere::BruteForce::Search<float>(train_ds, query_ds, conf, nullptr);


can we stick on fp32 here

liliu-z · 2024-01-04T08:35:33Z

tests/ut/test_diskann.cc

@@ -52,12 +52,13 @@ constexpr float kL2RangeAp = 0.9;
 constexpr float kIpRangeAp = 0.9;
 constexpr float kCosineRangeAp = 0.9;

+template <typename T>


nit: Let's avoid vague type name

liliu-z · 2024-01-04T08:36:02Z

tests/ut/test_diskann.cc

@@ -144,7 +145,9 @@ TEST_CASE("Invalid diskann params test", "[diskann]") {
    fs::remove(kDir);
 }

-TEST_CASE("Test DiskANNIndexNode.", "[diskann]") {
+template <typename data_type>


nit: Use capital letters

liliu-z · 2024-01-04T10:12:59Z

tests/ut/test_feder.cc

@@ -173,11 +173,11 @@ TEST_CASE("Test Feder", "[feder]") {
    const auto query_ds = GenDataSet(nq, dim, seed);

    const knowhere::Json conf = base_gen();
-    auto gt = knowhere::BruteForce::Search(train_ds, query_ds, conf, nullptr);
+    auto gt = knowhere::BruteForce::Search<float>(train_ds, query_ds, conf, nullptr);


nit: same, can we use fp32

liliu-z · 2024-01-04T10:14:32Z

thirdparty/DiskANN/include/diskann/utils.h

+  template<typename IN, typename OUT>
+  void convert_types_in_file(const std::string& inFileName,


Where is the usage?

Used in diskann，remove it first and add it next time.

Signed-off-by: cqy123456 <[email protected]> Add (b)float16 interface for pyknowhere Signed-off-by: Writer-X <[email protected]>

liliu-z · 2024-01-09T11:16:21Z

/lgtm

Signed-off-by: Yudong Cai <[email protected]>

sre-ci-robot requested a review from liliu-z November 23, 2023 03:55

sre-ci-robot requested a review from zhengbuqian November 23, 2023 03:55

sre-ci-robot added approved size/XXL labels Nov 23, 2023

mergify bot added dco-passed do-not-merge/missing-related-issue labels Nov 23, 2023

cqy123456 force-pushed the template_data branch 3 times, most recently from 30e7f15 to 4b272a5 Compare November 23, 2023 07:56

alexanderguzhva reviewed Nov 23, 2023

View reviewed changes

liliu-z reviewed Nov 23, 2023

View reviewed changes

cqy123456 force-pushed the template_data branch 4 times, most recently from 927d315 to 1cbb332 Compare December 12, 2023 02:59

sre-ci-robot added the do-not-merge/invalid-owners-file label Dec 12, 2023

sre-ci-robot reviewed Dec 12, 2023

View reviewed changes

cqy123456 force-pushed the template_data branch from 1cbb332 to 25f0abe Compare December 12, 2023 03:07

sre-ci-robot removed the do-not-merge/invalid-owners-file label Dec 12, 2023

cqy123456 force-pushed the template_data branch 6 times, most recently from bb74446 to 288c2d2 Compare December 13, 2023 12:16

Presburger reviewed Dec 18, 2023

View reviewed changes

cqy123456 force-pushed the template_data branch 2 times, most recently from 791279e to d79c576 Compare December 20, 2023 06:29

cqy123456 force-pushed the template_data branch 2 times, most recently from 57654a4 to 8435c40 Compare January 4, 2024 07:24

liliu-z reviewed Jan 4, 2024

View reviewed changes

cqy123456 force-pushed the template_data branch 2 times, most recently from 88e9606 to 92c0aeb Compare January 4, 2024 09:14

liliu-z reviewed Jan 4, 2024

View reviewed changes

cqy123456 force-pushed the template_data branch 2 times, most recently from dfb2ecb to e1ca8c1 Compare January 5, 2024 03:51

liliu-z requested a review from alexanderguzhva January 9, 2024 04:14

liliu-z added the kind/feature label Jan 9, 2024

mergify bot removed the do-not-merge/missing-related-issue label Jan 9, 2024

cqy123456 force-pushed the template_data branch 4 times, most recently from 040392b to 88a91e2 Compare January 9, 2024 09:42

knowhere support multi data type

0dab512

Signed-off-by: cqy123456 <[email protected]> Add (b)float16 interface for pyknowhere Signed-off-by: Writer-X <[email protected]>

cqy123456 force-pushed the template_data branch from 88a91e2 to 0dab512 Compare January 9, 2024 10:58

sre-ci-robot assigned liliu-z Jan 9, 2024

sre-ci-robot added the lgtm label Jan 9, 2024

mergify bot added the ci-passed label Jan 9, 2024

sre-ci-robot merged commit 4e8244e into zilliztech:main Jan 9, 2024
9 checks passed

cydrain added a commit to cydrain/knowhere that referenced this pull request Feb 23, 2024

Revert zilliztech#210 (support multi datatype)

fc184b3

Signed-off-by: Yudong Cai <[email protected]>

cydrain mentioned this pull request Jun 26, 2024

Use DataSetPtr in all knowhere interfaces #666

Closed

		public:
		using DistType = typename ResultType<DataType>::type;

		template<typename IN, typename OUT>
		void convert_types_in_file(const std::string& inFileName,

Knowhere support multi data type #210

Knowhere support multi data type #210

Conversation

cqy123456 commented Nov 23, 2023 • edited Loading

sre-ci-robot commented Nov 23, 2023

mergify bot commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liliu-z left a comment • edited Loading

Choose a reason for hiding this comment

sre-ci-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liliu-z commented Jan 9, 2024

cqy123456 commented Nov 23, 2023 •

edited

Loading

liliu-z left a comment •

edited

Loading