-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(nodejs): add better error handling when missing embedding functions #1290
Conversation
node/src/index.ts
Outdated
async add( | ||
data: Array<Record<string, unknown>> | ArrowTable | ||
): Promise<number> { | ||
const schema = await this.schema | ||
let tbl: ArrowTable | ||
const schema = await this.schema; | ||
|
||
let tbl: ArrowTable; | ||
|
||
const schemaWithoutEmbeddings = validateSchemaEmbeddings( | ||
schema, | ||
data, | ||
this._embeddings | ||
); | ||
|
||
if (data instanceof ArrowTable) { | ||
tbl = data | ||
tbl = data; | ||
} else { | ||
tbl = makeArrowTable(data, { schema }) | ||
tbl = makeArrowTable(data, { schema: schemaWithoutEmbeddings }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the relevant code change. Every thing else is just from running the linter
node/src/index.ts
Outdated
} | ||
} | ||
|
||
function validateSchemaEmbeddings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the relevant code change. Every thing else is just from running the linter
@wjones127 @westonpace, I'm unable to reproduce the CI failures locally. Could one of you take a look and see if you are able to reproduce. |
@universalmind303 It does reproduce locally for me. It appears that it's this line:
I suspect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be reading this wrong but does this prevent a user from providing a fixed-size-list column directly without using the embeddings function? In other words, if a user is calculating their own embeddings (not using lancedb to do it) will it still work?
Also, any change added to node/src/arrow.ts
is probably appropriate for nodejs/src/arrow.ts
.
Users can still provide their own embeddings manually without using the Example: const schema = new Schema([
new Field("id", new Int32()),
new Field("text", new Utf8()),
new Field(
"vector",
new FixedSizeList(2, new Field("item", new Float32(), true))
),
]); user provides schema and manual embeddings (OK) const data = [
{ id: 1, text: "foo", vector: [0.1, 0.2] },
{ id: 2, text: "bar", vector: [0.3, 0.4] },
];
let table = await con.createTable({
name: "test",
data,
schema,
}); user provides schema and embedding function (OK) const embeddingFunction = {...}
const data = [
{ id: 1, text: "foo"},
{ id: 2, text: "bar"},
];
let table = await con.createTable({
name: "test",
data,
schema,
embeddingFunction
}); User provides schema, but no embeddings or function (Not OK) const data = [
{ id: 1, text: "foo"},
{ id: 2, text: "bar"},
];
let table = await con.createTable({
name: "test",
data,
schema
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, looks good.
i accidentally left a console.log when doing #1290
note:
running the default lint command
npm run lint -- --fix
seems to have made a lot of unrelated changes.