Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use AllowSharedBufferSource for MLGraphBuilder.constant() #790

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

huningxin
Copy link
Contributor

@huningxin huningxin commented Nov 16, 2024

Fix #788


Preview | Diff

@huningxin
Copy link
Contributor Author

@a-sully , PTAL?

@@ -1503,9 +1504,6 @@ Build a composed graph up to a given output operand into a computational graph a
1. Set |graph|.{{MLGraph/[[context]]}} to [=this=].{{MLGraphBuilder/[[context]]}}.
1. [=set/For each=] |operand| in |inputs|:
1. Set |graph|.{{MLGraph/[[inputDescriptors]]}}[|operand|.{{MLOperand/[[name]]}}] to |operand|.{{MLOperand/[[descriptor]]}}.

Issue(566): If {{MLGraphBuilder/constant(descriptor, bufferView)|constants'}} {{ArrayBuffer}}s are not [=ArrayBuffer/transferred=], make copies for [=MLGraphBuilder/graph=]'s [=computational graph/constants=] here.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this issue is closed and not relevant.

index.bs Outdated
</summary>
1. If [=this=].{{MLGraphBuilder/[[hasBuilt]]}} is true, then [=exception/throw=] an "{{InvalidStateError}}" {{DOMException}}.
1. If [=MLOperandDescriptor/checking dimensions=] given |descriptor| returns false, then [=exception/throw=] a {{TypeError}}.
1. If [=validating buffer with descriptor=] given |bufferView| and |descriptor| returns false, then [=exception/throw=] a {{TypeError}}.
1. If [=validating buffer with descriptor=] given |bufferSource| and |descriptor| returns false, then [=exception/throw=] a {{TypeError}}.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This depends on #787 for supporting AllowSharedBufferSource in "validate buffer with descriptor" algorithm.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#787 is now complete.

@huningxin huningxin changed the title Allow shared array buffer view for MLGraphBuilder.constant() Use AllowSharedBufferSource for MLGraphBuilder.constant() Nov 21, 2024
Copy link
Contributor

@a-sully a-sully left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@huningxin huningxin requested a review from fdwr November 26, 2024 14:19
@huningxin
Copy link
Contributor Author

@fdwr , PTAL?

/cc @inexorabletash

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but +1 to a naming suggestion.

After this and #787 go in, probably when compute() is removed, we should review any lingering ArrayBufferView references in the spec and update them as appropriate (to be AllowSharedBufferSource, replace with MLTensor, reword entirely, etc)

Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

index.bs Outdated
</summary>
1. If [=this=].{{MLGraphBuilder/[[hasBuilt]]}} is true, then [=exception/throw=] an "{{InvalidStateError}}" {{DOMException}}.
1. If [=MLOperandDescriptor/checking dimensions=] given |descriptor| returns false, then [=exception/throw=] a {{TypeError}}.
1. If [=validating buffer with descriptor=] given |bufferView| and |descriptor| returns false, then [=exception/throw=] a {{TypeError}}.
1. If [=validating buffer with descriptor=] given |bufferSource| and |descriptor| returns false, then [=exception/throw=] a {{TypeError}}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#787 is now complete.

Copy link
Contributor

@a-sully a-sully left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM 👍

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@huningxin
Copy link
Contributor Author

Thanks for the review and approval. I am going to merge it.

@huningxin huningxin merged commit bcc5dc2 into webmachinelearning:main Nov 28, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Nov 28, 2024
SHA: bcc5dc2
Reason: push, by huningxin

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 2, 2024
This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel, mac14.arm64-blink-rel, mac14-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 3, 2024
This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 3, 2024
This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6050722
Reviewed-by: Austin Sullivan <[email protected]>
Reviewed-by: Weizhong Xia <[email protected]>
Commit-Queue: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1390750}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 3, 2024
This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6050722
Reviewed-by: Austin Sullivan <[email protected]>
Reviewed-by: Weizhong Xia <[email protected]>
Commit-Queue: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1390750}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 3, 2024
This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6050722
Reviewed-by: Austin Sullivan <[email protected]>
Reviewed-by: Weizhong Xia <[email protected]>
Commit-Queue: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1390750}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 5, 2024
… for constant, a=testonly

Automatic update from web-platform-tests
WebNN: Support `AllowSharedBufferSource` for constant

This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try​:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6050722
Reviewed-by: Austin Sullivan <[email protected]>
Reviewed-by: Weizhong Xia <[email protected]>
Commit-Queue: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1390750}

--

wpt-commits: 2643d924a518d89e3a8239ef92705bf1373504c9
wpt-pr: 49470
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Dec 6, 2024
… for constant, a=testonly

Automatic update from web-platform-tests
WebNN: Support `AllowSharedBufferSource` for constant

This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try​:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6050722
Reviewed-by: Austin Sullivan <[email protected]>
Reviewed-by: Weizhong Xia <[email protected]>
Commit-Queue: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1390750}

--

wpt-commits: 2643d924a518d89e3a8239ef92705bf1373504c9
wpt-pr: 49470
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Dec 13, 2024
… for constant, a=testonly

Automatic update from web-platform-tests
WebNN: Support `AllowSharedBufferSource` for constant

This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try​:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6050722
Reviewed-by: Austin Sullivan <asullychromium.org>
Reviewed-by: Weizhong Xia <weizhonggoogle.com>
Commit-Queue: ningxin hu <ningxin.huintel.com>
Cr-Commit-Position: refs/heads/main{#1390750}

--

wpt-commits: 2643d924a518d89e3a8239ef92705bf1373504c9
wpt-pr: 49470

UltraBlame original commit: 25acfa3eb6954786eb5663d66876762a2a82eec9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Dec 13, 2024
… for constant, a=testonly

Automatic update from web-platform-tests
WebNN: Support `AllowSharedBufferSource` for constant

This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try​:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6050722
Reviewed-by: Austin Sullivan <asullychromium.org>
Reviewed-by: Weizhong Xia <weizhonggoogle.com>
Commit-Queue: ningxin hu <ningxin.huintel.com>
Cr-Commit-Position: refs/heads/main{#1390750}

--

wpt-commits: 2643d924a518d89e3a8239ef92705bf1373504c9
wpt-pr: 49470

UltraBlame original commit: 25acfa3eb6954786eb5663d66876762a2a82eec9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Dec 13, 2024
… for constant, a=testonly

Automatic update from web-platform-tests
WebNN: Support `AllowSharedBufferSource` for constant

This CL implements the WebNN spec change proposal [1] that uses
`AllowSharedBufferSource` for `MLGraphBuilder.constant()`.

[1]: webmachinelearning/webnn#790

Bug: 380896836
Change-Id: Ib8fc58daaabf7493b08f9634daa1eeb08a50ad35
Cq-Include-Trybots: luci.chromium.try​:win11-blink-rel, mac14.arm64-blink-rel, mac15.arm64-blink-rel, linux-blink-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6050722
Reviewed-by: Austin Sullivan <asullychromium.org>
Reviewed-by: Weizhong Xia <weizhonggoogle.com>
Commit-Queue: ningxin hu <ningxin.huintel.com>
Cr-Commit-Position: refs/heads/main{#1390750}

--

wpt-commits: 2643d924a518d89e3a8239ef92705bf1373504c9
wpt-pr: 49470

UltraBlame original commit: 25acfa3eb6954786eb5663d66876762a2a82eec9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MLGraphBuilder.constant() should take an AllowSharedBufferSource
4 participants