-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[js/webgpu] Float16Array polyfill for uniform #19307
Conversation
declare global { | ||
// eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-explicit-any | ||
const Float16Array: any; | ||
const isFloat16Array: any; |
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.
isFloat16Array
is not going to be polyfilled in the global scope. we can still use a instanceof Float16Array
to perform the check.
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.
done
js/common/lib/tensor-impl.ts
Outdated
throw new TypeError( | ||
'Creating a float16 tensor from number array is not supported. Please use Uint16Array as data.'); | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
data = (typedArrayConstructor as any).from(arg1); |
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.
as explained in the old error message, you cannot do typedArrayConstructor.from
because it does not work with Uint16Array. you need to check if typedArrayConstructor is Float16Array. You can just use the code from that of my PR.
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.
done, thanks!
js/common/lib/tensor-impl.ts
Outdated
@@ -168,6 +168,8 @@ export class Tensor implements TensorInterface { | |||
} | |||
} else if (arg1 instanceof typedArrayConstructor) { | |||
data = arg1; | |||
} else if (isFloat16Array !== undefined && isFloat16Array(arg1)) { | |||
data = arg1 as InstanceType<typeof Float16Array>; |
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 the code actually call into this if branch? I assume that map NUMERIC_TENSOR_TYPE_TO_TYPEDARRAY_MAP should already set with key 'float16', so it should be handled in condition (arg1 instanceof typedArrayConstructor)
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.
No, this is not necessary now. Thanks!
scalesValidation(scales as number[], mode, isResize); | ||
return scales as number[]; |
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.
we can keep onnxjs code unchanged as they are going to deprecated. we don't plan to support new features to webgl.
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.
Remove this may complains: $ npm run build
[email protected] prebuild
tsc -p . --noEmit && tsc -p lib/wasm/proxy-worker --noEmit
lib/onnxjs/backends/webgl/ops/resize-packed.ts:244:20 - error TS2345: Argument of type 'unknown[]' is not assignable to parameter of type 'number[]'.
Type 'unknown' is not assignable to type 'number'.
244 scalesValidation(scales, mode, isResize);
~~~~~~
lib/onnxjs/backends/webgl/ops/resize-packed.ts:245:3 - error TS2322: Type 'unknown[]' is not assignable to type 'number[]'.
245 return scales;
~~~~~~
Found 2 errors in the same file, starting at: lib/onnxjs/backends/webgl/ops/resize-packed.ts:244
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.
let me try to make some modifications to unit test so that it no longer depends on onnxjs/tensor. Then you don't need to modify any file under onnxjs folder .
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.
#19358 should work with it
js/common/package.json
Outdated
"description": "ONNXRuntime JavaScript API library" | ||
"description": "ONNXRuntime JavaScript API library", | ||
"dependencies": { | ||
"@petamoriken/float16": "^3.8.4" |
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.
please add as devDependencies
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.
done
js/web/package.json
Outdated
@@ -11,6 +11,7 @@ | |||
"version": "1.18.0", | |||
"jsdelivr": "dist/ort.min.js", | |||
"dependencies": { | |||
"@petamoriken/float16": "^3.8.4", |
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.
please add as devDependencies
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.
done
@@ -1,14 +1,18 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. | |||
// Licensed under the MIT License. | |||
|
|||
import {Float16Array} from '@petamoriken/float16'; |
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.
no need to add this as we don't plan to add f16 support for webgl
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.
done
js/common/test/unit-tests/common.ts
Outdated
@@ -34,7 +35,7 @@ export const BIGINT_TYPES = [ | |||
/** | |||
* float16 type, data represented by Uint16Array | |||
*/ | |||
export const FLOAT16_TYPE = ['float16', Uint16Array, false] as const; | |||
export const FLOAT16_TYPE = ['float16', Float16Array, false] as const; |
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 suggest not to update this part. Both paths (with and without polyfill) need to be tested. to test f16 polyfill, may need to add a separated target.
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.
done
declare global { | ||
// eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-explicit-any | ||
const Float16Array: any; | ||
const isFloat16Array: any; |
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.
done
js/common/package.json
Outdated
"description": "ONNXRuntime JavaScript API library" | ||
"description": "ONNXRuntime JavaScript API library", | ||
"dependencies": { | ||
"@petamoriken/float16": "^3.8.4" |
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.
done
js/web/package.json
Outdated
@@ -11,6 +11,7 @@ | |||
"version": "1.18.0", | |||
"jsdelivr": "dist/ort.min.js", | |||
"dependencies": { | |||
"@petamoriken/float16": "^3.8.4", |
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.
done
js/common/lib/tensor-impl.ts
Outdated
throw new TypeError( | ||
'Creating a float16 tensor from number array is not supported. Please use Uint16Array as data.'); | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
data = (typedArrayConstructor as any).from(arg1); |
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.
done, thanks!
js/common/lib/tensor-impl.ts
Outdated
@@ -168,6 +168,8 @@ export class Tensor implements TensorInterface { | |||
} | |||
} else if (arg1 instanceof typedArrayConstructor) { | |||
data = arg1; | |||
} else if (isFloat16Array !== undefined && isFloat16Array(arg1)) { | |||
data = arg1 as InstanceType<typeof Float16Array>; |
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.
No, this is not necessary now. Thanks!
scalesValidation(scales as number[], mode, isResize); | ||
return scales as number[]; |
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.
Remove this may complains: $ npm run build
[email protected] prebuild
tsc -p . --noEmit && tsc -p lib/wasm/proxy-worker --noEmit
lib/onnxjs/backends/webgl/ops/resize-packed.ts:244:20 - error TS2345: Argument of type 'unknown[]' is not assignable to parameter of type 'number[]'.
Type 'unknown' is not assignable to type 'number'.
244 scalesValidation(scales, mode, isResize);
~~~~~~
lib/onnxjs/backends/webgl/ops/resize-packed.ts:245:3 - error TS2322: Type 'unknown[]' is not assignable to type 'number[]'.
245 return scales;
~~~~~~
Found 2 errors in the same file, starting at: lib/onnxjs/backends/webgl/ops/resize-packed.ts:244
js/common/test/unit-tests/common.ts
Outdated
@@ -34,7 +35,7 @@ export const BIGINT_TYPES = [ | |||
/** | |||
* float16 type, data represented by Uint16Array | |||
*/ | |||
export const FLOAT16_TYPE = ['float16', Uint16Array, false] as const; | |||
export const FLOAT16_TYPE = ['float16', Float16Array, false] as const; |
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.
done
js/web/test/data/ops/pad_f16.jsonc
Outdated
{ | ||
"name": "constant 2D", | ||
"operator": "Pad", | ||
"opset": { "domain": "", "version": 10 }, |
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.
Can you also add a test for version > 10, in which the value is as an input not attribute and have the type float16
? I suppose this can test the float16 uniform?
Do you need to update the code here?
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.
done
test/test-main.ts:18:12 - error TS2339: Property 'Float16Array' does not exist on type 'typeof globalThis'. 18 globalThis.Float16Array = Float16Array;
This is replaced by: |
To use this feature, first create a f16 model from f32 model:
Then test it like this: