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

Content: Define build() steps more rigorously #603

Merged
merged 5 commits into from
Mar 16, 2024

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Mar 13, 2024

  • Adds validation that passed outputs are neither inputs nor constants, matching the Chromium implementation.

  • Traverses the graph, starting with outputs, to visit all connected operands.

  • Previously the outputs were iterated over to process inputs and constants, which didn't make any sense. Inputs are hooked up to [[inputsDescriptors]]. Nothing is specifically mentioned about constants, but an issue about transferring is referenced. (Don't transfer input ArrayBuffers #566)

  • The impact of MLGraphBuilder re-use (Can an MLGraphBuilder be reused? #567) is called out, since it could allow for removing the graph traversal.

  • Populates graph's [[outputDescriptors]], which was previously just missing. (Add traversal algorithm to buildSync() #448)

  • Makes most of the validation behavior of build() happen synchronously rather than "in parallel". A promise is still returned, of course.

  • Converting the graph into an implementation-defined format is called out, which remains in "in parallel" steps.

Fixes #448, fixes #457, fixes #552.


Preview | Diff

- Adds validation that passed outputs are neither inputs nor
  constants, matching the Chromium implementation.

- Traverses the graph, starting with outputs, to visit all connected
  operands.

- Previously the outputs were iterated over to process inputs and
  constants, which didn't make any sense. Inputs are hooked up to
  [[inputsDescriptors]]. Nothing is specifically mentioned about
  constants, but an issue about transferring is referenced. (webmachinelearning#566)

- The impact of MLGraphBuilder re-use (webmachinelearning#567) is called out, since it
  could allow for removing the graph traversal.

- Populates graph's [[outputDescriptors]], which was previously just
  missing. (webmachinelearning#448)

- Makes most of the validation behavior of build() happen
  synchronously rather than "in parallel". A promise is still
  returned, of course.

- Converting the graph into an implementation-defined format is called
  out, which remains in "in parallel" steps.

Fixes webmachinelearning#448, fixes webmachinelearning#457, fixes webmachinelearning#552.
index.bs Outdated Show resolved Hide resolved
Co-authored-by: Reilly Grant <reillyeon@users.noreply.github.com>
@fdwr
Copy link
Collaborator

fdwr commented Mar 14, 2024

I'll look at this tomorrow, but I wonder if we expect an empty graph to behave like an empty memmove - a nop? e.g. I'd expect this would function as identity:

    // Build empty graph.
    const input = graphBuilder.input("input", {dataType: "float32", dimensions: [4]});
    const output = input;
    const graph = await graphBuilder.build({"output": output});

    // Bind inputs to the graph and execute for the result.
    const inputBuffer = new Float32Array([0,1,2,3]);
    const outputBuffer = new Float32Array(4).fill(0);
    var inputs = {"input": inputBuffer};
    var outputs = {"output": outputBuffer};
    const results = await mlContext.compute(graph, inputs, outputs);

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@inexorabletash
Copy link
Member Author

I wonder if we expect an empty graph to behave like an empty memmove

Yeah, we were discussing that too. The Chromium impl seems to reject this (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/ml/webnn/ml_graph.cc;l=159) but maybe it's fine?

inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Mar 16, 2024
Previously the spec had a "validate `MLOperand`" helper that (1)
ensured the operand was from the passed `MLGraphBuilder` and (2) that
the operand was internally consistent, and this was called during (3)
`build()` and (4) only `concat()` among the vending methods.

- (1) is needed but can be done when the `MLOperand` is created,
  giving better feedback, so (3) isn't needed.

- (2) is not needed - `MLOperands` are immutable so they can't be
  created in a bad state.

- (4) should be expanded to all `MLOperand` creations that take input
  `MLOperand`s.

This renames the helper, ensures it is called by every `MLOperand`
vending method that takes `MLOperand` inputs, and drops it from
`build()` (although that will probably collide with PR webmachinelearning#603 which
should land first). Similar validation is added for `MLActivation`s.

For webmachinelearning#572
Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM

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.

👍

@@ -1385,38 +1383,49 @@ Build a composed graph up to a given output operand into a computational graph a
<summary>
The <dfn method for=MLGraphBuilder>build(|outputs|)</dfn> method steps are:
</summary>
1. Let |promise| be [=a new promise=].
1. If |outputs| is empty, then return [=a new promise=] [=rejected=] with 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.

It's an interesting idea to consider what it would mean for a graph to have inputs but no outputs. I have seen some custom operators in models which emitted no outputs because they existed purely for other side effects, or they might exist for debugging (to print out or dump results). Within WebNN though there is no such thing anyway, and so this check makes sense 👍. The reverse is certainly true though that a graph can have outputs with no inputs. e.g. A graph with the fillSequence operator (renamed to a constant overload later) which fills a tensor with a sequence and has no input. The same would apply to random number generator operators too. (resolve me)

1. [=map/For each=] |name| → |operand| of |outputs|:
1. If |name| is empty, then return [=a new promise=] [=rejected=] with a {{TypeError}}.
1. If [=MLOperand/validating MLOperand=] given |operand| and [=this=] returns false, then return [=a new promise=] [=rejected=] with a {{TypeError}}.
1. If |operand| is in [=this=]'s [=MLGraphBuilder/graph=]'s [=computational graph/inputs=] or [=computational graph/constants=], then return [=a new promise=] [=rejected=] with 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.

Ah, so here is the check that would nullify the possibility of a nop graph (say one with just a constant and output), which conceptually seems valid to me, as one scenario is where you have a chained sequence of models with one step that can be an action or a nop, and it may be easier to express by always passing a valid (but empty) graph to that component which executes them all. Though, I'm not too worried about it either, since a caller can always insert a dummy identity node to satisfy this constraint. (resolve me)

index.bs Show resolved Hide resolved
@fdwr fdwr merged commit 1062297 into webmachinelearning:main Mar 16, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 16, 2024
SHA: 1062297
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Mar 18, 2024
This fell out of making build() more specific - these terms are no
longer needed, and the makeup of the MLGraph can be described more
abstractly.

Discussed here: webmachinelearning#603 (review)
fdwr pushed a commit that referenced this pull request Mar 20, 2024
* Synchronously validate input operands/validations

Previously the spec had a "validate `MLOperand`" helper that (1)
ensured the operand was from the passed `MLGraphBuilder` and (2) that
the operand was internally consistent, and this was called during (3)
`build()` and (4) only `concat()` among the vending methods.

- (1) is needed but can be done when the `MLOperand` is created,
  giving better feedback, so (3) isn't needed.

- (2) is not needed - `MLOperands` are immutable so they can't be
  created in a bad state.

- (4) should be expanded to all `MLOperand` creations that take input
  `MLOperand`s.

This renames the helper, ensures it is called by every `MLOperand`
vending method that takes `MLOperand` inputs, and drops it from
`build()` (although that will probably collide with PR #603 which
should land first). Similar validation is added for `MLActivation`s.

For #572

* Apply suggestions from code review

Missed a few "if it exists" clauses

Co-authored-by: Ningxin Hu <ningxin.hu@intel.com>

---------

Co-authored-by: Ningxin Hu <ningxin.hu@intel.com>
fdwr pushed a commit that referenced this pull request Mar 21, 2024
This fell out of making build() more specific - these terms are no
longer needed, and the makeup of the MLGraph can be described more
abstractly.

Discussed here: #603 (review)
@inexorabletash inexorabletash deleted the content-build-steps branch April 4, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants