Skip to content
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

community[minor]: feat: BaiduQianfan embeddings #4926

Merged

Conversation

zandko
Copy link
Contributor

@zandko zandko commented Mar 29, 2024

BaiduQianFanEmbeddings support, enhancing Chinese text analysis and processing capabilities for diverse applications.

Copy link

vercel bot commented Mar 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 6, 2024 0:15am
langchainjs-docs ✅ Ready (Inspect) Visit Preview Apr 6, 2024 0:15am

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. auto:enhancement A large net-new component, integration, or chain. Use sparingly. The largest features labels Mar 29, 2024
@@ -0,0 +1,215 @@
import { Embeddings, type EmbeddingsParams } from '@langchain/core/embeddings';
Copy link

Choose a reason for hiding this comment

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

Hey team, just a heads up that I've flagged a new external HTTP request using the fetch function in the code. This is for your review to ensure it aligns with our project's requirements and best practices. Let me know if you have any questions or need further clarification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the update. I understand the importance of adhering to our project's requirements and best practices, especially when it involves integrating third-party services. The use of this external HTTP request is essential for incorporating BaiduQianFanEmbeddings support, which is crucial for our project's ability to process and analyze Chinese text effectively.

I've ensured that the implementation complies with our security standards and performance expectations. However, I'm open to feedback and willing to make any necessary adjustments to align with our project guidelines fully. Please let me know if there are any concerns or further clarifications needed.

Appreciate your support and looking forward to your feedback.

@@ -0,0 +1,215 @@
import { Embeddings, type EmbeddingsParams } from '@langchain/core/embeddings';
Copy link

Choose a reason for hiding this comment

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

Hey there! I've reviewed the code changes, and it looks like the addition of getEnvironmentVariable is explicitly accessing environment variables. I've flagged this for your review to ensure it aligns with our environment variable handling practices. Let me know if you need further assistance with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the update. I understand the importance of adhering to our project's requirements and best practices, especially when it involves integrating third-party services. The use of this external HTTP request is essential for incorporating BaiduQianFanEmbeddings support, which is crucial for our project's ability to process and analyze Chinese text effectively.

I've ensured that the implementation complies with our security standards and performance expectations. However, I'm open to feedback and willing to make any necessary adjustments to align with our project guidelines fully. Please let me know if there are any concerns or further clarifications needed.

Appreciate your support and looking forward to your feedback.

@jacoblee93
Copy link
Collaborator

Thank you!

@jacoblee93
Copy link
Collaborator

Can you please run yarn format?

@@ -0,0 +1,7 @@
import { BaiduQianFanEmbeddings } from "@langchain/community/embeddings/baidu_qianfan";

const embeddings = new BaiduQianFanEmbeddings();
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Python QianFan doesn't have a capital F:

https://python.langchain.com/docs/integrations/text_embedding/baidu_qianfan_endpoint

Worth keeping this the same?

Copy link
Contributor Author

@zandko zandko Apr 3, 2024

Choose a reason for hiding this comment

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

The consistency of BaiduQianfanEmbeddings has been maintained.


this.modelName = fieldsWithDefaults?.modelName ?? this.modelName;

if (this.modelName === 'tao-8k' && !!fieldsWithDefaults?.batchSize) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to throw an error here instead of potentially misleading the user?

Copy link
Contributor Author

@zandko zandko Apr 3, 2024

Choose a reason for hiding this comment

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

You're right. I've made changes.

@jacoblee93 jacoblee93 added the close PRs that need one or two touch-ups to be ready label Apr 3, 2024
@jacoblee93 jacoblee93 changed the title feat: BaiduQianfan embeddings community[minor]: feat: BaiduQianfan embeddings Apr 3, 2024
@zandko
Copy link
Contributor Author

zandko commented Apr 3, 2024

Can you please run yarn format?

Executed

@jacoblee93 jacoblee93 added lgtm PRs that are ready to be merged as-is and removed close PRs that need one or two touch-ups to be ready labels Apr 5, 2024
@jacoblee93
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:enhancement A large net-new component, integration, or chain. Use sparingly. The largest features lgtm PRs that are ready to be merged as-is size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants