-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add scalar support in ORT backend #213
Conversation
6b42724
to
0b6b36a
Compare
0b6b36a
to
a855fa5
Compare
src/onnxruntime.cc
Outdated
model_state_->MaxBatchSize(), true /* compare_exact */)); | ||
// if max_batch_size == 0 and is a scalar tensor all the | ||
// dimensions specified must be equal to 1 | ||
if (model_state_->MaxBatchSize() > 0 || iit->second.dims_.size() > 0) { |
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.
Summarizing offline discussion, I'd rather see MaxBatchSize()
removed from this condition of "is_scalar" if not necesary. Maybe something like this:
const bool is_scalar = (iit->second.dims_.size() == 0);
if (is_scalar) {
// Dimensional "volume" of Triton dims must be 1 for scalars.
if std::any_of(dims.begin(), dims.end(), [](int dim) { dim != 1}) {
// ERROR
}
scalar_outputs_[io_name] = dims;
} else {
RETURN_IF_ERROR(CompareDimsSupported(...));
}
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.
@@ -885,7 +885,9 @@ ModelState::AutoCompleteIO(const char* key, const OnnxTensorInfoMap& io_infos) | |||
triton::common::TritonJson::Value reshape_dims( | |||
ModelConfig(), triton::common::TritonJson::ValueType::ARRAY); | |||
RETURN_IF_ERROR(reshape.Add("shape", std::move(reshape_dims))); | |||
RETURN_IF_ERROR(io.Add("reshape", std::move(reshape))); | |||
if (MaxBatchSize() > 0) { |
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.
Can you add a comment about why reshape causes issues with non-batching case for future reference?
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, is this breaking any functionality for non-batching models that specify a "reshape" in their model config? Such as our densenet example: https://github.com/triton-inference-server/server/blob/main/docs/examples/model_repository/densenet_onnx/config.pbtxt
This looks like it's restricted to (1) autocomplete and (2) dims are empty. But just double checking.
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.
added a comment.
I don't think it would break it since they all do have some dimensions.
src/onnxruntime.cc
Outdated
@@ -2283,6 +2352,22 @@ ModelInstanceState::ReadOutputTensors( | |||
batchn_shape, dtype, output_tensor, &output_buffer, string_buffers, | |||
offsets)); | |||
|
|||
// If the number of dimensions is equal to zero, it means that it is a | |||
// scalar and it would use the dimensions specified in the mdel |
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.
// scalar and it would use the dimensions specified in the mdel | |
// scalar and it would use the dimensions specified in the model |
testing: triton-inference-server/server#6343