Support async/sync functions #172

BruceDai · 2022-06-21T05:13:28Z

In first commit, I just updated add op tests using Async function. @huningxin PTAL, thanks.

huningxin

@BruceDai , thanks for the work, left some comments, please take a look.

package.json

src/nn/graph.ts

BruceDai · 2022-06-28T08:36:11Z

Thanks @huningxin.
I updated commit to address your comments, please take another look.
My second commit of updated other tests will be summitted later to fix CI failure.

createContext() / createContextSync() build() / buildSync() compute() / computeSync()

src/nn/graph.ts

src/nn/context.ts

src/nn/graph_builder.ts

BruceDai · 2022-07-05T07:33:40Z

Thanks @huningxin !
I've added these checks, and verified by running local WPT IDL tests in worker with WebNN-Polyfill.
Please take another look, thanks.

src/ml.ts

src/nn/context.ts

src/nn/graph.ts

test/api/graph_builder.js

huningxin · 2022-07-06T01:55:32Z

test/api/graph_builder.js

+
+  it('builder.build should throw for invalid parameters', async () => {
+    try {
+      await builder.build();


Should it reject with an error or throw?

test/api/graph_builder.js

BruceDai · 2022-07-26T08:02:43Z

@huningxin I've update this patch to address your comments, and added a sample by relu op test for invoking sync functions in worker, please take another look, thanks.

Fixes webmachinelearning#176

Honry

@BruceDai, leave a few comments, PTAL.

Honry · 2022-11-08T07:48:52Z

src/nn/graph.ts

      utils.assert(
          utils.isTypedArray(resource),
          'Only resource of ArrayBufferView type is supported.');
      utils.validateTypedArray(
-          resource as ArrayBufferView, inputOperand.desc.type, dimensions);
+          resource , inputOperand.desc.type, dimensions);


Nit:

Suggested change

resource , inputOperand.desc.type, dimensions);

resource, inputOperand.desc.type, dimensions);

Fixed, thanks.

Honry · 2022-11-09T01:45:19Z

src/nn/graph.ts

@@ -245,10 +199,18 @@ export class MLGraph {
  }

  /** @internal */
-  static buildAndCompile(outputs?: MLNamedOperands): MLGraph {
+  static async buildAndCompile(outputs?: MLNamedOperands): Promise<MLGraph> {


Maybe we can try merger buildAndCompile and buildAndCompileSync into one and declare the return types with |.

I tried to merge them to one function, but there's message The return type of an async function or method must be the global Promise<T> type., so I leave it as is.

Honry · 2022-11-09T01:48:56Z

src/nn/graph_builder.ts

+    return new Promise((resolve, reject) => {
+      let graph;
+      try {
+        graph = MLGraph.buildAndCompile(outputs);


buildAndCompile is async function, missing await keyword.

Fixed, please take another look at new commit, thanks.

Honry · 2022-11-09T01:52:24Z

test/api/context.js

-    expect(() => navigator.ml.createContext({
-      powerPreference: 'invalid',
-    })).to.throw(Error);
+describe('test MLContext.compute', function() {


It'd be better to use ES6 expression for consistency purpose.

Suggested change

describe('test MLContext.compute', function() {

describe('test MLContext.compute', () => {

Updated, thanks.

BruceDai · 2022-11-09T06:53:53Z

@Honry I've update PR, please take another review, thanks.

Honry

LGTM, thanks!

src/nn/graph.ts

huningxin · 2022-11-10T04:59:38Z

src/nn/graph.ts

-  compute(inputs: MLNamedInputs, outputs: MLNamedOutputs): void {
+  /** @internal */
+  async compute(
+    inputs:MLNamedArrayBufferViews, outputs: MLNamedArrayBufferViews,


nit: a blank space between inputs: and MLNamedArraybufferViews

Updated, thanks.

huningxin

Getting better, thanks for the update.

src/nn/graph.ts

huningxin

LGTM, thanks for the great work.

BruceDai · 2022-11-21T07:54:23Z

Thanks @Honry and @huningxin for your reviewing. I will merge this PR.

huningxin reviewed Jun 28, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/nn/graph.ts Show resolved Hide resolved

src/nn/graph.ts Outdated Show resolved Hide resolved

BruceDai force-pushed the support_context_based_graph branch from 95c9af5 to 7061d3a Compare June 28, 2022 08:31

Support async/sync functions:

7061d3a

createContext() / createContextSync() build() / buildSync() compute() / computeSync()

BruceDai force-pushed the support_context_based_graph branch from c0e1419 to 886b278 Compare July 1, 2022 07:58

Update tests with async functions.

886b278

huningxin reviewed Jul 5, 2022

View reviewed changes

src/nn/graph.ts Outdated Show resolved Hide resolved

src/nn/context.ts Show resolved Hide resolved

src/nn/graph_builder.ts Show resolved Hide resolved

huningxin reviewed Jul 6, 2022

View reviewed changes

BruceDai force-pushed the support_context_based_graph branch from ef227fc to d635418 Compare July 11, 2022 13:35

Fix comments of adding check for sync functions in workers.

d635418

miaobin mentioned this pull request Jul 14, 2022

[WebNN-native/Node] support context-based compute graph. webmachinelearning/webnn-native#281

Open

Add relu test in worker.

9b017f5

zesongw mentioned this pull request Aug 12, 2022

Refactor to sync with the latest WebNN-native. huningxin/emscripten#6

Merged

Drop dynamic tensor support.

1776063

Fixes webmachinelearning#176

Honry reviewed Nov 9, 2022

View reviewed changes

BruceDai force-pushed the support_context_based_graph branch 2 times, most recently from 0acebaa to d18b99c Compare November 9, 2022 06:50

Honry approved these changes Nov 9, 2022

View reviewed changes

BruceDai force-pushed the support_context_based_graph branch from d18b99c to 79e4ed1 Compare November 9, 2022 07:00

Adressed comments by using ES6 expression.

1baf6ac

BruceDai force-pushed the support_context_based_graph branch from 79e4ed1 to 1baf6ac Compare November 9, 2022 07:38

huningxin reviewed Nov 9, 2022

View reviewed changes

src/nn/graph.ts Outdated Show resolved Hide resolved

Honry mentioned this pull request Nov 10, 2022

"The context paramter [sic] is invalid" error if Experimental Web Platform features flag is on #186

Open

huningxin reviewed Nov 10, 2022

View reviewed changes

BruceDai mentioned this pull request Nov 10, 2022

[webnn] Add tests for concat operation. web-platform-tests/wpt#36782

Merged

BruceDai force-pushed the support_context_based_graph branch 2 times, most recently from 2b3fe63 to 988cdc3 Compare November 14, 2022 08:24

Align the usage of invoking sync and async functions.

988cdc3

huningxin reviewed Nov 15, 2022

View reviewed changes

src/nn/graph.ts Outdated Show resolved Hide resolved

src/nn/graph.ts Outdated Show resolved Hide resolved

src/nn/graph.ts Outdated Show resolved Hide resolved

Add validateAndSetOutputOperands() function.

2f578f4

huningxin approved these changes Nov 21, 2022

View reviewed changes

BruceDai merged commit abaf8ba into webmachinelearning:master Nov 21, 2022

BruceDai mentioned this pull request Nov 30, 2022

Support TF.js WebGPU backend #192

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support async/sync functions #172

Support async/sync functions #172

BruceDai commented Jun 21, 2022

huningxin left a comment

BruceDai commented Jun 28, 2022

BruceDai commented Jul 5, 2022

huningxin Jul 6, 2022

BruceDai commented Jul 26, 2022

Honry left a comment

Honry Nov 8, 2022

BruceDai Nov 9, 2022

Honry Nov 9, 2022

BruceDai Nov 9, 2022

Honry Nov 9, 2022

BruceDai Nov 9, 2022

Honry Nov 9, 2022

BruceDai Nov 9, 2022

BruceDai commented Nov 9, 2022

Honry left a comment

huningxin Nov 10, 2022 •

edited

Loading

BruceDai Nov 10, 2022

huningxin left a comment

huningxin left a comment

BruceDai commented Nov 21, 2022

	resource , inputOperand.desc.type, dimensions);
	resource, inputOperand.desc.type, dimensions);

	describe('test MLContext.compute', function() {
	describe('test MLContext.compute', () => {

Support async/sync functions #172

Support async/sync functions #172

Conversation

BruceDai commented Jun 21, 2022

huningxin left a comment

Choose a reason for hiding this comment

BruceDai commented Jun 28, 2022

BruceDai commented Jul 5, 2022

Choose a reason for hiding this comment

BruceDai commented Jul 26, 2022

Honry left a 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

BruceDai commented Nov 9, 2022

Honry left a comment

Choose a reason for hiding this comment

huningxin Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huningxin left a comment

Choose a reason for hiding this comment

huningxin left a comment

Choose a reason for hiding this comment

BruceDai commented Nov 21, 2022

huningxin Nov 10, 2022 •

edited

Loading