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

support connect milvus with token, fix auto id #2345

Merged
merged 4 commits into from
Aug 21, 2023

Conversation

nameczz
Copy link
Contributor

@nameczz nameczz commented Aug 21, 2023

Milvus node sdk support connect with token after v2.2.11 .
User need to use token if want to connect ZILLIZ cloud serverless cluster.
Also i find the hard code auto id = true in LangChain, so i optimized it.

Please help review this pr , thanks.

@vercel
Copy link

vercel bot commented Aug 21, 2023

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

Name Status Preview Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Aug 21, 2023 9:31pm

@dosubot dosubot bot added the auto:improvement Medium size change to existing code to handle new use-cases label Aug 21, 2023
@@ -632,7 +632,7 @@
"@typescript-eslint/parser": "^5.58.0",
Copy link

Choose a reason for hiding this comment

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

Great work on the PR! I've noticed a change in the "@zilliz/milvus2-sdk-node" dependency version, which seems to be a hard dependency change. This comment is to flag the change for maintainers to review and ensure it aligns with the project's requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only update package.json in this pr, not sure about yarn.lock

@@ -116,10 +119,26 @@ export class Milvus extends VectorStore {
Copy link

Choose a reason for hiding this comment

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

This comment is flagging the change in the PR for maintainers to review as it accesses an environment variable indirectly through the getEnvironmentVariable function.

@@ -421,7 +440,7 @@ export class Milvus extends VectorStore {
if (field.is_primary_key) {
this.primaryField = field.name;
}
const dtype = DataTypeMap[field.data_type.toLowerCase()];
const dtype = DataTypeMap[field.data_type];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In new sdk, data_type is keyof typeof DataType. So we don't need to lowerCase here, otherwise ts will throw error.

@jacoblee93
Copy link
Collaborator

Looks good! Appreciate the backwards compatibility shim!

@jacoblee93 jacoblee93 self-assigned this Aug 21, 2023
@jacoblee93 jacoblee93 added the lgtm PRs that are ready to be merged as-is label Aug 21, 2023
@jacoblee93 jacoblee93 merged commit b350f9b into langchain-ai:main Aug 21, 2023
8 checks passed
@jacoblee93
Copy link
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:improvement Medium size change to existing code to handle new use-cases lgtm PRs that are ready to be merged as-is
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants