Repository navigation
Remove builder.constant(value, type) variant #475
Description
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)
Activity
wacky6 commentedon Nov 2, 2023
sgtm :)
fdwr commentedon 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
constantscalar overload - even the cases where I specified only a single value, I still used the full form: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. 😉[-]Remove `builder.constant(value, dataType)` variant[/-][+]Remove `builder.constant(value, type)` variant[/+]inexorabletash commentedon Apr 18, 2024
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()... but it gets uglier if the overload is removed entirely.
fdwr commentedon 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
reludecomposition examplebuilder.constant(0)is definitely a typo, because that const overload needs a data type. @huningxin?Swap parameters of scalar constant() operand method (#650)