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

Multiple Model Configurations #7185

Merged
merged 7 commits into from May 16, 2024
Merged

Conversation

yinggeh
Copy link
Contributor

@yinggeh yinggeh commented May 6, 2024

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

@yinggeh yinggeh self-assigned this May 6, 2024
@yinggeh yinggeh changed the title Custom Model Configurations Multiple Model Configurations May 8, 2024
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.
Copy link
Collaborator

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?

Copy link
Contributor Author

@yinggeh yinggeh May 8, 2024

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".

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
model repository, it is nessecery to have models configured differently
model repository, it is necessary to have models configured differently

Copy link
Contributor Author

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.

Copy link
Contributor Author

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:

```
Copy link
Contributor

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**

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

set -e
if [ "$code" != "200" ]; then
cat $out.out
echo -e "\n***\n*** Test Failed\n***"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo -e "\n***\n*** Test Failed\n***"
echo -e "\n***\n*** Test Failed to get model config\n***"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@yinggeh yinggeh requested a review from jbkyang-nvi May 10, 2024 20:27
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):
Copy link
Contributor

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:
    ...

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unnecessary tests

@yinggeh yinggeh requested a review from GuanLuo May 14, 2024 21:21
@yinggeh yinggeh force-pushed the yinggeh-DLIS-4723-custom-model-configs branch from c6a59a3 to 44434b5 Compare May 15, 2024 09:52
@yinggeh yinggeh merged commit 6d9849d into main May 16, 2024
3 checks passed
@yinggeh yinggeh deleted the yinggeh-DLIS-4723-custom-model-configs branch May 16, 2024 18:02
tanmayv25 pushed a commit that referenced this pull request May 16, 2024
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`.
tanmayv25 added a commit that referenced this pull request May 16, 2024
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 <157551214+yinggeh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants