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

fix: change milvus init args from (host, port) to (url, token) #8019

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

zc277584121
Copy link
Contributor

@zc277584121 zc277584121 commented Sep 5, 2024

It's recommand to use URI and token in Milvus setting, instead of HOST, PORT. Here is an example to teach how to use URI and token in both Milvus and Zilliz: https://milvus.io/docs/build-rag-with-milvus.md#Load-data-into-Milvus
The ( HOST, PORT) appoach is about to depracate in the future, and it will bring confusion to the users.

In this PR, I changed the default setting from (host, port) to (uri , token), and depracated the unecessary milvus_secure. There was some duplicate code in the milvus_vector.py, and I refactor them using milvusClient, which made it more consice.

Checklist:

Important

Please review the checklist below before submitting your pull request.

  • Please open an issue before creating a PR or link to an existing issue:
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

Description

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue. Close issue syntax: Fixes #<issue number>, see documentation for more details.

Close #8017

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update, included: Dify Document
  • Improvement, including but not limited to code refactoring, performance optimization, and UI/UX improvement
  • Dependency upgrade

Testing Instructions

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Have end2end tested with milvus

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. 🐞 bug Something isn't working 👻 feat:rag Embedding related issue, like qdrant, weaviate, milvus, vector database. labels Sep 5, 2024
@crazywoola crazywoola removed the 🐞 bug Something isn't working label Sep 5, 2024
JohnJyong
JohnJyong previously approved these changes Sep 6, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 6, 2024
Signed-off-by: ChengZi <chen.zhang@zilliz.com>
@crazywoola
Copy link
Member

crazywoola commented Sep 6, 2024

Please update the docs here as well https://github.com/langgenius/dify-docs. All languages are required. :) Thanks. @zc277584121

@zc277584121
Copy link
Contributor Author

langgenius/dify-docs#252
@crazywoola please reivew

@crazywoola crazywoola merged commit 2060db8 into langgenius:main Sep 6, 2024
6 checks passed
mehrajagdish pushed a commit to Sbazar-GmbH/dify that referenced this pull request Sep 6, 2024
@Seayon
Copy link
Contributor

Seayon commented Sep 12, 2024

This is a breaking change. Users who are still using the MILVUS_HOST and MILVUS_PORT configuration will be unable to start their servers after this update. Would you consider optimizing this change or adding some documentation to guide users through the transition?

@crazywoola crazywoola mentioned this pull request Sep 24, 2024
5 tasks
cuiks pushed a commit to cuiks/dify that referenced this pull request Sep 26, 2024
@crazywoola
Copy link
Member

Please add my wechat crazyphage @zc277584121

lau-td pushed a commit to heydevs-io/dify that referenced this pull request Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👻 feat:rag Embedding related issue, like qdrant, weaviate, milvus, vector database. lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It's recommand to use URI and token in Milvus setting, instead of HOST, PORT
4 participants