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

Adapt to latest changes in llm microservice famliy #696

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

lianhao
Copy link
Collaborator

@lianhao lianhao commented Jan 14, 2025

Description

Adapt to latest changes in llm microservice famliy.

Issues

Fixes #691

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)

Dependencies

List the newly introduced 3rd party dependency if exists.

Tests

Describe the tests that you ran to verify your changes.

@lianhao lianhao requested a review from yongfengdu as a code owner January 14, 2025 12:18
@lianhao lianhao requested a review from Ruoyu-y January 14, 2025 12:18
@lianhao lianhao added this to the v1.2 milestone Jan 14, 2025
@lianhao
Copy link
Collaborator Author

lianhao commented Jan 14, 2025

PR #697 will fix the doc CI issue

Copy link
Contributor

@eero-t eero-t left a comment

Choose a reason for hiding this comment

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

Looks fine. I have few questions and doc suggestions though.

helm-charts/common/llm-uservice/README.md Outdated Show resolved Hide resolved
helm-charts/common/llm-uservice/README.md Outdated Show resolved Hide resolved
helm-charts/common/llm-uservice/README.md Outdated Show resolved Hide resolved
helm-charts/common/llm-uservice/README.md Outdated Show resolved Hide resolved
helm-charts/common/llm-uservice/templates/configmap.yaml Outdated Show resolved Hide resolved
helm-charts/common/llm-uservice/values.yaml Outdated Show resolved Hide resolved
helm-charts/common/llm-uservice/values.yaml Outdated Show resolved Hide resolved
@lianhao lianhao force-pushed the llm-uservice branch 2 times, most recently from 567cc14 to aa6b8f6 Compare January 15, 2025 05:18
Copy link
Collaborator

@Ruoyu-y Ruoyu-y left a comment

Choose a reason for hiding this comment

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

LGTM

@yongfengdu yongfengdu merged commit 70ad650 into opea-project:main Jan 15, 2025
14 checks passed
@lianhao lianhao deleted the llm-uservice branch January 15, 2025 06:09
Comment on lines +87 to +89
| TEXTGEN_BACKEND | string | `"tgi"` | backend inference engine, only valid for llm-textgen image, one of "TGI", "vLLM" |
| DOCSUM_BACKEND | string | `"tgi"` | backend inference engine, only valid for llm-docsum image, one of "TGI", "vLLM" |
| FAQGEN_BACKEND | string | `"tgi"` | backend inference engine, only valid for llm-faqgen image, one of "TGi", "vLLM" |
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigMap template does case-sensitive comparison, so the default values need to be updated too: tgi -> TGI.

Copy link
Collaborator Author

@lianhao lianhao Jan 15, 2025

Choose a reason for hiding this comment

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

since it's already merged, I'll do that in another pending PR.

@eero-t
Copy link
Contributor

eero-t commented Jan 15, 2025

Noticed couple of missed changes which could be also fixed in another PR:

$ git grep -e llm-docsum- -e llm-faqgen-
helm-charts/docsum/values.yaml:    repository: opea/llm-docsum-tgi
helm-charts/faqgen/values.yaml:    repository: opea/llm-faqgen-tgi
microservices-connector/config/manifests/docsum-llm-uservice.yaml:          image: "opea/llm-docsum-tgi:latest"
microservices-connector/config/manifests/faqgen-llm-uservice.yaml:          image: "opea/llm-faqgen-tgi:latest"

(First one is fixed also in in #649.)

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.

[ci-auto] GenAIComps llms docker compose file got changed.
4 participants