Skip to content

Initial Mistral implementation #43

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

OriPekelman
Copy link

Tried to follow existing implementation as much as possible and hopefully integrated correctly.

@OriPekelman OriPekelman mentioned this pull request Mar 18, 2025
@crmne crmne added the new provider New provider integration label Mar 21, 2025
@crmne crmne linked an issue Mar 23, 2025 that may be closed by this pull request
@crmne
Copy link
Owner

crmne commented Mar 23, 2025

Thank you @OriPekelman for your contribution!

Two new features to consider in your provider implementation:

  1. Model Aliases: Please add entries for your provider in aliases.json:

    "claude-3-5-sonnet": {
      "anthropic": "claude-3-5-sonnet-20241022",
      "your-provider": "your-provider-specific-id"
    }
  2. Provider Selection: Users will be able to specify your provider:

    chat = RubyLLM.chat(model: 'claude-3-5-sonnet', provider: 'your-provider')

Docs: https://rubyllm.com/guides/models#using-model-aliases

Please ask me for a code review when you're ready.

@crmne
Copy link
Owner

crmne commented Mar 25, 2025

Added configuration requirements handling in 75f99a1

Each provider now specifies what configuration is required via a simple configuration_requirements method (you will need to implement this in your main provider file) that returns an array of config keys as symbols. The Provider module uses this to:

  1. Determine if a provider is properly configured
  2. Throw an error if you're trying to use that provider without configuration
  3. Include ready-to-paste configuration code in the error message
  4. Skip unconfigured providers during model refresh while preserving their models

Example of the new error messages:

RubyLLM::ConfigurationError: anthropic provider is not configured. Add this to your initialization:

RubyLLM.configure do |config|
  config.anthropic_api_key = ENV['ANTHROPIC_API_KEY']
end

@crmne
Copy link
Owner

crmne commented Apr 17, 2025

@OriPekelman is this still on your radar? I'd love to merge Mistral support soon. Whenever you are ready, could you resolve the conflicts and rerequest a review? Thanks!

@OriPekelman
Copy link
Author

Yup, I'll try to get to it very soon.

@crmne
Copy link
Owner

crmne commented Apr 23, 2025

Hi @OriPekelman is Mistral OpenAI compatible?

@OriPekelman
Copy link
Author

Hi @OriPekelman is Mistral OpenAI compatible?

Well, like most providers, it has some basic OpenAI compatability ... but only for chat completions - and even there you are going to have quite a bit of differences in tool calling and multi-turn behaviour.

I rebased the whole thing and cleaned it up quite a bit + added the configure.

There are a number of tests I had to skip (for example Mistral does not support custom dimensions for embedding, the tool calling in multi-turn conversations doesn't support adding tools mid conversation, only the last chunk in a stream would have token count).

Hopefully this is useful.

Copy link
Owner

@crmne crmne left a comment

Choose a reason for hiding this comment

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

Good stuff! I left you some comments

Comment on lines +8 to +12
def completion_url
"#{Mistral.api_base(RubyLLM.config)}/chat/completions"
end
Copy link
Owner

Choose a reason for hiding this comment

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

you don't need to pass the api base here

Copy link
Author

Choose a reason for hiding this comment

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

Addressed

Array(tools)
end

puts "\n[DEBUG] Available tools: #{tools_array&.map { |t| t.name.to_s }}" if ENV["DEBUG"]
Copy link
Owner

Choose a reason for hiding this comment

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

Use RubyLLM.logger.debug

Copy link
Author

Choose a reason for hiding this comment

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

Addressed

"none"
end

puts "[DEBUG] Tool choice: #{effective_tool_choice.inspect}" if ENV["DEBUG"]
Copy link
Owner

Choose a reason for hiding this comment

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

same here

Copy link
Author

Choose a reason for hiding this comment

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

Addressed

frequency_penalty: frequency_penalty,
}.compact

puts "[DEBUG] Full payload: #{payload.inspect}" if ENV["DEBUG"]
Copy link
Owner

