google[minor]: Move Vertex embeddings to integration package #12
Conversation
![local-dev-korbit-ai-mentor[bot]](https://avatars.githubusercontent.com/in/318591?s=60&v=4){kind=small}
![[local-dev] Korbit - AI Mentor](https://avatars.githubusercontent.com/in/318591?s=40&u=4153787aed90096cc5d05c9c38a439007e524086&v=4){kind=avatar}
|
Clone of the PR langchain-ai/langchainjs#6459 |
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates enhance the Langchain library by reorganizing Google Vertex AI embeddings and authentication features, improving code maintainability and usability. Deprecation notices guide users toward new import paths, while new classes and interfaces facilitate seamless integration with Google’s AI capabilities. Logging enhancements and expanded test coverage further bolster functionality, ensuring that the library remains robust and user-friendly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GoogleAI
participant Embeddings
User->>Embeddings: Request embeddings for input data
Embeddings->>GoogleAI: Format request and authenticate
GoogleAI-->>Embeddings: Return embeddings
Embeddings-->>User: Send back embeddings
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 (
|
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
| /** | ||
| * Integration with a chat model. | ||
| */ | ||
| export class GoogleVertexAIEmbeddings extends GoogleEmbeddings { |
There was a problem hiding this comment.
The class description for GoogleVertexAIEmbeddings is incorrect. It's currently described as 'Integration with a chat model', but this class is actually for embeddings. Please update the comment to accurately describe the class, for example: 'Integration with Google Vertex AI for embeddings.'
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| async embedQuery(document: string): Promise<number[]> { | ||
| const data = await this.embedDocuments([document]); | ||
| return data[0]; | ||
| } |
There was a problem hiding this comment.
In the embedQuery method, there's no check to ensure that the data array returned by embedDocuments is not empty before accessing its first element. This could lead to a runtime error if embedDocuments returns an empty array. Consider adding a check and handling the case where no embeddings are returned.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| /** | ||
| * @deprecated Import and use from @langchain/google-vertexai or @langchain/google-vertexai-web | ||
| * Enables calls to the Google Cloud's Vertex AI API to access | ||
| * the embeddings generated by Large Language Models. | ||
| * |
There was a problem hiding this comment.
The class GoogleVertexAIEmbeddings has been marked as deprecated, which is good. However, to ensure smooth transition for users, it would be helpful to provide more detailed information in the deprecation notice. Consider adding a brief explanation of why the class is being deprecated and provide a code example of how to use the new import location. This will help users understand the change and quickly update their code.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| console.log( | ||
| `https://${this.endpoint}/v1/projects/${projectId}/locations/${this.location}/publishers/google/models/${this.model}:${method}` | ||
| ); |
There was a problem hiding this comment.
I noticed that a console.log statement has been added to print the URL method. While this might be helpful for debugging, it's generally not recommended to leave console.log statements in production code. Additionally, this log might expose sensitive information about the project ID, location, and model. Consider removing this console.log statement or replacing it with a more appropriate logging mechanism that respects different environments (development vs. production) and doesn't expose sensitive data.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| test("Test GoogleVertexAIEmbeddings.embedQuery", async () => { | ||
| const embeddings = new GoogleVertexAIEmbeddings({ | ||
| model: "textembedding-gecko", | ||
| }); | ||
| const res = await embeddings.embedQuery("Hello world"); | ||
| expect(typeof res[0]).toBe("number"); | ||
| }); |
There was a problem hiding this comment.
The current test cases for GoogleVertexAIEmbeddings are a good start, but they could be more comprehensive. Consider adding checks for the following:
- The length of each embedding vector (should be consistent and match the expected dimensionality for the chosen model).
- The range of values in the embeddings (typically between -1 and 1 for normalized embeddings).
- Ensure that different input texts produce different embeddings.
- Test edge cases like empty strings or very long texts.
These additional checks will provide more confidence in the correct functionality of the embeddings.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
|
/review |
|
@coderabbitai review |
Actions performedReview triggered.
|
PR Reviewer Guide 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (2)
libs/langchain-google-vertexai-web/src/tests/embeddings.int.test.ts (2)
4-10: Enhance test coverage forembedQuery.The test case for
embedQueryis basic. Consider adding checks for:
- The length of the embedding vector.
- The range of values.
- Edge cases like empty strings or very long texts.
12-29: Enhance test coverage forembedDocuments.The test case for
embedDocumentsis a good start but can be improved by:
- Checking the length of each embedding vector.
- Verifying the range of values.
- Testing with edge cases like empty or very long documents.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- libs/langchain-community/src/embeddings/googlevertexai.ts (4 hunks)
- libs/langchain-community/src/utils/googlevertexai-connection.ts (1 hunks)
- libs/langchain-google-common/src/connection.ts (2 hunks)
- libs/langchain-google-common/src/embeddings.ts (1 hunks)
- libs/langchain-google-common/src/index.ts (1 hunks)
- libs/langchain-google-gauth/src/auth.ts (2 hunks)
- libs/langchain-google-gauth/src/embeddings.ts (1 hunks)
- libs/langchain-google-gauth/src/index.ts (1 hunks)
- libs/langchain-google-vertexai-web/src/embeddings.ts (1 hunks)
- libs/langchain-google-vertexai-web/src/index.ts (1 hunks)
- libs/langchain-google-vertexai-web/src/tests/embeddings.int.test.ts (1 hunks)
- libs/langchain-google-vertexai/src/embeddings.ts (1 hunks)
- libs/langchain-google-vertexai/src/index.ts (1 hunks)
- libs/langchain-google-webauth/src/embeddings.ts (1 hunks)
- libs/langchain-google-webauth/src/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- libs/langchain-google-gauth/src/index.ts
Additional context used
Biome
libs/langchain-google-webauth/src/embeddings.ts
[error] 29-31: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
libs/langchain-google-gauth/src/embeddings.ts
[error] 30-32: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
libs/langchain-google-common/src/embeddings.ts
[error] 22-29: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
Additional comments not posted (12)
libs/langchain-google-vertexai-web/src/index.ts (1)
3-3: LGTM! Export addition for embeddings.js.The addition of
export * from "./embeddings.js";successfully extends the module's interface to include embeddings functionality, aligning with the refactoring goals.libs/langchain-google-vertexai/src/index.ts (1)
3-3: LGTM! Export addition for embeddings.js.The addition of
export * from "./embeddings.js";successfully extends the module's interface to include embeddings functionality, aligning with the refactoring goals.libs/langchain-google-webauth/src/index.ts (1)
3-3: LGTM! Export addition for embeddings.js.The addition of
export * from "./embeddings.js";successfully extends the module's interface to include embeddings functionality, aligning with the refactoring goals.libs/langchain-google-common/src/index.ts (1)
3-3: Export addition looks good.The addition of
export * from "./embeddings.js";enhances the modularity of the codebase by making embeddings-related functionality accessible through the index file.libs/langchain-google-vertexai/src/embeddings.ts (1)
11-14: Update the class description.The class description inaccurately states "Integration with a chat model." It should be updated to reflect that this class is for embeddings, such as "Integration with Google Vertex AI for embeddings."
libs/langchain-google-gauth/src/auth.ts (1)
Line range hint
30-34:
Verify the impact of changing the constructor parameter type.The constructor now accepts
GoogleConnectionParams<GoogleAuthOptions>instead ofGoogleBaseLLMInput<GoogleAuthOptions>. Ensure that all instances ofGAuthClientare updated to use the new parameter type and that this change does not introduce any breaking changes in the codebase.libs/langchain-community/src/embeddings/googlevertexai.ts (2)
13-16: Ensure deprecation notices are clear and informative.The deprecation notices guide users to import from
@langchain/google-vertexaior@langchain/google-vertexai-web. Ensure that these notices provide sufficient context, such as reasons for deprecation and examples of how to update the code.Also applies to: 23-26, 30-33, 39-42
55-58: Enhance deprecation notice with examples.The deprecation notice for
GoogleVertexAIEmbeddingsshould include a brief explanation and code examples to help users transition to the new import paths.libs/langchain-google-common/src/embeddings.ts (1)
198-201: Add a check for empty data inembedQuery.The
embedQuerymethod does not currently check if thedataarray returned byembedDocumentsis empty, which could lead to runtime errors. Consider adding a check to handle this case.libs/langchain-google-common/src/connection.ts (3)
169-170: Update type parameters for flexibility.The change from
MessageTypetoInputTypebroadens the scope of input data, enhancing the flexibility of theGoogleAIConnectionclass.
235-236: Update method signature forformatData.The method now accepts
InputType, aligning with the updated type parameters for broader input support.
240-243: Update method signature forrequest.The method now uses
InputTypeandAsyncCallerCallOptions, which enhances the asynchronous capabilities of the class.
| console.log( | ||
| `https://${this.endpoint}/v1/projects/${projectId}/locations/${this.location}/publishers/google/models/${this.model}:${method}` | ||
| ); |
There was a problem hiding this comment.
Remove or replace console.log for sensitive information.
The console.log statement exposes sensitive information, such as the project ID and model. Consider using a logging mechanism that respects different environments and does not expose sensitive data.
- console.log(
- `https://${this.endpoint}/v1/projects/${projectId}/locations/${this.location}/publishers/google/models/${this.model}:${method}`
- );Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log( | |
| `https://${this.endpoint}/v1/projects/${projectId}/locations/${this.location}/publishers/google/models/${this.model}:${method}` | |
| ); |
| constructor(fields: GoogleEmbeddingsInput) { | ||
| super(fields); | ||
| } |
There was a problem hiding this comment.
Remove the unnecessary constructor.
The constructor in GoogleEmbeddings is redundant as it only calls the superclass constructor. Removing it will simplify the code.
- constructor(fields: GoogleEmbeddingsInput) {
- super(fields);
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(fields: GoogleEmbeddingsInput) { | |
| super(fields); | |
| } |
Tools
Biome
[error] 29-31: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
| constructor(fields: GoogleEmbeddingsInput) { | ||
| super(fields); | ||
| } |
There was a problem hiding this comment.
Remove the unnecessary constructor.
The constructor in GoogleEmbeddings is redundant as it only calls the superclass constructor. Removing it will simplify the code.
- constructor(fields: GoogleEmbeddingsInput) {
- super(fields);
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(fields: GoogleEmbeddingsInput) { | |
| super(fields); | |
| } |
Tools
Biome
[error] 30-32: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
| /** | ||
| * Integration with a chat model. | ||
| */ | ||
| export class GoogleVertexAIEmbeddings extends GoogleEmbeddings { |
There was a problem hiding this comment.
Update the class description.
The class description inaccurately states "Integration with a chat model." It should be updated to reflect that this class is for embeddings, such as "Integration with Google Vertex AI for embeddings."
| constructor( | ||
| fields: GoogleConnectionParams<AuthOptions> | undefined, | ||
| caller: AsyncCaller, | ||
| client: GoogleAbstractedClient, | ||
| streaming: boolean | ||
| ) { | ||
| super(fields, caller, client, streaming); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary constructor.
The constructor in EmbeddingsConnection does not perform any additional logic and can be safely removed to simplify the code.
- constructor(
- fields: GoogleConnectionParams<AuthOptions> | undefined,
- caller: AsyncCaller,
- client: GoogleAbstractedClient,
- streaming: boolean
- ) {
- super(fields, caller, client, streaming);
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor( | |
| fields: GoogleConnectionParams<AuthOptions> | undefined, | |
| caller: AsyncCaller, | |
| client: GoogleAbstractedClient, | |
| streaming: boolean | |
| ) { | |
| super(fields, caller, client, streaming); | |
| } |
Tools
Biome
[error] 22-29: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
Will need to break this up into different PRs to make it pass CI
CC @afirstenberg
Description by Korbit AI
What change is being made?
Move Vertex embeddings to the integration package and deprecate the old imports.
Why are these changes being made?
This change is part of a refactoring effort to better organize the codebase by moving the Vertex embeddings functionality to a dedicated integration package. This improves modularity and maintainability. The old imports are deprecated to guide users towards the new structure.
Summary by CodeRabbit
New Features
Deprecation Notices
Improvements
Tests