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

Decide how to specify graph initialization #552

Closed
a-sully opened this issue Feb 4, 2024 · 1 comment · Fixed by #603
Closed

Decide how to specify graph initialization #552

a-sully opened this issue Feb 4, 2024 · 1 comment · Fixed by #603
Assignees

Comments

@a-sully
Copy link
Contributor

a-sully commented Feb 4, 2024

Graph initialization is an important step in optimizing an MLGraph for execution on the GPU using DirectML (see #303 (comment)). This is another piece of build() which should be better specified (see #457).

However, the spec currently refers to graph initialization in two different ways:

The former pretty clearly maps to the concept of graph initialization in DirectML. As does "Implementations MAY preprocess and optimize the tensor data of operand for the underlying platform" from the MLGraph.buildSync() algorithm. Since this initialization step is context-dependent and may not exist separate from compilation for other backends (see #341 (comment)), I tentatively suggest alluding to this step as an implementation-defined non-normative hint.

However, we can't just wave away "graph initialization" as entirely implementation-defined. There are some characteristics of this step (at least how it's currently specified) which still should be specified somehow:

  • Validating inputs
  • Checking the uniqueness of MLOperand names
    • Again, this seems like something we could do while adding to MLGraphBuilder?
  • What does "Register operand.[[operator]] to graphImpl." mean?
  • etc...

Should these steps be part of "graph initialization"? Or should we use some other term?

a-sully added a commit to a-sully/webnn that referenced this issue Feb 4, 2024
@a-sully
Copy link
Contributor Author

a-sully commented Feb 4, 2024

  • What does "Register operand.[[operator]] to graphImpl." mean?

Looks like @inexorabletash is one step ahead of me :) #549 (comment)

a-sully added a commit to a-sully/webnn that referenced this issue Feb 8, 2024
inexorabletash added a commit to inexorabletash/webnn that referenced this issue 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. (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.
@inexorabletash inexorabletash self-assigned this Mar 13, 2024
@fdwr fdwr closed this as completed in #603 Mar 16, 2024
@fdwr fdwr closed this as completed in 1062297 Mar 16, 2024
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 a pull request may close this issue.

2 participants