Choose a reason for hiding this comment

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

same here

arguments: tool_call.arguments,
},
}
puts "[DEBUG] Rendered tool call: #{tool_call_spec.inspect}" if ENV["DEBUG"]
Copy link
Owner

Choose a reason for hiding this comment

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

same here

@@ -31,6 +31,7 @@

it "#{provider}/#{model} successfully uses the system prompt" do
skip 'System prompt can be flaky for Ollama models' if provider == :ollama
skip 'Mistral API does not allow system messages after assistant messages' if provider == :mistral
Copy link
Owner

Choose a reason for hiding this comment

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

but in this case the chat should start with the system message, right?

Comment on lines +29 to +30
skip 'Mistral API only returns token count on the last chunk.' if provider == :mistral

Copy link
Owner

Choose a reason for hiding this comment

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

also the other APIs, so don't worry

@@ -36,6 +36,7 @@ def execute
model = model_info[:model]
provider = model_info[:provider]
it "#{provider}/#{model} can use tools" do # rubocop:disable RSpec/MultipleExpectations
skip 'Mistral does not reliably support tool usage' if provider == :mistral
Copy link
Owner

Choose a reason for hiding this comment

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

really? that would be super bad! in what way is it not reliable?

Copy link
Author

Choose a reason for hiding this comment

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

OK I addressed the mistral too hallucination issue - from what I can see it only happens with the small model. At any rate it no longer breaks the implementation. It tends to believe it has a "web_search" tool whether you have one or not.

@@ -23,6 +27,7 @@
end

it "#{provider}/#{model} can handle a single text with custom dimensions" do # rubocop:disable RSpec/MultipleExpectations
skip "Mistral embed does not support custom dimensions" if model == "mistral-embed"
Copy link
Owner

Choose a reason for hiding this comment

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

you can do if provider == :mistral

Copy link
Author

Choose a reason for hiding this comment

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

Went for provider, but if I am not mistaken it is a string here.

@@ -38,6 +43,7 @@
end

it "#{provider}/#{model} can handle multiple texts with custom dimensions" do # rubocop:disable RSpec/MultipleExpectations
skip "Mistral embed does not support custom dimensions" if model == "mistral-embed"
Copy link
Owner

Choose a reason for hiding this comment

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

same

@OriPekelman
Copy link
Author

Oh and now i'll try to do a clean rebase.

…ration

- Updated model aliases in aliases.json to include specific paths for Mistral models.
- Added `mistral_api_base` configuration option in configuration.rb.
- Introduced new models in models.json, including Mistral Tiny, Pixtral Large, and others.
- Refactored Mistral provider methods to support new API structure and error handling.
- Removed deprecated image generation methods from the Mistral provider.
- Updated tests to reflect changes in model handling and API responses.
…chat provider

- Updated debug output from `puts` to `RubyLLM.logger.debug` for better logging management.
- Commented out skip conditions in tests for Mistral provider to avoid flaky behavior during testing.
… cassettes

- Refactored the chat provider to be closer to the other implementations.
- No longer ignoring specs except custom dimensions.
- Refactored error message formatting in the Mistral provider.
- Mistral (small) may hallucinate tools which explains previous failures. Added robustness for that.
- Updated chat and streaming files to include necessary modules and methods.
- Removed outdated VCR cassettes related to Mistral functionality.
- Updated test fixtures to reflect changes in API responses and model handling.
- Added frozen string literal to Mistral provider files for performance.
@OriPekelman
Copy link
Author

I see there are some new tests, I'll see if I can implement the local image stuff.

@OriPekelman OriPekelman marked this pull request as draft May 5, 2025 20:43
@OriPekelman OriPekelman marked this pull request as ready for review May 5, 2025 20:44
@OriPekelman
Copy link
Author

OK this should work now (given that models.json is up-to-date) for the failing vision tests.

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

Successfully merging this pull request may close these issues.

Include Mistral AI
2 participants