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

pick onnx release candidate #7177

Merged
merged 26 commits into from
Apr 23, 2021
Merged

pick onnx release candidate #7177

merged 26 commits into from
Apr 23, 2021

Conversation

askhade
Copy link
Contributor

@askhade askhade commented Mar 30, 2021 •
edited
Loading

Description: Picking ONNX 1.9 release candidate. Updates in this PR include:

  1. Registration for opset 14 Add, Sub, Mul, Div (new data types added for these ops)
  2. Add implementation for Reshape 14
  3. Validate test data for Hardswish op (Hardswish is a function. This PR does not register an op for this instead uses the function body for test validation)
  4. Register Identity 14
  5. Remove transformer NonZeroShapeSetter. Shape inf function was added for NonZero in ONNX, so we don't need this transformer anymore.

Motivation and Context

  • Why is this change required? What problem does it solve?
  • If it fixes an open issue, please link to the issue here.

@askhade askhade requested a review from a team as a code owner March 30, 2021 03:19
snnn
snnn previously approved these changes Mar 30, 2021
View reviewed changes
@askhade askhade closed this Apr 1, 2021
@askhade askhade reopened this Apr 1, 2021
@askhade
Copy link
Contributor Author

askhade commented Apr 1, 2021

/azp run Linux CPU CI Pipeline, Linux CPU x64 NoContribops CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, MacOS NoContribops CI Pipeline, Windows CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline

@askhade
Copy link
Contributor Author

askhade commented Apr 1, 2021

/azp run Windows GPU CI Pipeline, WIndows GPU TensorRT CI Pipeline, centos7_cpu, centos7_cpu (linux_centos_ci Debug), centos7_cpu (linux_centos_ci Release), orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-distributed, orttraining-amd-gpu-ci-pipeline, Linux Nuphar CI Pipeline

@askhade askhade closed this Apr 2, 2021
@askhade askhade reopened this Apr 2, 2021
edgchen1
edgchen1 reviewed Apr 6, 2021
View reviewed changes
onnxruntime/core/providers/cuda/object_detection/non_max_suppression_impl.cu
@@ -14,16 +14,16 @@ limitations under the License.
==============================================================================*/
/* Modifications Copyright (c) Microsoft. */

#include <thrust/device_vector.h>
#include <thrust/execution_policy.h>

#include "non_max_suppression_impl.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

the related header should be first
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

edgchen1
edgchen1 reviewed Apr 6, 2021
View reviewed changes
orttraining/orttraining/test/optimizer/graph_transform_test.cc Outdated
@@ -104,7 +104,7 @@ TEST_F(GraphTransformationTests, NonZeroShapeSetter) {
auto nonzero_shape = GetNodeByName(graph, "nonzero")->OutputDefs()[0]->Shape();
ASSERT_TRUE(nonzero_shape->dim_size() == 2);
ASSERT_TRUE(nonzero_shape->dim(0).dim_value() == 2);
ASSERT_TRUE(nonzero_shape->dim(1).dim_param() == "nonzero_nonzero_count");
//ASSERT_TRUE(nonzero_shape->dim(1).dim_param() == "nonzero_nonzero_count");
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

marked the PR as WIP. This is a temp change... working on a formal fix...
In onnx, shape inference for NonZero was added, this shape inf method simply adds a unknown dim for 2nd dim and this takes precedence over the one added by transformer ... trying to understand whether we even need this transformer, now that onnx has added a shape inf method for this op.

Wanted to run validations for other changes so commented this one out to unblock

@askhade askhade changed the title pick onnx release candidate [WIP] pick onnx release candidate Apr 6, 2021
@SherlockNoMad SherlockNoMad mentioned this pull request Apr 7, 2021
Merged
@BowenBao BowenBao mentioned this pull request Apr 14, 2021
Merged
@askhade askhade changed the title [WIP] pick onnx release candidate pick onnx release candidate Apr 21, 2021
edgchen1
edgchen1 previously approved these changes Apr 21, 2021
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.

:shipit:

snnn
snnn previously approved these changes Apr 21, 2021
View reviewed changes
@askhade askhade dismissed stale reviews from snnn and edgchen1 via c8f4695 April 22, 2021 18:42
@snnn
Copy link
Member

snnn commented Apr 23, 2021

/azp run inux GPU TensorRT CI Pipeline

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@askhade askhade merged commit 75e054c into master Apr 23, 2021
@askhade askhade deleted the askhade/import_onnx1_9_candidate branch April 23, 2021 06:57
@jcwchen jcwchen mentioned this pull request Aug 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgchen1 edgchen1 edgchen1 approved these changes

@snnn snnn snnn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@askhade @snnn @edgchen1