- 
                Notifications
    You must be signed in to change notification settings 
- Fork 82
remove usage of SERVING_LOAD_MODELS in examples/docs/tests #1825
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
|  | ||
| def test_with_env(self): | ||
| envs = { | ||
| "OPTION_MODEL_ID": "NousResearch/Nous-Hermes-Llama2-13b", | 
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.
HF_MODEL_ID
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.
HF_MODEL_ID actually clash with OPTION_MODEL_ID. Some configuration doesn't really work
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.
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
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.
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"
|  | ||
| def test_with_env_chat(self): | ||
| envs = { | ||
| "OPTION_MODEL_ID": "TheBloke/Llama-2-7B-Chat-fp16", | 
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.
HF_MODEL_ID
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 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` | 
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.
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
| 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`. | 
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.
Avoid use "MPI engine" here,
If your engine requires use mpi launcher, you should also provide ...
|  | ||
| 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`. | 
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.
option_model_id and HF_MODEL_ID are different thing, we might want to document HF_MODEL_ID separately
75cc7c4    to
    e6767cc      
    Compare
  
    | Please rebase latest master branch | 
a5b4fdb    to
    c621b1a      
    Compare
  
    
Description
Brief description of what this PR is about