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

Introduce MLNumber for specifying numeric inputs of any type #647

Merged

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Apr 18, 2024

For some MLGraphBuilder methods the type of a numeric input can vary - e.g. for constant() an explicit MLOperandDataType is provided; for clamp() and pad() the data type is implied by input operands. In these cases, specifying the numeric value as either a float/double or int64 type runs into accuracy or range issues - you can't accurately represent all int64 values as a double, and you can't represent the full range of floats as int64. (You also can't represent all int64 values as an long long either - over 2^53 things get wierd. But that's a digression.)

  • Some methods that took a float or double argument/option but can operate on integer data types now take MLNumber - a "variant" (union) of either a JS Number (equivalent to a double a.k.a. float64) or BigInt (for full precision int64/uint64 use)
  • Other methods that took a float argument/option now take a double. These are all ops that only operate on floating point types, so no need for MLNumber though it would be harmless to use it there to allow BigInt inputs. This follows web API best practices, and allows full precision conversion to float16 (since float64 to float32 to float16 can yield different results than float64 directly to float16)
  • In all of these cases, the input number is cast to the input/output operand data type when it is used.
    Casting algorithms are spelled out, always have "clamp" semantics (i.e. no weird modulus wrapping), and never fail.
    • For MLOperand-vending methods, the conversion can be done eagerly.
    • For MLActivations, the conversion is done at "fusion time"; it's notable that the same MLActivation instance could be fused with multiple ops, and cast differently for each.
    • This fixes Specifies scalar values casted to match input type. #678
  • Using a BigInt/numeric union is novel here, but WebIDL experts agree it's reasonable (Intent to use BigInt/numeric union in WebNN whatwg/webidl#1388). Note that all existing WebIDL/JS APIs that are designed to take just a Number or just a BigInt will throw if the other thing is passed - there is intentionally no silent conversion. So we are in novel territory here and should look for implementation and developer feedback.

Preview | Diff

For some MLGraphBuilder methods the type of a numeric input can vary -
e.g. for constant() an explicit MLOperandDataType is provided; for
clamp() and pad() the data type is implied by input operands. In these
cases, specifying the numeric value as either a float/double or int64
type runs into accuracy or range issues - you can't accurately
represent all int64 values as a double, and you can't represent the
full range of floats as int64. (You also can't represent all int64
values as an long long either - over 2^53 things get wierd. But that's
a digression.)

Per discussion in whatwg/webidl#1388 this
change introduces a union between a bigint type and unrestricted
double called MLNumber. Callers can pass a JS number (1234, 1.1234e38)
or a JS bigint (9007199254740993n), and the implementation will treat
it properly based on the explicit or implicit MLOperandDataType. Usage
of this type should be limited to only those cases.

Fixes webmachinelearning#442

Note that webmachinelearning#492 proposes changes to the constant sequential filling
operation; this just adds IDL to match the current spec prose.

Some of the concerns raised in webmachinelearning#325 are addressed (e.g. clamp()'s
options). However, several other options are still specified as
"float", and should maybe be "double" - but MLNumber is likely not
appropriate for those, so they are not updated here.
@inexorabletash
Copy link
Member Author

FYI @fdwr @huningxin @zolkis - marked as "draft" but worth looking at.

  • We may want to settle Constant sequential filling operation needs output shape parameter #492 before landing this - and "unrestricted double" may not be appropriate (since we want to preclude Infinity for fill ops?)
  • Is there anywhere that we want to error on NaNs but allow Infinities? If so we need explicit prose steps.
  • bigint is not just int64 - it allows arbitrary sized ints (e.g. 2n**100n). If we want to error for a non-valid int64, we need explicit prose steps.
  • I put the MLNumber definition and prose in the least bad place I could think of; suggestions welcome!

index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated
@@ -1133,6 +1135,16 @@ The {{MLOperand}} objects are created by the methods of {{MLGraphBuilder}}, inte
To <dfn for="MLGraphBuilder">validate operand</dfn> given {{MLGraphBuilder}} |builder| and {{MLOperand}} |operand|, return true if |operand|.{{MLOperand/[[builder]]}} is |builder|, and false otherwise.
</p>

#### {{MLNumber}} #### {#api-mlnumber-typedef}

<dfn typedef>MLNumber</dfn> is used when specifying the type of a numeric option for an {{MLOperand}} which can be of any {{MLOperandDataType}}, including both 64-bit integer types ({{MLOperandDataType/"uint64"}} and {{MLOperandDataType/"int64"}}) and 32-bit floating point ({{MLOperandDataType/"float32"}}). Implementations must process the value according to the corresponding {{MLOperandDataType}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Implementations must process the value according to the corresponding {{MLOperandDataType}}.

What if there is no corresponding MLOperandDataType? For example, clamp() can be built as an MLActivation without needing to specify any particular dtype. What is the dtype or the activation's associated operator?

The concept of an activation's operator is a bit hand-wavy in the spec, but it's very concrete in the Chromium implementation and the data type must be known when we pass the activation as an input to some other builder method anyways (we need to check that the types match, assuming we don't want to allow mixed precision #283 (comment))

Copy link
Member Author

Choose a reason for hiding this comment

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

Presumably the data type of the eventual operator's input? i.e. for the impl we'd need to hold onto the union in the MLActivation until the operator is constructed?

We need to improve the spec text, but I want to understand the implementation first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably the data type of the eventual operator's input?

Fused activations should use operator's output as its own input, although the output's data type of operators that support fusion is usually the same as the input's.

i.e. for the impl we'd need to hold onto the union in the MLActivation until the operator is constructed?

I think so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a short note in 67b5a68 but we can probably improve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. for the impl we'd need to hold onto the union in the MLActivation until the operator is constructed?

I think so.

Does this make sense? What happens if an MLActivation is passed to multiple operators with different data types?

Copy link
Member Author

@inexorabletash inexorabletash Apr 19, 2024

Choose a reason for hiding this comment

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

minValue <= maxValue is particularly interesting depending on the conversions. If you pass {minValue: -1, maxValue: 128} this looks valid, but if the data type ends up being "uint8", does this turn into {minValue: 0xFF, maxValue: 0x80} ? Ooof.

with the idea that an MLActivation has one operator slot

Agreed that "the whole "operator" concept is pretty hand-wavy anyways". An MLActivation's internal [[operator]] slot is not the specific operator instance of the MLOperand that the activation is fused with, and is probably more like an operator type than a specific instance. So I don't think there's a conflict, but we could definitely improve the text - and explicitly mention that MLActivations can be re-used when creating multiple MLOperands and even with different data types.

Copy link
Member Author

@inexorabletash inexorabletash Apr 23, 2024

Choose a reason for hiding this comment

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

To be explicit, here's where I'm envisioning the conversion/validation happens:

  • for constant() - immediate conversion based on explicitly passed MLOperandDataType
  • for MLOperand clamp() - immediate conversion based on input's data type
  • for MLActivation clamp() - when the activation is passed to an operand-vending method
  • for pad() - immediate conversion based on input's data type

See #649 (comment) for a framework where activation validation could be plugged in.

Copy link
Contributor

Choose a reason for hiding this comment

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

question:
Does this mean for example for gemm, if the input a and b are passed as float16, and if you pass
{alpha: MLNumber(3.14123452435)}, it will automatically convert that to float16 as 3.141?

Copy link
Member Author

@inexorabletash inexorabletash May 3, 2024

Choose a reason for hiding this comment

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

I guess under the hood - yes (although it'd be just {alpha: 3.14123452435})

Presumably that's what the spec/implementation implicitly does today, even if we didn't introduce MLNumber?

Copy link
Contributor

Choose a reason for hiding this comment

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

Under the hood it preserves the fp32 precision right now (as what you are suggesting).
But I'd prefer we explicitly set the expectation that it will be casted to the precision to be the same as the input type.
In the gemm example, since it's emulated as alpha * matmul(a,b) + beta * c in CoreML, and CoreML requires all these binary ops to have the same type. so if a, b are fp16, then it's better to cast alpha to fp16 to multiply them. Otherwise we need to cast input to fp32 (which doesn't get executed on ANE).

So right now we just don't support fp16 inputs for gemm on CoreML until this is sorted out.

@anssiko
Copy link
Member

anssiko commented Jun 27, 2024

Cross-linking this async update from today's agenda:

webmachinelearning/meetings#24 (comment)

(Thanks @inexorabletash!)

@inexorabletash inexorabletash marked this pull request as ready for review July 1, 2024 17:17
@inexorabletash
Copy link
Member Author

Okay - I think this is ready for a review. Probably best not to worry about the commit history (and definitely squash on merge, and edit down the comment!) - I can squash/force-push if desired.

@a-sully has a work-in-progress CL for the Chromium prototype, and we'll want WPTs too.

Copy link
Contributor

@a-sully a-sully left a comment

Choose a reason for hiding this comment

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

LGTM 👍

index.bs Show resolved Hide resolved
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.

Great - thank you Joshua :). Just one question and one clarity request. (update: oops, found one more)

index.bs Outdated 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 4 commits July 3, 2024 09:14
Co-authored-by: Dwayne Robinson <dwayner@microsoft.com>
Co-authored-by: Ningxin Hu <ningxin.hu@intel.com>
@inexorabletash
Copy link
Member Author

Since it's relevant here and related to switching pad() to match constant(): for the places that still use double - operand options for epsilon, alpha, and beta - does it ever make sense to support Infinity, -Infinity or NaN? (i.e. should we switch to unrestricted double?)

I can't think of a case where those are ever useful, even if the behavior is well defined. Maybe leave it as double until someone complains?

@fdwr
Copy link
Collaborator

fdwr commented Jul 4, 2024

for the places that still use double - operand options for epsilon, alpha, and beta ... should we switch to unrestricted double?

@inexorabletash: tldr - I don't feel strongly either way.

does it ever make sense to support Infinity, -Infinity or NaN?

Does it make sense? No (especially not for epsilon). Though, I also generally feel we shouldn't unnecessarily restrict the API moreso than whatever the equivalent op decomposition would produce. So, if linear can be expressed as...

function linear(builder, input, options) {
  return builder.add(
    builder.mul(input, builder.constant(input.dataType(), options.alpha)),
    builder.constant(input.dataType(), options.beta));
}

...then it's nice to be consistent with the math it produces, NaN's and infinities alike; but then in practice, I don't see how these values would be useful, and so I'm fine with just double for now. 🤷

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!

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.

👍 Updates appreciated. Do you want a re-review from Phillis and Austin first before merging?

@inexorabletash
Copy link
Member Author

I think it's good to merge. Just remember to squash and tidy up the commit message!

@huningxin huningxin merged commit 9f88ebf into webmachinelearning:main Jul 5, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Jul 5, 2024
SHA: 9f88ebf
Reason: push, by huningxin

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@inexorabletash inexorabletash deleted the bigint-numeric-union branch July 5, 2024 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants