Skip to content

Conversation

@lianhao
Copy link
Collaborator

@lianhao lianhao commented Dec 30, 2024

Description

Currently, llm-vllm services use the environment variable LLM_MODEL while all other services in llm category are using LLM_MODEL_ID. This PR unify all the services in llm category to use the same environment variable LLM_MODEL_ID to minimize the configuration management efforts such as docker-compose, helm, etc.

Issues

n/a.

Type of change

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

  • [x ] 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.

Use environment variable LLM_MODEL_ID to replace LLM_MODEL, so all
microservices in llm catogory use the same environment variable to
reduce the configuration management efforts.

Signed-off-by: Lianhao Lu <[email protected]>
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.

This PR replaces only first instance of LLM_MODEL in each file, but several of the files contain multiple instances of it:

$ git grep -c [^_]LLM_MODEL[:=]
comps/llms/text-generation/README.md:5
comps/llms/text-generation/vllm/langchain/README.md:1
comps/llms/text-generation/vllm/langchain/docker_compose_llm.yaml:2
comps/llms/text-generation/vllm/langchain/launch_microservice.sh:1
comps/llms/text-generation/vllm/llama_index/README.md:1
comps/llms/text-generation/vllm/llama_index/docker_compose_llm.yaml:2
comps/llms/text-generation/vllm/llama_index/launch_microservice.sh:1
tests/llms/test_llms_text-generation_vllm_langchain_on_intel_hpu.sh:2
tests/llms/test_llms_text-generation_vllm_llamaindex_on_intel_hpu.sh:2

@eero-t
Copy link
Contributor

eero-t commented Dec 30, 2024

PRs containing workarounds for the bug this PR will fix:

@lianhao lianhao changed the title vllm: unify environment variable LLM_MODEL_ID llm-vllm: unify environment variable LLM_MODEL_ID Dec 31, 2024
@lianhao
Copy link
Collaborator Author

lianhao commented Dec 31, 2024

This PR replaces only first instance of LLM_MODEL in each file, but several of the files contain multiple instances of it:

$ git grep -c [^_]LLM_MODEL[:=]
comps/llms/text-generation/README.md:5
comps/llms/text-generation/vllm/langchain/README.md:1
comps/llms/text-generation/vllm/langchain/docker_compose_llm.yaml:2
comps/llms/text-generation/vllm/langchain/launch_microservice.sh:1
comps/llms/text-generation/vllm/llama_index/README.md:1
comps/llms/text-generation/vllm/llama_index/docker_compose_llm.yaml:2
comps/llms/text-generation/vllm/llama_index/launch_microservice.sh:1
tests/llms/test_llms_text-generation_vllm_langchain_on_intel_hpu.sh:2
tests/llms/test_llms_text-generation_vllm_llamaindex_on_intel_hpu.sh:2

Already double checked, the remaining bash environment variable LLM_MODEL is used by the upstream tgi/vllm, not by the llm-vllm services this PR is trying to modify.

@eero-t
Copy link
Contributor

eero-t commented Dec 31, 2024

These are not Bash exports:

$ git grep  [^_]LLM_MODEL[:=] | grep compose
comps/llms/text-generation/vllm/langchain/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/langchain/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/llama_index/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/llama_index/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}

PR changes only first of those?

@lianhao
Copy link
Collaborator Author

lianhao commented Jan 2, 2025

These are not Bash exports:

$ git grep  [^_]LLM_MODEL[:=] | grep compose
comps/llms/text-generation/vllm/langchain/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/langchain/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/llama_index/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/llama_index/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}

PR changes only first of those?

Yes, the remaining LLM_MODEL is not for llm-vllm wrapper container, but for the vllm itself(which is a opea rebuild of upstream vllm).

@xiguiw
Copy link
Collaborator

xiguiw commented Jan 2, 2025

These are not Bash exports:

$ git grep  [^_]LLM_MODEL[:=] | grep compose
comps/llms/text-generation/vllm/langchain/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/langchain/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/llama_index/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}
comps/llms/text-generation/vllm/llama_index/docker_compose_llm.yaml:      LLM_MODEL: ${LLM_MODEL}

PR changes only first of those?

Yes, the remaining LLM_MODEL is not for llm-vllm wrapper container, but for the vllm itself(which is a opea rebuild of upstream vllm).

@lianhao
Greate! This is helpful. It makes things simple.
Do you know the status of TEI_MODEL and RERANKING MODLEL environment variables? Do you plan to unify them?
Thanks!

@lianhao
Copy link
Collaborator Author

lianhao commented Jan 2, 2025

Do you know the status of TEI_MODEL and RERANKING MODLEL environment variables? Do you plan to unify them?
Thanks!

Sorry, I'm not aware of that

@eero-t
Copy link
Contributor

eero-t commented Jan 2, 2025

These are not Bash exports:
...
PR changes only first of those?

Yes, the remaining LLM_MODEL is not for llm-vllm wrapper container, but for the vllm itself(which is a opea rebuild of upstream vllm).

Sure, but it's given to vLLM with --model option, so it could be changed also. Usage of multiple env var names for the same purpose in same file is confusing. Why not change all of them?

@lianhao
Copy link
Collaborator Author

lianhao commented Jan 15, 2025

closed as new llm-textgen doesn't have this issue

@lianhao lianhao closed this Jan 15, 2025
@lianhao lianhao deleted the LLM_MODEL_ID branch January 18, 2025 03:08
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.

3 participants