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

Refactor milvus dataprep and retriever #728

Merged

Conversation

bjzhjing
Copy link
Contributor

Refactor the Milvus Dataprep and Retriever modules to enhance code readability and facilitate user-friendly configuration.

@bjzhjing bjzhjing changed the title Refactor milvus dataprep retriever Refactor milvus dataprep and retriever Sep 24, 2024
@bjzhjing bjzhjing force-pushed the consolidate-milvus-dataprep-retriever branch from 558994d to d75c16b Compare September 24, 2024 04:53
@bjzhjing bjzhjing force-pushed the consolidate-milvus-dataprep-retriever branch from f0e42b5 to da89dfa Compare September 24, 2024 12:09
@bjzhjing
Copy link
Contributor Author

@XinyuYe-Intel Hi, could you please help review this PR?

@bjzhjing bjzhjing force-pushed the consolidate-milvus-dataprep-retriever branch 2 times, most recently from 7022459 to dd5081b Compare September 27, 2024 08:12
@bjzhjing
Copy link
Contributor Author

bjzhjing commented Sep 27, 2024

@chensuyue Hi Suyue, I checked the CI failure.

For Microservice-test(dataprep_milvus_langchain, ...), the environment parameter is changed from MILVUS to MILVUS_HOST for dataprep_milvus_langchain, but this parameter seems to be hardcoded in CI, could you please update it?

@bjzhjing bjzhjing force-pushed the consolidate-milvus-dataprep-retriever branch from dd5081b to 6f174d5 Compare September 27, 2024 09:17
@bjzhjing
Copy link
Contributor Author

@XinyuYe-Intel please help merge this PR, thanks!

@XinyuYe-Intel
Copy link
Collaborator

@XinyuYe-Intel please help merge this PR, thanks!

please make sure to pass all tests.

@bjzhjing
Copy link
Contributor Author

@XinyuYe-Intel please help merge this PR, thanks!

please make sure to pass all tests.

That is due to the hard code in CI script as I mentioned above. Could anyone from CI side take a look? @daisy-ycguo @chensuyue

@lvliang-intel
Copy link
Collaborator

@bjzhjing,
Please check the CI issue, microservice test reported errors.

Milvus dataprep and retriever leverage the same embedding enpoints, but
the embedding-related code is somewhat messed up, unify the namings and
logic to improve code readability and facilitate user-friendly
configuration.

Signed-off-by: Cathy Zhang <[email protected]>
This is to fix the CI issue for MILVUS environment variable name is
update.

Signed-off-by: Cathy Zhang <[email protected]>
@bjzhjing bjzhjing force-pushed the consolidate-milvus-dataprep-retriever branch from b98f908 to b8d8d9a Compare October 9, 2024 01:29
@bjzhjing
Copy link
Contributor Author

bjzhjing commented Oct 9, 2024

@bjzhjing, Please check the CI issue, microservice test reported errors.

@lvliang-intel @XinyuYe-Intel Hi, the test script is modified and CI has passed.

@mkbhanda mkbhanda merged commit 84374a5 into opea-project:main Oct 10, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants