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.

Sign up for GitHub

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

Jump to bottom

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

Open
huningxin opened this issue Nov 2, 2023 · 4 comments · May be fixed by #650
Open

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

huningxin opened this issue Nov 2, 2023 · 4 comments · May be fixed by #650
Labels
operator specific

Comments

@huningxin
Copy link
Contributor

huningxin commented Nov 2, 2023 •
edited by anssiko

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 •
edited

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. 😉

@fdwr fdwr mentioned this issue Dec 7, 2023
Merged
@anssiko anssiko changed the title Remove builder.constant(value, dataType) variant Remove builder.constant(value, type) variant Dec 14, 2023
@inexorabletash inexorabletash mentioned this issue Feb 6, 2024
Merged
3 tasks
@inexorabletash
Copy link
Contributor

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 •
edited

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
@inexorabletash
Swap parameters of scalar constant() operand method
e5fb862 
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
@inexorabletash inexorabletash linked a pull request Apr 18, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
operator specific
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Swap parameters of scalar constant() operand method inexorabletash/webnn
5 participants
@anssiko @inexorabletash @huningxin @fdwr @wacky6