-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Multiple Model Configurations #7185
Conversation
qa/L0_custom_model_config/test.sh
Outdated
SERVER_ARGS="--model-repository=`pwd`/models --model-config-name=h200" | ||
test_custom_config $VERSION_DEFAULT | ||
|
||
# TODO: It seems that command argument parser does not accept value with spaces. |
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.
Is this still a TODO?
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 mainly a question from me. Should I take care of the case where configuration file name contains space? In another word, I want to confirm that our command line arguement does not accept value with space. I wasn't able to pass a value with space or \space or "xxx xxx".
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.
Maybe we should just add a note in the configurations part to not include names with spaces in them. Then we can also remove this test.
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 was able to pass file name with space to the server.
/opt/tritonserver/bin/tritonserver --model-repository=/opt/tritonserver/qa/L0_custom_model_config/models --model-config-name="h100 v100"
The server will look for config file named "h100 v100".pbtxt
and preserve quotes. So I think we should tell user to not use space in file name.
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.
Fixed
## Custom Model Configuration | ||
|
||
Sometimes when multiple devices running Triton instances that share one | ||
model repository, it is nessecery to have models configured differently |
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.
model repository, it is nessecery to have models configured differently | |
model repository, it is necessary to have models configured differently |
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 what happens when working overnight.
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.
Fixed
|
||
The repository layout looks like: | ||
|
||
``` |
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 would recommend adding the examples you listed in the TEP instead to make the configurations clearer
Example 1: --model-config-name=8gpus
.
└── model repository/
├── model_a/
│ ├── configs/
│ │ ├── 4gpus.pbtxt
│ │ └── **8gpus.pbtxt**
│ └── config.pbtxt
├── model_b/
│ ├── configs/
│ │ └── 4gpus.pbtxt
│ └── **config.pbtxt**
└── model_c/
├── configs/
│ └── config.pbtxt
└── **config.pbtxt**
Example 2: --model-config-name=config
.
└── model repository/
├── model_a/
│ ├── configs/
│ │ ├── 4gpu.pbtxt
│ │ └── 8gpu.pbtxt
│ └── **config.pbtxt**
├── model_b/
│ ├── configs/
│ │ └── 4gpu.pbtxt
│ └── **config.pbtxt**
└── model_c/
├── configs/
│ └── **config.pbtxt**
└── config.pbtxt
Example 3: --model-config-name not specified
.
└── model repository/
├── model_a/
│ ├── configs/
│ │ ├── 4gpu.pbtxt
│ │ └── 8gpu.pbtxt
│ └── **config.pbtxt**
├── model_b/
│ ├── configs/
│ │ └── 4gpu.pbtxt
│ └── **config.pbtxt**
└── model_c/
├── configs/
│ └── config.pbtxt
└── **config.pbtxt**
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.
Fixed
qa/L0_custom_model_config/test.sh
Outdated
set -e | ||
if [ "$code" != "200" ]; then | ||
cat $out.out | ||
echo -e "\n***\n*** Test Failed\n***" |
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.
echo -e "\n***\n*** Test Failed\n***" | |
echo -e "\n***\n*** Test Failed to get model config\n***" |
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.
Fixed
qa/L0_lifecycle/lifecycle_test.py
Outdated
self.assertTrue(False, "unexpected error {}".format(ex)) | ||
|
||
# Run inference on the model on all versions. Only version 1, 3 should work. | ||
for model_name, model_shape in zip(models_base, models_shape): |
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.
You can iterate different backend types and shapes but that is not essential for testing this feature.. I would rather just test one model setting for simplicity and group the version testings below to be:
# check available version
for version in expected_available_versions:
...
for version in expected_unavailable_versions:
...
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.
Actually, I don't know if the inference check is necessary as you already check the model version readiness. With that, it just seems to be an sanity check for me.
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.
Removed unnecessary tests
c6a59a3
to
44434b5
Compare
Add `--mode-config-name` option when starting Triton server. Allow users to create multiple configurations and select a custom configuration other than the default `model/config.pbtxt`.
Add `--mode-config-name` option when starting Triton server. Allow users to create multiple configurations and select a custom configuration other than the default `model/config.pbtxt`. Co-authored-by: Yingge He <[email protected]>
Add
--mode-config-name
option when starting Triton server. Allow users to select custom configurations other than default config.pbtxt.core: triton-inference-server/core#348