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

update with onnx 1.11 release #10441

Merged
merged 31 commits into from Mar 8, 2022
Merged

update with onnx 1.11 release #10441

merged 31 commits into from Mar 8, 2022

Conversation

liqunfu
Copy link
Contributor

@liqunfu liqunfu commented Jan 31, 2022 •
edited

Description:
Update with onnx 1.11.0 release,
Fix build breaks due to new onnx 1.11.0 APIs,
Use FunctionBuilder from onnx,
support new and updated onnx ops with new and updated cpu kernels: ROIAlign ScatterElements, ScatterND, GridSample

Motivation and Context
prepare for onnxruntime 1.11.0 release

@snnn
Copy link
Member

snnn commented Jan 31, 2022

in case you need: https://github.com/microsoft/onnxruntime/blob/master/docs/How_To_Update_ONNX_Dev_Notes.md

@liqunfu liqunfu changed the title (WIP ) validate onnx 1.11 release update with onnx 1.11 release Feb 22, 2022
gramalingam
gramalingam previously approved these changes Mar 2, 2022
View reviewed changes
gramalingam
gramalingam previously approved these changes Mar 4, 2022
View reviewed changes
edgchen1
edgchen1 reviewed Mar 4, 2022
View reviewed changes
onnxruntime/core/providers/cpu/tensor/grid_sample.cc
#include "core/framework/TensorSeq.h"
#include "core/providers/common.h"
#include "core/framework/copy.h"
#include "core/providers/op_kernel_type_control.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

are "core/framework/element_type_lists.h", "core/framework/TensorSeq.h", and "core/providers/op_kernel_type_control.h" needed?

this file doesn't seem to use any of the op kernel type control infrastructure or TensorSeq

Copy link
Contributor

Choose a reason for hiding this comment

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

can they be removed?

edgchen1
edgchen1 reviewed Mar 4, 2022
View reviewed changes
onnxruntime/core/providers/cpu/tensor/scatter.cc
struct Func_Add<BFloat16> {
void operator()(BFloat16*, const BFloat16*) const {
ORT_NOT_IMPLEMENTED("CPU execution provider: BFloat16 data type is not supported with ScatterElements opset 16 when reduction is 'add'.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

general nit: run formatter on changed files

Copy link
Contributor

Choose a reason for hiding this comment

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

there's still some odd formatting here

edgchen1
edgchen1 reviewed Mar 4, 2022
View reviewed changes
onnxruntime/core/providers/cpu/tensor/scatter_nd.cc
@@ -120,7 +145,7 @@ Status ScatterNDBase::PrepareForCompute(OpKernelContext* context, Prepare& p) co
auto* dst = output_tensor->template MutableData<std::string>();
std::copy(str_begin, str_end, dst);
} else {
memcpy(dst_base, src_base, input_tensor->SizeInBytes());
memcpy((void*)dst_base, (const void*)src_base, input_tensor->SizeInBytes());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: casting to void* shouldn't be necessary
also prefer to avoid C-style casts, see https://google.github.io/styleguide/cppguide.html#Casting

@edgchen1
Copy link
Contributor

edgchen1 commented Mar 4, 2022

would it be possible to uncomment these tests too? they depend on a fix in ONNX

onnxruntime/onnxruntime/test/optimizer/qdq_transformer_test.cc

Lines 1760 to 1765 in f1b6f0b

// TODO re-enable tests after updating ONNX to get QuantizeLinear shape inference fix
// https://github.com/onnx/onnx/pull/3806
// test_case({1, 13, 13, 23}, 4, {0, 3, 1, 2}, false, false);
test_case({1, 13, 13, 23}, 4, {0, 3, 1, 2}, false, true);
// test_case({1, 13, 13, 23}, 4, {0, 3, 1, 2}, true, false);
test_case({1, 13, 13, 23}, 4, {0, 3, 1, 2}, true, true);

onnxruntime/onnxruntime/test/optimizer/qdq_transformer_test.cc

Lines 1837 to 1842 in f1b6f0b

// TODO re-enable tests after updating ONNX to get QuantizeLinear shape inference fix
// https://github.com/onnx/onnx/pull/3806
// test_case({1, 13, 13, 23}, 4, {0, 3, 1, 2}, false, false);
test_case({1, 13, 13, 23}, 4, {0, 3, 1, 2}, false, true);
// test_case({1, 13, 13, 23}, 4, {0, 3, 1, 2}, true, false);
test_case({1, 13, 13, 23}, 4, {0, 3, 1, 2}, true, true);

thanks for updating this!

edgchen1
edgchen1 approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@edgchen1 edgchen1 left a comment

Choose a reason for hiding this comment

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

signing off on the kernel def hash changes. not as familiar with the rest so it would be good if someone else can take a look too.

@liqunfu liqunfu merged commit da885a7 into master Mar 8, 2022
@liqunfu liqunfu deleted the liqun/validate_onnx1.11 branch March 8, 2022 05:10
@hariharans29 hariharans29 mentioned this pull request Mar 8, 2022
Merged
@chilo-ms
Copy link
Contributor

This PR is already in rel-1.11.0 branch, so remove the label.

lavanyax pushed a commit to intel/onnxruntime that referenced this pull request Mar 29, 2022
@liqunfu @lavanyax
update with onnx 1.11 release (microsoft#10441)
a5ae0ea
siweic0 pushed a commit to siweic0/onnxruntime-web that referenced this pull request May 9, 2024
@liqunfu
update with onnx 1.11 release (microsoft#10441)
4b79264
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hariharans29 hariharans29 hariharans29 left review comments

@edgchen1 edgchen1 edgchen1 approved these changes

@gramalingam gramalingam gramalingam left review comments

@skottmckay skottmckay Awaiting requested review from skottmckay

@guoyu-wang guoyu-wang Awaiting requested review from guoyu-wang

@YUNQIUGUO YUNQIUGUO Awaiting requested review from YUNQIUGUO

Assignees

@hariharans29 hariharans29

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@liqunfu @snnn @edgchen1 @chilo-ms @hariharans29 @gramalingam @faxu