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

remove usage of SERVING_LOAD_MODELS in examples/docs/tests #1825

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

siddvenk
Copy link
Contributor

Description

Brief description of what this PR is about

  • If this change is a backward incompatible change, why must this change be made?
  • Interesting edge cases to note here

@siddvenk siddvenk requested review from zachgk, frankfliu and a team as code owners April 26, 2024 23:55
sindhuvahinis
sindhuvahinis previously approved these changes Apr 26, 2024
@@ -53,7 +53,6 @@ def test_all_code(self):
def test_with_env(self):
envs = {
"OPTION_MODEL_ID": "NousResearch/Nous-Hermes-Llama2-13b",
Copy link
Contributor

Choose a reason for hiding this comment

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

HF_MODEL_ID

Copy link
Member

Choose a reason for hiding this comment

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

HF_MODEL_ID actually clash with OPTION_MODEL_ID. Some configuration doesn't really work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea ideally should be HF_MODEL_ID, but as Qing said some configurations don't really work with OPTION_MODEL_ID. I'll need to fix some parts of the test here, but will update to HF_MODEL_ID

Copy link
Contributor

@frankfliu frankfliu Apr 29, 2024

Choose a reason for hiding this comment

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

In which case HF_MODEL_ID will clash with OPTION_MODEL_ID?

OPTION_MODEL_ID requires a model directory being defined already, while HF_MODEL_ID is treated as a new model. You can consider HF_MODEL_ID equals command line "-m model_path"

@@ -110,7 +109,6 @@ def test_all_code_chat(self):
def test_with_env_chat(self):
envs = {
"OPTION_MODEL_ID": "TheBloke/Llama-2-7B-Chat-fp16",
Copy link
Contributor

Choose a reason for hiding this comment

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

HF_MODEL_ID

Copy link
Contributor

Choose a reason for hiding this comment

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

We should never use OPTION_MODEL_ID, I cannot think of a real use case that requires OPTION_MODEL_ID

The translation for `engine` is unique. The configuration `engine=<engine>` is translated to `SERVING_LOAD_MODELS=test::<engine>=/opt/ml/model`.
For example:

* `engine=Python` is translated to environment variable `SERVING_LOAD_MODELS=test::Python=/opt/ml/model`
Copy link
Contributor

Choose a reason for hiding this comment

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

By default, LMI will use engine=Python will be used, we can use OPTION_ENGINE=Python to explictly set the engine.

You can also force use OPTION_MPI_MODE=true to force use MPI mode

@siddvenk
Copy link
Contributor Author

I've included the removal of OPTION_MODEL_ID usage in this PR now.

* `engine=MPI` is translated to environment variable `SERVING_LOAD_MODELS=test::MPI=/opt/ml/model`
The property `engine` is translated to `OPTION_ENGINE`.
By default, LMI will use the Python engine. You can use `OPTION_ENGINE=Python` to explicitly set the engine.
To use the MPI engine, you should also provide `OPTION_MPI_MODE=true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid use "MPI engine" here,

If your engine requires use mpi launcher, you should also provide ...

@@ -125,17 +125,20 @@ You can find these configurations in the respective [user guides](../user_guides

All LMI Configuration keys available in the `serving.properties` format can be specified as environment variables.

The translation for `engine` is unique. The configuration `engine=<engine>` is translated to `SERVING_LOAD_MODELS=test::<engine>=/opt/ml/model`.
For example:
The property `option.model_id` is unique. It is translated to `HF_MODEL_ID`.
Copy link
Contributor

Choose a reason for hiding this comment

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

option_model_id and HF_MODEL_ID are different thing, we might want to document HF_MODEL_ID separately

@sindhuvahinis sindhuvahinis dismissed their stale review May 3, 2024 17:05

frank requested some changes

@siddvenk siddvenk force-pushed the env branch 2 times, most recently from 75cc7c4 to e6767cc Compare June 6, 2024 22:10
@frankfliu
Copy link
Contributor

Please rebase latest master branch

@siddvenk siddvenk force-pushed the env branch 3 times, most recently from a5b4fdb to c621b1a Compare June 7, 2024 04:08
@siddvenk siddvenk merged commit 1e883d5 into deepjavalibrary:master Jun 7, 2024
8 checks passed
@siddvenk siddvenk deleted the env branch June 7, 2024 04:27
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.

None yet

4 participants