-
Notifications
You must be signed in to change notification settings - Fork 142
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
PR: OpenSearch integration components for OPEA #908
base: main
Are you sure you want to change the base?
PR: OpenSearch integration components for OPEA #908
Conversation
@cameronmorin, |
Hi @cameronmorin , thanks for your contribution. Please check out the comments above, and remember to add CI test scripts like this for each of the new components. Please name the test scripts following the schema of |
Hi @letonghan ! Thank you so much for reviewing my PR and leaving your helpful comments. I've addressed the simple ones of removing unnecessary comments & files, and I'm in the process of adding the test scripts you mentioned that are required. |
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.
Hi @cameronmorin and thanks for this PR!
I just did a quick look and it needs those 2 files fixed for formatting.
I need to do deeper review once those changes are made.
Thanks.
Thanks for your PR. Could you please check the failures in the CI test? Thanks. |
comps/dataprep/opensearch/README.md
Outdated
apt update | ||
apt install default-jre | ||
apt-get install tesseract-ocr -y | ||
apt-get install libtesseract-dev -y | ||
apt-get install poppler-utils -y |
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.
Why not combine some of these commands together like:
apt-get update
apt-get install default-jre libtesseract-dev poppler-utils tesseract-ocr
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.
92163fa
to
34ec1ec
Compare
Lastly, I've tried to address the DCO comment by rebasing my branch in order to sign all of my commits, however now I'm seeing issues with commits that I didn't author after addressing & signing all of my commits. I'll try to see if I can fix these as well, but if it comes to it after I address all of the comments and get the tests working, can I move my changes to a new PR with a single signed commit from me to be merged instead? If not, some help or advice with the commit signing would also be appreciated :) Thanks! |
Codecov ReportAttention: Patch coverage is
|
The rebase seems not correct, the diff file including some history change. |
Follow the CI reminder to fix the issue, https://github.com/opea-project/GenAIComps/actions/runs/12091595827/job/33765700276?pr=908 |
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
* Pass down model id for ChatQnA Signed-off-by: lvliang-intel <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * update logic Signed-off-by: lvliang-intel <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: lvliang-intel <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Signed-off-by: Cameron Morin <[email protected]>
…-project#909) Signed-off-by: Chun Tao <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
* Add outputs. Signed-off-by: ZePan110 <[email protected]> * Add empty list check Signed-off-by: ZePan110 <[email protected]> * test CI. Signed-off-by: ZePan110 <[email protected]> * Remove test files Signed-off-by: ZePan110 <[email protected]> * remove debug code Signed-off-by: chensuyue <[email protected]> --------- Signed-off-by: ZePan110 <[email protected]> Signed-off-by: chensuyue <[email protected]> Co-authored-by: chensuyue <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: ZePan110 <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
…roject#915) * fix retriever and reranker to process chat completion request Signed-off-by: minmin-intel <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: minmin-intel <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Signed-off-by: Cameron Morin <[email protected]>
* fix html content loading problem Signed-off-by: letonghan <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add empty list check (opea-project#914) * Add outputs. Signed-off-by: ZePan110 <[email protected]> * Add empty list check Signed-off-by: ZePan110 <[email protected]> * test CI. Signed-off-by: ZePan110 <[email protected]> * Remove test files Signed-off-by: ZePan110 <[email protected]> * remove debug code Signed-off-by: chensuyue <[email protected]> --------- Signed-off-by: ZePan110 <[email protected]> Signed-off-by: chensuyue <[email protected]> Co-authored-by: chensuyue <[email protected]> * Fix hardware tag retrieval issue (opea-project#916) Signed-off-by: ZePan110 <[email protected]> * fix html content loading problem Signed-off-by: letonghan <[email protected]> * fix milvus connection issue Signed-off-by: letonghan <[email protected]> * update parse_html function for all dbs Signed-off-by: letonghan <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: letonghan <[email protected]> Signed-off-by: ZePan110 <[email protected]> Signed-off-by: chensuyue <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: ZePan110 <[email protected]> Co-authored-by: chensuyue <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: ZePan110 <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
* Add PREDICTIONGUARD_API_KEY Signed-off-by: ZePan110 <[email protected]> * Increase timeout Signed-off-by: ZePan110 <[email protected]> * Fix log name issue Signed-off-by: ZePan110 <[email protected]> * Remove useless code. Signed-off-by: ZePan110 <[email protected]> --------- Signed-off-by: ZePan110 <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: chensuyue <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
…#928) * Update requirements to pin protobuf version and fix grpc conflict, and limit vdms version Signed-off-by: Lacewell, Chaunte W <[email protected]> * Update fix by removing grpcio pin and pinning opentelemetry-proto to 1.23.0 Signed-off-by: Lacewell, Chaunte W <[email protected]> --------- Signed-off-by: Lacewell, Chaunte W <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: chensuyue <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: isaacncz <[email protected]> Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
Signed-off-by: Cameron Morin <[email protected]>
cbb4be8
to
b342bd5
Compare
I agree, I thought the rebase seemed off when I initially performed it because I saw a lot of historical changes when I rebased to sign all of the commits in the log. I tried to rebase manually and only sign the commits that I authored, but I still saw the historical changes being added to my PR in addition to the DCO still failing. I just pushed a change that fixed the DCO, however I still see the historical changes added to the PR. Would it be a bad idea to resolve all of the reviewer comments, and split my changes into a fresh branch / PR that doesn't have to rebase historical changes in order to merge that instead of this PR? I'd want to link this PR so that all conversations and reviews persist and are tracked historically, but I'm not sure how to clear up the other changes flowing into my branch. |
I've moved the changes over to a new PR in order to resolve the history and DCO issues. Please review & approve that PR instead since all tests are passing with the same file changes and new git log. |
Description
These proposed changes create the OPEA components necessary to integrate OpenSearch with GenAIExamples, specifically tested & validated with ChatQnA
Issues
n/a
Type of change
List the type of change like below. Please delete options that are not relevant.
Dependencies
opensearch-py
Tests
I tested the components independently, as well as end-to-end by creating my own ChatQnA cluster replacing the redis microservices (dataprep, retriever, and vectorstore) with the OpenSearch components I've created here.