Skip to content

Conversation

rescrv
Copy link
Contributor

@rescrv rescrv commented Oct 19, 2025

Description of changes

Propel suggested using the reqwest pool. I'll do that.

Test plan

CI

Migration plan

N/A

Observability plan

N/A

Documentation Changes

N/A

@rescrv rescrv requested a review from Sicheng-Pan October 19, 2025 22:59
@propel-code-bot
Copy link
Contributor

propel-code-bot bot commented Oct 19, 2025

Reuse Persistent HTTP Client in OllamaEmbeddingFunction

This PR refactors the OllamaEmbeddingFunction implementation in rust/chroma/src/embed/ollama.rs to eliminate the creation of a new reqwest::Client for every request. Instead, the HTTP client is now a field on the struct, instantiated once during construction and reused for all API interactions. This aligns the implementation with recommendations for using connection pooling for improved efficiency.

Key Changes

• Added a client: reqwest::Client field to the OllamaEmbeddingFunction struct.
• Modified the new constructor to create and store a single reqwest::Client.
• Refactored the EmbedRequest::make_request method to accept a reference to OllamaEmbeddingFunction (providing access to the shared client and host).
• Removed per-request instantiation of the reqwest::Client.
• Updated request creation and use throughout the embedding code to leverage the persistent client.

Affected Areas

rust/chroma/src/embed/ollama.rs
OllamaEmbeddingFunction struct and methods
EmbedRequest request creation logic

This summary was automatically generated by @propel-code-bot

@github-actions
Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Base automatically changed from rescrv/embeddings to main October 19, 2025 23:17
@rescrv rescrv force-pushed the rescrv/embeddings2 branch from 4add911 to 534048e Compare October 20, 2025 00:01
@rescrv rescrv merged commit c92f6d9 into main Oct 20, 2025
115 of 119 checks passed
@rescrv rescrv deleted the rescrv/embeddings2 branch October 20, 2025 17:44
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.

2 participants