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

Jump to bottom

fix type inference issue (scalar initializers and Resize) #2983

Merged
merged 3 commits into from Aug 28, 2020
Merged

fix type inference issue (scalar initializers and Resize) #2983

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b931c1
fix type inference issue
gramalingam Aug 28, 2020
e3aed06
fix initializer shape in split_to_sequence test-cases
gramalingam Aug 28, 2020
ebfaf2e
Merge branch 'master' into fix-type-inf
gramalingam Aug 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion onnx/defs/tensor/utils.cc
View file
Open in desktop
Expand Up @@ -47,10 +47,10 @@ void resizeShapeInferenceHelper(
}

void resizeShapeInference(InferenceContext& ctx, bool is_resize_op) {
propagateElemTypeFromInputToOutput(ctx, 0, 0);
if (!hasNInputShapes(ctx, 1)) {
return;
}
propagateElemTypeFromInputToOutput(ctx, 0, 0);
const auto& input_shape = getInputShape(ctx, 0);
auto* output_shape = getOutputShape(ctx, 0);
const auto* scales = ctx.getInputData(is_resize_op ? 2 : 1);
Expand Down
12 changes: 5 additions & 7 deletions onnx/shape_inference/implementation.cc
View file
Open in desktop
Expand Up @@ -191,15 +191,13 @@ static void InferShapesImpl(
// Consider the tensors from the initializer
TypeProto *initializerType = new TypeProto();
TypeProto_Tensor* initializerTensorType = initializerType->mutable_tensor_type();
initializerTensorType->set_elem_type(tp.data_type());
// set the shape according to the initializer shape info
// if the shape info of initializer is not empty
if (tp.dims_size() != 0) {
initializerTensorType->set_elem_type(tp.data_type());
TensorShapeProto* shape = initializerTensorType->mutable_shape();
for (int i = 0 ; i < tp.dims_size(); ++i) {
shape->add_dim()->set_dim_value(tp.dims(i));
}
TensorShapeProto* shape = initializerTensorType->mutable_shape();
Copy link
Contributor

@askhade askhade Aug 28, 2020 •
edited

Choose a reason for hiding this comment

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

I think this check -> tp.dims_size() !=0 is needed before output tensor shape is initialized?
This can cause issues because has_shape for this tensor will return true from here onwards and depending what the calling code (whether it does necessary checks or not ) it can cause segfaults

See PR : #1951

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dims_size check here is not related to whether the shape is known or not known. Here, the shape is known exactly. If dims_size is 0, it means it is rank 0 (a scalar). This check is different from a "has_shape" kind of check.

Copy link
Member

@jcwchen jcwchen Aug 28, 2020 •
edited

Choose a reason for hiding this comment

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

I added tp.dims_size() != 0 because 3 tests in shape_inference_test will fail without this. My thought was same as @askhade that this can check whether the shape is initialized.
If dims_size=0 means a scalar, there might be other potential bugs in splitToSequence because after applying this PR, the ranks of initializer and input are different: RuntimeError: Inferred shape and existing shape differ in rank: (0) vs (1) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible that there are bugs elsewhere (in requiring rank 0 or rank 1 for supposedly-scalar inputs). Or, if the spec calls for rank 1, and the model uses rank 0, it is an error in the model. Adding the check here will only mask problems elsewhere, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The split_to_sequence testcases had an issue. I fixed them. Hope this goes through the CI.

for (int i = 0 ; i < tp.dims_size(); ++i) {
shape->add_dim()->set_dim_value(tp.dims(i));
}

auto iter = valueTypesByName.find(tp.name());
// If it already exists in input, check input and initializer is sync
// use shape info from input (input has priority over initializer)
Expand Down