-
Notifications
You must be signed in to change notification settings - Fork 49
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
huningxin
merged 52 commits into
webmachinelearning:main
from
inexorabletash:bigint-numeric-union
Jul 5, 2024
Merged
Changes from 11 commits
Commits
Show all changes
52 commits
Select commit
Hold shift + click to select a range
f83ecfd
Introduce MLNumber for specifying numeric inputs of any type
inexorabletash 29d5191
Merge remote-tracking branch 'origin/main' into bigint-numeric-union
inexorabletash 67b5a68
Add note about fused activations
inexorabletash adb5742
Merge remote-tracking branch 'origin/main' into bigint-numeric-union
inexorabletash a36d97f
Merge remote-tracking branch 'origin/main' into bigint-numeric-union
inexorabletash 8a8b3d8
Merge remote-tracking branch 'origin/main' into bigint-numeric-union
inexorabletash dc655a6
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash c47b31d
Merge remote-tracking branch 'origin/main' into bigint-numeric-union
inexorabletash 88a29b1
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash bf32b3f
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 6072331
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash c873c35
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 07aaedc
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 84b4cfc
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 4c2a041
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 3389f87
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 35453d6
Capitalize WebIDL ref
inexorabletash fc0a090
Added more explanatory text around processing
inexorabletash a3ff823
Add MLFiniteNumber
inexorabletash aa5d31f
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 28707fe
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 5061a8c
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 0452b10
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash ab54710
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 9f0dd89
WIP for https://github.com/webmachinelearning/webnn/issues/678
inexorabletash 588f7ac
Settle on using 'double' instead of 'float' per #325
inexorabletash 6278afc
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash a357c10
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 98a6263
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 77816a3
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 4f1ebc9
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash c972a88
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash f2a7de3
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 4a58736
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 83a8c46
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 7c9fc64
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 47d8b6c
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash f0e6f85
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 6c7215a
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 5f313b0
Update example MLActivation
inexorabletash 2cce26e
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash d7f8da4
Remove obsolete note
inexorabletash 3120474
resolve lint errors
inexorabletash 024f576
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash e5345fb
Merge branch 'refs/heads/draft' into bigint-numeric-union
inexorabletash 18dd7e3
Fix upperBound and lowerBound values in ConvertToFloat
inexorabletash 6f75f2b
Always clamp, unrestricted FP, and no failures.
inexorabletash b5c7d4b
Remove MLActivation text for MLNumber as it is unused
inexorabletash 1934d25
Parenthesize expression
inexorabletash c353079
Restore MLNumber reference lost in merge
inexorabletash a5f299d
Align pad() with constant(), remove MLFiniteNumber
inexorabletash 2e06299
Revert change re: scalar
inexorabletash File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there is no corresponding
MLOperandDataType
? For example,clamp()
can be built as anMLActivation
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))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 think so.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make sense? What happens if an
MLActivation
is passed to multiple operators with different data types?There was a problem hiding this comment.
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.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.
There was a problem hiding this comment.
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:
constant()
- immediate conversion based on explicitly passedMLOperandDataType
MLOperand clamp()
- immediate conversion based on input's data typeMLActivation clamp()
- when the activation is passed to an operand-vending methodpad()
- immediate conversion based on input's data typeSee #649 (comment) for a framework where activation validation could be plugged in.
There was a problem hiding this comment.
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 inputa
andb
are passed as float16, and if you pass{alpha: MLNumber(3.14123452435)}
, it will automatically convert that to float16 as3.141
?There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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 asalpha * matmul(a,b) + beta * c
in CoreML, and CoreML requires all these binary ops to have the same type. so ifa
,b
are fp16, then it's better to castalpha
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.