[Draft] Add function body implementation in opschema #7

raymondxyang · 2019-02-13T06:08:40Z

Add function_body field in opschema, add verification for function op register
Make type/shape inference optional for opschema with function body
Add related python APIs and cpp test case

raymondxyang · 2019-02-13T06:12:39Z

@gramalingam could you also help take a look? Thanks

onnx/checker.cc

gramalingam · 2019-02-21T19:48:55Z

onnx/defs/schema.cc

+  ctx.set_opset_imports(op_set);
+  ctx.set_is_main_graph(false);
+
+  checker::TENSOR_TYPES_MAP tc_map;


This type checking part is more complex. I suggest removing this from the current PR into a separate one. There are two challenges. One is that if the function inputs are allowed to have one of several different types, then the checking becomes more complex. The simplest way to do that is to check for every possible type assignment to all variables. Secondly, input.GetTypes() may return a type-variable name instead of a type.

gramalingam · 2019-02-21T19:51:35Z

onnx/checker.cc

+  }
+  verify_node_schema(node, schema, domain_version);
+
+  // Type constraints check between op and function body.


I suggest removing this check from the current PR. There are a couple of issues here. First, the "type strings" being compared here may be type-variables, and not types. Second, this check is incomplete, since it does not do type inference for all nodes, and checks only direct references to function parameters.

gramalingam · 2019-02-22T20:36:26Z

onnx/defs/schema.cc

