make primary ops function step 2 #4512

liqunfu · 2022-09-15T00:31:14Z

Description

update reduce ops so that axes is the second optional input
change reduce ops to function op

Motivation and Context

to reduce amount of primary ops

…e simple function? Signed-off-by: Liqun Fu <liqfu@microsoft.com>

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

xadupre · 2022-10-05T04:12:56Z

This PR will have a significant impact on converting librairies.

onnx/test/shape_inference_test.py

onnx/shape_inference/implementation.cc

liqunfu · 2022-10-05T16:10:49Z

This PR will have a significant impact on converting librairies.

Yes it does. For updated ops, I have added appropriate adapters for model converter. Anything else I need to do?

gramalingam · 2022-10-05T16:16:32Z

@BowenBao : for your awareness: this PR proposes to promote the axes attribute to input for various Reduce* ops. (This was already done for ReduceSum previously, but not for other ops.) I hope the impact on the converter won't be much? It would be good to know, thanks.

onnx/reference/ops/_op.py

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

xadupre · 2022-11-16T08:27:04Z

onnx/cpp2py_export.cc

+                all_function_opset_versions.end(),
+                context_dependent_function_opset_versions.begin(),
+                context_dependent_function_opset_versions.end());
+            std::sort(all_function_opset_versions.begin(), all_function_opset_versions.end());


Is there a possibility the result contains duplicates?

good point, added code to remove duplications. op authors shall not define both type of functions with the same version. all_function_opset_versions is used to check whether there is any function so we generate doc properly.

xadupre · 2022-11-16T08:33:03Z

onnx/reference/ops/op_reduce_l1.py

+        noop_with_empty_axes = noop_with_empty_axes or self.noop_with_empty_axes  # type: ignore
+        return self._run(data, axes, keepdims, noop_with_empty_axes)
+
+    def _run(self, data, axes, keepdims=1, noop_with_empty_axes=0):  # type: ignore


You may remove def run and define the default values for def _run as None. If None, the main class automatically looks into the node definition to replace them or the schema if they are not specified.

But is this wrong? I find this current form useful since a reader can understand what happens (what is the default value) without having to look elsewhere.

@xadupre , I agree with @gramalingam here. We need at least on call (_run here) to follow precisely the op schema signature. (I was thinking that _run does not have to use None as default for op attributes. It turned out it is not true)

In addition, I think the suggested _run signature does not work with nest calls - when the op is part of other function ops. There for now I like to keep the current code.

xadupre · 2022-11-16T08:35:07Z

onnx/shape_inference/implementation.cc

@@ -294,6 +295,10 @@ class ShapeInferenceImplBase {
          } else if (attr.type() == AttributeProto::SPARSE_TENSOR && attr.has_sparse_tensor()) {
            input_sparse_data_by_name[n.output(0)] = &attr.sparse_tensor();
          }
+        } else if (attr.name() == "value_ints" && attr.type() == AttributeProto::INTS) {


I would put attr.type() == AttributeProto::INTS first. Both conditions should be equivalent but checking an int is faster than a string.

xadupre · 2022-11-16T08:43:57Z

onnx/test/reference_evaluator_backend_test.py

@@ -60,6 +60,26 @@
    # not implemented
    "test__simple_gradient_of_add",  # gradient not implemented
    "test__simple_gradient_of_add_and_mul",  # gradient not implemented
+    "test_layer_normalization_2d_axis1_expanded",  # https://github.com/onnx/onnx/issues/4653


See https://github.com/onnx/onnx/blob/main/onnx/backend/test/case/node/split.py#L13, I followed the convention defined in this file. You can specify an opset with suffix _opset<opset>. This might not fully answer issue #4653. The test test_layer_normalization_4d_axis_negative_3_expanded_ver18 should work if the function is written with operator defined in opset 18.

you are right that every test ending with _ver18 shall pass. I enabled those tests.
regarding naming of expended tests, they are auto generated. I used _ver# for opset import for the function body.

xadupre · 2022-11-16T08:50:43Z

onnx/test/reference_evaluator_backend_test.py

@@ -461,7 +481,7 @@ class TestOnnxBackEndWithReferenceEvaluator(unittest.TestCase):

    @classmethod
    def add_test_methods(cls):
-        for folder in ["node", "pytorch-converted", "pytorch-operator", "simple"]:
+        for folder in ["node"]:


Why this change?

My mistake. I commented for debugging. Forgot to bring it back.

xadupre · 2022-11-16T08:50:59Z

onnx/test/reference_evaluator_backend_test.py

-        for te in enumerate_onnx_tests("node", lambda folder: folder == name):
-            code.append(te)
-        self.assertEqual(len(code), 1)
+    # def test_onnx_backend_test_abs(self):


Why disabling the test?

forgot bring it back after debugging

gramalingam · 2022-11-16T18:56:59Z

onnx/defs/schema.cc

-  else
-    return false;
+bool OpSchema::BuildContextDependentFunction(const FunctionBodyBuildContext& ctx, FunctionProto& function_proto) const {
+  return BuildContextDependentFunctionWithOpsetVersion(ctx, function_proto, since_version_);


Nit: Do we need these two different methods? Or, can we just use BuildContextDependentFunction with a default value for parameter requested_opset_version?

gramalingam · 2022-11-16T19:02:07Z

onnx/defs/schema.cc

+}
+
+const FunctionProto* OpSchema::GetFunctionWithOpsetVersion(int requested_opset_version) const {
+  return const_cast<OpSchema*>(this)->GetFunctionWithOpsetInternal(requested_opset_version);


Why do we need this const_cast ? May be outdated. I don't see any UpdateFunctionProtoOpsetImportVersion any more. May be simpler to inline the internal function here.

onnx/shape_inference/implementation.cc

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

jcwchen

Some nit comments. I saw some test_data_set_0 only has input_1.pb without input_0.pb. Is it expected? Since this PR really introduces a lot of test files (model.onnx, input/output pb files), it would be super great if we can verify them in ORT in advance.

jcwchen · 2022-11-21T17:47:37Z

onnx/test/shape_inference_test.py

@@ -2309,24 +2310,70 @@ def test_gemm_no_bias(self) -> None:
            graph, [make_tensor_value_info("out", TensorProto.FLOAT, (13, 17))]
        )

-    def test_reduce_op_shape_2_axis(self) -> None:
+    def test_reduce_op_shape_2_axis_opset_version_13(self) -> None:


nit: could we keep the test name short like test_reduce_op_shape_2_axis_opset13?
Existing test uses something like test_scan_opset9 actually.

onnx/version_converter/convert.h

docs/Operators-ml.md

jcwchen · 2022-11-21T17:57:38Z

onnx/backend/test/case/node/__init__.py

+            onnx_ai_opset_imports = [
+                oi
+                for oi in kwargs["opset_imports"]
+                if oi.domain == "" or oi.domain == "ai.onnx"


tiny nit: more pythonic.

Suggested change

if oi.domain == "" or oi.domain == "ai.onnx"

if oi.domain in ("", "ai.onnx")

onnx/defs/schema.cc

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

liqunfu · 2022-11-22T00:02:10Z

Some nit comments. I saw some test_data_set_0 only has input_1.pb without input_0.pb. Is it expected? Since this PR really introduces a lot of test files (model.onnx, input/output pb files), it would be super great if we can verify them in ORT in advance.

That is because due to the test change, only input1 is changed. input0 stays unchanged so it is not in the PR.

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

gramalingam

LGTM. Only nit comment left is whether HasContextDependentFunctionWithOpsetVersion can be renamed HasContextDependentFunction.

hit with optional attribute (axes) w/o default value. Can it still us…

c2dce2d

…e simple function? Signed-off-by: Liqun Fu <liqfu@microsoft.com>

liqunfu requested a review from a team as a code owner September 15, 2022 00:31

liqunfu marked this pull request as draft September 15, 2022 00:31

liqunfu added 5 commits September 15, 2022 13:41

use function builder

519ccd0

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

Merge branch 'main' into liqun/primary-to-function2

9d4c000

reduce ops, pass local tests

e16f176

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

Merge branch 'main' into liqun/primary-to-function2

07eeaf4

merge main

0070f80

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

liqunfu marked this pull request as ready for review September 26, 2022 19:58

liqunfu added 2 commits September 26, 2022 14:00

include algorithm

f54c6bc

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

update convert.h

d415239

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

liqunfu requested a review from a team as a code owner September 26, 2022 21:50

liqunfu added 13 commits September 26, 2022 15:07

fix ints with value_ints

7cce6b5

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

e081b1a

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

6dc90ec

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

gtest fix

a81b687

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

merge main

fcec71f

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

axes as input

32b6472

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

update operator_set and op version

5c13d4d

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

merge main

af2dc2f

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

fix buil and test failures

a147fe0

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

shape inference

6a77292

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

merge main

02a9013

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

2caf875

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

1dd84dd

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

gramalingam reviewed Oct 5, 2022

View reviewed changes

onnx/test/shape_inference_test.py Show resolved Hide resolved

gramalingam reviewed Oct 5, 2022

View reviewed changes

onnx/shape_inference/implementation.cc Show resolved Hide resolved

gramalingam reviewed Nov 15, 2022

View reviewed changes

onnx/reference/ops/_op.py Show resolved Hide resolved

liqunfu added 3 commits November 15, 2022 17:53

reviewer's comments

c355e57

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

3a6e8a1

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

a1e06bc

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

xadupre reviewed Nov 16, 2022

View reviewed changes

gramalingam reviewed Nov 16, 2022

View reviewed changes

gramalingam reviewed Nov 17, 2022

View reviewed changes

onnx/shape_inference/implementation.cc Show resolved Hide resolved

gramalingam reviewed Nov 17, 2022

View reviewed changes

onnx/shape_inference/implementation.cc Outdated Show resolved Hide resolved

liqunfu added 4 commits November 17, 2022 13:44

reviewers' comments

ed4e986

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

merge main

3636d74

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

5b767aa

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

2491879

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

jcwchen reviewed Nov 21, 2022

View reviewed changes

gramalingam reviewed Nov 21, 2022

View reviewed changes

onnx/defs/schema.cc Outdated Show resolved Hide resolved

reviewers' comments

bfcc095

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

liqunfu added 2 commits November 21, 2022 16:07

format

33d82aa

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

format

7e6006f

Signed-off-by: Liqun Fu <liqfu@microsoft.com>

gramalingam approved these changes Nov 22, 2022

View reviewed changes

liqunfu merged commit 7a1fae4 into onnx:main Nov 22, 2022

liqunfu deleted the liqun/primary-to-function2 branch November 22, 2022 00:45

jcwchen mentioned this pull request Dec 15, 2022

Referencing _function_body of operator schema causes SEGV #4703

Closed

justinchuby pushed a commit to justinchuby/onnx that referenced this pull request Jan 27, 2023

make primary ops function step 2 (onnx#4512)

524566e

broune pushed a commit to broune/onnx that referenced this pull request May 6, 2023

make primary ops function step 2 (onnx#4512)

31b7b45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make primary ops function step 2 #4512

make primary ops function step 2 #4512

liqunfu commented Sep 15, 2022 •

edited

xadupre commented Oct 5, 2022

liqunfu commented Oct 5, 2022

gramalingam commented Oct 5, 2022

xadupre Nov 16, 2022

liqunfu Nov 17, 2022

xadupre Nov 16, 2022

gramalingam Nov 16, 2022

liqunfu Nov 17, 2022 •

edited

liqunfu Nov 17, 2022

xadupre Nov 16, 2022

xadupre Nov 16, 2022 •

edited

liqunfu Nov 17, 2022

xadupre Nov 16, 2022

liqunfu Nov 17, 2022

xadupre Nov 16, 2022

liqunfu Nov 17, 2022

gramalingam Nov 16, 2022

gramalingam Nov 16, 2022

jcwchen left a comment

jcwchen Nov 21, 2022

jcwchen Nov 21, 2022

liqunfu commented Nov 22, 2022

gramalingam left a comment

	if oi.domain == "" or oi.domain == "ai.onnx"
	if oi.domain in ("", "ai.onnx")

make primary ops function step 2 #4512

make primary ops function step 2 #4512

Conversation

liqunfu commented Sep 15, 2022 • edited

Description

Motivation and Context

xadupre commented Oct 5, 2022

liqunfu commented Oct 5, 2022

gramalingam commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liqunfu Nov 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xadupre Nov 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcwchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liqunfu commented Nov 22, 2022

gramalingam left a comment

Choose a reason for hiding this comment

liqunfu commented Sep 15, 2022 •

edited

liqunfu Nov 17, 2022 •

edited

xadupre Nov 16, 2022 •

edited