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

[Bug]: False negative (instanceof Float32Array) #11864

Closed
o-alexandre-felipe opened this issue Sep 10, 2021 · 10 comments
Closed

[Bug]: False negative (instanceof Float32Array) #11864

o-alexandre-felipe opened this issue Sep 10, 2021 · 10 comments

Comments

@o-alexandre-felipe
Copy link

o-alexandre-felipe commented Sep 10, 2021

Version

27.1.1

Steps to reproduce

I prepared an example here, simply run npm install then you have the node run.js that shows the expected behavior (without jest), and npx jest will show the behavior when running in jest.

Expected behavior

In this instance I expected that Float32Array created by native code to be instanceof Float32Array.

Actual behavior

It seems that jest redefined Float32Array globally, maybe we could simply surround commands like this by some function that would restore the native functions. jest.withNativeTypes( () => do my test )

Additional context

onnxjs is a frame work to evaluate neural network models in the format ONNX, it supports
different backends. onnxjs-node enables the use of onnxruntime in onnxjs.

These libraries use tensors objects, a tensor class is a multidimensional view of a typed array.
The outputs of a neural network are constructed in NAPI as typed arrays
[1].

Both the native code and javascript code have typed array maps [2],[3], and the javascript code checks if the value
returned is an array of the expected type[4]. Normally this works, but if we invoke this from a jest test, that check will evaluate to false,
and finally it will throw an error[5].

Environment

For a way to reproduce, check Dockerfile, and docker-compose.yaml.

@morungos
Copy link

morungos commented Nov 19, 2021

I've come across this problem too. It appears to be related to/a recurrence of: #10786, which also replicates for me in version 27.3.1.

As far as I can tell, there is no workaround for this, and Jest cannot be used to test any code using onnxruntime. I'd live with any backdoor way to get access to the original Float32Array, but without the original, no compatible data can be passed. It might be a corner case, but it's a blocking corner case.

@m-r-r
Copy link

m-r-r commented Sep 28, 2022

Hello,

I had this same problem, and after a lot of tinkering I eventually found a workaround.

As far as I can tell, the problem happens because Jest uses the module node:vm to run tests. This module allows running JavaScript code in a different V8 context. The module onnxruntime doesn't recognize the instances of Float32Array because they were defined in another VM context. Their constructor is different from the Float32Array of the current context, so the operator instanceof returns false and an exception is thrown.

For some reason, the utility functions in the module node:utils/types seems immune to this problem : they return the correct result no matter which VM context the argument comes from.

I ended up making use of Symbol.hasInstance to overload the instanceof operator.
It's not a clean solution, but it works, and it allows me to run my model using Jest.

// Import Node typing utilities
import * as types from "node:util/types";

// Import onnxruntime-node's default backend
import { onnxruntimeBackend } from "onnxruntime-node/dist/backend";
import { registerBackend } from "onnxruntime-common";

// Define the constructors to monkey-patch
const TYPED_ARRAYS_CONSTRUCTOR_NAMES = [
  "Int8Array",
  "Int16Array",
  "Int32Array",
  "Uint8Array",
  "Uint8ClampedArray",
  "Uint16Array",
  "Uint32Array",
  "Float32Array",
  "Float64Array",
] as const;

// Keep a reference to the original initialization method
const originalMethod = onnxruntimeBackend.init;

// Monkey-patch the initialization function
onnxruntimeBackend.init = function (...args) {
  // There is probably a better way to do this
  Array.isArray = (x: any): x is any[] =>
    typeof x === "object" &&
    x !== null &&
    typeof x.length === "number" &&
    x?.constructor.toString() === Array.toString();

  // For each typed array constructor
  for (const ctorName of TYPED_ARRAYS_CONSTRUCTOR_NAMES) {
    // Get the constructor from the current context
    const ctor: Function = global[ctorName]!;

    // Get the corresponding test function from the `util` module
    const value = types[`is${ctorName}`].bind(types);

    // Monkey-patch the constructor so "x instanceof ctor" returns "types[`is${ctorName}`](x)"
    Object.defineProperty(ctor, Symbol.hasInstance, {
      value,
      writable: false,
      configurable: false,
      enumerable: false,
    });
  }

  // Call the original method
  return originalMethod.apply(this, args);
};

// Register the backend with the highest priority, so it is used instead of the default one
registerBackend("test", onnxruntimeBackend, Number.POSITIVE_INFINITY);

I hope this code can be useful to other people who have the same problem.

mattsoulanille added a commit to mattsoulanille/tfjs that referenced this issue Dec 15, 2022
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 tensorflow/tfjs that referenced this issue Dec 16, 2022
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 #7175.
This may also address #7064, but it does not fix the root cause.
Linchenn pushed a commit to Linchenn/tfjs that referenced this issue 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.
@mathiasbockwoldt
Copy link

Anything new on this issue? The work around by @m-r-r works nicely, but this can't be a viable long-term solution.

@xenova
Copy link

xenova commented May 2, 2023

Also just bumped into this issue. Would be nice to have a proper fix for it.

@adam-harwood
Copy link

I hit this issue as well. I couldn't get the above workaround to work, so I solved it by mocking Array.isArray. Details are here in case it helps anyone.

@fs-eire
Copy link

fs-eire commented Jul 9, 2023

I don't know whether there should be a general fix in testjs, or onnxruntime should submit a fix for compatibility for supporting testjs.

Is the tfjs PR mentioned above (tensorflow/tfjs#7181) a good example for enhance test compatibility? Perhaps I can port that change to onnxruntime

Copy link

github-actions bot commented Jul 8, 2024

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jul 8, 2024
Copy link

github-actions bot commented Aug 7, 2024

This issue was closed because it has been stalled for 30 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 7, 2024
@SimenB
Copy link
Member

SimenB commented Aug 7, 2024

FWIW, this is essentially a duplicate of #2549, like #11864 (comment) touches upon

Copy link

github-actions bot commented Sep 8, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants