lib: update operator resolve #108

fs-eire · 2019-03-14T22:43:37Z

This change updates the behavior how to resolve a Graph.Node to an Operator in a session's initialization stage.

Change the IR version check to support IR version 4
Enable opset version check
Replace the switch-case based resolve to a data driven resolve
Allows opset version selector (currently support exact match and range match, eg. 7, 7+)

hariharans29 · 2019-03-20T06:21:06Z

lib/backend.ts

@@ -32,10 +33,9 @@ export interface SessionHandler {
  /**
   * Resolves the operator from the name; backend specific


nit: I think the comment would be - resolve the operator from the name and the opset version ?

hariharans29 · 2019-03-20T06:33:03Z

lib/backends/webgl/op-resolve-rules.ts

+  ['Less', '', '7+', () => new binaryOps.WebGLBinaryOp(NUMBER_TYPES, binaryOps.glslLess(), undefined, 'bool')],
+  ['Log', '', '6+', () => new unaryOps.WebGLUnaryOp(FLOAT_TYPES, unaryOps.glslLog())],
+  ['MatMul', '', '1+', () => new WebGLMatMul()],
+  ['MaxPool', '', '1-7', () => new WebGLMaxPool()],  // TODO: support new attributes for MaxPool-8 and MaxPool-10


Developer-centric question - I am guessing the way we support different versions of an op (where there is majority overlap across versions) is we just extend the current working class and override appropriate methods so as to align with the new spec ?

there are 2 ways.
one is to extend the existing implementation, if the new version is compatible with the old one according to the spec. (eg. add a new optional attribute, whose default value is the same of the old behavior)
one is to have different implementation, if the new version has a different behavior, or it is necessary to do so due to performance concern.

hariharans29 · 2019-03-20T06:34:37Z

lib/opset.ts

+  }
+
+  /**
+   * Domain of an opset, it can be an empty string(default value, represent for ai.onnx), or 'ai.onnx.ml'


can the domain be ai.onnx OR default value (empty string) to represent the ai.onnx domain ?

empty string means ai.onnx as in domain. the string 'ai.onnx' is not a valid value here

hariharans29 · 2019-03-20T06:40:43Z

lib/opset.ts

+          }
+        }
+      }
+      throw new TypeError(`cannot resolve operator '${opType}'`);


can error thrown also mention requested opset version of the operator cannot be resolved ?

hariharans29 · 2019-03-20T06:51:52Z

lib/backends/cpu/op-resolve-rules.ts

+  ['And', '', '7+', () => new CpuBinaryOp(['bool'], (e1, e2) => (e1 && e2))],
+  ['ArgMax', '', '1+', () => new CpuArgMax()],
+  ['Asin', '', '7+', () => new unaryOps.CpuUnaryOp(NUMBER_TYPES, unaryOps.asin)],
+  ['Atan', '', '7+', () => new unaryOps.CpuUnaryOp(NUMBER_TYPES, unaryOps.atan)],


How were the ResolveRules created for the very first time? How can we make sure it is accurate? Do we need to maintain any script(s) used to generate them ?

this is a pain point. one possible way is to grab more test cases from other place.

meanwhile, I am writing a script to generate the operator support table from data (to replace /docs/operators.md). that helps but still not a guarantee of 100% accuracy.

hariharans29 · 2019-03-20T07:00:19Z

At a high level, the changes look okay.

Looking at the build failures it looks to me that there might be issues concerning the array of ResolveRule that have been generated for each backend (or some bug in matchSelector() and/or resolveOperator() which I am sure can be ironed out). My only concern is the general accuracy of these set of ResolveRule for each backend.

fs-eire · 2019-03-20T07:04:06Z

At a high level, the changes look okay.
Looking at the build failures it looks to me that there might be issues concerning the array of ResolveRule that have been generated for each backend (or some bug in matchSelector() and/or resolveOperator() which I am sure can be ironed out). My only concern is the general accuracy of these set of ResolveRule for each backend.

I think the final solution is to make sure we have a good test coverage. Maybe we can leverage some test cases from onnxruntime.

hariharans29 · 2019-03-20T22:01:21Z

lib/backends/cpu/op-resolve-rules.ts

+  ['Atan', '', '7+', () => new unaryOps.CpuUnaryOp(NUMBER_TYPES, unaryOps.atan)],
+  ['AveragePool', '', '7+', () => new CpuAveragePool()],
+  ['BatchNormalization', '', '7+', () => new CpuBatchNormalization()],
+  ['Ceil', '', '6+', () => new unaryOps.CpuUnaryOp(NUMBER_TYPES, unaryOps.ceil)],


Also, how can we ensure potentially a bad commit that could have conflicting operator resolves possible ?

For example, Ceil 6+ could be mapped to an op, let's say Ceil 10 has has a different op implementation, but the developer forgets to change 6+ to 6-9. In that case, multiple op resolves are possible.

One way is to run a script at check-in time but we will have to maintain that...

hariharans29 · 2019-03-21T02:43:41Z

lib/backends/cpu/op-resolve-rules.ts

+  ['Sqrt', '', '6+', () => new unaryOps.CpuUnaryOp(NUMBER_TYPES, unaryOps.sqrt)],
+  ['Squeeze', '', '1+', () => new CpuSqueeze()],
+  ['Sub', '', '7+', () => new CpuBinaryOp(NUMBER_TYPES, (e1, e2) => (e1 - e2))],
+  ['Sum', '', '6+', () => new CpuSum()],  // TODO: support multidirectional broadcast for Sum-8


Do we need to throw exception when it looks like broadcasting support maybe required in the implementation (since it support 6+) ?

the exception is thrown anyway. currently it will be thrown due to checkInput failure. the only difference is the error message

hariharans29 · 2019-03-22T00:36:05Z

There are a couple of nits left out from being resolved/addressed-

comment update in backend.ts
update error message in opset.ts to indicate that the opset version of the operator cannot be resolved (as opposed to the generic - operator could not be resolved which might be misleading to anyone who looks at the code base and finds implementation for that operator (from an earlier opset))

fs-eire added 2 commits March 14, 2019 14:49

lib: update operator resolve

9c9060d

Merge remote-tracking branch 'ms/master' into operators-resolve

f75c96b

fs-eire mentioned this pull request Mar 19, 2019

How to transform pytotrch model to onnx model with opset 7? onnx/onnx#1715

Closed

fs-eire added 3 commits March 19, 2019 12:04

fix some operator resolve rules

32820ff

support range version selector

d7f1198

fix version for some operators

29d9661

hariharans29 reviewed Mar 20, 2019

View reviewed changes

fs-eire added 4 commits March 20, 2019 16:35

resolve comments and fix bugs

c960054

Merge remote-tracking branch 'ms/master' into operators-resolve

1f2b39a

fix rules for wasm and webgl

7a6c154

fix webgl rule for Sum

3a517a1

hariharans29 reviewed Mar 21, 2019

View reviewed changes

hariharans29 approved these changes Mar 22, 2019

View reviewed changes

resolve comments

c24c79b

fs-eire merged commit 563dbd6 into microsoft:master Mar 22, 2019

fs-eire deleted the operators-resolve branch March 22, 2019 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: update operator resolve #108

lib: update operator resolve #108

fs-eire commented Mar 14, 2019

hariharans29 Mar 20, 2019

fs-eire Mar 20, 2019

hariharans29 Mar 20, 2019

fs-eire Mar 20, 2019

hariharans29 Mar 20, 2019

fs-eire Mar 20, 2019 •

edited

Loading

hariharans29 Mar 20, 2019

hariharans29 Mar 20, 2019

fs-eire Mar 20, 2019

hariharans29 commented Mar 20, 2019

fs-eire commented Mar 20, 2019

hariharans29 Mar 20, 2019

hariharans29 Mar 21, 2019

fs-eire Mar 21, 2019

hariharans29 commented Mar 22, 2019 •

edited

Loading

		@@ -32,10 +33,9 @@ export interface SessionHandler {
		/**
		* Resolves the operator from the name; backend specific

lib: update operator resolve #108

lib: update operator resolve #108

Conversation

fs-eire commented Mar 14, 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

fs-eire Mar 20, 2019 • edited Loading

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

hariharans29 commented Mar 20, 2019

fs-eire commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hariharans29 commented Mar 22, 2019 • edited Loading

fs-eire Mar 20, 2019 •

edited

Loading

hariharans29 commented Mar 22, 2019 •

edited

Loading