ollama[minor]: Port embeddings to ollama package #5
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
|
Clone of the PR langchain-ai/langchainjs#6464 |
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
WalkthroughThe recent changes enhance the functionality and usability of the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
| constructor(fields?: OllamaEmbeddingsParams) { | ||
| super({ maxConcurrency: 1, ...fields }); | ||
|
|
||
| this.client = new Ollama({ | ||
| host: fields?.baseUrl, | ||
| }); |
There was a problem hiding this comment.
The headers property is defined and set in the constructor, but it's not being used in the API calls. To ensure custom headers are applied, you should pass the headers to the Ollama client when it's initialized. Update the constructor to include headers in the Ollama client initialization:
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| export interface OllamaCamelCaseOptions { | ||
| numa?: boolean; | ||
| numCtx?: number; | ||
| numBatch?: number; | ||
| numGpu?: number; | ||
| mainGpu?: number; | ||
| lowVram?: boolean; | ||
| f16Kv?: boolean; | ||
| logitsAll?: boolean; | ||
| vocabOnly?: boolean; | ||
| useMmap?: boolean; | ||
| useMlock?: boolean; | ||
| embeddingOnly?: boolean; | ||
| numThread?: number; | ||
| numKeep?: number; | ||
| seed?: number; | ||
| numPredict?: number; | ||
| topK?: number; | ||
| topP?: number; | ||
| tfsZ?: number; | ||
| typicalP?: number; | ||
| repeatLastN?: number; | ||
| temperature?: number; | ||
| repeatPenalty?: number; | ||
| presencePenalty?: number; | ||
| frequencyPenalty?: number; | ||
| mirostat?: number; | ||
| mirostatTau?: number; | ||
| mirostatEta?: number; | ||
| penalizeNewline?: boolean; | ||
| /** | ||
| * @default "5m" | ||
| */ | ||
| keepAlive?: string | number; | ||
| stop?: string[]; | ||
| } |
There was a problem hiding this comment.
The OllamaCamelCaseOptions interface could benefit from more specific type constraints and documentation. For example, properties like numCtx, numBatch, numGpu, etc., should probably be non-negative integers. The temperature property typically ranges from 0 to 1. Consider adding these constraints and including JSDoc comments to explain the purpose and expected values for each property. This will improve type safety and make the interface more self-documenting.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| test("Test OllamaEmbeddings.embedQuery", async () => { | ||
| const embeddings = new OllamaEmbeddings(); | ||
| const res = await embeddings.embedQuery("Hello world"); | ||
| expect(res).toHaveLength(1024); | ||
| expect(typeof res[0]).toBe("number"); | ||
| }); |
There was a problem hiding this comment.
The current tests for OllamaEmbeddings look good for basic functionality, but they don't cover error handling or edge cases. Consider adding tests for:
- Empty string inputs
- Very long inputs (to test any potential length limits)
- Network errors or API failures (you may need to mock the API calls)
- Invalid inputs
This will ensure the OllamaEmbeddings class behaves correctly under various conditions and improves the overall reliability of the package.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| @@ -0,0 +1,36 @@ | |||
| export interface OllamaCamelCaseOptions { | |||
There was a problem hiding this comment.
The OllamaCamelCaseOptions interface has a large number of optional parameters, which can make it difficult to use and understand. Consider breaking down this interface into smaller, more specific interfaces to improve readability and maintainability. Also, if some parameters are necessary for the interface to function correctly, consider making them required.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
|
/review |
PR Reviewer Guide 🔍(Review updated until commit 677f974)
|
|
@coderabbitai full review |
Actions performedFull review triggered. |
|
Persistent review updated to latest commit 677f974 |
PR Code Suggestions ✨
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- libs/langchain-ollama/src/chat_models.ts (3 hunks)
- libs/langchain-ollama/src/embeddings.ts (1 hunks)
- libs/langchain-ollama/src/index.ts (1 hunks)
- libs/langchain-ollama/src/tests/embeddings.int.test.ts (1 hunks)
- libs/langchain-ollama/src/types.ts (1 hunks)
Additional comments not posted (8)
libs/langchain-ollama/src/index.ts (1)
2-3: LGTM! Expanded module exports.The addition of exports from
embeddings.jsandtypes.jsenhances the module's interface, providing access to more functionalities.libs/langchain-ollama/src/tests/embeddings.int.test.ts (2)
4-9: Existing comment acknowledged: Enhance test coverage.The current tests are good for basic functionality. Consider adding tests for edge cases and error handling as suggested in the existing comments.
11-19: Existing comment acknowledged: Enhance test coverage.The current tests are good for basic functionality. Consider adding tests for edge cases and error handling as suggested in the existing comments.
libs/langchain-ollama/src/types.ts (1)
1-36: Existing comment acknowledged: Improve type constraints and documentation.Consider adding more specific type constraints and JSDoc comments for better type safety and self-documentation, as suggested in the existing comments.
libs/langchain-ollama/src/embeddings.ts (2)
63-68: Existing comment acknowledged: Ensure headers are used in API calls.Ensure that the
headersproperty is utilized in the API calls to apply custom headers, as suggested in the existing comments.
84-129: LGTM! Well-implemented_convertOptionsmethod.The
_convertOptionsmethod effectively converts camelCase options to snake_case, ensuring consistency with the API requirements.libs/langchain-ollama/src/chat_models.ts (2)
31-31: LGTM! StreamlinedChatOllamaInputinterface.The extension of
ChatOllamaInputwithOllamaCamelCaseOptionssimplifies the interface, promoting modularity and maintainability.
59-61: Verify impact of interface changes.Ensure that the removal of properties from
ChatOllamaInputdoes not break existing code. Verify that all necessary adjustments have been made in the codebase.Run the following script to check for usages of
ChatOllamaInput:
User description
Description by Korbit AI
Note
This feature is in early access. You can enable or disable it in the Korbit Console.
What change is being made?
Port embeddings functionality to the
ollamapackage by addingOllamaEmbeddingsclass, updatingChatOllamaInputinterface, and including new types and tests.Why are these changes being made?
This change integrates the embeddings feature into the
ollamapackage, allowing for embedding operations using the Ollama model. It also refactors the code to use camelCase options for consistency and maintainability.PR Type
enhancement, tests
Description
OllamaEmbeddingsclass to handle embedding operations using the Ollama model.ChatOllamaInputinterface to includeOllamaCamelCaseOptionsfor more consistent parameter handling.OllamaEmbeddingsto ensure functionality of embedding methods.Changes walkthrough 📝
chat_models.ts
Update ChatOllamaInput interface with camelCase optionslibs/langchain-ollama/src/chat_models.ts
OllamaCamelCaseOptionstoChatOllamaInputinterface.ChatOllamaInput.embeddings.ts
Add OllamaEmbeddings class for embedding functionalitylibs/langchain-ollama/src/embeddings.ts
OllamaEmbeddingsclass for embedding operations._convertOptionsmethod for camelCase to snake_caseconversion.
index.ts
Update exports to include embeddings and typeslibs/langchain-ollama/src/index.ts
embeddings.jsandtypes.js.types.ts
Define OllamaCamelCaseOptions interface for request optionslibs/langchain-ollama/src/types.ts
OllamaCamelCaseOptionsinterface.embeddings.int.test.ts
Add integration tests for OllamaEmbeddingslibs/langchain-ollama/src/tests/embeddings.int.test.ts
OllamaEmbeddings.embedQueryandembedDocumentsmethods.Summary by CodeRabbit
New Features
ChatOllamaInputinterface with enhanced configuration options.OllamaEmbeddingsclass for generating embeddings for documents and queries.embeddingsandtypesfiles.OllamaCamelCaseOptionsinterface for customizable model configuration.Bug Fixes
ChatOllamaInputto simplify its usage.Tests
OllamaEmbeddingsclass to validate core functionalities.