Update model execution API #94

wchao1115 · 2020-09-28T01:12:00Z

Support optional outputs and optional input shapes. (Model Execution API #87)
Model builder interface for state separation between context state and graph state. (Model Execution API #87)
Fold the execution interface into the compilation interface for simplicity. (Model Execution API #87)
Remove quantization-related params from the operand descriptor. (Remove quantization-specific params from OperandDescriptor #44)
Simplify operand type enum. (Model Execution API #87)
Update URL references of first-wave operators. (Evaluate noise suppression models for required ops #66)
Update spec code samples and comments accordingly.

…uilder interface for state separation between context and graph state. Fold the execution interface into the compilation interface for simplicity. Remove quantization-related params from the operand descriptor. Simplify operand type enum. Update code examples and comments accordingly. Update URL references of first-wave operators.

huningxin

@wchao1115 , thanks for your great work. I left some comments, please take a look.

index.bs

op_compatibility/first_wave_models.md

index.bs

huningxin · 2020-09-28T02:24:46Z

index.bs

@@ -217,13 +213,6 @@ dictionary OperandDescriptor {
  // The dimensions field is only required for tensor operands.


As we are going to remove the tensor-type values of OperandType, we may need to define how to define a scalar. The options are no dimensions member, e.g. {type: 'float32'} or empty dimensions, e.g. {type: 'float32', dimensions: []}.

I plan to open a new issue on scalar tensor.

…Operands, Inputs and Outputs to NamedOperands, NamedInputs, and NamedOutputs. Update URL links of the first-wave operators doc.

huningxin · 2020-09-28T14:19:07Z

Another thing is whether we need to change the informative sample code where nn is used. e.g. for relu, the sample code is

return nn.max(nn.constant(0), x);

huningxin

Looks good to me. Thanks @wchao1115 .

huningxin · 2020-10-01T14:22:58Z

index.bs

+        builder.mul(
+          builder.reshape(scale, shape),
+          builder.div(
+            builder.sub(input, reshape(mean, shape)),


one nit: should it be builder.reshape? it was an old issue.

pyu10055 · 2020-10-01T14:29:20Z

index.bs

@@ -1033,23 +1031,29 @@ dictionary CompilationOptions {
 };

 interface Model {
-  Promise<Compilation> createCompilation(optional CompilationOptions options = {});
+  Promise<Compilation> compile(optional CompilationOptions options = {});


With this API, the computation is tied with the compilation. should the compile method have the inputs/outputs pair and execution can only execution the graph compiled with the input/output pair? Otherwise, the compilation might not support the execution of the sub-graph scenario?

@pyu10055 Good idea. Will this work now?

Do we need this change?

@pyu10055 , according to #87 (comment), you seemed to agree the Promise<NamedOutputs> compute(NamedInputs inputs, optional NamedOutputs outputs); (the ability to specify the outputs when executing) would support the sub-graph execution use case, correct?

@huningxin I actually do not recommend partial execution of a compiled graph as any operator in between the input and output operands may be folded with the surrounding operators from fusions or even eliminated by constant folding. The only guaranteed executable operands are the input and the output operands of the compiled graph. Partial compilation of a model is much more robust. Once a compilation is produced for the partial model, it stays valid and can be executed as such.

@wchao1115 , I only mean the compute method now allows developers to select the output operands of a compiled graph. I didn't recommend the partial execution for any intermediate operands between inputs and outputs.

My point is that the graph building is in the context of new ModelBuilder object. Developers are free to create a sub-graph by createModel API and compile/execute it. I think it would support the sub-graph execution use case without adding inputs/outputs pair to compile method.

@huningxin and I discussed this specific change offline. We want to track partial graph execution as a separate issue. @pyu10055 you may create a new issue just for this topic if you wish. I will now complete this PR with the original set of changes as outlined in the PR description above.

Created #105 as follow-up.

anssiko · 2020-10-06T17:42:21Z

@pyu10055 PTAL. This is the last call for comments prior to merging this PR. Please provide your feedback by Pacific Time afternoon today.

We'd like to get this PR landed to unblock other pending changes. Thanks!

zolkis · 2020-10-07T07:35:19Z

Good changes, helps clarity.

pyu10055 · 2020-10-19T20:56:11Z

index.bs

-          nn.div(
-            nn.sub(input, reshape(mean, shape)),
-            nn.sqrt(nn.add(nn.reshape(variance, shape), nn.constant(epsilon)))
+    return builder.add(


ideally it would be great to allow chained api for the Operands.
builder.constant(...).add(builder.constant(...))

I think this is a good idea. Created #106 to follow up.

wchao1115 requested review from pyu10055, anssiko and huningxin September 28, 2020 01:12

huningxin reviewed Sep 28, 2020

View reviewed changes

Add int8/uint8 operand types. Make createModel a sync method. Rename …

0970115

…Operands, Inputs and Outputs to NamedOperands, NamedInputs, and NamedOutputs. Update URL links of the first-wave operators doc.

Update existing sample code with builder instead of nn.

fd576ba

anssiko approved these changes Oct 1, 2020

View reviewed changes

huningxin approved these changes Oct 1, 2020

View reviewed changes

huningxin reviewed Oct 1, 2020

View reviewed changes

pyu10055 reviewed Oct 1, 2020

View reviewed changes

wchao1115 added 2 commits October 3, 2020 22:27

Allow subgraph compilation of the model.

5f71470

Fix an old typo.

d8e170f

Revert the change to allow partial compilation for now.

24eba44

wchao1115 merged commit 65772f1 into master Oct 7, 2020

wchao1115 deleted the wchao/execution_api_change branch October 7, 2020 05:55

This was referenced Oct 7, 2020

Remove quantization-specific params from OperandDescriptor #44

Closed

Model Execution API #87

Closed

pyu10055 reviewed Oct 19, 2020

View reviewed changes

This was referenced Oct 20, 2020

Support the execution of the sub-graph scenario #105

Closed

Chained API for the Operands #106

Closed

		@@ -217,13 +213,6 @@ dictionary OperandDescriptor {
		// The dimensions field is only required for tensor operands.

Update model execution API #94

Update model execution API #94

Uh oh!

Conversation

wchao1115 commented Sep 28, 2020 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huningxin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huningxin commented Sep 28, 2020

Uh oh!

huningxin left a comment

Choose a reason for hiding this comment

Uh oh!

huningxin Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anssiko commented Oct 6, 2020

Uh oh!

zolkis commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wchao1115 commented Sep 28, 2020 •

edited by pr-preview bot

Loading

huningxin Oct 1, 2020 •

edited

Loading