-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add ai rag provider #1129
Add ai rag provider #1129
Conversation
Use default prompt if prompt.custom is not provided, but messages are.
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.
A few initial comments
packages/ai/src/types.ts
Outdated
| ContentBlockStop | ||
| MessageDelta | ||
| MessageStop; | ||
const _edgedbRagChunkSchema = z.discriminatedUnion("type", [ |
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.
We are not exporting this Zod schema or using it in this file: why are we using a Zod schema instead of writing this as a TypeScript interface?
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 used this in the vercel provider so it was easier to copy paste it.
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.
Could we export it then maybe? Not a blocker.
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.
we can export it, I just dont understand/see when would someone need it?
we export:
export type StreamingMessage = z.infer<typeof _edgedbRagChunkSchema>;
which will user most probably use?
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 just dont understand/see when would someone need it?
Oh, I thought you mean that you are using a copy-pasted version of this schema in the Vercel provider, no? I'm saying export it here and import it in the Vercel provider instead of copy-pasting it. Or export it from the Vercel provider and import the type here. Whichever makes sense.
This is not a blocker for merging, it just feels a little strange here to have an essentially "private" Zod schema that isn't used for anything, but has to be kept in sync across two packages.
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.
hm okay I can do this, even tho it makes sense and is better to have one source of truth it feels a bit weird to me to install the whole lib just because one type, but maybe that's usually the way to go in this kind of situations?
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.
Oh, I hadn't noticed: Does the Vercel AI SDK Provider not use the @edgedb/ai
package for anything already?
There is a new Vercel AI SDK version out - 4. :D Let's complete this and see how we proceed. |
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.
A few more comments. Still not finished reviewing everything, but wanted to leave these thoughts.
packages/ai/src/types.ts
Outdated
| ContentBlockStop | ||
| MessageDelta | ||
| MessageStop; | ||
const _edgedbRagChunkSchema = z.discriminatedUnion("type", [ |
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.
Could we export it then maybe? Not a blocker.
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.
A few more comments. In general, I think we can merge this as-is and follow up on some of the ideas and comments I laid out to keep this PR from getting to stale.
const connectConfig = await client.resolveConnectionParams(); | ||
|
||
const fetch = await getAuthenticatedFetch( | ||
connectConfig, | ||
httpSCRAMAuth, | ||
"ext/ai/", | ||
); |
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.
Mostly leaving this as a note for the future:
We can avoid having to construct an authenticated fetch in every package by having the Client
instance return one. The client has already had to construct an authenticated fetch client, and in this particular case, if you are in a Node environment, the one the driver constructs uses the native node crypto instead of browser crypto like this.
Maybe an API like:
const fetch = await client.getAuthenticatedFetch({
path: "ext/ai/",
});
Remove undefined props from return object in edgedb-prepare-tools Use type guard for RagRequest Add buildPrompt & buildQuery func to provider & simplify postJsonToApi obj creation Add handleResponseError func and use it in fetchRag
No description provided.