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

tfjs does not work with hermes engine #6526

Closed
paradite opened this issue Jun 12, 2022 · 14 comments · Fixed by #7947
Closed

tfjs does not work with hermes engine #6526

paradite opened this issue Jun 12, 2022 · 14 comments · Fixed by #7947
Assignees

Comments

@paradite
Copy link
Contributor

paradite commented Jun 12, 2022

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

  • Install dependencies with yarn (it is not necessary to setup tfjs git submodule)
  • Run npm run tfjs:node
    • Runs tfjs code with Node.js
    • Prints "result 2" without error
  • Run npm run tfjs:buildrun
    • Bundles tfjs code with Metro
    • Runs tfjs code with Hermes
    • Gives the error "Uncaught Error: Argument 'x' passed to 'argMax' must be a Tensor or TensorLike, but got 'e'"

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 paradite added the type:bug Something isn't working label Jun 12, 2022
@rthadur
Copy link
Contributor

rthadur commented Jun 13, 2022

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

@paradite
Copy link
Contributor Author

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.

@rthadur
Copy link
Contributor

rthadur commented Jun 14, 2022

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

@StampixSMO
Copy link

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.

@rthadur rthadur assigned mattsoulanille and unassigned rthadur Jul 12, 2022
@Caundy
Copy link

Caundy commented Aug 2, 2022

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

@admbtlr
Copy link

admbtlr commented Sep 20, 2022

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.

@mattsoulanille
Copy link
Member

mattsoulanille commented Sep 21, 2022

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.

@paradite
Copy link
Contributor Author

paradite commented Oct 4, 2022

@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;
     }
npx patch-package @tensorflow/tfjs

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

@Dakuan
Copy link

Dakuan commented Aug 23, 2023

@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

@paradite
Copy link
Contributor Author

paradite commented Sep 2, 2023

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 #5972 (comment) to:

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

Let me know what you think. Thanks!

@kenhuang1964
Copy link

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.

@paradite
Copy link
Contributor Author

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.

@kenhuang1964
Copy link

Oh okay thanks!

@google-ml-butler
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants