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-node does not implement kernel FlipLeftRight #4066

Closed
vladmandic opened this issue Oct 14, 2020 · 26 comments
Closed

tfjs-node does not implement kernel FlipLeftRight #4066

vladmandic opened this issue Oct 14, 2020 · 26 comments

Comments

@vladmandic
Copy link
Contributor

vladmandic commented Oct 14, 2020

Both BlazeFace and HandDetector models work just fine in NodeJS using tensorflow backend

The issue is in how post-processing is done in JS code which uses FlipLeftRight kernel func to reorient input for better second-pass detection.

So if you use tfjs examples, you'll run into this issue

Error: Kernel 'FlipLeftRight' not registered for backend 'tensorflow'

Unfortunately, those models are internally used by several others as well such as FaceMesh and HandSkeleton which makes list of affected models quite long.

Request is to implement FlipLeftRight in tfjs-node - expectation is to have feature parity between main backends.

Also, is there a matrix of implemented features per backend?

Environment: TFJS 3.6.0 on Ubuntu 20.04

@vladmandic vladmandic added the type:feature New feature or request label Oct 14, 2020
@rthadur
Copy link
Contributor

rthadur commented Oct 14, 2020

@vladmandic can you please check for a similar issue here

@rthadur rthadur self-assigned this Oct 14, 2020
@vladmandic
Copy link
Contributor Author

vladmandic commented Oct 14, 2020

There is no version conflict and I'm running latest TFJS 2.6.0. Just to confirm, I created a simple test that only has tfjs-node listed in package.json, zero other libraries so there is nothing to have conflict with.

And I did note the issue #3883 that clearly lists that forwardFunc is not implemented in tensorflow kernel

And it's easy to check:

nm -D node_modules/@tensorflow/tfjs-node/deps/lib/libtensorflow.so | grep forwardFunc

function is not present in tensorflow.so shared library which is the backend for tfjs-node (unless it's supposed to use a fallback defined in @tensorflow/tfjs-core/src/engine.ts).

@tafsiri
Copy link
Contributor

tafsiri commented Oct 14, 2020

I'm going to rename this issue, because the underlying issue is that as mentioned in #3883 blazeface and handpose run kernels that are not implemented in the node.js backend. The error message is a bit confusing to that can be improved as well. I'll file a separate issue for that.

@tafsiri tafsiri changed the title forwardFunc is not implemented in tfjs-node Support BlazeFace and HandDetector in tfjs-node Oct 14, 2020
@vladmandic
Copy link
Contributor Author

i understand the need for better error message, but will this feature request to actually implement kernel function stay open and eventually be completed?

@rthadur rthadur removed their assignment Oct 14, 2020
@tafsiri
Copy link
Contributor

tafsiri commented Nov 4, 2020

I'll try and get an update on this.

@rthadur rthadur added the P3 label Dec 3, 2020
@Nuzzlet
Copy link

Nuzzlet commented Dec 4, 2020

I get
(node:15556) UnhandledPromiseRejectionWarning: Error: Backend name 'webgl' not found in registry

with node-gpu on Windows 10

@vladmandic
Copy link
Contributor Author

vladmandic commented Jan 30, 2021

i've tested with tfjs 3.0.0 and error message is improved:

Error: Kernel 'FlipLeftRight' not registered for backend 'tensorflow'

now the remaining part is to actually implement function FlipLeftRight for NodeJS (backend: tensorflow).

@rthadur @tafsiri and plans for implementing this? the issue is open and not even assigned.

@rthadur
Copy link
Contributor

rthadur commented Feb 1, 2021

Usually feature request are prioritized by the team and are assigned later , hopefully it will get picked up soon. I am tagging this for contributions welcome if any one interested.

cc @pyu10055 @lina128 @mattsoulanille @jinjingforever

@vladmandic vladmandic changed the title Support BlazeFace and HandDetector in tfjs-node tfjs-node does not implement FlipLeftRight kernel May 18, 2021
@vladmandic vladmandic changed the title tfjs-node does not implement FlipLeftRight kernel tfjs-node does not implement kernel FlipLeftRight Jun 1, 2021
@rthadur rthadur added this to To do in TensorFlow.js Community Contributions via automation Jun 3, 2021
@DCtheTall
Copy link
Contributor

@rthadur I am interested in implementing this if you're still accepting external contributors.

@rthadur
Copy link
Contributor

rthadur commented Jun 21, 2021

@DCtheTall please go-ahead , thanks for your interest.

@DCtheTall
Copy link
Contributor

DCtheTall commented Jun 26, 2021

@rthadur you're welcome! I was wondering if there is a place in tfjs-node which would be a good example of how you typically implement new kernel functions for Node.JS. Just an example of what to go off of should be enough for me to hack something together for a PR

@jinjingforever
Copy link
Collaborator

Thank you @DCtheTall! This PR might be a good example of adding missing ops to node JS, and this doc might also be helpful. Really appreciate it!

@DCtheTall
Copy link
Contributor

@jinjingforever thank you for the links. This should be all I need to get started!

@DCtheTall
Copy link
Contributor

@rthadur @jinjingforever I have opened a #5261 which implements this function for Node.JS. Lmk if anything else needs to be done.

@jinjingforever
Copy link
Collaborator

Thank you @DCtheTall for the PR!

@rthadur
Copy link
Contributor

rthadur commented Jun 29, 2021

This change will be available in next release , thank you so much @DCtheTall for your contribution.

@jinjingforever
Copy link
Collaborator

Sorry, thanks to @mattsoulanille, looks like the underlying TF C API (1.15) doesn't support this op. It is not supported in the latest version (2.5) either.

What is the right approach to support this kind of ops? (@pyu10055 @lina128 @mattsoulanille). Thanks!

TensorFlow.js Community Contributions automation moved this from Done to In progress Jun 29, 2021
@lina128
Copy link
Collaborator

lina128 commented Jun 30, 2021

Hi @jinjingforever, I see that there's a ScaleAndTranslate in the c API: https://www.tensorflow.org/api_docs/cc/class/tensorflow/ops/scale-and-translate

Can we use that for FlipLeftAndRight?

@lina128
Copy link
Collaborator

lina128 commented Jun 30, 2021

If ImageProjectiveTransformV3 is available in the c API, it's even better, it's easier to express flip with an Affine transformation matrix: https://www.tensorflow.org/api_docs/python/tf/raw_ops/ImageProjectiveTransformV3

@jinjingforever
Copy link
Collaborator

Thank you @lina128! I checked the code and looks like the scale factors have to be greater than 0. So it probably cannot be used to flip an image. And looks like ImageProjectiveTransformV3 is not available in the C API.

Maybe we could use "reverse" to do the flip, i.e. reverse on the 'H' dimension: tf.reverse(image, [2])?

Also, there might be a bug in the current tfjs flipLeftRight implementations that might need to be fixed first. Will discuss with @lina128 offline.

@lina128
Copy link
Collaborator

lina128 commented Jun 30, 2021

reverse sounds good to me.

@DCtheTall
Copy link
Contributor

@lina128 thanks for the reply. I'd be happy to assist if I can with any other work needed to reland my original PR

@jinjingforever
Copy link
Collaborator

Thanks @DCtheTall! I am working on a fix for the existing implementations in other backends and related tests. After that is merged, it would be great if you could help refactor your PR to use "reverse" to implement flipLeftRight in node.

Really appreciate it!

@DCtheTall
Copy link
Contributor

@jinjingforever SGTM. Let me know when your change has merged and I can refactor my PR accordingly

@jinjingforever
Copy link
Collaborator

Hi @DCtheTall, I've merged my PR. Now you can try implementing flipLeftRight using reverse. The PR also updated the tests. You can test your changes locally by running yarn test in the tfjs-node directory.

Thank you! Really appreciate your help!

@DCtheTall
Copy link
Contributor

@jinjingforever @lina128 I just uploaded a PR that implements the kernel using reverse. Let me know if this is what you had in mind 😄

@rthadur rthadur closed this as completed Jul 29, 2021
TensorFlow.js Community Contributions automation moved this from In progress to Done Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants