Use node's util.types.isUint8Array etc for isTypedArray #7181
Conversation
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
isTypedArray is implemented with `instanceof`, which does not work in jest (jestjs/jest#11864). Instead, use node's builtin `util.types.isUint8Array`, `util.types.isFloat32Array`, etc to perform this check. Fixes tensorflow#7175. This may also address tensorflow#7064, but it does not fix the root cause.
pyu10055
requested changes
Dec 15, 2022
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn and @mattsoulanille)
tfjs-core/src/util.ts line 135 at r1 (raw file):
} export function isTypedArray(a: {}): a is Float32Array|Int32Array|Uint8Array|
please add a test to ensure it fixes the problem for different platforms. thanks
…exts This change also removes platform_node_test from our normal glob of tests that run on the browser and node and places it in its own Bazel target. Additionally, upgrade @types/node from 12 to 18 to allow importing `node:vm`.
mattsoulanille
commented
Dec 15, 2022
There was a problem hiding this comment.
Thanks for the review!
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @pyu10055)
tfjs-core/src/util.ts line 135 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
please add a test to ensure it fixes the problem for different platforms. thanks
Tests added
pyu10055
approved these changes
Dec 16, 2022
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:
Linchenn
pushed a commit
to Linchenn/tfjs
that referenced
this pull request
Jan 9, 2023
) isTypedArray is implemented with `instanceof`, which does not work in jest (jestjs/jest#11864). Instead, use node's builtin `util.types.isUint8Array`, `util.types.isFloat32Array`, etc to perform this check. Fixes tensorflow#7175. This may also address tensorflow#7064, but it does not fix the root cause.
mattsoulanille
added a commit
to mattsoulanille/tfjs
that referenced
this pull request
Mar 16, 2023
A new function, `isTypedArray` was added to the `platform` interface by tensorflow#7181 and first published in tfjs-core 4.2.0. This made 4.2.0 incompatible with earlier versions of backends that implemented `platform`, such as node and react-native. This change adds a fallback to the use of `isTypedArray` so earlier versions of platforms that don't implement `isTypedArray` will not throw an error. Note that the behavior may not be perfect, such as when running Jest tests in node. See tensorflow#7175 for more details and upgrade all @tensorflow scoped packages to ^4.2.0 to avoid this. Fixes tensorflow#7273
mattsoulanille
added a commit
to mattsoulanille/tfjs
that referenced
this pull request
Mar 16, 2023
A new function, `isTypedArray` was added to the `platform` interface by tensorflow#7181 and first published in tfjs-core 4.2.0. This made 4.2.0 incompatible with earlier versions of backends that implemented `platform`, such as node and react-native. This change adds a fallback to the use of `isTypedArray` so earlier versions of platforms that don't implement `isTypedArray` will not throw an error. Note that the fallback behavior may not be perfect, such as when running Jest tests in node. See tensorflow#7175 for more details and upgrade all @tensorflow scoped packages to ^4.2.0 to avoid this. Fixes tensorflow#7273
mattsoulanille
added a commit
that referenced
this pull request
Mar 21, 2023
…#7489) A new function, `isTypedArray` was added to the `platform` interface by #7181 and first published in tfjs-core 4.2.0. This made 4.2.0 incompatible with earlier versions of backends that implemented `platform`, such as node and react-native. This change adds a fallback to the use of `isTypedArray` so earlier versions of platforms that don't implement `isTypedArray` will not throw an error. Note that the fallback behavior may not be perfect, such as when running Jest tests in node. See #7175 for more details and upgrade all @tensorflow scoped packages to ^4.2.0 to avoid this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
isTypedArray is implemented with
instanceof, which does not work in jest (jestjs/jest#11864). Instead, use node's builtinutil.types.isUint8Array,util.types.isFloat32Array, etc to perform this check.Fixes #7175.
This may also address #7064, but it does not fix the root cause.
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is