feat: js embedding registry #1308

universalmind303 · 2024-05-15T21:06:30Z

todo:

add more tests
add more examples

…edding

westonpace

Some initial questions

westonpace · 2024-05-15T21:43:17Z

nodejs/lancedb/arrow.ts

+  for (const functionEntry of functions.values()) {
+    const sourceColumn = columns[functionEntry.sourceColumn];
+    const destColumn = functionEntry.vectorColumn ?? "vector";
+    if (sourceColumn === undefined) {


This is just for my own knowledge but how do you decide between sourceColumn === undefined and sourceColumn === undefined || sourceColumn === null in these validation checks? (i.e. how do you know when you only need to check for one and not both)

so null usually has to be explicitly set, while undefined is just the absence of a value. So as long as we never explicitly start setting values to null, we generally only need to check for undefined.

nodejs/lancedb/arrow.ts

westonpace · 2024-05-15T21:46:20Z

nodejs/lancedb/arrow.ts

+  const newTable = new ArrowTable(columns);
+  return alignTable(newTable, schema);
+}
+
 /** Helper function to apply embeddings to an input table */
 async function applyEmbeddings<T>(


What's the relationship between applyEmbeddings and appendVectorColumn?

so I want to eventually get rid of applyEmbeddings in favor of appendVectorColumn to consolidate functionality & better align with the python implementation. It's essentially applyEmbeddingsFromSchema while the current applyEmbeddings is applyEmbeddingsFromEmbeddingFunctions

nodejs/lancedb/connection.ts

westonpace · 2024-05-15T21:48:18Z

nodejs/lancedb/connection.ts

@@ -65,6 +66,8 @@ export interface CreateTableOptions {
   * The available options are described at https://lancedb.github.io/lancedb/guides/storage/
   */
  storageOptions?: Record<string, string>;
+  schema?: Schema;
+  embeddingFunction?: EmbeddingFunctionConfig;


Minor nit: maybe use the name embeddingConfig instead of embeddingFunction since there can be multiple functions?

Actually, does this support multiple embedding functions?

the current implementation does not, but it should be pretty easy to add that in a follow up.

westonpace · 2024-05-15T21:51:43Z

nodejs/lancedb/embedding/embedding_function.ts


  /**
-   * Should the source column be excluded from the resulting table
+   * vectorField is used in combination with `LanceSchema` to provide a declarative data model


I'm not sure I understand this comment

This is the JS equivalent to python's func.VectorField. I'm open to suggestions on improving the docs here.

Would a user override this? I'm wondering if it would be more appropriate as a private free function, so it's hidden from the users.

In JS, you can override anything, anywhere if you try hard enough.

So we can't explicitly forbid a user from overriding this, but not declaring it as abstract generally means it's not meant to be overridden.

westonpace · 2024-05-15T21:52:36Z

nodejs/lancedb/embedding/embedding_function.ts

+  ndims(): number | undefined {
+    return undefined;
+  }


Should this just be abstract?

if it's abstract, it requires the user to provide an implementation, even if the function can return undefined. This just provides a default implementation so users don't need to implement it if they dont want.

nodejs/lancedb/embedding/registry.ts

…edding

wjones127

Got some initial comments. There are a some tests I'd like to see, but this is looking well designed so far.

nodejs/__test__/registry.test.ts

wjones127 · 2024-05-28T16:20:56Z

nodejs/lancedb/embedding/embedding_function.ts


  /**
-   * Should the source column be excluded from the resulting table
+   * vectorField is used in combination with `LanceSchema` to provide a declarative data model


Would a user override this? I'm wondering if it would be more appropriate as a private free function, so it's hidden from the users.

wjones127 · 2024-05-28T16:21:31Z

nodejs/lancedb/embedding/embedding_function.ts

-}
+  abstract computeSourceEmbeddings(
+    data: T[],
+  ): Promise<number[][] | Float32Array[] | Float64Array[]>;


I think we could allow Float16Array[] here as well.

These are the JS native Float32Array and Float64Array, not the arrow arrays. There is no native Float16Array in JS.

wjones127 · 2024-05-28T16:23:28Z

nodejs/lancedb/embedding/index.ts

+ * ```
+ */
+export function LanceSchema(
+  fields: Record<string, [DataType, Map<string, EmbeddingFunction>] | DataType>,


Does Record preserve order? I worry we can't give the column ordering users would expect. Column order does matter in our schemas right now, I believe. I could be wrong about this, though.

String keys are ordered by insertion order, so this should always be deterministic.

wjones127 · 2024-05-28T16:58:07Z

nodejs/__test__/table.test.ts

@@ -420,6 +424,111 @@ describe("when dealing with versioning", () => {
  });
 });

+describe("embedding functions", () => {


One behavior I'm interested: What happens if you create a table with a custom embedding, then another process opens that table without having first registered that custom embedding. Does it error on open, or not until you add / query data? And does the error message make sense? This seems like it would be worth a unit test to confirm we have consistent behavior on this over time. Probably could use the reset to test this.

similar to the python api, this'll throw when adding, not on open

…edding

wjones127

This is looking good now. Thanks for the additional tests :)

nodejs/__test__/table.test.ts

Co-authored-by: Will Jones <willjones127@gmail.com>

universalmind303 added 5 commits May 15, 2024 12:05

wip

429c0b0

wip

08d5b74

feat: js embedding functions & registry

1e1afce

lint

1c2014a

update docs

3035e20

github-actions bot added the enhancement New feature or request label May 15, 2024

Merge branch 'main' of https://github.com/lancedb/lancedb into js-emb…

90afcc0

…edding

westonpace reviewed May 15, 2024

View reviewed changes

universalmind303 added 2 commits May 21, 2024 19:24

pr feedback

21f6f8e

Merge branch 'main' of https://github.com/lancedb/lancedb into js-emb…

2d7b44d

…edding

universalmind303 marked this pull request as ready for review May 22, 2024 00:27

universalmind303 added 5 commits May 21, 2024 19:39

update pkg lock

22ceb05

fmt

f07cbd2

fmt

a24207a

fmt

bed7b59

fix precommit

cc7c5fd

universalmind303 requested a review from westonpace May 22, 2024 01:09

pr feedback

e594fbd

universalmind303 requested a review from wjones127 May 26, 2024 15:55

wjones127 reviewed May 28, 2024

View reviewed changes

universalmind303 added 3 commits May 28, 2024 13:25

pr feedback

d425157

Merge branch 'main' of https://github.com/lancedb/lancedb into js-emb…

408bca3

…edding

Merge branch 'main' of https://github.com/lancedb/lancedb into js-emb…

f04c22d

…edding

wjones127 approved these changes May 29, 2024

View reviewed changes

nodejs/__test__/table.test.ts Outdated Show resolved Hide resolved

Update nodejs/__test__/table.test.ts

12ffdce

Co-authored-by: Will Jones <willjones127@gmail.com>

universalmind303 merged commit dbea3a7 into lancedb:main May 29, 2024
6 checks passed

universalmind303 mentioned this pull request May 29, 2024

Feature: port python's embedding funtions to Rust #994

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: js embedding registry #1308

feat: js embedding registry #1308

universalmind303 commented May 15, 2024 •

edited

westonpace left a comment

westonpace May 15, 2024

universalmind303 May 16, 2024

westonpace May 15, 2024

universalmind303 May 16, 2024

westonpace May 15, 2024

universalmind303 May 16, 2024

westonpace May 15, 2024

universalmind303 May 16, 2024

wjones127 May 28, 2024

universalmind303 May 28, 2024

westonpace May 15, 2024

universalmind303 May 16, 2024

wjones127 left a comment

wjones127 May 28, 2024

wjones127 May 28, 2024

universalmind303 May 28, 2024

wjones127 May 29, 2024

wjones127 May 28, 2024

universalmind303 May 28, 2024

wjones127 May 28, 2024

universalmind303 May 28, 2024

wjones127 left a comment

feat: js embedding registry #1308

feat: js embedding registry #1308

Conversation

universalmind303 commented May 15, 2024 • edited

westonpace 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

wjones127 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

wjones127 left a comment

Choose a reason for hiding this comment

universalmind303 commented May 15, 2024 •

edited