Skip to content

fix: ollama embedding_model dims Option panic. #1199

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.

Sign up for GitHub

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
KanonWY wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: ollama embedding_model dims Option panic. #1199

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion rig/rig-core/src/providers/ollama.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,14 @@ enum ApiResponse<T> {
pub const ALL_MINILM: &str = "all-minilm";
pub const NOMIC_EMBED_TEXT: &str = "nomic-embed-text";

fn model_dimensions_from_identifier(identifier: &str) -> Option<usize> {
match identifier {
ALL_MINILM => Some(384),
NOMIC_EMBED_TEXT => Some(768),
_ => None,
}
}

Comment on lines +150 to +157
Copy link
Collaborator

@joshua-mo-143 joshua-mo-143 Dec 29, 2025

Choose a reason for hiding this comment

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

I think this part is largely unmaintainable, so we should avoid adding this in. It is usually quite trivial to find the dimension sizes

Copy link
Author

@KanonWY KanonWY Dec 30, 2025

Choose a reason for hiding this comment

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

yes, I think this part is largely unmaintainable too. A better approach is to use embedding_model_with_ndims. I added it this way because I noticed that other modules did the same. openai/embedding.rs:row59

Copy link
Collaborator

@joshua-mo-143 joshua-mo-143 Jan 7, 2026

Choose a reason for hiding this comment

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

Yeah OpenAI only has 2/3 models so it's fine. Unfortunately for us, ollama seems to have an absolute tonne of them which means we can't reliably keep track of all of them

#[derive(Debug, Serialize, Deserialize)]
pub struct EmbeddingResponse {
pub model: String,
Expand Down Expand Up @@ -208,7 +216,11 @@ where
type Client = Client<T>;

fn make(client: &Self::Client, model: impl Into<String>, dims: Option<usize>) -> Self {
Self::new(client.clone(), model, dims.unwrap())
let model = model.into();
let dims = dims
.or(model_dimensions_from_identifier(&model))
.unwrap_or_default();
Self::new(client.clone(), model, dims)
}

const MAX_DOCUMENTS: usize = 1024;
Expand Down