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.

Sign up for GitHub

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

Jump to bottom

text generation, embedding and reranking with ovms #1318

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

text generation, embedding and reranking with ovms #1318

wants to merge 7 commits into from

Conversation

dtrawins
Copy link

@dtrawins dtrawins commented Feb 21, 2025 •
edited
Loading

Description

Text generation, Embeddings and Reranking microservices based on OVMS component

Issues

CVS-161458

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)
  • Others (enhancement, documentation, validation, etc.)

Dependencies

OpenVINO Model Server

Tests

Following documentation examples

@dtrawins dtrawins changed the title embedding and reranking with ovms text generation, embedding and reranking with ovms Feb 26, 2025
@dtrawins dtrawins marked this pull request as ready for review February 26, 2025 15:37
Spycsh and others added 7 commits February 27, 2025 00:23
@Spycsh @pre-commit-ci @dtrawins
Fix telemetry connection issue when disabling telemetry (opea-project…
102c718 
…#1290)

* Fix telemetry connection issue when disabling telemetry

- use ENABLE_OPEA_TELEMETRY to control whether to enable open telemetry, default false
- fix the issue that logs always show telemetry connection error with each request when telemetry is disabled
- ban the above error propagation to microservices when telemetry is disabled

Signed-off-by: Spycsh <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fix ut failure where required the flag to be on

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Signed-off-by: Spycsh <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: Dariusz Trawinski <[email protected]>
@lvliang-intel @dtrawins
Refine CLIP embedding microservice by leveraging the third-party CLIP (…
790bbfe 
…opea-project#1298)

* Refine CLI embedding microservice using dependency
Signed-off-by: lvliang-intel <[email protected]>
Signed-off-by: Dariusz Trawinski <[email protected]>
@dtrawins
embedding and reranking with ovms
ed1ef07 
Signed-off-by: Dariusz Trawinski <[email protected]>
@dtrawins
llm endpoint
e3196aa 
Signed-off-by: Dariusz Trawinski <[email protected]>
@pre-commit-ci @dtrawins
[pre-commit.ci] auto fixes from pre-commit.com hooks
cbd7345 
for more information, see https://pre-commit.ci

Signed-off-by: Dariusz Trawinski <[email protected]>
@dtrawins
fix ovms compose entrypoint
fc911ea 
Signed-off-by: Dariusz Trawinski <[email protected]>
@pre-commit-ci @dtrawins
[pre-commit.ci] auto fixes from pre-commit.com hooks
f344401 
for more information, see https://pre-commit.ci

Signed-off-by: Dariusz Trawinski <[email protected]>
chensuyue
chensuyue reviewed Mar 4, 2025
View reviewed changes
comps/rerankings/src/Dockerfile
@@ -36,6 +36,7 @@ fi && \
pip install --no-cache-dir --upgrade pip setuptools && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/opea-project/GenAIComps/actions/runs/13555365160/job/37926867062?pr=1318
image
Requirement file name not correct.

chensuyue
chensuyue reviewed Mar 4, 2025
View reviewed changes
comps/third_parties/ovms/deployment/kubernetes/README.md
@@ -0,0 +1,15 @@
# Deploy OVMS on kubernetes cluster
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yongfengdu we should think about how to handle those case in CI https://github.com/opea-project/GenAIComps/actions/runs/13555365164/job/37888386000?pr=1318

Copy link
Collaborator

Choose a reason for hiding this comment

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

The ovms helm chart is from external repos, I think it's not necessary to trigger CI here, a README to guide how user can deploy is good enough.
I suggest we don't add the cpu-values.yaml file here and put everything in the readme.
Adding something like this in the readme and remove cpu-values.yaml file:
cat > cpu-values.yaml <EOF
xxxx
EOF

In the future, if some Examples or llm-microservice supported helm chart deployment with OVMS, we can follow how data-prep use milvus (Just define customized values in subcharts) here:
https://github.com/opea-project/GenAIInfra/blob/main/helm-charts/common/data-prep/milvus-values.yaml

@codecov Codecov
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
comps/cores/mega/orchestrator.py 80.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
comps/cores/telemetry/opea_telemetry.py 94.87% <100.00%> (-5.13%) ⬇️
comps/cores/mega/orchestrator.py 91.21% <80.00%> (+0.03%) ⬆️

... and 3 files with indirect coverage changes

@chensuyue
Copy link
Collaborator

This PR need some new e2e tests, test_embeddings_ovms.sh, test_llms_ovms.sh, test_rerankings_ovms.sh.

yongfengdu
yongfengdu reviewed Mar 4, 2025
View reviewed changes
comps/third_parties/ovms/deployment/kubernetes/cpu-values.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Per suggested above, this file should be removed and put the contents in README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yongfengdu yongfengdu yongfengdu left review comments

@chensuyue chensuyue chensuyue left review comments

@lkk12014402 lkk12014402 Awaiting requested review from lkk12014402 lkk12014402 is a code owner

@lvliang-intel lvliang-intel Awaiting requested review from lvliang-intel lvliang-intel is a code owner

@letonghan letonghan Awaiting requested review from letonghan letonghan is a code owner

@XinyaoWa XinyaoWa Awaiting requested review from XinyaoWa XinyaoWa is a code owner

@ftian1 ftian1 Awaiting requested review from ftian1 ftian1 is a code owner

@Spycsh Spycsh Awaiting requested review from Spycsh Spycsh is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dtrawins @chensuyue @yongfengdu @Spycsh @lvliang-intel