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.

Sign up for GitHub

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

Unify default naming on different instance group count #231

Merged
merged 5 commits into from
Jul 14, 2023
Merged

Unify default naming on different instance group count #231

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eab8486
Add name into instance signature
kthui Jul 3, 2023
3991676
Use actual instance name
kthui Jul 3, 2023
a1f7589
Unify instance name across different group count
kthui Jul 5, 2023
5c5952a
Use different instance name unification method
kthui Jul 7, 2023
90a64dd
Revert "Add name into instance signature" and "Use actual instance name"
kthui Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/backend_model_instance.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,7 @@ TritonModelInstance::SetInstances(
secondary_device.device_id());
}
for (int32_t c = 0; c < group.count(); ++c) {
std::string instance_name{
group.count() > 1 ? group.name() + "_" + std::to_string(c)
: group.name()};
std::string instance_name{group.name() + "_" + std::to_string(c)};
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have check that the groups must not share the same name? Otherwise we can have instances with the exact same name?

Copy link
Contributor Author

@kthui kthui Jul 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have check that the groups must not share the same name?

No, we actually allow one or more instances to share the same name. There are two scenarios:

  1. One or more instances are the same and share the same name.
    • Exactly which instance is reused or removed during an instance update is undefined, because there is no way to tell the difference between those instances.
    • There is a new test that manipulate instances under this case. The test will pass as long as the number of instances added or removed is correct.
  2. One or more instances are different but share the same name.
    • The instances are disambiguated by their instance group configuration.
    • There is a new test checking the correct instance is removed and created, if the config of an instance is changed but reused the same name.

Otherwise we can have instances with the exact same name?

Yes. They are distinguished by their instance configuration, if they are sharing the same name.

Copy link
Contributor

@rmccorm4 rmccorm4 Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the ability for different instances to have the same name will be confusing in the long term, but is functional now since the logic is based on config and not names. I would like to see this disambiguated in the future.

However, with my other comment to add more descriptive instance names like this:

What do you think about including device type / kind in the name? ex: "bert_cpu0_instance0", "bert_cpu0_instance1", "bert_gpu0_instance0", etc.

Then I think the names will be unique by default, unless users specify an identical instance group name in two or more models.

I would say we could disallow identical group names, but after adding device kind to the name, I believe deuplicate names would only ever happen by an explicit decision by the user to set them to the same name - so maybe that part is a non-issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason of allowing instances with the same name?

Copy link
Contributor Author

@kthui kthui Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It is always allowed on Triton. I am not sure on the reason why it was allowed in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than what is currently implemented. I do not think there is a good reason to allow instances with the same name.

const bool passive = group.passive();
std::vector<std::tuple<
std::string, TRITONSERVER_InstanceGroupKind, int32_t,
Expand Down