tfjs icon indicating copy to clipboard operation
tfjs copied to clipboard

tfjs does not work with hermes engine

Open paradite opened this issue 3 years ago • 6 comments

System information

  • Have I written custom code (as opposed to using a stock example script provided in TensorFlow.js): No
  • OS Platform and Distribution (e.g., Linux Ubuntu 16.04): MacOS 12.2.1 (21D62), M1
  • Mobile device (e.g. iPhone 8, Pixel 2, Samsung Galaxy) if the issue happens on mobile device: NA
  • TensorFlow.js installed from (npm or script link): npm
  • TensorFlow.js version (use command below): 3.18.0
  • Browser version: NA
  • Tensorflow.js Converter Version: NA

Describe the current behavior

When using tfjs for React Native, tfjs does not work when using hermes JavaScript engine. It works with the default jsc engine.

It fails at instanceof Tensor assertion, even though the constructor is correctly identified as Tensor: Screenshot 2022-06-11 at 4 54 15 PM

Screenshot 2022-06-11 at 6 54 02 PM copy

Describe the expected behavior

tfjs should run without errors when using hermes engine.

Standalone code to reproduce the issue

Minimum reproduction repo: https://github.com/paradite/tfjs-hermes-bug

From the repo, running npm run test:hermes throws an error:

tensor.js:1:1: error: class declaration exports are unsupported

This means that probably the original code was transformed by babel/typescript before reaching hermes and caused the instance of assertion to fail.

I should raise an issue in https://github.com/facebook/hermes, but it is hard to reproduce the exact issue when tfjs uses a lot of import across different files (hermes-cli needs to take input of all files being imported).

On the other hand, the fix for this issue on tfjs side is easy:

Other info / logs

I have tried a hacky fix and it seems to work:

Instead of using

if (x instanceof Tensor) {
    assertDtype(parseAsDtype, x.dtype, argName, functionName);
    return x;
}

, use

if (x instanceof Tensor || x.constructor.name === 'Tensor') {
    assertDtype(parseAsDtype, x.dtype, argName, functionName);
    return x;
}

paradite avatar Jun 12 '22 06:06 paradite

Did you get chance to check similar issue here https://github.com/tensorflow/tfjs/issues/5972 ?

rthadur avatar Jun 13 '22 19:06 rthadur

Did you get chance to check similar issue here #5972 ?

I haven't and I should have. It seems to be the same problem as one comment noted, tfjs doesn't work together with hermes.

I will leave it to you to decide if this should be fixed on tfjs side or hermes.

What I can contribute is the reproduction code above prove that hermes doesn't support export class by default, and the incompatibility might lie in interaction between build tools like babel or typescript and hermes.

paradite avatar Jun 14 '22 01:06 paradite

@mattsoulanille could you please help with this issue , if we can support hermes from tfjs ?

rthadur avatar Jun 14 '22 01:06 rthadur

Same here, cannot get TFJS to work with React Native and Hermes JS engine enabled. The checkInputs method keeps throwing. Exactly the same build works fine with JSC.

StampixSMO avatar Jul 12 '22 09:07 StampixSMO

Having had to disable hermes, too, is there any update on the matter or maybe another board where progress could be tracked @mattsoulanille? Cheers! :)

Caundy avatar Aug 02 '22 11:08 Caundy

Has there been any progress on this? Now that React Native uses Hermes by default, it would be great to figure out how tfjs can support it.

admbtlr avatar Sep 20 '22 06:09 admbtlr

Sorry for the lack of updates. We would like to support Hermes, if possible, but it's not at the top of our priority list right now.

Looking at the error message, this seems to be a case of having multiple versions of the Tensor class, possibly due to importing Tensor from @tensorflow/tfjs-core, which points to a rollup bundle, and from @tensorflow/tfjs-core/dist/tensor, which points to a file. If you . In the future, we may add an exports field to the package.json file, which should prevent these kinds of imports by only allowing a set of imports we specify. ~~I expect this change will fix the issue here or at least reveal what's causing it.~~

I took a quick look at the reproduction (thanks for posting one!), and it seems like it's not using tfjs (It has its own tensor class). I don't think Hermes supports exporting classes yet, but the example might work if it's compiled down to use prototypes instead of classes (es5 I think).

I also tried getting hermes to import @tensorflow/tfjs. Here are a few fixes we need to make in tfjs to make this happen:

  1. Use globalThis as the global variable in getGlobalNamespace (For my test, I replaced global with globalThis in node_modules/@tensorflow/tfjs/dist/tf.js bundle)
  2. Use print instead of console.log - We should probably add a log function to the Platform interface (For my test, I just string-replaced console.log with print in node_modules/@tensorflow/tfjs/dist/tf.js).
  3. Make tfjs backends work.

After these changes, the following test code works (on my linux machine):

// index.js
import { Tensor } from './node_modules/@tensorflow/tfjs/dist/tf.js';