+      for (auto& opset_entry_ : m) {
+        for (auto& op_versions_entry_ : opset_entry_.second) {
+          for (auto& op_entry_ : op_versions_entry_.second) {
+            VerifyFunction(op_entry_.second, m);


Hi, why not do the call to VerifyFunction as an extra step in "Finalize" (around line 690 in old file and line 740 in your version)? I think that will be better.

gramalingam · 2019-02-26T00:51:42Z

Hi Raymond, the changes look good. I have a question/suggestion: there seems to some code duplication between check_function and check_graph. Is it worth eliminating this duplication, may be using a common shared function called from both?

gramalingam · 2019-02-26T01:07:45Z

Another question: is VerifyFunctionNode used anywhere? Now, it seems to have no usage inside ONNX. (Expanding the function definition and checking it is required only for type-checking; so, it seems unnecessary for the checking done here.)

gramalingam · 2019-02-26T01:47:42Z

onnx/test/cpp/function_verify_test.cc

+      }
+      for (auto& type : tensor_name_tc.second) {
+        if (allowed_types.find(type) == allowed_types.end()) {
+          fail_check(


We can't call this an error yet, because the elements in allowed_types might be a type variable. Furthermore, the two separate loops (lines 53 to 56 and the one in line 57 is not required, since we should be able to directly do the check at line 55 for the ith argument.

Hi Rama.. sorry just saw your comment. see L22-L29. I exclude the type variable already. This one is only enforced on primitive types

For the other two comments: the check_function is a existing function. I didnt change any of that (just misplaced it so there is diff.) Ill put it back and see if we can do any modification on that. For VerifyFunctionNode Ill remove it. Thanks for pointing that out 😃

L23 should be all_tensor_types(). Ill change it.

Hi Raymond, you excluded it in tc_map. But allowed_types can include type-variables as well. You have not excluded it from that. In any case, I don't think that the overall structure for this type-checking is the right one for extending it to handle all cases.

Sorry I am not familiar with the type variable: If a node in a function node, which consumes or produce a tensor with variable type, and is also a exposed tensor of the whole function op, should this variable type also appear in tc_map, which generated from the overall defined type constraints?

I try to figure out a way to check constaints between op's type variable and node inside function body but somehow cannot find a specific rule in order to generate one.. I think we can explore how to resolve this later.. its a huge item

Sorry, ignore my comment about type-variables. I see that "Finalize" on OpSchema would have converted the type-variable to a list of proper types using type-constraints already. (But I still think we will need to replace this with the logic we have in "Graph::InferAndVerifyTypeMatch(…)" etc.)

But we can do that separately. Can you just add a comment line saying "TODO: apply complete type inference across function body for a more complete check?"

Sure. I already put a todo there.

@staticmethods

* Make Runner class more flexible * Change @staticmethods to @classmethods in test Runner * Rename assert_similar_outputs and download_model functions

This reverts commit cdbe6b4.

This reverts commit 8882688.

Treat auto_pad notset as no auto_pad, since our newest Modelzoo onnx models still make auto_pad notset instead of removing it.

…ion-op

gramalingam · 2019-03-04T18:33:43Z

onnx/defs/experiments/experiments_functions.cc

-  func.set_status(OperatorStatus::STABLE);
-
-  NodeProto* initial_node0 = func.add_node();
+static std::vector<NodeProto> BuildMVN() {


I am slightly concerned that there may be some extra copying going on here: but may be the vectors will be "moved" cheaply by the compiler (avoiding redundant copies)?

gramalingam · 2019-03-04T18:46:44Z

onnx/defs/schema.cc

@@ -622,6 +622,38 @@ void OpSchema::ParseAndSetTypes(
  }
 }

+OpSchema& OpSchema::FunctionBody(const std::vector<NodeProto>& func_nodes) {


Can we avoid copies if we use a non-reference parameter "std::vector func_nodes" and do a std::move? Just wondering what is the best way to avoid some of the needless copies of nodes.

* Remove exp op: Affine, ImageScaler,ParametricSoftplus, Crop. * update the test case.

* More changes * More changes * More changes * More changes * Check-in modified model for topK * More changes * More changes * More cahnges * Changes to md files * More changes * More changes * Fix broken build * Fix build break * Fix build break * Fix build break * More changes * Fix build break * revert * revert file to old state

…t bumped when changing its type constraint declaration. (onnx#1848)

and all other objects.

Add function body implementation in onnx opschema

d4ac10e

raymondxyang requested a review from linkerzhang February 13, 2019 06:08

Raymond Yang added 14 commits February 13, 2019 13:11

Merge with master

b2e8b1c

Fix

ece2498

Fix

abefa29

Fix copy constructor and gcc warning

b828382

Fix

18f4e1b

Update doc genration scripts to contain function information

baf04d6

Sync with master branch

acd8767

Update

00cef71

Flake

48cc91d

typecheck

30ef083

Fix header

fd25ba5

Add support type

7bf5c92

Rename testcase

196bb1e

Update documents

e58524e

raymondxyang requested a review from gramalingam February 19, 2019 19:28

gramalingam reviewed Feb 19, 2019

View reviewed changes

onnx/checker.cc Outdated Show resolved Hide resolved

gramalingam reviewed Feb 19, 2019

View reviewed changes

onnx/checker.cc Outdated Show resolved Hide resolved

Raymond Yang added 3 commits February 20, 2019 11:29

Fix draft comments

be8a12b

Sync with master

a0667f1

Update doc

6a2ef1a

gramalingam reviewed Feb 21, 2019

View reviewed changes

gramalingam reviewed Feb 22, 2019

View reviewed changes

Raymond Yang added 2 commits February 25, 2019 15:55

Move function verify test to CI

abc815f

Merge branch 'master' of github.com:onnx/onnx into function-op

9a69250

gramalingam reviewed Feb 26, 2019

View reviewed changes

Raymond Yang and others added 14 commits February 26, 2019 10:51

Fix

b3bb0a5

Remove redundant field

cdbe6b4

More extendable Runner (onnx#1809)

873ddbb

* Make Runner class more flexible * Change @staticmethods to @classmethods in test Runner * Rename assert_similar_outputs and download_model functions

Add missed changes in proto files

8882688

Merge branch 'master' into function-op

993478b

fix

8bc648f

Update __init__.py

1229c74

Revert "Remove redundant field"

279c798

This reverts commit cdbe6b4.

Revert "Add missed changes in proto files"

3b46fa1

This reverts commit 8882688.

Fix shape inference when auto_pad is notset again (onnx#1830)

96c58ce

Treat auto_pad notset as no auto_pad, since our newest Modelzoo onnx models still make auto_pad notset instead of removing it.

Resolve comments

9c5ed17

Merge branch 'function-op' of github.com:raymondxyang/onnx into funct…

79fce86

…ion-op

Merge branch 'master' into function-op

2ad193e

Fix gcc

71e22be

gramalingam reviewed Mar 4, 2019

View reviewed changes

Raymond Yang and others added 14 commits March 4, 2019 13:58

Resolve comments on doc

d23ea68

Resolve comments

f6d4be9

Resolve comments

08be38e

Fix const reference

84aa344

Renaming

4d03a61

Remove exp op: Affine, ImageScaler,ParametricSoftplus, Crop. (onnx#1832)

a89a4a1

* Remove exp op: Affine, ImageScaler,ParametricSoftplus, Crop. * update the test case.

Resolve PR comments

67225a6

Rename function

d95a240

bump the version for drop out - fix the issue that the version was no…

1cca873

…t bumped when changing its type constraint declaration. (onnx#1848)

remove global variable in header file (onnx#1850)

3b0d3bb

Resolve PR comments

f8576af

Handle string comparision represented as np.objects (onnx#1851)

3dbf8fe

and all other objects.

Merge branch 'master' into function-op

022af20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Add function body implementation in opschema #7

[Draft] Add function body implementation in opschema #7

raymondxyang commented Feb 13, 2019 •

edited

Loading

raymondxyang commented Feb 13, 2019

gramalingam Feb 21, 2019

gramalingam Feb 21, 2019

gramalingam Feb 22, 2019

gramalingam commented Feb 26, 2019

gramalingam commented Feb 26, 2019

gramalingam Feb 26, 2019

raymondxyang Feb 26, 2019

raymondxyang Feb 26, 2019

raymondxyang Feb 26, 2019

gramalingam Feb 26, 2019

raymondxyang Feb 26, 2019

raymondxyang Feb 26, 2019

gramalingam Feb 26, 2019

gramalingam Feb 26, 2019

raymondxyang Feb 26, 2019

gramalingam Mar 4, 2019

gramalingam Mar 4, 2019

[Draft] Add function body implementation in opschema #7

Are you sure you want to change the base?

[Draft] Add function body implementation in opschema #7

Conversation

raymondxyang commented Feb 13, 2019 • edited Loading

raymondxyang commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gramalingam commented Feb 26, 2019

gramalingam commented Feb 26, 2019

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

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

raymondxyang commented Feb 13, 2019 •

edited

Loading