diskann support new data type(fp16/bf16) #393

cqy123456 · 2024-02-19T10:51:59Z

issue: #287
related milvus pr：milvus-io/milvus#30716 , sift1m result also can see milvus-io/milvus#30716
knowhere-test pr: https://github.com/milvus-io/knowhere-test/pull/176

sre-ci-robot · 2024-02-19T10:52:03Z

[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 · 2024-02-19T10:52:37Z

@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 · 2024-02-20T18:14:04Z

src/index/diskann/diskann.cc

@@ -32,7 +32,8 @@
 namespace knowhere {
 template <typename DataType>
 class DiskANNIndexNode : public IndexNode {
-    static_assert(std::is_same_v<DataType, fp32>, "DiskANN only support float");
+    static_assert(KnowhereFloatTypeCheck<DataType>::value,
+                  "DiskANN only support floating point data type(float32, float16, brain_float16)");


Please use bfloat16 instead of brain_float16 :)

alexanderguzhva · 2024-02-20T18:15:06Z

tests/ut/test_diskann.cc

@@ -355,29 +354,36 @@ TEST_CASE("Test DiskANN GetVectorByIds", "[diskann]") {
            json["data_path"] = kRawDataPath;
            json["max_degree"] = 5;
            json["search_list_size"] = kK;
-            json["pq_code_budget_gb"] = sizeof(float) * dim * kNumRows * 0.03125 / (1024 * 1024 * 1024);
+            json["pq_code_budget_gb"] = sizeof(DataType) * dim * kNumRows * 0.125 / (1024 * 1024 * 1024);


what is 0.125 magic number and why it got changed from 0.03125 (1/32th) ?

0.03125 make fp16/bf16 crash in small chunk num(<10), but this problem fixed in pr391

alexanderguzhva · 2024-02-20T18:16:32Z

tests/ut/test_diskann.cc

@@ -412,3 +418,15 @@ TEST_CASE("Test DiskANN GetVectorByIds", "[diskann]") {
    fs::remove_all(kDir);
    fs::remove(kDir);
 }
+
+TEST_CASE("Test DiskANN Base ", "[diskann]") {


maybe, a 3 tests for each of the data types instead?

remove these cases to e2e test

alexanderguzhva · 2024-02-20T18:16:37Z

tests/ut/test_diskann.cc

+    BaseSearchTest<knowhere::fp16>();
+}
+
+TEST_CASE("Test DiskANN GetVectorByIds", "[diskann]") {


remove these cases to e2e test

alexanderguzhva · 2024-02-20T18:17:59Z

thirdparty/DiskANN/include/diskann/utils.h

@@ -583,6 +585,12 @@ namespace diskann {
  template<typename T>
  float prepare_base_for_inner_products(const std::string in_file,
                                        const std::string out_file) {
+    if (!knowhere::KnowhereFloatTypeCheck<T>::value) {
+      std::stringstream stream;
+      stream << "DiskANN currently only supports floating point data for IP."


I would interpret this message that only float is supported, but not fp16 or bf16. Please mention fp16 and bf16 if they are also supported.

alexanderguzhva · 2024-02-20T18:18:28Z

thirdparty/DiskANN/include/diskann/utils.h

+    if (!knowhere::KnowhereFloatTypeCheck<T>::value) {
+      std::stringstream stream;
+      stream
+          << "DiskANN currently only supports floating point data for Cosine."


same note for float, fp16 and bf16

alexanderguzhva · 2024-02-20T18:21:30Z

thirdparty/DiskANN/src/distance.cpp

        float res = 0;
-        for (size_t i = 0; i < size; i++) {
-          res += ((float) x[i] - (float) y[i]) * ((float) x[i] - (float) y[i]);
+        auto  x_fp32 = std::make_unique<float[]>(size);


So, every invokation of distance calculation will trigger two vector allocations? O_o
Should I add fp16 and bf16 distance calculations into Faiss instead?

fp16/bf16 distance function will implemented later, like faiss::fvec_inner_product and fvec_L2sqr.

alexanderguzhva · 2024-02-20T18:21:36Z

thirdparty/DiskANN/src/distance.cpp

        float res = 0;
-        for (size_t i = 0; i < size; i++) {
-          res += (float) x[i] * (float) y[i];
+        auto  x_fp32 = std::make_unique<float[]>(size);


alexanderguzhva · 2024-02-21T12:06:53Z

/lgtm

liliu-z · 2024-02-22T07:26:56Z

thirdparty/DiskANN/include/diskann/utils.h

@@ -583,6 +585,13 @@ namespace diskann {
  template<typename T>
  float prepare_base_for_inner_products(const std::string in_file,
                                        const std::string out_file) {
+    if (!knowhere::KnowhereFloatTypeCheck<T>::value) {


do constexpr

alexanderguzhva · 2024-02-26T15:43:06Z

/lgtm

Signed-off-by: cqy123456 <[email protected]>

liliu-z · 2024-02-27T11:43:31Z

/lgtm

liliu-z · 2024-02-27T11:43:49Z

/kind feature

sre-ci-robot requested review from cydrain and liliu-z February 19, 2024 10:52

sre-ci-robot added approved size/L labels Feb 19, 2024

mergify bot added the dco-passed label Feb 19, 2024

mergify bot added the do-not-merge/missing-related-issue label Feb 19, 2024

cqy123456 force-pushed the diskann-new-type branch from 309cf27 to 3d07dc7 Compare February 20, 2024 02:23

alexanderguzhva reviewed Feb 20, 2024

View reviewed changes

cqy123456 force-pushed the diskann-new-type branch 2 times, most recently from b5989fe to 4c951e1 Compare February 21, 2024 03:41

mergify bot added the ci-passed label Feb 21, 2024

sre-ci-robot assigned alexanderguzhva Feb 21, 2024

sre-ci-robot added the lgtm label Feb 21, 2024

liliu-z reviewed Feb 22, 2024

View reviewed changes

cqy123456 force-pushed the diskann-new-type branch from 4c951e1 to bdbd137 Compare February 22, 2024 09:32

sre-ci-robot removed the lgtm label Feb 22, 2024

mergify bot removed the ci-passed label Feb 22, 2024

cqy123456 force-pushed the diskann-new-type branch 2 times, most recently from 2691198 to 1c14de3 Compare February 26, 2024 09:35

mergify bot added the ci-passed label Feb 26, 2024

sre-ci-robot added the lgtm label Feb 26, 2024

diskann support new data type(fp16/bf16)

52f899f

Signed-off-by: cqy123456 <[email protected]>

cqy123456 force-pushed the diskann-new-type branch from 1c14de3 to 52f899f Compare February 27, 2024 02:40

sre-ci-robot removed the lgtm label Feb 27, 2024

mergify bot added ci-passed and removed ci-passed labels Feb 27, 2024

sre-ci-robot assigned liliu-z Feb 27, 2024

sre-ci-robot added the lgtm label Feb 27, 2024

sre-ci-robot added the kind/feature label Feb 27, 2024

mergify bot removed the do-not-merge/missing-related-issue label Feb 27, 2024

sre-ci-robot merged commit 60a5c9c into zilliztech:main Feb 27, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diskann support new data type(fp16/bf16) #393

diskann support new data type(fp16/bf16) #393

cqy123456 commented Feb 19, 2024 •

edited

Loading

sre-ci-robot commented Feb 19, 2024

mergify bot commented Feb 19, 2024

alexanderguzhva Feb 20, 2024

cqy123456 Feb 21, 2024

alexanderguzhva Feb 20, 2024

cqy123456 Feb 21, 2024 •

edited

Loading

alexanderguzhva Feb 20, 2024

cqy123456 Feb 21, 2024 •

edited

Loading

alexanderguzhva Feb 20, 2024

cqy123456 Feb 21, 2024 •

edited

Loading

alexanderguzhva Feb 20, 2024

cqy123456 Feb 21, 2024

alexanderguzhva Feb 20, 2024

cqy123456 Feb 21, 2024

alexanderguzhva Feb 20, 2024

cqy123456 Feb 21, 2024

alexanderguzhva Feb 20, 2024

alexanderguzhva commented Feb 21, 2024

liliu-z Feb 22, 2024

alexanderguzhva commented Feb 26, 2024

liliu-z commented Feb 27, 2024

liliu-z commented Feb 27, 2024

diskann support new data type(fp16/bf16) #393

diskann support new data type(fp16/bf16) #393

Conversation

cqy123456 commented Feb 19, 2024 • edited Loading

sre-ci-robot commented Feb 19, 2024

mergify bot commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqy123456 Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqy123456 Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqy123456 Feb 21, 2024 • edited Loading

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

alexanderguzhva commented Feb 21, 2024

Choose a reason for hiding this comment

alexanderguzhva commented Feb 26, 2024

liliu-z commented Feb 27, 2024

liliu-z commented Feb 27, 2024

cqy123456 commented Feb 19, 2024 •

edited

Loading

cqy123456 Feb 21, 2024 •

edited

Loading

cqy123456 Feb 21, 2024 •

edited

Loading

cqy123456 Feb 21, 2024 •

edited

Loading