Depend on node-pre-gyp to use pre-compiled addon #276
Conversation
There was a problem hiding this comment.
Reviewed 19 of 19 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang and @nkreeger)
binding.gyp, line 59 at r1 (raw file):
"@rpath/libtensorflow.1.dylib", "@loader_path/../../deps/lib/libtensorflow.dylib", "<@(PRODUCT_DIR)/tfjs_binding.node"
Just checking - this needs to be removed?
DEVELOPMENT.md, line 3 at r1 (raw file):
support
s/support/supports the
DEVELOPMENT.md, line 3 at r1 (raw file):
This guide lists commands to development the tfjs-node package.
This guide lists commands to use when developing the this package.
DEVELOPMENT.md, line 7 at r1 (raw file):
Install dependencies and addon module
nit: You already have a super-section titled 'Install' - maybe call this one 'Dependencies and addon module'?
DEVELOPMENT.md, line 10 at r1 (raw file):
yarn
nit: $ yarn clarifies that this is a bash command. Same throughout this file.
DEVELOPMENT.md, line 13 at r1 (raw file):
It also download the download the TensorFlow C library and native node addon.
It also downloads the TensorFlow C library and the native node addon.
DEVELOPMENT.md, line 18 at r1 (raw file):
install
Should this just be build-from-source? Install implies something is being installed I think.
DEVELOPMENT.md, line 21 at r1 (raw file):
This command clear local binary and addon resources, then download the TensorFlow C library and compile native node addon from source binary.
This command does the following:
- Clears local binary and addon resources (maybe clarify this on a new line)
- Downloads the TensorFlow C library
- Compiles the native addon from source binary (what is this? Do you mean just "source files")?
DEVELOPMENT.md, line 23 at r1 (raw file):
Switch to developing GPU
nit: Switching local workflow to CUDA/GPU
DEVELOPMENT.md, line 29 at r1 (raw file):
This command download then TensorFlow C library for GPU and compile native node addon from source binary.
Might say this is the same as yarn install-from-source except it uses the GPU library.
DEVELOPMENT.md, line 39 at r1 (raw file):
local
I'd specify that this uses yalc to handle this "locally".
DEVELOPMENT.md, line 45 at r1 (raw file):
This command pack the tfjs-node package and publish to other repos through yalc. Note this repo must have been installed through yalc in other repos.
This command packs the tfjs-node package and publishes locally through yalc (add link here).
NOTE: Dependent packages must install this locally published package through ya
DEVELOPMENT.md, line 55 at r1 (raw file):
#### Build and upload node addon to Google Cloud Platform
What about the step of installing the gcp binding? Can you link to that? Seems like a first prerequisite.
DEVELOPMENT.md, line 61 at r1 (raw file):
This command compile a new node addon, then compress and upload it to GCP
This command will compile, compress, and upload a new node addon to GCP.
DEVELOPMENT.md, line 69 at r1 (raw file):
This command clear existing resources and compile new node addon from source, then pack a npm package. NOTE: this command does not update the node addon in GCP.
This command will publish a new version of tfjs-node/tfjs-node-gpu to NPM.
NOTE: This command does not update the pre-compiled node addon to GCP (see build-addon upload).
DEVELOPMENT.md, line 77 at r1 (raw file):
This command combines the above two commands
Period at end of sentence.
DEVELOPMENT.md, line 85 at r1 (raw file):
This command compile a new node addon, upload it to GCP, then build and publish a new npm package. Please read instruction in [publish-npm.sh](./scripts/publish.sh) before publishing.
This command compiles a new node addon, uploads it to GCP, then builds and publishs a new npm package. Please read instruction in publish-npm.sh before publishing.
scripts/build-and-upload-addon.sh, line 17 at r1 (raw file):
sting pre-built addon tarball
Is there a command for this?
scripts/build-and-upload-addon.sh, line 19 at r1 (raw file):
Compress and upload the pre-built addon tarball.
Command for this too?
Might be worth pointing out the doc you wrote for this already.
scripts/build-and-upload-addon.sh, line 39 at r1 (raw file):
Delete deps and lib folder
Command? (yarn clean-deps)
scripts/build-and-upload-addon.sh, line 44 at r1 (raw file):
Quoted 5 lines of code…
# for /f %i in ('node scripts/get-addon-name.js') do set PACKAGE_NAME=%i # for /f %i in ('node -p "process.versions.napi"') do set NAPI_VERSION=%i # tar -czvf %PACKAGE_NAME% -C lib napi-v%NAPI_VERSION%/tfjs_binding.node # for /f %i in ('node scripts/print-full-package-host') do set PACKAGE_HOST=%i # gsutil cp %PACKAGE_NAME% gs://%PACKAGE_HOST%
Is this a TODO(kangyizhang): Make this work in a script?
scripts/build-and-upload-addon.sh, line 51 at r1 (raw file):
4) Change field "name" in package.json to "@tensorflow/tfjs-node-gpu"
We have a separate npm publish script for GPU now - do we need the same for this file?
scripts/get-addon-name.js, line 20 at r1 (raw file):
// const napiVersion = require('../package.json').binary.napi_versions[0];
No longer used?
scripts/get-addon-name.js, line 22 at r1 (raw file):
isCPU = !name.includes('gpu');
nit: Check for non-negative:
if (name.includes('gpu')) { // ... } else { // ... } scripts/get-addon-name.js, line 49 at r1 (raw file):
console.log(addonName);
Delete this.
scripts/print-full-package-host.js, line 1 at r1 (raw file):
Quoted 6 lines of code…
console.log( require('../package.json').binary.host.split('.com/')[1] + '/napi-v' + process.versions.napi + '/' + require('../package.json').version + '/');
What is this used for?
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @nkreeger)
binding.gyp, line 59 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
Just checking - this needs to be removed?
<@(PRODUCT_DIR) works the same as <(PRODUCT_DIR). I remove the @ so it aligns with all other places through out this file.
DEVELOPMENT.md, line 3 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
supports/support/supports the
Done
DEVELOPMENT.md, line 3 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
This guide lists commands to development the tfjs-node package.This guide lists commands to use when developing the this package.
Done.
DEVELOPMENT.md, line 7 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
Install dependencies and addon modulenit: You already have a super-section titled 'Install' - maybe call this one 'Dependencies and addon module'?
Done
DEVELOPMENT.md, line 10 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
yarnnit:
$ yarnclarifies that this is a bash command. Same throughout this file.
Done
DEVELOPMENT.md, line 13 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
It also download the download the TensorFlow C library and native node addon.It also downloads the TensorFlow C library and the native node addon.
Done
DEVELOPMENT.md, line 18 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
installShould this just be
build-from-source? Install implies something is being installed I think.
This command would install dependencies if any of them is missing. This command can be used when developers fork the repo, make some change, and want to compile addon with local change.
Another script command yarn build-addon-from-source is added, which only compile a new node addon from source files.
DEVELOPMENT.md, line 21 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
This command clear local binary and addon resources, then download the TensorFlow C library and compile native node addon from source binary.This command does the following:
- Clears local binary and addon resources (maybe clarify this on a new line)
- Downloads the TensorFlow C library
- Compiles the native addon from source binary (what is this? Do you mean just "source files")?
Updated:
This command does the following:
- Clears local binary and addon resources
- Downloads the TensorFlow C library
- Compiles the native addon from source files (instead of downloading pre-compile addon)
DEVELOPMENT.md, line 23 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
Switch to developing GPUnit: Switching local workflow to CUDA/GPU
Done
DEVELOPMENT.md, line 29 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
This command download then TensorFlow C library for GPU and compile native node addon from source binary.Might say this is the same as
yarn install-from-sourceexcept it uses the GPU library.
Done.
DEVELOPMENT.md, line 39 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
localI'd specify that this uses yalc to handle this "locally".
Done.
DEVELOPMENT.md, line 45 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
This command pack the tfjs-node package and publish to other repos through yalc. Note this repo must have been installed through yalc in other repos.This command packs the
tfjs-nodepackage and publishes locally through yalc (add link here).
NOTE: Dependent packages must install this locally published package through ya
Done.
DEVELOPMENT.md, line 55 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
#### Build and upload node addon to Google Cloud PlatformWhat about the step of installing the gcp binding? Can you link to that? Seems like a first prerequisite.
Added a prerequisite section.
DEVELOPMENT.md, line 61 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
This command compile a new node addon, then compress and upload it to GCPThis command will compile, compress, and upload a new node addon to GCP.
Done.
DEVELOPMENT.md, line 69 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
This command clear existing resources and compile new node addon from source, then pack a npm package. NOTE: this command does not update the node addon in GCP.This command will publish a new version of tfjs-node/tfjs-node-gpu to NPM.
NOTE: This command does not update the pre-compiled node addon to GCP (seebuild-addon upload).
Done. (This command build a new version of npm tarball, but not publish).
DEVELOPMENT.md, line 77 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
This command combines the above two commandsPeriod at end of sentence.
Done.
DEVELOPMENT.md, line 85 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
This command compile a new node addon, upload it to GCP, then build and publish a new npm package. Please read instruction in [publish-npm.sh](./scripts/publish.sh) before publishing.This command compiles a new node addon, uploads it to GCP, then builds and publishs a new npm package. Please read instruction in publish-npm.sh before publishing.
Done.
scripts/build-and-upload-addon.sh, line 17 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
sting pre-built addon tarballIs there a command for this?
Sorry the comment is misleading. Actually the following commands do these steps. The comment has been updated.
scripts/build-and-upload-addon.sh, line 19 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
Compress and upload the pre-built addon tarball.Command for this too?
Might be worth pointing out the doc you wrote for this already.
The following commands are doing these steps.
Updated the comment to direct developers to DEVELOPMENT.md
scripts/build-and-upload-addon.sh, line 39 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
Delete deps and lib folderCommand? (yarn clean-deps)
Added command to clean-deps for windows.
scripts/build-and-upload-addon.sh, line 44 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
# for /f %i in ('node scripts/get-addon-name.js') do set PACKAGE_NAME=%i # for /f %i in ('node -p "process.versions.napi"') do set NAPI_VERSION=%i # tar -czvf %PACKAGE_NAME% -C lib napi-v%NAPI_VERSION%/tfjs_binding.node # for /f %i in ('node scripts/print-full-package-host') do set PACKAGE_HOST=%i # gsutil cp %PACKAGE_NAME% gs://%PACKAGE_HOST%Is this a TODO(kangyizhang): Make this work in a script?
Moved these commands to build-and-upload-windows-addon.bat
Also added bash command yarn upload-windows-addon and yarn upload-windows-addon-gpu to do these work through one command. The DEVELOPMENT.md has been updated as well.
scripts/build-and-upload-addon.sh, line 51 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
4) Change field "name" in package.json to "@tensorflow/tfjs-node-gpu"We have a separate npm publish script for GPU now - do we need the same for this file?
Yes. These commands has been moved to build-and-upload-windows-addon-gpu.bat
scripts/get-addon-name.js, line 20 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
// const napiVersion = require('../package.json').binary.napi_versions[0];No longer used?
Done. Thanks for catching this.
scripts/get-addon-name.js, line 22 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
isCPU = !name.includes('gpu');nit: Check for non-negative:
if (name.includes('gpu')) { // ... } else { // ... }
Done
scripts/get-addon-name.js, line 49 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
console.log(addonName);Delete this.
These are used in bash scripts. Added comment to explain this.
scripts/print-full-package-host.js, line 1 at r1 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
console.log( require('../package.json').binary.host.split('.com/')[1] + '/napi-v' + process.versions.napi + '/' + require('../package.json').version + '/');What is this used for?
These are used in bash scripts. Added comment to explain this.
There was a problem hiding this comment.
Nice work!
Reviewed 7 of 7 files at r2.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @kangyizhang and @nkreeger)
package.json, line 34 at r2 (raw file):
"upload-windows-addon": "./scripts/build-and-upload-windows-addon.bat", "upload-windows-addon-gpu": "./scripts/build-and-upload-windows-addon-gpu.bat"
Curious - did this work OK?
There was a problem hiding this comment.
Reviewable status:
package.json, line 34 at r2 (raw file):
Previously, nkreeger (Nick Kreeger) wrote…
"upload-windows-addon": "./scripts/build-and-upload-windows-addon.bat", "upload-windows-addon-gpu": "./scripts/build-and-upload-windows-addon-gpu.bat"Curious - did this work OK?
Yes, I have tried these on my windows machine. Also I can login to our GCP account on my personal machine.
2 participants
This PR switch to use pre-compiled addon with package node-pre-gyp
The changes include:
add required change for node-pre-gyp
binaryinpackage.jsonto manage node addon resource (local dir, cloud url)action_after_buildin binding.gyp for node-pre-gyp to manage node addonnode-gyp rebuildtonode-pre-gyp installininstall.js, also moved callout ofdeps-stagefrombinding.gyptoinstall.js.add scripts to build and publish node addon
DEVELOPMENT.mdto list related commandadd new folders/files in .gitignore and .npmignore
This change is