Skip to content
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

EdgeDB AI provider #1121

Closed
wants to merge 32 commits into from
Closed

EdgeDB AI provider #1121

wants to merge 32 commits into from

Conversation

diksipav
Copy link
Contributor

@diksipav diksipav commented Oct 17, 2024

I think we need a call to discuss what we want to support with the provider and extension (or maybe I'm just not aligned with the goals and provider should just be built to support current capabilities of the extension).

Examples of how we can use this with the Vercel SDK:
the completion route,
the chat route

This PR has the basic support for completion and chat that we also support with edgedb-js ai lib. I made it to work with the AI ext we have and its current capabilities. And it can be used as this but definitely needs improvements in order to have fully compatible interfaces with Vercel LanguageModelV1 provider.

TODO: I need to add README and update some identifiers and interface names throughout the provider since we are not really a language model.

QUESTIONS:

  1. We don't support any of LLM settings
    maxTokens,
    temperature,
    topP,
    topK,
    frequencyPenalty,
    presencePenalty,
    stopSequences,
    responseFormat,
    seed etc,
    so I don't know do we wan't to add support for these or should I return an error from the provider if user tries to set any of these.

  2. Do we want to support setting for safe_prompt?

  3. Do we support structured outputs?

  4. Do we have support for tools?

  5. In the output our RAG returns it doesn't return:

usage: {
  promptTokens: ...,
  completionTokens: ...,
},

and some other things that are used/returned from the Vercel LanguageModelV1 provider. So I return 0 for these 2 values for now from the provider.

  1. Do we want to support different settings for different LLMs or we navigate more towards supporting basic works and settings all LLMs support. I mean some of them even support images and audio.

@diksipav diksipav requested a review from scotttrinh October 17, 2024 19:33
@diksipav diksipav changed the title Stream ai EdgeDB AI provider Oct 17, 2024
@1st1
Copy link
Member

1st1 commented Oct 17, 2024

I'd like @deepbuzin to also look at this

@diksipav diksipav requested a review from anbuzin October 18, 2024 08:58
@diksipav diksipav marked this pull request as draft October 18, 2024 15:51
@diksipav
Copy link
Contributor Author

Here are the examples of how we can use this with the Vercel SDK:
the completion route,
the chat route

@scotttrinh
Copy link
Collaborator

so I don't know do we wan't to add support for these or should I return an error from the provider if user tries to set any of these.

https://github.com/vercel/ai/blob/65e108f94f40b80890b00ccc12eeb04c792a4b92/packages/mistral/src/mistral-chat-language-model.ts#L71-L78

Yeah it looks like there is some expectation that we return some warnings.

Do we want to support setting for safe_prompt?

Do we support structured outputs?

Do we have support for tools?

We can skip this stuff for now.

So I return 0 for these 2 values for now from the provider.

Looks like the Mistral provider uses NaN 😂 https://github.com/vercel/ai/blob/65e108f94f40b80890b00ccc12eeb04c792a4b92/packages/mistral/src/mistral-chat-language-model.ts#L242-L245

Do we want to support different settings for different LLMs or we navigate more towards supporting basic works and settings all LLMs support. I mean some of them even support images and audio.

I think for now we support just the API that our AI extension exposes and we treat it as if we're a really restricted LLM that has built in RAG. Developers will be configuring the AI extension itself to take advantage of features in the underlying model, but we'll need to expose it directly from the extension in order for this provider to take advantage of it.

debug(
` - CLI found in PATH at: ${location} (resolved to: ${actualLocation})`,
);
if (locations) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nit: maybe just treat this as an early return instead of wrapping this whole section in an if?

if (locations == null) {
  debug("  - No CLI found in PATH.");
  return null;
}

I'm annoyed that we aren't catching this in our tests 🫤

@diksipav
Copy link
Contributor Author

diksipav commented Nov 6, 2024

RAG provider should be fine. But I think we need to extend capabilities of the AI binding. It should support other parameters besides the query and also function calling.

@diksipav
Copy link
Contributor Author

diksipav commented Nov 6, 2024

I created a new cleaner PR for this: #1129.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants