Buffers, IP, Factories & more #40

asilvas · 2023-09-05T01:49:54Z

Resolves #37 and much more:

Considerable changes to accommodate new features and future growth (which wouldn't have made sense had I not added new features in same PR)
Support for IndexFlatIP
Support for serialization via toBuffer & fromBuffer (10x+ faster than re-adding vectors, but still somehow 2x slower than reading from disk)
Support for factory indexes to unlock most of faiss potential
Support for training

Sorry for the big sweeping changes. I don't like to put this much into 1 PR, but due to considerable complications with NAPI they were necessary.

ewfian · 2023-09-05T05:01:52Z

@asilvas Thanks a lot for this great PR, please allow me some time to review.

asilvas · 2023-09-05T13:12:13Z

src/faiss.cc

  {
    Napi::Env env = info.Env();
-    if (info[0].IsExternal())
+
+    if (!info.IsConstructCall())


greatly simplified the constructors responsibilities and removed the need to pass around external data. This moves the responsibility of each feature to different functions.

asilvas · 2023-09-09T16:16:01Z

Let me know if there's anything I can do to progress this PR. Thanks

ewfian · 2023-09-11T12:11:16Z

@asilvas Sorry for slow reply. I have been occupied with work recently. I'll deal with it later this week.

ewfian · 2023-09-11T12:34:38Z

test/Index.test.js

+    });
+
+    it('Flat /w IP', () => {
+      const index = Index.fromFactory(2, 'Flat', 0 /* METRIC_INNER_PRODUCT */);


add MetricType enum into lib/index.js, so that we can use enum directly

const faiss = require('bindings')('faiss-node'); faiss.MetricType = void 0; var MetricType; (function (MetricType) { MetricType[MetricType["METRIC_INNER_PRODUCT"] = 0] = "METRIC_INNER_PRODUCT"; MetricType[MetricType["METRIC_L2"] = 1] = "METRIC_L2"; MetricType[MetricType["METRIC_L1"] = 2] = "METRIC_L1"; MetricType[MetricType["METRIC_Linf"] = 3] = "METRIC_Linf"; MetricType[MetricType["METRIC_Lp"] = 4] = "METRIC_Lp"; MetricType[MetricType["METRIC_Canberra"] = 20] = "METRIC_Canberra"; MetricType[MetricType["METRIC_BrayCurtis"] = 21] = "METRIC_BrayCurtis"; MetricType[MetricType["METRIC_JensenShannon"] = 22] = "METRIC_JensenShannon"; MetricType[MetricType["METRIC_Jaccard"] = 23] = "METRIC_Jaccard"; })(MetricType || (faiss.MetricType = MetricType = {})); module.exports = faiss;

ewfian · 2023-09-17T10:17:23Z

src/faiss.cc

+    std::string fname;
+    if (info[0].IsExternal())
+    {
+      fname = *info[0].As<Napi::External<std::string>>().Data();


It seems that this branch will not reach. maybe we can get fname directly:
std::string fname = info[0].As<Napi::String>().Utf8Value();

ewfian · 2023-09-17T10:17:53Z

src/faiss.cc

+    std::string description;
+    if (info[1].IsExternal())
+    {
+      description = *info[1].As<Napi::External<std::string>>().Data();


same as previous comment

ewfian · 2023-09-17T10:28:16Z

src/faiss.cc

+  static Napi::Object NewInstance(Napi::Env env, const std::vector<napi_value> &args)
+  {
+    Napi::EscapableHandleScope scope(env);
+    Napi::Object obj = GetInstanceData(env, T::CLASS_NAME).New(args);


I found before in this experimental branch that it is simpler to store the pointer to the constructor using the following way:
https://github.com/ewfian/faiss-node/blob/feat/support-IndexFlatIP/src/faiss.cc#L71

inline static Napi::FunctionReference *constructor = new Napi::FunctionReference();

This eliminates the need to use GetInstanceData and AttachInstanceData

Please confirm if you can modify it as above?

My original changes had used that pattern but was having stability issues with the Bun runtime so I opted for the more complex pattern. It resolved one of the bugs, but still has stability issues, so I'll revert for now until Bun is more reliable with NAPI packages.

ewfian · 2023-09-18T05:02:04Z

src/faiss.cc

+
+  static Napi::Object Init(Napi::Env env, Napi::Object exports)
+  {
+    Napi::HandleScope scope(env);


It seems that HandleScope is no longer needed.

asilvas · 2023-09-19T00:18:46Z

src/faiss.cc

+    });
+    // clang-format on
+
+    constructor = new Napi::FunctionReference();


Not as clean as constructing within Base, but that was causing stability issues. I don't fully understand the difference, but this works around whatever was going on.

What kind of stability issues is it and how to reproduce it?

Bun runtime (https://bun.sh/) segfaults via bun test.

bun test still failed in test/IndexFlatL2.test.js

after remove this case, it works

my test is that the following code will never be reached.

and the Error was thrown by napi internally

Yeah I'm fine merging with that issue for now since I created the issue on the Bun side. oven-sh/bun#4526

thank you, i will remove this test case and the following useless code

seems that this case still failed on bun test

Yes all tests which rely on throwing won't work in Bun until the issue I linked is resolved. That's fine, and not this packages problem. Technically we can work around it by replacing all the exception handling to use native exceptions, but I can throw up another PR later.

Invalid argument is not thrown by our code and it was thrown by napi, maybe the bun was not well handle this case

ewfian · 2023-09-19T14:44:24Z

Another point that I'm a little concerned about is that Index is an abstract class in faiss. It has only a few methods, but we warped many things that are not originally on Index.
maybe we should have a IndexFlat class and a pure Index class?

btw, I'm sorry that it's time for me to go to bed. i will reply tomorrow.

asilvas · 2023-09-19T15:15:46Z

Another point that I'm a little concerned about is that Index is an abstract class in faiss. It has only a few methods, but we warped many things that are not originally on Index. maybe we should have a IndexFlat class and a pure Index class?

btw, I'm sorry that it's time for me to go to bed. i will reply tomorrow.

Index is abstract, and any index you've created that does not implement a function (ala add_with_ids) will throw. The behavior is highly consistent with the native implementation. All faiss-node needs to be is a wrapper -- let native lib throw when used incorrectly.

ewfian · 2023-09-20T11:26:51Z

Index is abstract, and any index you've created that does not implement a function (ala add_with_ids) will throw. The behavior is highly consistent with the native implementation. All faiss-node needs to be is a wrapper -- let native lib throw when used incorrectly.

I reconfirmed and it's as you said. But I will remove fromFactory from IndexFlatIP and IndexFlatL2

asilvas added 2 commits September 4, 2023 18:44

Buffers, IP, Factories & more

e5d29c6

Merge branch 'main' into buffers-base-factory

ff8e789

asilvas commented Sep 5, 2023

View reviewed changes

scope & instance data

9a027be

ewfian reviewed Sep 11, 2023

View reviewed changes

asilvas and others added 4 commits September 11, 2023 08:24

export MetricType enum

767eefc

fix d.ts

deffc1c

fix JSDoc

c6c3a3f

fix type of sub class

2d4af02

ewfian reviewed Sep 17, 2023

View reviewed changes

asilvas added 2 commits September 17, 2023 13:26

feedback changes

2698f15

fix factory metric

7a78b3b

ewfian reviewed Sep 18, 2023

View reviewed changes

remove handlescope

f5d581e

asilvas requested a review from ewfian September 18, 2023 19:42

asilvas mentioned this pull request Sep 18, 2023

Ability to encode/decode buffers when FS not available? #37

Closed

inline constructor causes stability issues

53eb992

asilvas commented Sep 19, 2023

View reviewed changes

ewfian added 3 commits September 19, 2023 19:56

fix inline variables warning

edba6f1

update JSDoc

98595e6

remove unreached code and its test case

0ecf23a

remove fromFactory from IndexFlatL2 and IndexFlatIP

8c0fc9e

update docs and example

2fabf92

ewfian merged commit 0171f4f into ewfian:main Sep 20, 2023
36 checks passed

asilvas deleted the buffers-base-factory branch September 20, 2023 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffers, IP, Factories & more #40

Buffers, IP, Factories & more #40

asilvas commented Sep 5, 2023 •

edited

Loading

ewfian commented Sep 5, 2023

asilvas Sep 5, 2023

asilvas commented Sep 9, 2023

ewfian commented Sep 11, 2023

ewfian Sep 11, 2023

ewfian Sep 17, 2023

ewfian Sep 17, 2023

ewfian Sep 17, 2023 •

edited

Loading

asilvas Sep 17, 2023 •

edited

Loading

ewfian Sep 18, 2023

asilvas Sep 19, 2023

ewfian Sep 19, 2023

asilvas Sep 19, 2023

ewfian Sep 19, 2023

asilvas Sep 19, 2023

ewfian Sep 19, 2023

ewfian Sep 19, 2023

asilvas Sep 19, 2023 •

edited

Loading

ewfian Sep 19, 2023

ewfian commented Sep 19, 2023 •

edited

Loading

asilvas commented Sep 19, 2023

ewfian commented Sep 20, 2023

Buffers, IP, Factories & more #40

Buffers, IP, Factories & more #40

Conversation

asilvas commented Sep 5, 2023 • edited Loading

ewfian commented Sep 5, 2023

Choose a reason for hiding this comment

asilvas commented Sep 9, 2023

ewfian commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewfian Sep 17, 2023 • edited Loading

Choose a reason for hiding this comment

asilvas Sep 17, 2023 • 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

Choose a reason for hiding this comment

asilvas Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewfian commented Sep 19, 2023 • edited Loading

asilvas commented Sep 19, 2023

ewfian commented Sep 20, 2023

asilvas commented Sep 5, 2023 •

edited

Loading

ewfian Sep 17, 2023 •

edited

Loading

asilvas Sep 17, 2023 •

edited

Loading

asilvas Sep 19, 2023 •

edited

Loading

ewfian commented Sep 19, 2023 •

edited

Loading