Remove link-master. Refactor test steps.

[lina128]

This PR is mainly clean-up, it touches these packages: layers, converter, data, union package, node, and node-gpu.

It has below changes:

1. Remove link-master in package.json, and deleted link-master.js and link-core-master.js from top level scripts folder. Since the new release process is in place, master's dependency structure will be stable. Remove the short term solution.

2. Refactor package.json and cloudbuild.yml. Make sure for both test and test-ci, it build dependency packages and then build itself and then lint and then test. The differences between test and test-ci are (1) test-ci use the ci version of build for the dependency packages instead of regular build; (2) test-ci also has lint and build, whereas test doesn't to make it lightweight for local development.

p.s. We have to put the build dependencies step in test-ci, not in cloudbuild.yml, because integration test also needs to call test-ci, but will not use the same cloudbuild.yml. Put build dependencies in test-ci makes sure it will always have all the things it needs for testing no matter where test-ci is called.

p.p.s. We made the decision for each package's build, only build itself, build-deps shouldn't be included. This is because include it will cause some common packages to be rebuilt many many times in a transitive build process.

---

This change is 

[dsmilkov]

Great cleanup! Just a few smaller comments

Reviewed 17 of 17 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)

---

tfjs-converter/cloudbuild.yml, line 23 at r1 (raw file):

  entrypoint: 'bash'
  args: ['./build-pip-package.sh', '--test', '/tmp/tfjs-pips']
  waitFor: ['test-js']

why not run the python tests in parallel with the js tests? here and below

---

tfjs-converter/package.json, line 57 at r1 (raw file):

  },
  "scripts": {
    "build": "yarn gen-json --test && tsc",

let's add "build-deps" step for every project, for consistency. this way we can see immediately what the pre-steps are.

---

tfjs-converter/package.json, line 65 at r1 (raw file):

yarn gen-json --test && yarn build-ci

since "yarn gen-json --test" is called from "yarn build-ci" , you can remove it from here

---

tfjs-data/cloudbuild.yml, line 36 at r1 (raw file):

- name: 'node:10'
  dir: 'tfjs-data'
  entrypoint: 'yarn'

No action need, just observation: one side-effect of moving these steps from a yml file into a package.json script, is that it will be harder to instantly see what fails without diving into the logs. e.g. if yarn lint fails, previously you would see that as a top-level step in the cloudbuild UI.

---

tfjs-layers/scripts/test-ci.sh, line 14 at r1 (raw file):

# Regular testing.
yarn
yarn build-core-ci

let's name this step "build-deps" for consistency, across all projects.

[lina128]

Great cleanup! Just a few smaller comments

Reviewed 17 of 17 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)

tfjs-converter/cloudbuild.yml, line 23 at r1 (raw file):

  entrypoint: 'bash'
  args: ['./build-pip-package.sh', '--test', '/tmp/tfjs-pips']
  waitFor: ['test-js']

why not run the python tests in parallel with the js tests? here and below

tfjs-converter/package.json, line 57 at r1 (raw file):

  },
  "scripts": {
    "build": "yarn gen-json --test && tsc",

let's add "build-deps" step for every project, for consistency. this way we can see immediately what the pre-steps are.

tfjs-converter/package.json, line 65 at r1 (raw file):

yarn gen-json --test && yarn build-ci

since "yarn gen-json --test" is called from "yarn build-ci" , you can remove it from here

tfjs-data/cloudbuild.yml, line 36 at r1 (raw file):

- name: 'node:10'
  dir: 'tfjs-data'
  entrypoint: 'yarn'

No action need, just observation: one side-effect of moving these steps from a yml file into a package.json script, is that it will be harder to instantly see what fails without diving into the logs. e.g. if yarn lint fails, previously you would see that as a top-level step in the cloudbuild UI.

tfjs-layers/scripts/test-ci.sh, line 14 at r1 (raw file):

# Regular testing.
yarn
yarn build-core-ci

let's name this step "build-deps" for consistency, across all projects.

Hi Daniel, thank you for the review! I changed to use build-deps across the packages for consistency. Regarding lint, you're right, people need to look at log to see whether the errors come from (lint, or build or test), because now they are combined. I've thought about this, lint can only be run after build, otherwise it will fail. So technically, we can add the lint step after the test-ci step in cloudbuild.yml. But there's one caveat, if there's a lint error, we want it to fail fast, so that it can prevent running the more expensive unit/integration test. Moving it after test againsts this purpose.

[nsthorat]

Reviewed 10 of 17 files at r1, 4 of 4 files at r2.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)

---

tfjs/package.json, line 55 at r2 (raw file):

    "build": "tsc",
    "build-ci": "tsc",
    "build-core": "cd ../tfjs-core && yarn && yarn build",

do you need all these now?

---

tfjs/scripts/test-ci.sh, line 20 at r2 (raw file):

set -e

yarn

do you need yarn?

[lina128]

Reviewed 10 of 17 files at r1, 4 of 4 files at r2.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)

tfjs/package.json, line 55 at r2 (raw file):

    "build": "tsc",
    "build-ci": "tsc",
    "build-core": "cd ../tfjs-core && yarn && yarn build",

do you need all these now?
Yes, to make 'yarn build-deps' short.

tfjs/scripts/test-ci.sh, line 20 at r2 (raw file):

set -e

yarn

do you need yarn?
Yes, so that test-ci is a standalone command to be called by integration tests from other places, for this it needs to make sure it installs packages, build dependencies, before start testing.

[dsmilkov]

Reviewed 1 of 4 files at r2.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)

---

tfjs-layers/scripts/test-ci.sh, line 14 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Hi Daniel, thank you for the review! I changed to use build-deps across the packages for consistency. Regarding lint, you're right, people need to look at log to see whether the errors come from (lint, or build or test), because now they are combined. I've thought about this, lint can only be run after build, otherwise it will fail. So technically, we can add the lint step after the test-ci step in cloudbuild.yml. But there's one caveat, if there's a lint error, we want it to fail fast, so that it can prevent running the more expensive unit/integration test. Moving it after test againsts this purpose.

sgtm.

[dsmilkov]

Reviewable status: complete! 2 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @pyu10055)