const tensor = new Tensor([1,2,3]);

print(tensor.size);
yarn add @tensorflow/tfjs
./hermes -commonjs index.js node_modules/@tensorflow/tfjs/dist/tf.js

However, I'm not able to load any tfjs backends, so all I can do is print the size.

Edit: Actually, I'm able to load the CPU backend, but it seems as though the weak map that holds tensor data is losing the data too early when I try to run readSync.

Please leave this issue open so I can take another look when I have some time.

mattsoulanille avatar Sep 21 '22 17:09 mattsoulanille

@mattsoulanille thanks for taking a look.

I want to share a walkaround that I found to be working for my use case:

  • Patch the tfjs code directly at node_modules/@tensorflow/tfjs/dist/tf.node.js
 function convertToTensor(x, argName, functionName, parseAsDtype = 'numeric') {
-    if (x instanceof Tensor) {
+    if (x instanceof Tensor || x.constructor.name === 'Tensor') {
         assertDtype(parseAsDtype, x.dtype, argName, functionName);
         return x;
     }
  • Use https://github.com/ds300/patch-package to generate a patch inside the repo
npx patch-package @tensorflow/tfjs
  • Follow https://github.com/ds300/patch-package instructions for adding a postinstall hook after npm i/yarn

I am using yarn v1 so I ran

yarn add patch-package postinstall-postinstall

The additional setup in package.json looks like this:

{
  "scripts": {
    "postinstall": "patch-package"
  },
  "dependencies": {
    "patch-package": "^6.4.7",
    "postinstall-postinstall": "^2.1.0"
  }
}

I did also spent a few hours digging into this issue but did not make much progress. Some findings:

  • @tensorflow/tfjs-react-native package does not matter. the issue happens with or without it
  • the issue gets trigger when a Tensor was returned by some operation and then the Tensor instance chains other operations such as argMax or mul, which leads to getGlobalTensorClass call:
getGlobalTensorClass().prototype.argMax = function (axis) {
    this.throwIfDisposed();
    return argMax(this, axis);
};


getGlobalTensorClass().prototype.mul = function (b) {
  this.throwIfDisposed();
  return mul(this, b);
};
  • Rewriting chained operation tensor.argMax(-1) into tf.argMax(tensor, -1) seems to solve the issue for that particular instance, as I was getting different errors after the rewrite
  • Tensor object from @tensorflow/tfjs, @tensorflow/tfjs/dist/tf.node and @tensorflow/tfjs-core/dist/tf-core.node appear to be identical on Hermes in my testing:
import * as tf from '@tensorflow/tfjs';
import * as tfnode from '@tensorflow/tfjs/dist/tf.node';
import * as tfcorenode from '@tensorflow/tfjs-core/dist/tf-core.node';
console.log('tf.Tensor === tfnode.Tensor', tf.Tensor === tfnode.Tensor);
console.log(
  'tfcorenode.Tensor === tfnode.Tensor',
  tfcorenode.Tensor === tfnode.Tensor
);
// true
// true

paradite avatar Oct 04 '22 03:10 paradite

@paradite 's patch package worked for us, there were a few more places that needed the same fix (adding || x.constructor.name === 'Tensor' ) so don't give up if that patch doesnt work for you straight away

Dakuan avatar Aug 23 '23 09:08 Dakuan

Hi @rthadur @gaikwadrahul8 @mattsoulanille

After playing with metro, hermes and tfjs-platform-react-native for a bit, I have managed to come up with a minimal reproduction repo:

https://github.com/paradite/tfjs-hermes-bug

It is now easy to reproduce this issue by just running yarn and npm run tfjs:buildrun:

Screenshot 2023-09-03 at 12 38 41 AM

I am willing to work on a PR to fix this issue since it is affecting more people: #5972 #7056

I could follow the suggest in https://github.com/tensorflow/tfjs/issues/5972#issuecomment-1107360396 to:

  • replace instanceof Tensor with instanceof getGlobalTensorClass()
  • verify that it works
  • and submit a PR

Let me know what you think. Thanks!

paradite avatar Sep 02 '23 16:09 paradite

Hey @paradite , I'm wondering if this issue ever got resolved? I seem to be facing this issue still where I have to switch to jsc. However, the issue with jsc is that it doesn't seem to be compatible with expo 49.

kenhuang1964 avatar Sep 25 '23 15:09 kenhuang1964

Hey @paradite , I'm wondering if this issue ever got resolved? I seem to be facing this issue still where I have to switch to jsc. However, the issue with jsc is that it doesn't seem to be compatible with expo 49.

I have sent a PR #7947 but seems like it's stuck in review.

paradite avatar Sep 25 '23 20:09 paradite

Oh okay thanks!

kenhuang1964 avatar Sep 26 '23 01:09 kenhuang1964

Are you satisfied with the resolution of your issue? Yes No

google-ml-butler[bot] avatar Oct 11 '23 21:10 google-ml-butler[bot]