Add aarch64 support for tfjs-node #2513

yhwang · 2019-12-07T00:59:51Z

Modify the install.js to support external binaries while installing
npm package. It reads the custom-binary.json configuration file to
get URLs for the following resources:

'tf-lib': libtensorflow and libtensorflow_framework shared libraries.
Its value is a URL that points to a tar.gz or zip and
contains the shared libaries and header files.
'addon': pre-compiled node binding. Its value is a object contains
three values: host, remote_path and package_name. These three
values are concatenated into a URL that points to the
pre-compiled node binding.

If you only provide tf-lib and no addon. It compiles the node
binding while installing the npm package.

Signed-off-by: Yihong Wang yh.wang@ibm.com

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

This change is

yhwang · 2019-12-07T01:06:02Z

Hi @kangyizhang, in order to support aarch64, I added architecture into the filename of those pre-compiled resources which are stored in the cloud storage. For example, CPU-darwin-1.14.0.tar.gz becomes CPU-darwin-x86_64-1.14.0.tar.gz. Please review the naming change and see if it can support more platform and architectures in the future.

And I can help to provide the arm64 binaries for linux platform.

kangyizhang

Hi Yihong, thanks a lot for this PR. Left minor comment. Let me know when you want me to upload the libtensorflow of arm64.

Reviewable status: 0 of 1 approvals obtained (waiting on @yhwang)

tfjs-node/scripts/install.js, line 93 at r1 (raw file):

  }

  return `${BASE_URI}${libType}-${PLATFORM_MAPPING[platform]}-` +

Can you add a special case here for GPU+darwin? Since there is no gpu libtensorflow for darwin, here the libType should always be CPU if platform is darwin.

yhwang

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang)

tfjs-node/scripts/install.js, line 93 at r1 (raw file):

Previously, kangyizhang (Kangyi Zhang) wrote…

Can you add a special case here for GPU+darwin? Since there is no gpu libtensorflow for darwin, here the libType should always be CPU if platform is darwin.

or should we have a list to store all supported platforms, types and architectures. then reject the unsupported combination?

yhwang

I have gpu-linux-arm64 binary. how about cpu-linux-arm64?

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang)

yhwang · 2019-12-10T03:10:06Z

@kangyizhang I made some changes and it detects if the system is supported or not. In arm64 system, it also runs node-pre-gyp command with --build-from-source option.

Let me know when you want me to upload the libtensorflow of arm64.

I put gpu-linux-arm64 here

yhwang · 2019-12-13T23:46:27Z

In the latest change, install.js checks custom-binary.json at the same directory and retrieve the custom binaries if external resource URLs are specified. If custom-binary.json doesn't exist, then install.js retrieves binaries from GCP.

yhwang

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang)

tfjs-node/scripts/install.js, line 93 at r1 (raw file):

Previously, yhwang (Yihong Wang) wrote…

or should we have a list to store all supported platforms, types and architectures. then reject the unsupported combination?

I added an array to store all official supported systems and throw an error if it's unsupported system.

yhwang

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang)

tfjs-node/scripts/install.js, line 93 at r1 (raw file):

Previously, yhwang (Yihong Wang) wrote…

I added an array to store all official supported systems and throw an error if it's unsupported system.

Done.

kangyizhang

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang and @yhwang)

tfjs-node/scripts/get-addon-name.js, line 74 at r2 (raw file):

Quoted 6 lines of code…

  customTFLibUri: customBinaries['tf-lib'],
  customAddon: customBinaries['addon'],
  ARCH_MAPPING,
  PLATFORM_MAPPING,
  PLATFORM_EXTENSION,
  ALL_SUPPORTED_COMBINATION

can you move these constant values to scripts/deps-constants.js? I think you were having these fields in deps-constants.js file in the previous version of this PR. I'm requesting this because this file is suppose to be only used to generate pre-compiled addon-name, which is heavily used when we develop and publish new NPM package. Other constants should stay in the deps-constants.js file.

tfjs-node/scripts/install.js, line 93 at r1 (raw file):

Previously, yhwang (Yihong Wang) wrote…

Done.

Thanks for adding the list of supported platforms. But we should still make an exception for mac+gpu with no customized binary. Because there might be existing users using tfjs-node-gpu on their mac, maybe developing process that runs on cloud with CUDA GPU, and a new version of this library should not break their development.

tfjs-node/scripts/install.js, line 168 at r2 (raw file):

      cp.exec('node scripts/deps-stage.js symlink ' + modulePath);
    }
    revertAddonName();

can you keep revertAddonName() here to ensure that it's called after node-pre-gyp install is executed. Otherwise the addon name in package.json might be reverted before node-pre-gyp runs.

kangyizhang

Thank you for the update. Do you mind to provide an example of the custom-binary.json file (base on the code it seems like it should include all the fields in package.json.binary) so that I can use it to update the readme file?

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang and @yhwang)

Modify the install.js to support external binaries while installing npm package. It reads the `custom-binary.json` configuration file to get URLs for the following resources: - 'tf-lib': libtensorflow and libtensorflow_framework shared libraries. Its value is a URL that points to a `tar.gz` or `zip` and contains the shared libaries and header files. - 'addon': pre-compiled node binding. Its value is a object contains three values: `host`, `remote_path` and `package_name`. These three values are concatenated into a URL that points to the pre-compiled node binding. If you only provide `tf-lib` and no `addon`. It compiles the node binding while installing the npm package. Signed-off-by: Yihong Wang <yh.wang@ibm.com>

yhwang

I added custom-binary.sample.json as the example for custom-binary.json

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang)

tfjs-node/scripts/get-addon-name.js, line 74 at r2 (raw file):

Previously, kangyizhang (Kangyi Zhang) wrote…

  customTFLibUri: customBinaries['tf-lib'],
  customAddon: customBinaries['addon'],
  ARCH_MAPPING,
  PLATFORM_MAPPING,
  PLATFORM_EXTENSION,
  ALL_SUPPORTED_COMBINATION
can you move these constant values to scripts/deps-constants.js? I think you were having these fields in deps-constants.js file in the previous version of this PR. I'm requesting this because this file is suppose to be only used to generate pre-compiled addon-name, which is heavily used when we develop and publish new NPM package. Other constants should stay in the deps-constants.js file.

sure. my thought is those constants belong to add-on. That's why I had them here. I moved them to deps-constants.js now

tfjs-node/scripts/install.js, line 93 at r1 (raw file):

Previously, kangyizhang (Kangyi Zhang) wrote…

Thanks for adding the list of supported platforms. But we should still make an exception for mac+gpu with no customized binary. Because there might be existing users using tfjs-node-gpu on their mac, maybe developing process that runs on cloud with CUDA GPU, and a new version of this library should not break their development.

add an exception for mac+gpu case. done

tfjs-node/scripts/install.js, line 168 at r2 (raw file):

Previously, kangyizhang (Kangyi Zhang) wrote…

can you keep revertAddonName() here to ensure that it's called after node-pre-gyp install is executed. Otherwise the addon name in package.json might be reverted before node-pre-gyp runs.

done. I also moved some related code to here. It's easier to read.

kangyizhang

Thank you for the contribution!

I made some edits for readability and added some instruction in readme file. Please verify if the edition works with your binary. Otherwise LGTM.

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @kangyizhang)

yhwang · 2019-12-18T00:50:50Z

looks good to me. thanks for removing the try/finally. I left it there and forgot to remove it.

kangyizhang · 2019-12-18T01:04:13Z

thank you for the PR!

googlebot added the cla: yes label Dec 7, 2019

yhwang force-pushed the tfjs-nodejs-support-aarch64 branch from 86775a4 to 62381d8 Compare December 9, 2019 18:25

kangyizhang suggested changes Dec 9, 2019

View reviewed changes

yhwang force-pushed the tfjs-nodejs-support-aarch64 branch from 62381d8 to a337cb7 Compare December 9, 2019 21:46

yhwang commented Dec 10, 2019

View reviewed changes

yhwang force-pushed the tfjs-nodejs-support-aarch64 branch from a337cb7 to 5df4cd2 Compare December 10, 2019 03:02

yhwang force-pushed the tfjs-nodejs-support-aarch64 branch from 5df4cd2 to 2b3be58 Compare December 10, 2019 19:05

yhwang requested a review from kangyizhang December 13, 2019 00:56

yhwang force-pushed the tfjs-nodejs-support-aarch64 branch 3 times, most recently from b888da2 to c2022ca Compare December 13, 2019 23:42

yhwang commented Dec 13, 2019

View reviewed changes

kangyizhang suggested changes Dec 17, 2019

View reviewed changes

kangyizhang reviewed Dec 17, 2019

View reviewed changes

yhwang force-pushed the tfjs-nodejs-support-aarch64 branch from c2022ca to 73c8461 Compare December 17, 2019 18:11

yhwang commented Dec 17, 2019

View reviewed changes

yhwang requested a review from kangyizhang December 17, 2019 18:30

add readme

bff77f6

kangyizhang approved these changes Dec 17, 2019

View reviewed changes

kangyizhang added 2 commits December 17, 2019 16:06

missed readme

e7b2706

Merge branch 'master' into tfjs-nodejs-support-aarch64

b9a05d4

kangyizhang merged commit 6323566 into tensorflow:master Dec 18, 2019

yhwang deleted the tfjs-nodejs-support-aarch64 branch December 18, 2019 01:20

rthadur mentioned this pull request Jul 1, 2020

Unable to specify custom binary #3542

Closed

Add aarch64 support for tfjs-node #2513

Add aarch64 support for tfjs-node #2513

Uh oh!

Conversation

yhwang commented Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhwang commented Dec 7, 2019

Uh oh!

kangyizhang left a comment

Choose a reason for hiding this comment

Uh oh!

yhwang left a comment

Choose a reason for hiding this comment

Uh oh!

yhwang left a comment

Choose a reason for hiding this comment

Uh oh!

yhwang commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhwang commented Dec 13, 2019

Uh oh!

yhwang left a comment

Choose a reason for hiding this comment

Uh oh!

yhwang left a comment

Choose a reason for hiding this comment

Uh oh!

kangyizhang left a comment

Choose a reason for hiding this comment

Uh oh!

kangyizhang left a comment

Choose a reason for hiding this comment

Uh oh!

yhwang left a comment

Choose a reason for hiding this comment

Uh oh!

kangyizhang left a comment

Choose a reason for hiding this comment

Uh oh!

yhwang commented Dec 18, 2019

Uh oh!

kangyizhang commented Dec 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yhwang commented Dec 7, 2019 •

edited

Loading

yhwang commented Dec 10, 2019 •

edited

Loading