microsoft / semantic-kernel

Integrate cutting-edge LLM technology quickly and easily into your apps
https://aka.ms/semantic-kernel
MIT License
21.96k stars 3.27k forks source link

.Net: Adding Connectors.Mistral #4381

Closed Jenscaasen closed 5 months ago

Jenscaasen commented 11 months ago

I would like to add a Connector to Mistral API, the "recommended contribution workflow" states that i should make an issue about that first. I will take on the implementation.

If you choose to accept this, i need guidance on the following topics: -What should the test cases cover? Do you have a mistral key to make an end-to-end test? -After looking at the OpenAI Connector, what is the purpose of the IReadOnlyDictionary<string, object?> Attributes, should it be used in the Mistral Connector as well, and what are the requirements? -After looking at the OpenAI Connector: does the private static readonly Counter<int> s_promptTokensCounter need to be implemented in the mistral connector as well or is it only necessary for streaming (which mistral does not support)? -What is the preferred way of instantiating HttpClient?

You can find the working prototype here: https://github.com/Jenscaasen/semantic-kernel/tree/mistral/dotnet/src/Connectors/Connectors.Mistral

stephentoub commented 11 months ago

What is the preferred way of instantiating HttpClient?

Ideally it would follow the same general scheme used with the OpenAI connector, eg

matthewbolanos commented 11 months ago

We’d love your contribution! I’ll start working with the team to see how we can get a mistral key available. Are you able to share which service you’re wanting to build the connector for? Were you going to use Azure AI? Or somewhere else?

Let me answer some of your other questions. The attributes are used by the AI service selector to decide which service to use. I’d recommend exposing any attributes of Mistral you think are relevant (where it’s deployed, how many tokens it can use, etc). I don’t think you need to implement the counter since it’s private.

Jenscaasen commented 11 months ago

We’d love your contribution! I’ll start working with the team to see how we can get a mistral key available. Are you able to share which service you’re wanting to build the connector for? Were you going to use Azure AI? Or somewhere else?

Let me answer some of your other questions. The attributes are used by the AI service selector to decide which service to use. I’d recommend exposing any attributes of Mistral you think are relevant (where it’s deployed, how many tokens it can use, etc). I don’t think you need to implement the counter since it’s private.

Thank you. You do not need to provide a Mistral API Key if there should not be End-to-end tests that include an inference, as i have not seen this being done with the OpenAI connector (or i did not find it). For development, i got a mistral key.

I am planning on using the connector in custom copilots hosted as web services in Azure. I do not have experience with Azure AI (ai.azure.com, right?) + SK. If there are circumstances requiring specifics for Azure AI compatibility we best bring in another person to help with that.

Thank you for the hints @matthewbolanos and @stephentoub ,

matthewbolanos commented 11 months ago

Good to know! Go ahead and build using your Azure deployment. On the side, I'll see how much different it'll be to support deployments from ai.azure.com. Do you have links to docs you used to get your Azure deployment up and running?

Jenscaasen commented 11 months ago

Good to know! Go ahead and build using your Azure deployment. On the side, I'll see how much different it'll be to support deployments from ai.azure.com. Do you have links to docs you used to get your Azure deployment up and running?

Oh wait now i understand your inquiry. There are mistral models hosted by Microsoft on ai.azure.com. My idea was to use the official Mistral API from MistralAI, including mistral-medium, which AFAIK is only available from the API of MistralAI themselves. Models served from ai.azure.com can also be exposed as an API. But yes, for this we need external support, i have never done that (because the cost was too high when it was still called "Machine Learning Studio").

This would also actually be the 3rd option for including Mistral: -Someone on Discord mentioned that someone else is doing an integration for local Mistral models via Ollama -This API (which does not need a deployment) -The models hosted on ai.azure.com

The case i am after, the copilots, is deployed as WebAPI or Azure Functions, but as i understand SK, it will not be limited to that.

matthewbolanos commented 11 months ago

Gotcha, this makes sense. I think you're right that we'll ultimately need to support three different flavors of Mistral: MistralAI, Azure AI, and local (e.g., via Ollama). If you want to get started on the MistralAI one, we can help support it and broaden it out to the other deployment types.

Jenscaasen commented 11 months ago

Update: didn't see the integration tests. Yes please make the CI have a mistral key available

Jenscaasen commented 10 months ago

Hello, i am sorting out the last unit tests, but there is one thing left to discuss before i can do the PR: https://github.com/Jenscaasen/semantic-kernel/blob/mistral/dotnet/src/Connectors/Connectors.Mistral/MistralAPI/MistralClientCore.cs#L161C21-L161C21

The Default on InvokePrompt in the Kernel is setting the AuthorRole to "System". This is fine with OpenAI. Mistral does not allow the last Message to be anything else but a User message. That is why i added that hack in line 161. How should this be handled?

stephentoub commented 10 months ago

The Default on InvokePrompt in the Kernel is setting the AuthorRole to "System"

@RogerBarreto, sounds like this should be changed? Does the OpenAI one need it to be system?

Jenscaasen commented 10 months ago

An idea for this: Extend ITextCompletionwith an IConnectorDefaultsproperty. The IConnectorDefaultscan provide connector specific defaults like this one ("AuthorRole TextCompletionAuthorRole"). OpenAI would keep System, Mistral can use User, and others in the future might need something else entirely.

matthewbolanos commented 10 months ago

I agree with @Jenscaasen that somewhere the connector should be able to define the default behavior of things like this. @RogerBarreto, do you have any thoughts on how to best achieve this?

Jenscaasen commented 10 months ago

Hello, can i make the PR now so some code review can happen in parallel to the "ADR" for external connectors?

One note on the key for the integration tests: The default mistral keys do not have enough requests per minute to support the 10 integration unit tests in a burst. You need to contact the mistral support to bump that up, it was pretty quick when i asked them. (Or you use my key in exchange for some azure credits :) )

Jenscaasen commented 9 months ago

@matthewbolanos in the last SK office hour you showed this file: https://github.com/RogerBarreto/semantic-kernel/blob/27e5e6ac59109de37cfde1f71e7892ae10ef79bb/docs/decisions/0031-feature-branch-strategy.md

Mistral is not listed with support for mistral api, which this fork has implemented. Should i close this issue and continue to deliver that connector as independent nuget?

KSemenenko commented 9 months ago

I also have to use mistral for SK and KM, so nuget will be awesome!

Jenscaasen commented 9 months ago

I also have to use mistral for SK and KM, so nuget will be awesome!

Here you go: https://www.nuget.org/packages/JC.SemanticKernel.Connectors.Mistral/

And if you want to compile yourself or contribute, here is the fork: https://github.com/Jenscaasen/semantic-kernel

stephentoub commented 9 months ago

Nice job, @Jenscaasen. I just tried it, and at least the basic things I tried "just worked". Thanks!

Nurgo commented 8 months ago

@Jenscaasen Thanks for your Mistral connector, it works flawlessly!

Any plans to add support for function calls? Because mistral-small-latest, mistral-medium-latest and mistral-large-latest now support them natively, with the same syntax than OpenAI.

Jenscaasen commented 8 months ago

@Jenscaasen Thanks for your Mistral connector, it works flawlessly!

Any plans to add support for function calls? Because mistral-small-latest, mistral-medium-latest and mistral-large-latest now support them natively, with the same syntax than OpenAI.

Good point, thanks for the quick reminder. I will look into it. Bump to 1.5 is also missing.

For anyone wanting to support that effort as well: https://github.com/Jenscaasen/semantic-kernel/tree/mistral That is the fork with the branch for the development

deepinderdeol commented 7 months ago

Hi - is SK going to have a Mistral connector with function calling support? And would function calling work for local models?

matthewbolanos commented 5 months ago

Closing this now that we have a Mistral connector with Function calling support

cloud9tn commented 4 months ago

Hi - is SK going to have a Mistral connector with function calling support? And would function calling work for local models?

I'd also be interested in a Mistral connector (or any local model supporting func calls) for use with local model. Function calling works great with OpenAI, but when using local models (that support function calling), not so much.

I do see some docs related to Mistral function calling and using "raw mode". Unfortunately I havent found a way to pass params to the model. https://github.com/ollama/ollama/blob/main/docs/api.md#request-raw-mode

Unrelated side note, I am fan boying a bit over here seeing Stephen Toub in the thread.