[core] Move variations of the same set of tests to nightly.

[lina128]

This PR has below changes:

1. Change the run-build.sh logic, trigger build either there's change in the package or env is nightly. Basically, nightly build will build and test the whole repo.

2. Change Core's test-ci.sh logic, for regular build only test for chrome with default settings, for nightly build test all permutations. Basically, moving testing of different settings to nightly.

p.s. Some observation: Core test duration reduced from ~10 mins. to ~6mins., browserstack instances usage reduced from 12 instances to 1 instance.

---

This change is 

[dsmilkov]

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

---

scripts/run-build.sh, line 21 at r1 (raw file):

DIR=$1
if [[ -f "$DIR/run-ci" || "$NIGHTLY" = true ]]; then
  gcloud builds submit . --config=$DIR/cloudbuild.yml

do you need to explicitly propagate the NIGHTLY env to core's cloud build? I.e. does test-ci.sh in core see the NIGHTLY from run-build.sh

[dsmilkov]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)

---

cloudbuild.yml, line 25 at r1 (raw file):

  args: ['./scripts/run-build.sh', 'tfjs-core']
  waitFor: ['find-affected-packages']
  env: ['NIGHTLY=$_NIGHTLY']

so we propagate nightly for core, but what happens for the other repos (converter, data, etc). How do they get tested on nightly? If it's separate script, then maybe we don't need to do anything here.

[dsmilkov]

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @nsthorat, and @pyu10055)

---

cloudbuild.yml, line 25 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

so we propagate nightly for core, but what happens for the other repos (converter, data, etc). How do they get tested on nightly? If it's separate script, then maybe we don't need to do anything here.

Maybe we keep this here but we shouldn't run tfjs-core/scripts/test-integration.sh on nightly anymore since we will be double-testing every night: https://github.com/tensorflow/tfjs/blob/master/tfjs-core/scripts/test-integration.sh

[dsmilkov]

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @nsthorat, and @pyu10055)

---

cloudbuild.yml, line 25 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Maybe we keep this here but we shouldn't run tfjs-core/scripts/test-integration.sh on nightly anymore since we will be double-testing every night: https://github.com/tensorflow/tfjs/blob/master/tfjs-core/scripts/test-integration.sh

I'm assume we will enable nightly for all other repos here

[lina128]

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

---

cloudbuild.yml, line 25 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

I'm assume we will enable nightly for all other repos here

I did a search of NIGHTLY in the repo, converter uses NIGHTLY, which also propagate nightly: https://github.com/tensorflow/tfjs/blob/master/tfjs-converter/cloudbuild.yml#L16

Other packages didn't, because the test will be same. For these packages, they use the same setup. So we can propagate NIGHTLY variable only for packages that need to run different tests for PR vs. Nightly build.

There are two strategies we can think of separating tests for PR and Nightly, Option(1): Keep the same cloudbuild.yml, use flags to compute what tests to run and what test settings (e.g. browserstack, etc.), this allows us to only maintain one cloudbuild.yml at the same time have the capacity to select test settings . Option(2): Have a separate cloudbuild_nightly.yml, this gives us the maximum freedom to define what should be included and what tests to run. I think eventually we will have a mixed strategy. For unit tests, i.e. tests that are run every PR, use Option1. So Nightly basically runs the same thing, but for the whole repo. This is literally what Nightly build means. And then for integration tests, correctness tests, benchmark tests (think about those tests in Layers, Converter, and tfjs's integration_tests), we can have a separate cloudbuild_nightly.yml, because these may have different flows. We can also use this set in other scenarios, e.g. before release. So this set may even use a broader name, such as cloudbuild_regression.yml, etc.

Regarding test-integration.sh, right, those are redundant. I planned to do the cleanup in a separate PR, but I'm also fine removing it in this PR. WDYT?

---

scripts/run-build.sh, line 21 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

do you need to explicitly propagate the NIGHTLY env to core's cloud build? I.e. does test-ci.sh in core see the NIGHTLY from run-build.sh

The idea here is use the NIGHTLY flag to trigger cloudbuild of all packages specified in this file: https://github.com/tensorflow/tfjs/blob/master/cloudbuild.yml#L19-L94

[lina128]

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

scripts/run-build.sh, line 21 at r1 (raw file):

DIR=$1
if [[ -f "$DIR/run-ci" || "$NIGHTLY" = true ]]; then
  gcloud builds submit . --config=$DIR/cloudbuild.yml

do you need to explicitly propagate the NIGHTLY env to core's cloud build? I.e. does test-ci.sh in core see the NIGHTLY from run-build.sh

Because of the original logic, Nightly doesn't build any package, because there's no run-ci in those packages, in other words, no change in those packages. It was just running test-integration, see an example run: https://pantheon.corp.google.com/cloud-build/builds/30bf60f1-f051-41b4-8545-b3a2fcd1e5fc?project=learnjs-174218

I added the logic here so that we can use the NIGHTLY flag to trigger a build for every package.

[lina128]

Hi Daniel, I replied inline, thank you for the detailed review!

[nsthorat]

can you indent the body of this?

[lina128]

Done. Thank you for the review, Nikhil!

[pyu10055]

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

[lina128]

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

scripts/run-build.sh, line 21 at r1 (raw file):

DIR=$1
if [[ -f "$DIR/run-ci" || "$NIGHTLY" = true ]]; then
  gcloud builds submit . --config=$DIR/cloudbuild.yml

do you need to explicitly propagate the NIGHTLY env to core's cloud build? I.e. does test-ci.sh in core see the NIGHTLY from run-build.sh

Discussed offline, thank you for catching this Daniel. This follow-up PR adds the fix to properly propagate NIGHTLY, #2965