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

Remove builder.constant(value, type) variant #475

Closed
huningxin opened this issue Nov 2, 2023 · 4 comments · Fixed by #650
Closed

Remove builder.constant(value, type) variant #475

huningxin opened this issue Nov 2, 2023 · 4 comments · Fixed by #650

Comments

@huningxin
Copy link
Contributor

huningxin commented Nov 2, 2023

In the current spec, MLGraphBuilder.constant(value, type) allows to build a scalar from the specified number, e.g.,

const a = graphBuilder.constant(42.0, 'float32');

However, MLGraphBuilder.constant(descriptor, bufferView) also allows to build a scalar from data buffer, like the example in #390

const b = graphBuilder.constant({type: 'float32'}, new Float32Array([42.0]));

The proposal is to drop the scalar-specific MLGraphBuilder.constant(value, type) method and only keep more generic MLGraphBuilder.constant(descriptor, bufferView) method.

The scalar-specific build method can be polyfilled by user code. This would help reduce the browser implementation complexity.

/cc @wchao1115 @fdwr @wacky6 @BruceDai @Honry

(Edit by @anssiko: fix link and naming to account for name changes dataType -> type and buffer -> bufferView)

@wacky6
Copy link

wacky6 commented Nov 2, 2023

sgtm :)

@fdwr
Copy link
Collaborator

fdwr commented Nov 2, 2023

I don't oppose it. Are there any impacts besides just our hello world sample? Does ORT WebNN EP use them? Wanming says no impact to ORT WebNN EP.

Having the constant scalar overload is convenient for JS callers, but if we don't expect ordinary JS users to call WebNN directly anyway, instead expecting calls from existing tensor definitions in libraries, then it doesn't matter much. There is no perf benefit to it either, because under the hood, that single constant becomes a buffer anyway.

🔎 One data point is that I just scanned my own SimpleWebNN.html test file (~3000 lines long), and I found 0 occurrences of the constant scalar overload - even the cases where I specified only a single value, I still used the full form:

const constant = graphBuilder.constant(constantDesc, new Float32Array([44.0]));

So, I guess the biggest impact of this change codewise would be our hello world samples :b.

Btw, I never noticed these two overloads have swapped parameters, with the value/type in different orders 🙃:

  • graphBuilder.constant(42.0, 'float32' );
  • graphBuilder.constant({type: 'float32'}, new Float32Array([42.0]));

I suppose the reasoning was that for the first, you could just specify the constant and default the type to float32, but it seems a likely point of confusion that users could accidentally reverse them. So having consistent ordering between them would be nice, but deleting one entirely (like you propose) would also avoid this. 😉

@anssiko anssiko changed the title Remove builder.constant(value, dataType) variant Remove builder.constant(value, type) variant Dec 14, 2023
@inexorabletash
Copy link
Member

Given discussions in #492 about rethinking constant() overloads in general... how are people feeling about this issue? Proceed with removal? Retain but change the signature? Other?

I note that many of the decompositions given in the spec use this, e.g. builder.constant(0), builder.constant(options.alpha), etc. Updating those to take a required dataType is doable, e.g. for relu()

-return builder.max(builder.constant(0), x);
+return builder.max(builder.constant(x.dataType, 0), x);

... but it gets uglier if the overload is removed entirely.

@fdwr
Copy link
Collaborator

fdwr commented Apr 18, 2024

I'm warming to keeping the scalar constant overload for convenience and to benefit for decompositions in the spec, but passing the parameters in the order (MLOperandDataType, MLNumber) would be more self consistent.

The relu decomposition example builder.constant(0) is definitely a typo, because that const overload needs a data type. @huningxin?

inexorabletash added a commit to inexorabletash/webnn that referenced this issue Apr 18, 2024
Make MLGraphBuilder's constant() operand-vending methods consistent
and take the data type as the first parameter. This implicitly makes
the type required instead of optional.

Use of constant() in decompositions are updated as well, and now use
"input" consistently instead of "x" sometimes.

Fixes webmachinelearning#475
@fdwr fdwr closed this as completed in #650 Jun 4, 2024
fdwr pushed a commit that referenced this issue Jun 4, 2024
* Swap parameters of scalar constant() operand method

Make MLGraphBuilder's constant() operand-vending methods consistent
and take the data type as the first parameter. This implicitly makes
the type required instead of optional.

Use of constant() in decompositions are updated as well, and now use
"input" consistently instead of "x" sometimes.

Fixes #475

* fix typo - thanks @zolkis

* Update explainer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants