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 operand concept, simplify graph connection steps #591

Merged

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Feb 28, 2024

As discussed in #572:

  • Define an "operator" concept, with inputs, outputs, activations.
  • Defer "platform operator" and "platform operand" to build.
  • Standardize the graph connection steps across builder methods.
  • Simplify activation, input and constant steps.

Not covered in this change:

For #549 and #572.


Preview | Diff

As discussed in webmachinelearning#572:

- Define an "operator" concept, with inputs, outputs, activations.
- Defer "platform operator" and "platform operand" to build.
- Standardize the graph connection steps across builder methods.
- Simplify activation, input and constant steps.

Not covered in this change:

- Erroring if input's [[builder]] doesn't match `this`
- Build algorithm - covered by webmachinelearning#448 and webmachinelearning#457
- gru() is missing steps to populate output. Added "Issue" note.
- Introducing "Validate arguments" section for each method.
- Introducing "Calculate output shape" section for each method.

For webmachinelearning#549 and webmachinelearning#572.
@inexorabletash
Copy link
Member Author

inexorabletash commented Feb 28, 2024

One thing that wasn't discussed in #572 in detail is: what to do with MLActivation-vending methods? This PR currently simplifies the "create an MLActivation" steps so that they don't throw, on the assumption that the caller will validate. But none of the invocations do any validation (except clamp()), and they all include "If that throws an error, re-throw the error." (except elu())

I think the right answer is: validate in the caller (e.g. the actual method), just like MLOperand-vending methods. So drop the "re-throw" steps (like elu()), and we can add validation steps if needed (like clamp()). But confirmation would be appreciated.

@inexorabletash
Copy link
Member Author

Another slight tweak we might want to make to this - there are multiple styles used for declaring the operator. Here are all the distinct examples:

  • MLActivation creation:
    • Let operator be an operator for the name operation. (1 usage)
  • Groups of ops with shared steps:
    • Let operator be an operator for the operation op... (1 usage)
    • Let operator be an operator for the binary operation op... (3 usages)
    • Let operator be an operator for the op pooling operation... (2 usages)
  • Other ops:
    • Let operator be an operator for "gru"... (2 usages)
    • Let operator be an operator for the batchNormalization operation... (34 usages)

I'm not sure it really matters, but the "gru" style stands out as the odd duck. That said, there's something to be said for quoting the name, e.g. this reads strangely: "Let operator be an operator for the where operation..."

Spelling is also all over the place, e.g. "Leaky RELU" vs. "LSTM cell" vs. "Gather" vs. "instance normalization" vs. "batchNormalization".

@zolkis
Copy link
Collaborator

zolkis commented Feb 29, 2024

there are multiple styles used for declaring the operator

That all is probably due to my sloppiness while mass-defining those algorithms at different times and fatigue levels :).
Let's choose one and apply it consistently.

In tip-of-tree, the "desc" MLOperandDescriptor is populated, used,
then modified and conditionally used again (if `returnSequence` is
true) when creating "output2". This was broken in previous commits in
this branch. Restore the intent, using a separate "desc2"
MLOperandDescriptor only conditionally populated and then
conditionally used.
@huningxin
Copy link
Contributor

Thanks @inexorabletash ! I am not feeling well today. I'll catch up ASAP.

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.

Looks correct to me. Being more concise is nice (less "Make a request to the underlying platform to..."). Let specification be a specification that has as much verbiage as is necessary to be clear but no more 😉. TY JB.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs 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
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
inexorabletash and others added 5 commits March 10, 2024 11:46
Co-authored-by: Ningxin Hu <ningxin.hu@intel.com>
* Add other operand inputs
* PreLU -> PReLU
* Define split() output shapes calculations
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, thanks so much for this great effort @inexorabletash !

semantics, with no side effects.
Each operation invocation conceptually returns a distinct new value, without
changing the value of any other {{MLOperand}}.
In WebNN, a [=computational graph=] is composed of <dfn>operators</dfn> which act on data, and are the nodes of the graph. {{MLOperand}}s are a representation of data that flows within the computational graph, and are the edges of the graph. {MLOperand}}s include a [=computational graph=]'s <dfn for="computational graph">input</dfn> values for inference, <dfn for="computational graph">constants</dfn> (including trained weights) used for inference, intermediate values (often referred to as activations) computed during inference, as well as the output values of inference. An [=operator=]'s <dfn for=operator>input</dfn> is one or more {{MLOperand}}s. An [=operator=]'s <dfn for=operator>output</dfn> is one or more {{MLOperand}}s. [=Operators=] have operator-specific parameters that control their behavior, which can include zero or more <dfn for=operator lt="activation|activation function">activation functions</dfn>, which are {{MLActivation}}s.
Copy link
Contributor

Choose a reason for hiding this comment

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

The only remaining thing is whether we should define outputs. Because it is used by build() method, we probably can defer it to #457.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - I didn't want to define an unused term just yet, but that was the intention.

@inexorabletash
Copy link
Member Author

Okay, sounds like this is ready for a merge - @fdwr maybe one last look?

@fdwr
Copy link
Collaborator

fdwr commented Mar 12, 2024

Okay, sounds like this is ready for a merge - @fdwr maybe one last look?

@inexorabletash : 🔎👀 ETA 16:10... (Oxford comma for the clarity win 👍)

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.

👍 Thanks JB.

@fdwr fdwr merged commit 1d1b531 into webmachinelearning:main Mar 12, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 12, 2024
SHA: 1d1b531
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@anssiko
Copy link
Member

anssiko commented Mar 13, 2024

Great work everyone!

I believe we were just able to squeeze these changes into the CR Snapshot release train before it departs. I expect the programming model section to be read by people outside this WG, so improvements there were timely.

@inexorabletash inexorabletash deleted the content-programming-model-operators branch March 13, 2024 15:00
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request Mar 22, 2024
The "create an MLActivation" steps don't throw, so there's no
need to re-throw.

Discussed in webmachinelearning#591 (comment)
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request May 3, 2024
Places that create ops use an inconsistent mixed of simple phrases,
camelCase, Title Case, ACRONYMS, and "quoted strings". The most common
was camelCase, but the wording can be weird, and the bulk-defined
binary/unary/logical/pooling/reduction ops and activations use a
"quotedCamelCase", so I went with that.

See webmachinelearning#591 (comment)
for the most commentary.

Resolves webmachinelearning#591
inexorabletash added a commit to inexorabletash/webnn that referenced this pull request May 3, 2024
Places that create ops use an inconsistent mixed of simple phrases,
camelCase, Title Case, ACRONYMS, and "quoted strings". The most common
was camelCase, but the wording can be weird, and the bulk-defined
binary/unary/logical/pooling/reduction ops and activations use a
"quotedCamelCase", so I went with that.

See webmachinelearning#591 (comment)
for the most commentary.

Resolves webmachinelearning#572
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.

None yet

5 participants