Allow MLGraphBuilder.build() to be called only once #717

a-sully · 2024-07-07T22:00:08Z

Fixes #567

If MLGraphBuilder.build() results in anything other than a TypeError - e.g. it successfully resolves with an or MLGraph or rejects with a NotSupportedError - that MLGraphBuilder has now been "built" and all subsequent methods will now reject with an InvalidStateError.

Preview | Diff

a-sully · 2024-07-07T22:10:39Z

index.bs

        1. If |operand| is in [=this=]'s [=MLGraphBuilder/graph=]'s [=computational graph/inputs=], [=set/append=] |operand| to |inputs|.
        1. [=list/For each=] |input| of |operand|.{{MLOperand/[[operator]]}}'s [=operator/inputs=]:
            1. [=queue/Enqueue=] |input| to |queue|.


Note: this PR does not address #567 (comment) since there didn't appear to be consensus on whether that was desirable (FYI @inexorabletash @fdwr). If folks have strong feelings in either direction then I propose we file a separate issue to discuss; otherwise I propose we leave this as-is for now. WDYT?

#567 (comment)

Agree it is a separate issue.

Even with this change, we can still AFAICS create a disconnected graph with a single graphBuilder, right? It's a legitimate graph construct.

Correct 👍

a-sully · 2024-07-10T15:47:00Z

@huningxin @fdwr PTAL?

index.bs

inexorabletash · 2024-07-10T17:45:12Z

index.bs

        1. If |operand| is in [=this=]'s [=MLGraphBuilder/graph=]'s [=computational graph/inputs=], [=set/append=] |operand| to |inputs|.
        1. [=list/For each=] |input| of |operand|.{{MLOperand/[[operator]]}}'s [=operator/inputs=]:
            1. [=queue/Enqueue=] |input| to |queue|.


index.bs

huningxin

LGTM with a comment

Checked with @Honry that ORT WebNN EP currently doesn't share constant operands when building sub-graphs for a (merged) model.

index.bs

huningxin · 2024-07-11T02:57:54Z

index.bs

        1. If |operand| is in [=this=]'s [=MLGraphBuilder/graph=]'s [=computational graph/inputs=], [=set/append=] |operand| to |inputs|.
        1. [=list/For each=] |input| of |operand|.{{MLOperand/[[operator]]}}'s [=operator/inputs=]:
            1. [=queue/Enqueue=] |input| to |queue|.


#567 (comment)

Agree it is a separate issue.

fdwr · 2024-07-11T14:28:57Z

index.bs

        1. If |operand| is in [=this=]'s [=MLGraphBuilder/graph=]'s [=computational graph/inputs=], [=set/append=] |operand| to |inputs|.
        1. [=list/For each=] |input| of |operand|.{{MLOperand/[[operator]]}}'s [=operator/inputs=]:
            1. [=queue/Enqueue=] |input| to |queue|.


Even with this change, we can still AFAICS create a disconnected graph with a single graphBuilder, right? It's a legitimate graph construct.

fdwr · 2024-07-11T14:34:58Z

index.bs

@@ -5966,6 +6027,8 @@ partial interface MLGraphBuilder {
  <summary>
    The <dfn method for=MLGraphBuilder>where(|condition|, |input|, |other|)</dfn> method steps are:
  </summary>
+    1. If [=this=].{{MLGraphBuilder/[[hasBuilt]]}} is true, then [=exception/throw=] an "{{InvalidStateError}}" {{DOMException}}.


🫤 Seeing "if hasBuilt is true then throw this kind of error" repeated in the logic of every single operator feels unclean. Can we have a common graph builder "operator initialization" function that they all call, and that error check can be the very first (and currently only) step in it, kinda like how we centralized casting and broadcasting functionality? Mind you, every single operator still then needs "1. Perform builder-operator-initialization", but it feels 🧼➕ imo. Opinion?

Hmm I agree the current state is annoyingly duplicative, but I couldn't decide on a useful name of a helper method... This check is performed in MLGraphBuilder.build() as well all methods which mint an MLOperand. Calling a "builder-operator-initialization" algorithm from the build() steps is a bit odd (and nothing is being initialized anyways).

Lacking a good name, I figured inlining the one-liner would be more readable ¯\(ツ)/¯

If you can think of a better name then I'm happy to change it :P

The pervasive naming challenge... Let me ponder a bit, after some sleep. 😴

Throwing out another idea: we could maybe use a Bikeshed text macro to reduce the burden on spec authors. We define one today for EMULATED to avoid repetition. This doesn't reduce the burden on spec readers, though.

True, though we'd still have to name the macro :P

THROW_IF_BUILT?

THROW_IF_ALREADY_BUILT?
EnsureNotAlreadyBuilt?
...
No particularly bright insights came to me after sleep or over the weekend 😅. Going to merge now and maybe we'll think of something later...

fdwr

👍 The builder patterns I've seen generally let you create multiple objects from the same builder, but I see advantages to both, and it's easier to potentially relax this later if needed than to restrict it later.

See webmachinelearning/webnn#717 Bug: 354724062 Change-Id: I8ac2bf94b1f5a0db93a042babdc2556eab35034a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5684454 Reviewed-by: Reilly Grant <reillyg@chromium.org> Commit-Queue: Austin Sullivan <asully@chromium.org> Cr-Commit-Position: refs/heads/main@{#1332702}

…at most one MLGraph, a=testonly Automatic update from web-platform-tests webnn: Allow an MLGraphBuilder to build at most one MLGraph See webmachinelearning/webnn#717 Bug: 354724062 Change-Id: I8ac2bf94b1f5a0db93a042babdc2556eab35034a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5684454 Reviewed-by: Reilly Grant <reillyg@chromium.org> Commit-Queue: Austin Sullivan <asully@chromium.org> Cr-Commit-Position: refs/heads/main@{#1332702} -- wpt-commits: b7777fabd7363bb2e65a3a5da4538bcf0eedee9c wpt-pr: 47285

Currently WebNN spec only allows MLGraphBuilder.build() to be called once, we need to create new builder for every subgraph in WebNN EP. Spec change: webmachinelearning/webnn#717

allow build to be called only once

9c9bfa2

a-sully commented Jul 7, 2024

View reviewed changes

inexorabletash reviewed Jul 10, 2024

View reviewed changes

address inexorabletash nits

57e3962

huningxin approved these changes Jul 11, 2024

View reviewed changes

fdwr reviewed Jul 11, 2024

View reviewed changes

fdwr approved these changes Jul 16, 2024

View reviewed changes

fdwr merged commit f08f9f7 into webmachinelearning:main Jul 16, 2024
2 checks passed

a-sully deleted the build-once branch July 22, 2024 14:12

chromium-wpt-export-bot mentioned this pull request Jul 25, 2024

webnn: Allow an MLGraphBuilder to build at most one MLGraph web-platform-tests/wpt#47285

Merged

huningxin mentioned this pull request Jul 26, 2024

[WebNN EP] Create MLGraphBuilder for every model builder microsoft/onnxruntime#21514

Merged

ibelem mentioned this pull request Jul 26, 2024

ORT Update: Create MLGraphBuilder for every model builder microsoft/webnn-developer-preview#21

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow MLGraphBuilder.build() to be called only once #717

Allow MLGraphBuilder.build() to be called only once #717

a-sully commented Jul 7, 2024 •

edited by pr-preview bot

Loading

a-sully Jul 7, 2024

inexorabletash Jul 10, 2024

huningxin Jul 11, 2024

fdwr Jul 11, 2024

a-sully Jul 11, 2024

a-sully commented Jul 10, 2024

inexorabletash Jul 10, 2024

huningxin left a comment

huningxin Jul 11, 2024

fdwr Jul 11, 2024

fdwr Jul 11, 2024

a-sully Jul 11, 2024

fdwr Jul 11, 2024 •

edited

Loading

inexorabletash Jul 11, 2024

a-sully Jul 12, 2024

fdwr Jul 16, 2024 •

edited

Loading

fdwr left a comment •

edited

Loading

Allow MLGraphBuilder.build() to be called only once #717

Allow MLGraphBuilder.build() to be called only once #717

Conversation

a-sully commented Jul 7, 2024 • edited by pr-preview bot 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

Choose a reason for hiding this comment

a-sully commented Jul 10, 2024

Choose a reason for hiding this comment

huningxin 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

fdwr Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdwr Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

fdwr left a comment • edited Loading

Choose a reason for hiding this comment

a-sully commented Jul 7, 2024 •

edited by pr-preview bot

Loading

fdwr Jul 11, 2024 •

edited

Loading

fdwr Jul 16, 2024 •

edited

Loading

fdwr left a comment •

edited

Loading