Rework the sync async algorithms based on PR 323 by zolkis · Pull Request #329 · webmachinelearning/webnn

zolkis · 2023-01-18T19:06:27Z

Factor out graph input/output validation.
Factor out graph execution algorithm.
Used them in the sync and async algorithm steps.

Preview | Diff

zolkis · 2023-01-18T19:15:45Z

Looks like there are some changes due to auto-save: in some PRs before me, there were some trailing spaces. Please set your editors to remove trailing spaces.

zolkis · 2023-01-19T09:37:51Z

@huningxin @wchao1115 @anssiko PTAL

zolkis · 2023-01-19T16:42:41Z

I tried the the diff and preview, but they won't help much in reviewing these changes.
I found it easier to place 2 windows side by side and compare the versions "manually":

check the resource validation algorithm vs the validation sections of the compute algorithms;
check the usages of the resource validation algorithm vs the original;
check the execute graph algorithm vs the sync compute() vs the original;
check the async compute() vs the original;
ignore the (automatic) white space changes (we can talk about them in the call).

zolkis · 2023-01-20T14:09:03Z

Still work in progress... I want to find a better way to express underlying graph execution / compute requests, it also overlaps with other algorithms (graph internal slots, operand internal slots etc.)

zolkis · 2023-02-08T09:12:20Z

@huningxin could you please check for correctness?

index.bs

huningxin

LGTM, thanks @zolkis !

…ing#329 Signed-off-by: Zoltan Kis <[email protected]>

index.bs

Squashed the following commits: Enclose arg + return descriptions to separate from normative algorithms Change manual link to autolink Use the 'get a copy' reference for buffer source Factor our validating bufferview with descriptor Fix initialization typo Add reference to the platform operand object MLOperand/[[operand]] Lift dependency changes from webmachinelearning#337 Fix typos, remove unneeded dependencies from webmachinelearning#337 Add dependency for buffer validation from webmachinelearning#329 Fix the scalar algorithm / comments Discern between scalar and tensor constant, fix typos Fix review comments. Add stylistic boxes for algorithm steps, informal steps, internal slots Fix typo. Remove stylistic boxes for now. Address reviews comments. Remove MLOperandKind typedef from the IDL. Move note to text. Make the check on kind an assert. Remove the MLOperand/[[kind]] internal slot and creation parameter. Remove back quotes from title Signed-off-by: Zoltan Kis <[email protected]>

Squashed the following commits: Enclose arg + return descriptions to separate from normative algorithms Change manual link to autolink Use the 'get a copy' reference for buffer source Factor our validating bufferview with descriptor Fix initialization typo Add reference to the platform operand object MLOperand/[[operand]] Lift dependency changes from #337 Fix typos, remove unneeded dependencies from #337 Add dependency for buffer validation from #329 Fix the scalar algorithm / comments Discern between scalar and tensor constant, fix typos Fix review comments. Add stylistic boxes for algorithm steps, informal steps, internal slots Fix typo. Remove stylistic boxes for now. Address reviews comments. Remove MLOperandKind typedef from the IDL. Move note to text. Make the check on kind an assert. Remove the MLOperand/[[kind]] internal slot and creation parameter. Remove back quotes from title Signed-off-by: Zoltan Kis <[email protected]>

…rning#323 Squashed from the following commits: Add link to asserts Clarify the graph execution steps Address review comments for graph execution Factor out buffer validation vs descriptor Remove note about execution Signed-off-by: Zoltan Kis <[email protected]>

zolkis · 2023-05-16T15:41:49Z

@wchao1115 PTAL: this is editorial, but reformulates wording and factors out sub-steps.
Cleaned up to one squashed commit.

zolkis · 2023-06-19T20:15:13Z

This would need to be merged, as needed in zk-conventions-integration, and only adds editorial changes to a previous PR.

wchao1115 · 2023-06-19T20:59:51Z

@zolkis Should we discard this PR in favor of merging the work into the staging branch? This is one of the oldests in the queue.

zolkis · 2023-06-19T21:01:44Z

This is quite different from that staging set of changes. Mainly editorial, factoring out some parts. This still belongs to the main branch IMHO. Technically it is possible to move there, but its value is in the main branch.

wchao1115 · 2023-06-20T04:36:58Z

@zolkis Can you help me understand why can't this change be staged in the staging branch like others? Given that it's been many months in the PR queue and still requires 1 more approval with several comments still outstanding, can we please move this out to the staging branch?

zolkis · 2023-06-20T06:54:51Z

@zolkis Can you help me understand why can't this change be staged in the staging branch like others? Given that it's been many months in the PR queue and still requires 1 more approval with several comments still outstanding, can we please move this out to the staging branch?

I didn't say it can't be staged. I said it does not belong there.
This change has been reviewed and approved on March 17 by one editor.
It has only been missing your review.
I don't see any comments that are outstanding. Could you point to those?

anssiko · 2023-06-20T07:27:10Z

I marked the last remaining discussion as resolved.

@wchao1115 would merging this to main now complicate your expected future changes?

If it does, we can merge to the integration branch instead.

This enhancement PR is a by-product of discussion in PR #323. I understand this context may have been lost because there were multiple now closed WIP PRs before this one.

zolkis · 2023-06-20T07:35:36Z

Right, if merging this conflicts too much with your upcoming PRs @wchao1115 , we can move this to staging, but let's try to merge it here.

anssiko · 2023-06-29T11:10:20Z

CI build fails with:

WARNING: Multiple elements have the same ID 'validate-buffer-with-descriptor'.
Deduping, but this ID may not be stable across revisions.
FATAL ERROR: Multiple local 'dfn' <dfn>s have the same linking text 'validate buffer with descriptor'.

@zolkis please check and fix.

…to improve-sync-async-execution

Signed-off-by: Zoltan Kis <[email protected]>

zolkis · 2023-08-09T17:45:55Z

@wchao1115 could you please check if you'd be fine merging this? It concerns @huningxin 's changes, has 2 approvals and it's editorial, factoring out common parts and making the algorithms more compact. It doesn't strictly belong to the kind of changes in conventions integration, and it's been discussed several times in calls.

Is there something left undone? Or do you sustain this should be discarded and include the changes in the conventions integration?

wchao1115 · 2023-08-10T05:52:27Z

@wchao1115 could you please check if you'd be fine merging this?

Yes, I approved.

anssiko · 2023-08-10T11:01:04Z

Thanks @wchao1115 for your review. I'll squash and merge so that @zolkis can pull these changes into his integration branch.

SHA: 8e63e07 Reason: push, by anssiko Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

zolkis mentioned this pull request Jan 18, 2023

WiP: Improve graph execution steps #319

Closed

zolkis changed the title ~~Rework the sync async algorithms based on #323~~ WiP: Rework the sync async algorithms based on #323 Jan 20, 2023

anssiko mentioned this pull request Jan 24, 2023

Use modern WebIDL and Infra standard conventions #210

Closed

zolkis mentioned this pull request Feb 7, 2023

Fix #334: Improve the batch norm algorithm #339

Closed

zolkis changed the title ~~WiP: Rework the sync async algorithms based on #323~~ Rework the sync async algorithms based on #323 Feb 7, 2023

anssiko mentioned this pull request Feb 16, 2023

Review sync vs async compute differences #316

Closed

huningxin reviewed Mar 16, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

dontcallmedom linked an issue Mar 16, 2023 that may be closed by this pull request

Review sync vs async compute differences #316

Closed

anssiko requested a review from wchao1115 March 16, 2023 16:02

huningxin approved these changes Mar 17, 2023

View reviewed changes

huningxin mentioned this pull request Mar 23, 2023

Add the constant() method steps. #365

Merged

zolkis pushed a commit to zolkis/webnn that referenced this pull request Mar 24, 2023

constant(): add dependency for buffer validation from webmachinelearn…

68dedef

…ing#329 Signed-off-by: Zoltan Kis <[email protected]>

wchao1115 reviewed Apr 14, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

zolkis changed the title ~~Rework the sync async algorithms based on #323~~ Rework the sync async algorithms based on PR 323 May 16, 2023

zolkis force-pushed the improve-sync-async-execution branch from bf7341a to 95c9bd1 Compare May 16, 2023 15:40

anssiko approved these changes Jun 20, 2023

View reviewed changes

Zoltan Kis added 2 commits June 29, 2023 16:04

Merge branch 'main' of https://github.com/webmachinelearning/webnn in…

e987056

…to improve-sync-async-execution

Remove extra copy of the 'validate buffer with descriptor' steps

ac2c8e9

Signed-off-by: Zoltan Kis <[email protected]>

wchao1115 approved these changes Aug 10, 2023

View reviewed changes

anssiko merged commit 8e63e07 into webmachinelearning:main Aug 10, 2023

github-actions bot added a commit that referenced this pull request Aug 10, 2023

Rework the sync async algorithms based on PR 323 (#329)

e70df37

SHA: 8e63e07 Reason: push, by anssiko Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

zolkis deleted the improve-sync-async-execution branch November 12, 2024 14:00

Comments

Conversation

zolkis commented Jan 18, 2023 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zolkis commented Jan 18, 2023

Uh oh!

zolkis commented Jan 19, 2023

Uh oh!

zolkis commented Jan 19, 2023

Uh oh!

zolkis commented Jan 20, 2023

Uh oh!

zolkis commented Feb 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huningxin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zolkis commented May 16, 2023

Uh oh!

zolkis commented Jun 19, 2023

Uh oh!

wchao1115 commented Jun 19, 2023

Uh oh!

zolkis commented Jun 19, 2023

Uh oh!

wchao1115 commented Jun 20, 2023

Uh oh!

zolkis commented Jun 20, 2023

Uh oh!

anssiko commented Jun 20, 2023

Uh oh!

zolkis commented Jun 20, 2023

Uh oh!

anssiko commented Jun 29, 2023

Uh oh!

zolkis commented Aug 9, 2023

Uh oh!

wchao1115 commented Aug 10, 2023

Uh oh!

anssiko commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zolkis commented Jan 18, 2023 •

edited by pr-preview bot

Loading