-
Notifications
You must be signed in to change notification settings - Fork 0
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
google[minor]: Move Vertex embeddings to integration package #6
base: main
Are you sure you want to change the base?
Conversation
Clone of the PR langchain-ai/langchainjs#6459 |
My review is in progress 📖 - I will have feedback for you in a few minutes! |
Warning Rate limit exceeded@furwellness has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 25 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed your code and found 5 potential issues.
/** | ||
* Integration with a chat model. | ||
*/ | ||
export class GoogleVertexAIEmbeddings extends GoogleEmbeddings { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class comment for GoogleVertexAIEmbeddings is incorrect. It currently states 'Integration with a chat model', but this class is actually for generating embeddings using Google Vertex AI. Please update the comment to accurately reflect the purpose of this class, such as 'Integration with Google Vertex AI for generating embeddings'.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
model: "textembedding-gecko", | ||
}); | ||
const res = await embeddings.embedQuery("Hello world"); | ||
expect(typeof res[0]).toBe("number"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current tests only check if the first element of each embedding is a number. To improve the test coverage, add assertions to verify that all elements of the embeddings are numbers within an expected range (e.g., between -1 and 1 for normalized embeddings). Also, check if the embedding vectors have the expected dimensionality for the chosen model.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that a console.log statement has been added which prints out the full URL, including sensitive information like the project ID, endpoint, and model name. This could potentially lead to security vulnerabilities if the logs are exposed. Consider removing this console.log statement or replacing it with a more secure logging method that doesn't expose sensitive information in production environments.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
export * from "./chat_models.js"; | ||
export * from "./llms.js"; | ||
export * from "./embeddings.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to add inline comments explaining the purpose and functionality of the exported modules. This will make it easier for other developers to understand the code and contribute to it. For example, you could add a comment like // Exporting the chat models module which contains...
before the export * from './chat_models.js';
line.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current test cases for GoogleVertexAIEmbeddings are a good start, but they're not comprehensive enough to ensure full functionality. Consider adding more test cases to cover different scenarios such as empty input, very long input, and input with special characters. Also, verify the length and structure of the returned embeddings to ensure they meet the expected format for the chosen models.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
PR Reviewer Guide 🔍
|
@coderabbitai full review |
Actions performedFull review triggered. |
PR Code Suggestions ✨
|
No comment from coderabbit |
User description
Will need to break this up into different PRs to make it pass CI
CC @afirstenberg
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?
Move Vertex embeddings to the integration package and deprecate the old imports.
Why are these changes being made?
This change is part of a refactor to better organize the codebase by moving the Vertex embeddings functionality to a dedicated integration package. This improves modularity and maintainability. The deprecation warnings guide users to the new import paths, ensuring a smooth transition.
PR Type
enhancement, tests
Description
Changes walkthrough 📝
14 files
googlevertexai.ts
Add deprecation warnings for Vertex AI embeddings.
libs/langchain-community/src/embeddings/googlevertexai.ts
googlevertexai-connection.ts
Add logging for constructed URL in connection.
libs/langchain-community/src/utils/googlevertexai-connection.ts
GoogleVertexAILLMConnection.
connection.ts
Update type parameters in GoogleAIConnection class.
libs/langchain-google-common/src/connection.ts
embeddings.ts
Introduce GoogleEmbeddings class for embeddings handling.
libs/langchain-google-common/src/embeddings.ts
index.ts
Export new embeddings module.
libs/langchain-google-common/src/index.ts
auth.ts
Update constructor parameter type for GAuthClient.
libs/langchain-google-gauth/src/auth.ts
embeddings.ts
Add GoogleEmbeddings class for GoogleAuth.
libs/langchain-google-gauth/src/embeddings.ts
index.ts
Export new embeddings module.
libs/langchain-google-gauth/src/index.ts
embeddings.ts
Add GoogleVertexAIEmbeddings class for web integration.
libs/langchain-google-vertexai-web/src/embeddings.ts
index.ts
Export new embeddings module.
libs/langchain-google-vertexai-web/src/index.ts
embeddings.ts
Add GoogleVertexAIEmbeddings class for Vertex AI.
libs/langchain-google-vertexai/src/embeddings.ts
index.ts
Export new embeddings module.
libs/langchain-google-vertexai/src/index.ts
embeddings.ts
Add GoogleEmbeddings class for web authentication.
libs/langchain-google-webauth/src/embeddings.ts
index.ts
Export new embeddings module.
libs/langchain-google-webauth/src/index.ts
1 files
embeddings.int.test.ts
Add integration tests for GoogleVertexAIEmbeddings.
libs/langchain-google-vertexai-web/src/tests/embeddings.int.test.ts