-
Notifications
You must be signed in to change notification settings - Fork 98
Add support for external OpenAI-compatible LLM endpoints across Helm charts (chatqna, codegen) #993
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
Conversation
lianhao
left a comment
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.
2 more things besides the embedded comment:
- Will there be a different image of opea/chatqna for the external LLM endpoint use case? Or the image will be the same?
- In order to make CI happy to test external-llm-values.yaml, we should have a valid LLM_SERVER_HOST_IP and OPENAI_API_KEY in CI environment. I believe you should have the similar CI requirements in GenAIExamples repo when make changes to chatqna mega gateway. We should follow the same convention here.
|
@lianhao |
yes. Also maybe we can split PRs for chatqna, codegen, docsum respectively? Just in case the mega gateway PR in GenAIExamples are not merged in the same time? |
|
@devpramod We need you to do us a favor. Could you please do a manual rebase on your branch before your next round updating of this PR? Otherwise, it will trigger lots of unnecessary test cases. |
0da28c5 to
50abb8c
Compare
eaacd8b to
eb0bdc8
Compare
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.
Please be noted that currently CI is under heavy burden doing release CD tests for 1.3
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.
Looks OK once Lianhao's comments are resolved (and CI passes).
18bacac to
95fef45
Compare
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.
@devpramod please address the 2 remaining embedded comment. Also please rebase and fix the test error
Thx~
95fef45 to
8f6e1ea
Compare
3f701c5 to
8f6e1ea
Compare
poussa
left a comment
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.
LGTM. Please address @lianhao comments and we are good to merge, I think.
@devpramod ping. |
|
Hi @poussa |
0a69a59 to
897fa9d
Compare
|
Deployment templates are missing And for "DocSum": I'm not sure why
and this PR opea-project/GenAIComps#1583 merged few weeks ago, that changed above code. |
lianhao
left a comment
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.
We should follow the chatqna method to set external LLM related config to avoid the above errors @eero-t mentioned
for more information, see https://pre-commit.ci
…rnal LLM and data-prep configurations; update values.yaml for productivity suite and docsum to enable necessary services and endpoints. Signed-off-by: devpramod <[email protected]>
…onditionals in deployment and configmap templates for chatqna, codegen, and docsum; update values.yaml to enable whisper and redis-vector-db services. Signed-off-by: devpramod <[email protected]>
Signed-off-by: devpramod <[email protected]>
…o override them if necessary Signed-off-by: devpramod <[email protected]>
…d env variable for api key Signed-off-by: devpramod <[email protected]>
…iables in deployments Signed-off-by: devpramod <[email protected]>
…le charts Signed-off-by: devpramod <[email protected]>
c713304 to
167d0f5
Compare
|
@devpramod please do the following steps to make CI happy:
|
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.
I'm ok with this PR itself. But we need to make some changes to make CI happy, following the steps as I listed here #993 (comment)
167d0f5 to
8b1b207
Compare
Signed-off-by: devpramod <[email protected]>
Signed-off-by: devpramod <[email protected]>
…point ready Signed-off-by: Tsai, Louie <[email protected]>
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.
@louie-tsai @devpramod I would suggest some minor changes, and also please help resolve the merge conflict issue. Others LGTM
| #helm install chatqna chatqna --set global.HUGGINGFACEHUB_API_TOKEN=${HFTOKEN} --set global.modelUseHostPath=${MODELDIR} --set tgi.LLM_MODEL_ID=${MODELNAME} -f chatqna/rocm-tgi-values.yaml | ||
|
|
||
| # To use with external OpenAI compatible LLM endpoint | ||
| #helm install chatqna chatqna -f chatqna/external-llm-values.yaml --set externalLLM.LLM_SERVER_HOST_IP="http://your-llm-server" --set externalLLM.LLM_MODEL="your-model" --set externalLLM.OPENAI_API_KEY="your-api-key" |
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.
Please change the file name external-llm-values.yaml accordingly
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.
| # helm install codegen codegen --set global.HUGGINGFACEHUB_API_TOKEN=${HFTOKEN} --set global.modelUseHostPath=${MODELDIR} --set llm-uservcie.LLM_MODEL_ID=${MODELNAME} --set tgi.LLM_MODEL_ID=${MODELNAME} -f codegen/rocm-tgi-values.yaml | ||
|
|
||
| # To use with external OpenAI compatible LLM endpoint | ||
| # helm install codegen codegen -f codegen/external-llm-values.yaml --set externalLLM.LLM_SERVER_HOST_IP="http://your-llm-server" --set externalLLM.LLM_MODEL="your-model" --set externalLLM.OPENAI_API_KEY="your-api-key" |
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.
Please change the file name external-llm-values.yaml accordingly
| condition: data-prep.enabled | ||
| - name: ui | ||
| alias: chatqna-ui | ||
| version: 0-latest | ||
| repository: "file://../common/ui" | ||
| condition: chatqna-ui.enabled |
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.
Disabling either data-prep or chatqna-ui means that Helm fails parsing the ChatQnA NGINX template: https://github.com/opea-project/GenAIInfra/blob/main/helm-charts/chatqna/templates/nginx.yaml
(Because their names have dashes, Helm does not understand if one adds ifs to the template for them being enabled.)
| condition: data-prep.enabled | ||
| - name: ui | ||
| version: 0-latest | ||
| repository: "file://../common/ui" | ||
| alias: codegen-ui | ||
| condition: codegen-ui.enabled |
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.
Disabling either data-prep or codegen-ui means that Helm fails parsing the CodeGen NGINX template:
https://github.com/opea-project/GenAIInfra/blob/main/helm-charts/codegen/templates/ui-nginx.yaml
| # Disable internal LLM services when using external LLM | ||
| llm-uservice: | ||
| enabled: false | ||
|
|
||
| vllm: | ||
| enabled: false | ||
|
|
||
| tgi: | ||
| enabled: false |
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.
This is missing:
ollama:
enabled: false
|
Added PR #1166 for fixing above and some additional issues I noticed later on. |
Description
This PR implements support for using external OpenAI-compatible LLM endpoints with ChatQnA, CodeGen, and DocSum Helm charts while maintaining backward compatibility with existing configurations.
Motivation
Users need the flexibility to connect to external LLM services (like OpenAI or self-hosted OpenAI-compatible endpoints) instead of always relying on the included LLM components. This adds versatility to our Helm charts without disrupting existing functionality.
Key Changes
For each Helm chart (ChatQnA, CodeGen, DocSum):
external-llm-values.yamlconfiguration file:templates/external-llm-configmap.yaml:templates/deployment.yaml:Chart.yaml:README.md:Issues
Fixes #1015
Type of change
List the type of change like below. Please delete options that are not relevant.
Dependencies
'n/a'
Tests
Tested backward compatibility to ensure existing helm charts with default values.yaml is working