[MOD-6775] introduce fp16 type! #462

meiravgri · 2024-05-07T14:07:23Z

This PR introduces a new data type, FLOAT16, to the VecSim API.

New

Implementation of FLOAT16 in the indices factories.
Addition of FLOAT16 utilities (FLOAT16_STRING etc.).
Implementation of a normalization function for FLOAT16.
Unit and flow tests for FLOAT16.
Benchmarks for FLOAT16, including a serialized HNSW index file uploaded to the AWS bucket.

Changes

change bfloat16 and fp16 from a typedef to uint16_t to a struct to avoid ambiguity between them
change the dimension across all size estimation unit tests to decrease their run time.

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

change bfloat and fp16 to struct typedef to avoid ambiguty between them

codecov · 2024-05-07T14:32:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.18%. Comparing base (d2e1d4f) to head (29c1773).

Additional details and impacted files

@@                     Coverage Diff                      @@
##           meiravg_feature_bfloat16     #462      +/-   ##
============================================================
+ Coverage                     97.10%   97.18%   +0.07%     
============================================================
  Files                            85       85              
  Lines                          4633     4684      +51     
============================================================
+ Hits                           4499     4552      +53     
+ Misses                          134      132       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

add fp16 files to bm files decrease testSizeEstimation dim size to make it run fasetr in debug

GuyAv46

Code review (without tests/benchmarks)

GuyAv46 · 2024-05-09T07:01:38Z

src/VecSim/spaces/spaces.cpp

+    switch (metric) {
+    case VecSimMetric_Cosine:
+    case VecSimMetric_IP:
+        return IP_FP16_GetDistFunc(dim, nullptr, alignment);


why does this function get a void pointer that is always null? can we just not pass it?

It was part of the optimization bit map refactor
It was left mainly for testing purposes

consider cleaning it now? I don't think it is needed for the test either (unless we suspect the hardware will change between calls)

we pass a specific optimization in the tests to enforce lower optimization
see test_spaces.cpp

src/VecSim/types/bfloat16.h

src/VecSim/types/float16.h

src/python_bindings/bindings.cpp

add default ctor

GuyAv46 · 2024-05-09T14:36:26Z

tests/benchmark/data/serializer.py

@@ -128,7 +141,8 @@
 TYPES_ATTR = {
    VecSimType_FLOAT32: {"size_in_bytes": 4, "vector_type": np.float32},
    VecSimType_FLOAT64: {"size_in_bytes": 8, "vector_type": np.float64},
-    VecSimType_BFLOAT16: {"size_in_bytes": 2, "vector_type": bfloat16}
+    VecSimType_BFLOAT16: {"size_in_bytes": 2, "vector_type": bfloat16},
+    VecSimType_FLOAT16: {"size_in_bytes": 2, "vector_type": np.float16}


Suggested change

VecSimType_FLOAT16: {"size_in_bytes": 2, "vector_type": np.float16}

VecSimType_FLOAT16: {"size_in_bytes": 2, "vector_type": np.float16},

GuyAv46 · 2024-05-09T14:42:31Z

tests/unit/CMakeLists.txt

@@ -52,4 +54,5 @@ gtest_discover_tests(test_bruteforce)
 gtest_discover_tests(test_allocator)
 gtest_discover_tests(test_spaces)
 gtest_discover_tests(test_common)
-gtest_discover_tests(test_bf16)
+gtest_discover_tests(test_bf16 TEST_PREFIX BF16UNIT_)


What's this prefix?

it adds a prefix to all the tests' names in this file making it easier to run a specific set with ctest. for example:
make unit_test CTEST_ARGS="-R BF16UNIT_"

Should we add it to other types as well? (Not in this PR)

alonre24

LGTM

alonre24 · 2024-05-09T16:56:24Z

change bfloat and fp16 to struct typedef to avoid ambiguty between them

Describe the changes in the pull request

A clear and concise description of what the PR is solving.

Which issues this PR fixes

#...

MOD...

Main objects this PR modified

...

...

Mark if applicable

This PR introduces API changes

This PR introduces serialization changes

Let's try to better use the PR template... in this particular case we do introduce (small) API change

GuyAv46 · 2024-05-10T05:21:49Z

.github/workflows/flow-temp

declaring a default ctor as constexpr is allowed as of c++20, and this feature in particular is supported as of gcc10 I don't see any benefit using this feature...

GuyAv46 · 2024-05-12T07:28:28Z

src/VecSim/types/bfloat16.h

@@ -13,7 +13,7 @@
 namespace vecsim_types {
 struct bfloat16 {
    uint16_t val;
-    constexpr bfloat16() = default;
+    bfloat16() = default;


Is this a compiler version issue? How come it works with fp16? Can we try this option?

Suggested change

bfloat16() = default;

constexpr bfloat16() noexcept {}

declaring a default ctor as constexpr is allowed as of c++20, and this feature in particular is supported as of gcc10
fp16 worked because it didn't have constexpr at first place.
Anyway, since we have another ctor that is constexpr, it doesn't matter.

GuyAv46 · 2024-05-12T07:34:35Z

.github/workflows/flow-temp.yml

@@ -9,13 +9,14 @@ name: temporary testing

 on:
  push:
-    branches-ignore: ['**'] # ignore all branches. Comment this line to run your workflow below on every push.
+    # branches-ignore: ['**'] # ignore all branches. Comment this line to run your workflow below on every push.


* upgrade codcov instance to c7i * [MOD-6763] bfloat16 - distance calculation functions (#445) * update arch_opt macros in bf16 BM * fix macro check * Remove arch_opt enum and use cpu features flags instead (#459) * FP16 distance functions (#453) * [MOD-6764] Introduce bfloat16 datatype (#454) * Introduce bfloat16 datatype add Datatype to `VecSimIndexAbstract` template args. add template to SetDistFunc and have specialized implemntation for each <dataType, distType> combintation now the dist function pointer argument is not neseccesary, we can simple return the function. move using dist_func_t to spaces_aux.h (should be in a file included by both L2_spaces.h/IP.spaces.h and spaces.h) * move getdistance imp to spaces.cpp * move normalize function from vec_utils to spaces: spaces.h: get normalize function. spaces.cpp: specialized templates according to the datatype for the normalization functions spaces/normalize/normalize_naive.h: templated naive implementations of normalization functions VecSimIndexAbstract must have both DataType and DistType add VecSimType_BFLOAT16 update vec_utile accordingly bfloat16.h: add bfloat16_to_float32 for little and big TieredFactory::NewIndex calls TieredHNSWFactory::TieredFactory::NewIndex instead of implmentation duplication add bfloat to index factory for NewIndex and EstimateInitialSize * add no_opt functions tests for bfloat16 also add normalization function test for bf16 * fixed nohrmalized value * add bf16 benchmarks: add serialized hnsw bf16 files hnswfactory: add ctor from file for bf16 add fitMemory and run it at the begining of each benchmark to tighten memory. * update bm_files script * add flow tests to bf16 bruteforce: L2 batch iterator range query multivalue hnsw: serialization L2 batch iterator range multi tiered: create search and insert * add bfloat16 module to toml * upgrade scipy nupy * also use python 3.10 * upgrade PR python to 3.10 * update serializer * bm: fix casts unit tests: add test_bf16 * more unit tests * tiered tests * add index_type assertion ti hnsw serialization test add desgnated classes for bf and flat in bf16 unit test * unit test final * repalce cast to dynamic casts * add get neighors test to tiered add normalize test for bfloat16 in testcommon * use bit_cast * upgrade codecov machine * Revert "upgrade codecov machine" This reverts commit 7619b05. * Revert "use bit_cast" This reverts commit 59b8fab. * fix alignment intialization by changing the order * make bfloat16_to_float32 templated accroding to system's endianess * eliminate default in GetDistFunc add test for invalid metric * add test fot fit memory * split fit memory tests * run merge queue flow * add override to fit memory * upload valgrind artifacts on failure * _mm256_loadu_epi16 is supported as of gcc1, use _mm256_lddqu_si256 instead * fix fit memory test (#461) * fix fit memory test * remove xenial checks * remove irrelevant comment * free index in seralization test * Remove avx512_BW_VL (#460) * remove bw_vl arch (not needed, avx512 is enough) * remove xenial from merge queue flow * fix fit memory test (#461) * fix fit memory test * remove xenial checks * remove irrelevant comment * free index in seralization test * install gcc10 bionic * install newer vg ubuntu18 * exit valgrind directory * [MOD-6775] introduce fp16 type! (#462) * remove bw_vl arch (not needed, avx512 is enough) * remove xenial from merge queue flow * introduce fp16 type! change bfloat and fp16 to struct typedef to avoid ambiguty between them * added flow tests * fp16 benchmarks * fix hnsw flow test * add float616 to serializing script add fp16 files to bm files decrease testSizeEstimation dim size to make it run fasetr in debug * set log call back to null in benchmarks * remove struct typedef add default ctor * add comma seriaizer file * disable temp flow * disabe for real * remove constexpr from bfloat default ctor declaring a default ctor as constexpr is allowed as of c++20, and this feature in particular is supported as of gcc10 I don't see any benefit using this feature... * test focal * disable temp flow --------- Co-authored-by: alonre24 <alonreshef24@gmail.com>

* remove bw_vl arch (not needed, avx512 is enough) * remove xenial from merge queue flow * introduce fp16 type! change bfloat and fp16 to struct typedef to avoid ambiguty between them * added flow tests * fp16 benchmarks * fix hnsw flow test * add float616 to serializing script add fp16 files to bm files decrease testSizeEstimation dim size to make it run fasetr in debug * set log call back to null in benchmarks * remove struct typedef add default ctor * add comma seriaizer file * disable temp flow * disabe for real * remove constexpr from bfloat default ctor declaring a default ctor as constexpr is allowed as of c++20, and this feature in particular is supported as of gcc10 I don't see any benefit using this feature... * test focal * disable temp flow (cherry picked from commit 704282b)

* upgrade codcov instance to c7i (cherry picked from commit b5cc93b) * [MOD-6763] bfloat16 - distance calculation functions (#445) (cherry picked from commit 850d6a8) * update arch_opt macros in bf16 BM (cherry picked from commit 253dcd3) * fix macro check (cherry picked from commit 26b047d) * Remove arch_opt enum and use cpu features flags instead (#459) (cherry picked from commit f98793a) * FP16 distance functions (#453) (cherry picked from commit 9420e77) * [MOD-6764] Introduce bfloat16 datatype (#454) * Introduce bfloat16 datatype add Datatype to `VecSimIndexAbstract` template args. add template to SetDistFunc and have specialized implemntation for each <dataType, distType> combintation now the dist function pointer argument is not neseccesary, we can simple return the function. move using dist_func_t to spaces_aux.h (should be in a file included by both L2_spaces.h/IP.spaces.h and spaces.h) * move getdistance imp to spaces.cpp * move normalize function from vec_utils to spaces: spaces.h: get normalize function. spaces.cpp: specialized templates according to the datatype for the normalization functions spaces/normalize/normalize_naive.h: templated naive implementations of normalization functions VecSimIndexAbstract must have both DataType and DistType add VecSimType_BFLOAT16 update vec_utile accordingly bfloat16.h: add bfloat16_to_float32 for little and big TieredFactory::NewIndex calls TieredHNSWFactory::TieredFactory::NewIndex instead of implmentation duplication add bfloat to index factory for NewIndex and EstimateInitialSize * add no_opt functions tests for bfloat16 also add normalization function test for bf16 * fixed nohrmalized value * add bf16 benchmarks: add serialized hnsw bf16 files hnswfactory: add ctor from file for bf16 add fitMemory and run it at the begining of each benchmark to tighten memory. * update bm_files script * add flow tests to bf16 bruteforce: L2 batch iterator range query multivalue hnsw: serialization L2 batch iterator range multi tiered: create search and insert * add bfloat16 module to toml * upgrade scipy nupy * also use python 3.10 * upgrade PR python to 3.10 * update serializer * bm: fix casts unit tests: add test_bf16 * more unit tests * tiered tests * add index_type assertion ti hnsw serialization test add desgnated classes for bf and flat in bf16 unit test * unit test final * repalce cast to dynamic casts * add get neighors test to tiered add normalize test for bfloat16 in testcommon * use bit_cast * upgrade codecov machine * Revert "upgrade codecov machine" This reverts commit 7619b05. * Revert "use bit_cast" This reverts commit 59b8fab. * fix alignment intialization by changing the order * make bfloat16_to_float32 templated accroding to system's endianess * eliminate default in GetDistFunc add test for invalid metric * add test fot fit memory * split fit memory tests (cherry picked from commit e018d91) * run merge queue flow (cherry picked from commit 4670c59) * add override to fit memory (cherry picked from commit f994c79) * upload valgrind artifacts on failure (cherry picked from commit 7ece68d) * _mm256_loadu_epi16 is supported as of gcc1, use _mm256_lddqu_si256 instead (cherry picked from commit c521d82) * fix fit memory test (#461) * fix fit memory test * remove xenial checks * remove irrelevant comment * free index in seralization test (cherry picked from commit a0b70c8) * Remove avx512_BW_VL (#460) * remove bw_vl arch (not needed, avx512 is enough) * remove xenial from merge queue flow * fix fit memory test (#461) * fix fit memory test * remove xenial checks * remove irrelevant comment * free index in seralization test * install gcc10 bionic * install newer vg ubuntu18 * exit valgrind directory (cherry picked from commit d2e1d4f) * [MOD-6775] introduce fp16 type! (#462) * remove bw_vl arch (not needed, avx512 is enough) * remove xenial from merge queue flow * introduce fp16 type! change bfloat and fp16 to struct typedef to avoid ambiguty between them * added flow tests * fp16 benchmarks * fix hnsw flow test * add float616 to serializing script add fp16 files to bm files decrease testSizeEstimation dim size to make it run fasetr in debug * set log call back to null in benchmarks * remove struct typedef add default ctor * add comma seriaizer file * disable temp flow * disabe for real * remove constexpr from bfloat default ctor declaring a default ctor as constexpr is allowed as of c++20, and this feature in particular is supported as of gcc10 I don't see any benefit using this feature... * test focal * disable temp flow (cherry picked from commit 704282b) --------- Co-authored-by: meiravgri <meirav.grimberg@redis.com> Co-authored-by: meiravgri <109056284+meiravgri@users.noreply.github.com> Co-authored-by: alonre24 <alonreshef24@gmail.com>

meiravgri added 3 commits May 6, 2024 14:19

remove bw_vl arch (not needed, avx512 is enough)

6858240

remove xenial from merge queue flow

ef2c719

introduce fp16 type!

26088bf

change bfloat and fp16 to struct typedef to avoid ambiguty between them

meiravgri changed the title ~~introduce fp16 type!~~ [MOD-6775] introduce fp16 type! May 7, 2024

meiravgri added 3 commits May 7, 2024 15:09

added flow tests

fce1754

fp16 benchmarks

55ef50f

fix hnsw flow test

4eb7056

Base automatically changed from meiravg_remove_AVX512_BW_VL to meiravg_feature_bfloat16 May 8, 2024 09:50

meiravgri added 2 commits May 8, 2024 10:55

Merge branch 'meiravg_feature_bfloat16' into meiravg_fp16_vecsim_api

8cd5673

add float616 to serializing script

29bbd7e

add fp16 files to bm files decrease testSizeEstimation dim size to make it run fasetr in debug

meiravgri requested review from alonre24 and GuyAv46 May 8, 2024 12:28

meiravgri added bm-basics-fp16-single bm-basics-fp32-single bm-basics-bf16-single labels May 8, 2024

set log call back to null in benchmarks

2525371

GuyAv46 reviewed May 9, 2024

View reviewed changes

remove struct typedef

ac17793

add default ctor

GuyAv46 reviewed May 9, 2024

View reviewed changes

alonre24 previously approved these changes May 9, 2024

View reviewed changes

add comma seriaizer file

775c41c

meiravgri dismissed alonre24’s stale review via 775c41c May 10, 2024 04:37

meiravgri added 2 commits May 10, 2024 04:40

disable temp flow

f94d731

disabe for real

db2521d

meiravgri requested review from GuyAv46 and alonre24 May 10, 2024 04:48

GuyAv46 previously approved these changes May 10, 2024

View reviewed changes

.github/workflows/flow-temp Outdated

Copy link

Collaborator

GuyAv46 May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

meiravgri reacted with confused emoji

meiravgri added this pull request to the merge queue May 10, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 10, 2024

remove constexpr from bfloat default ctor

0edd29d

declaring a default ctor as constexpr is allowed as of c++20, and this feature in particular is supported as of gcc10 I don't see any benefit using this feature...

meiravgri dismissed GuyAv46’s stale review via 0edd29d May 11, 2024 05:47

test focal

4369584

meiravgri requested a review from GuyAv46 May 12, 2024 04:16

GuyAv46 reviewed May 12, 2024

View reviewed changes

disable temp flow

29c1773

GuyAv46 approved these changes May 12, 2024

View reviewed changes

meiravgri enabled auto-merge May 12, 2024 08:19

meiravgri added this pull request to the merge queue May 12, 2024

Merged via the queue into meiravg_feature_bfloat16 with commit 704282b May 12, 2024
24 checks passed

meiravgri deleted the meiravg_fp16_vecsim_api branch May 12, 2024 09:26

	VecSimType_FLOAT16: {"size_in_bytes": 2, "vector_type": np.float16}
	VecSimType_FLOAT16: {"size_in_bytes": 2, "vector_type": np.float16},

[MOD-6775] introduce fp16 type! #462

[MOD-6775] introduce fp16 type! #462

Uh oh!

Conversation

meiravgri commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New

Changes

Uh oh!

codecov bot commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GuyAv46 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alonre24 left a comment

Choose a reason for hiding this comment

Uh oh!

alonre24 commented May 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

meiravgri commented May 7, 2024 •

edited

Loading

codecov bot commented May 7, 2024 •

edited

Loading