Introduce Mistral AI support #32
Conversation
ThomasVitale
commented
Jul 6, 2024
ThomasVitale
commented
- Configure MistralAiClient using the Spring HTTP infrastructure
- Define auto-configuration for using Mistral AI chat and embedding models in Spring Boot
- Create a Spring Boot starter to provide the integration with Mistral AI
* Configure MistralAiClient using the Spring HTTP infrastructure * Define auto-configuration for using Mistral AI chat and embedding models in Spring Boot * Create a Spring Boot starter to provide the integration with Mistral AI Signed-off-by: Thomas Vitale <[email protected]>
|
The integration tests only run when there's a MISTRAL_AI_API_KEY environment variable defined. |
|
This change in core LangChain4j would enhance the Spring support for Mistral AI, including being able to use an auto configured |
There was a problem hiding this comment.
Hi @ThomasVitale, thanks a lot!
BTW, how will this work together with langchain4j/langchain4j#1103?
With the proposed approach each new model provider will require adding it's own langchain4j-spring-{model-provider} module and have quite a lot of duplication. (The aim of langchain4j/langchain4j#1103 is to have model-agnostic http clients that can plug into any model, so one does not have to re-implement client code over and over again)
| .toSingleValueMap() | ||
| .entrySet() | ||
| .stream() | ||
| .filter(e -> !e.getKey().equals(HttpHeaders.AUTHORIZATION)) |
There was a problem hiding this comment.
There are also X-Auth-Token and X-API-KEY common auth headers (they can also be written in different cases)
| @Override | ||
| public MistralAiChatCompletionResponse chatCompletion(MistralAiChatCompletionRequest chatCompletionRequest) { | ||
| Assert.notNull(chatCompletionRequest, "chatCompletionRequest cannot be null"); | ||
| Assert.isTrue(!chatCompletionRequest.getStream(), "stream mode must be disabled"); |
There was a problem hiding this comment.
nit:
| Assert.isTrue(!chatCompletionRequest.getStream(), "stream mode must be disabled"); | |
| Assert.isFalse(chatCompletionRequest.getStream(), "stream mode must be disabled"); |
| Assert.notNull(chatCompletionRequest, "chatCompletionRequest cannot be null"); | ||
| Assert.isTrue(!chatCompletionRequest.getStream(), "stream mode must be disabled"); | ||
|
|
||
| logger.debug("Sending chat completion request: {}", chatCompletionRequest); |
There was a problem hiding this comment.
Is this required? It is already logged in HttpLoggingInterceptor
(same for other methods below)
| * Adapted from MistralAiChatModelIT in the LangChain4j project. | ||
| */ | ||
| @EnabledIfEnvironmentVariable(named = "MISTRAL_AI_API_KEY", matches = ".*") | ||
| class MistralAiChatModelIT { |
There was a problem hiding this comment.
what about making MistralAiChatModelIT in main repo abstract and inherit from it here (instead of having duplicate tests), similar to EmbeddingStoreIT?
| @ConfigurationProperties(prefix = MistralAiChatProperties.CONFIG_PREFIX) | ||
| public class MistralAiChatProperties { | ||
|
|
||
| public static final String CONFIG_PREFIX = "langchain4j.mistralai.chat"; |
There was a problem hiding this comment.
nit:
| public static final String CONFIG_PREFIX = "langchain4j.mistralai.chat"; | |
| public static final String CONFIG_PREFIX = "langchain4j.mistral-ai.chat-model"; |
| /** | ||
| * What sampling temperature to use, between 0.0 and 1.0. Higher values like 0.8 will make the output more random, while lower values like 0.2 will make it more focused and deterministic. We generally recommend altering this or "top_p" but not both. | ||
| */ | ||
| private Double temperature = 0.7; |
There was a problem hiding this comment.
Do we need to set default values here? (except for the mandatory params (model)).
I would avoid setting defaults here
| @ConfigurationProperties(prefix = MistralAiEmbeddingProperties.CONFIG_PREFIX) | ||
| public class MistralAiEmbeddingProperties { | ||
|
|
||
| public static final String CONFIG_PREFIX = "langchain4j.mistralai.embedding"; |
There was a problem hiding this comment.
| public static final String CONFIG_PREFIX = "langchain4j.mistralai.embedding"; | |
| public static final String CONFIG_PREFIX = "langchain4j.mistral-ai.embedding-model"; |
| @ConfigurationProperties(MistralAiProperties.CONFIG_PREFIX) | ||
| public class MistralAiProperties { | ||
|
|
||
| public static final String CONFIG_PREFIX = "langchain4j.mistralai"; |
There was a problem hiding this comment.
| public static final String CONFIG_PREFIX = "langchain4j.mistralai"; | |
| public static final String CONFIG_PREFIX = "langchain4j.mistral-ai"; |
| class MistralAiAutoConfigurationIT { | ||
|
|
||
| private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() | ||
| .withPropertyValues("langchain4j.mistralai.client.apiKey=" + System.getenv("MISTRAL_AI_API_KEY")) |
There was a problem hiding this comment.
Having apiKey and other client parameters configured once for all models simplifies configuration a bit, but it is not flexible enough: one cannot use different API keys for different models, or e.g. enable logging responses for chat model only (because embedding model responses are huge and make little sense to log), or set different timeouts for different model types (chat models are much-much slower than embedding models)
|
@langchain4j thanks so much for the review and sorry for the late answer. I'm trying to find the time to look more into the new HTTP client abstractions in Core to rethink what I've done in this PR. |
|
@ThomasVitale thanks a lot in advance! BTW langchain4j/langchain4j#1103 is not set in stone, so feel free to suggest changes if you see the need! |
