Add affected packages analysis. #2793

lina128 · 2020-02-26T02:08:05Z

Introduced dependency.json in this PR. After this PR, when calculating diff, CI will also analyze affected packages and run tests for those packages too. dependency.json is manually maintained, if there is change tfjs package dependencies, please update dependency.json as well.

p.s.: Test of this PR will not be able to validate whether this works or not, because the change is in diff.js, which triggers all builds, and skip the added logic. In order to test the logic, in one of the debug commit, I removed diff.js from the whitelist and made some changes to Core, the build shows it works as expected. You can check the log in that build for commit e604026.

This change is

nsthorat

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

scripts/dependency.json, line 1 at r1 (raw file):

can we call this file package_dependencies.json

scripts/diff.js, line 113 at r1 (raw file):

  triggeredBuilds.forEach(triggeredBuild => {
    const affectedPackages =
        calculateAffectedPackages(dependencyGraph, triggeredBuild);

shouldn't this take into consideration the diff?

scripts/diff.js, line 115 at r1 (raw file):

        calculateAffectedPackages(dependencyGraph, triggeredBuild);
    affectedPackages.forEach(package => {
      writeFileSync(join(package, 'diff'), '');

why do you have to write '' here?

scripts/diff.js, line 121 at r1 (raw file):

  // Deduplicate the triggered builds for log.
  triggeredBuilds = [...new Set(triggeredBuilds)];

do Array.from here so we're using the default constructors

scripts/test-util.js, line 31 at r1 (raw file):

// Example:
//   dependencyGraph = {
//     "tfjs-core": ["tfjs-converter", "tfjs", ...],

any reason the .json file doesn't just look like this map?

scripts/test-util.js, line 41 at r1 (raw file):

  const dependencyGraph = {};

  dependencyInfo.packages.forEach(

nit: this block should be (not semicolons and indentation):

dependencyInfo.packages.forEach(package => {
  package.dependencies.forEach(dependency => {
      if (!dependencyGraph[dependency]) {
        dependencyGraph[dependency] = [];
      }
      dependencyGraph[dependency].push(package.package);
   });
});

scripts/test-util.js, line 56 at r1 (raw file):

  traverseDependencyGraph(dependencyGraph, package, affectedPackages);

  return [...affectedPackages];

you can do Array.from(affectedPackages)

dsmilkov

Great stuff. Tiny comments, non-major thus LGTM

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

scripts/diff.js, line 113 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

shouldn't this take into consideration the diff?

I think triggeredBuilds is already pre-populated with the touched packages, from the code above.

scripts/diff.js, line 115 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

why do you have to write '' here?

+1 no need to write.

scripts/diff.js, line 116 at r1 (raw file):

    affectedPackages.forEach(package => {
      writeFileSync(join(package, 'diff'), '');
      triggeredBuilds.push(package);

push into a separate array, not the one that you are currently traversing (triggeredBuilds.forEach loop above) to avoid looking into the same items multiple times.

lina128 · 2020-02-27T21:59:39Z

scripts/test-util.js, line 31 at r1 (raw file):
// Example:
//   dependencyGraph = {
//     "tfjs-core": ["tfjs-converter", "tfjs", ...],
any reason the .json file doesn't just look like this map?
Hi Nikhil,
Yeah, we can make the .json file look like this and we can avoid compute the dependency graph each time. However, I think have the .json file mirroring the package.json file of each package, allows us to possibly automate the dependency file. Also it makes adding new package to this file easy, just copy paste from package.json of the new package. But either way has some pros, so I don't have strong preference. What do you think? Which pros is more important?

nsthorat

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

scripts/test-util.js, line 31 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Hi Nikhil, Yeah, we can make the .json file look like this and we can avoid compute the dependency graph each time. However, I think have the .json file mirroring the package.json file of each package, allows us to possibly automate the dependency file. Also it makes adding new package to this file easy, just copy paste from package.json of the new package. But either way has some pros, so I don't have strong preference. What do you think? Which pros is more important?

I think we should let this be manual so there's less magic / moving parts :)

lina128 · 2020-02-28T18:24:57Z

Great stuff. Tiny comments, non-major thus LGTM

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

scripts/diff.js, line 113 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…
I think triggeredBuilds is already pre-populated with the touched packages, from the code above.

scripts/diff.js, line 115 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…
+1 no need to write.
You are right. Removed ''.

scripts/diff.js, line 116 at r1 (raw file):
    affectedPackages.forEach(package => {
      writeFileSync(join(package, 'diff'), '');
      triggeredBuilds.push(package);
push into a separate array, not the one that you are currently traversing (triggeredBuilds.forEach loop above) to avoid looking into the same items multiple times.

Ah, nice catch. Pushed to a separate array.

lina128 · 2020-02-28T18:27:13Z

Hi @nsthorat and @dsmilkov , thank you for your review! I changed the dependency file to simpler format as we discussed offline, such as "tfjs-converter": ["tfjs-core"]. It still reflects package.json structure. Also addressed your other comments too, great comments. Thank you both!

dsmilkov

Thanks! LGTM

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

pyu10055

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

scripts/diff.js, line 117 at r3 (raw file):

        computeAffectedPackages(dependencyGraph, triggeredBuild);
    affectedPackages.forEach(package => {
      writeFileSync(join(package, 'diff'));

package seems to be the name of the packages, with out the directory information, where you are trying to write this file?
and how would this file used?

nsthorat

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @nsthorat)

scripts/diff.js, line 115 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

You are right. Removed ''.

I meant why do you need to write the file at all?

lina128 · 2020-02-28T22:00:55Z

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @nsthorat)

scripts/diff.js, line 115 at r1 (raw file):

Previously, lina128 (Na Li) wrote…
I meant why do you need to write the file at all?

Because we rely on whether the dir has a diff file to trigger cloud build, right? https://github.com/tensorflow/tfjs/blob/master/scripts/run-build.sh#L20

nsthorat

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

scripts/diff.js, line 115 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Because we rely on whether the dir has a diff file to trigger cloud build, right? https://github.com/tensorflow/tfjs/blob/master/scripts/run-build.sh#L20

ah, right. can we rename this file from 'diff' to something like 'run-ci'? here and above, and maybe make this file called diff.js something like 'find-affected-packages.js'?

lina128 · 2020-02-28T22:04:25Z

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

scripts/diff.js, line 117 at r3 (raw file):
        computeAffectedPackages(dependencyGraph, triggeredBuild);
    affectedPackages.forEach(package => {
      writeFileSync(join(package, 'diff'));
package seems to be the name of the packages, with out the directory information, where you are trying to write this file?
and how would this file used?

After line 72, I'm in the root directory, so the package name is also the dir name. https://github.com/tensorflow/tfjs/blob/master/scripts/diff.js#L71

I'm trying to write this file under each affected package. So that when the code detects a diff file, it will trigger cloud build: https://github.com/tensorflow/tfjs/blob/master/scripts/run-build.sh

…un-ci. Also debug.

lina128 · 2020-02-28T22:48:02Z

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

scripts/diff.js, line 115 at r1 (raw file):

Previously, lina128 (Na Li) wrote…
ah, right. can we rename this file from 'diff' to something like 'run-ci'? here and above, and maybe make this file called diff.js something like 'find-affected-packages.js'?

Done. Changed diff to run-ci in three places in find-affected-packages.js file. Also add run-ci to gitignore. Changed file name to find-affected-packages, also changed yarn command, and cloudbuild accordingly.

pyu10055

Reviewed 3 of 5 files at r2, 5 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: complete! 3 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

googlebot added the cla: yes label Feb 26, 2020

lina128 requested review from dsmilkov, nsthorat and pyu10055 February 27, 2020 01:32

nsthorat reviewed Feb 27, 2020

View reviewed changes

dsmilkov approved these changes Feb 27, 2020

View reviewed changes

nsthorat reviewed Feb 28, 2020

View reviewed changes

lina128 added 16 commits February 28, 2020 09:59

Add affected packages analysis.

913147f

Add run condition.

42905fe

Remove diff from whitelist for testing.

400b8e6

.

5c77e56

Change file path.

981a2ed

debug.

1d3af68

.

9aee28d

Test Core change.

b106dc2

Debug.

5cc0923

.

5eff909

debug.

6d23908

debug.

7d190c5

debug.

c60d5ab

Clean up.

6459d60

Change let to const.

b52cb9d

Simplify dependency.json and debug.

fc3abe1

lina128 force-pushed the test_ci branch from e794123 to fc3abe1 Compare February 28, 2020 17:59

.

7f85117

Final clean up.

132104f

dsmilkov approved these changes Feb 28, 2020

View reviewed changes

pyu10055 requested changes Feb 28, 2020

View reviewed changes

nsthorat reviewed Feb 28, 2020

View reviewed changes

nsthorat approved these changes Feb 28, 2020

View reviewed changes

lina128 added 5 commits February 28, 2020 14:12

Change diff.js to find-affected-packages.js and change diff file to r…

82401bb

…un-ci. Also debug.

Change cloudbuild diff to find-affected-packages.

01ce445

debug.

d3a47d9

debug.

fcafe73

Clean up.

5330717

lina128 added 3 commits February 28, 2020 15:48

Update layers integration test to python3.

0f86642

Remove tf-nightly test in layers because unresolved version issues.

5b33815

.

c9e74f8

pyu10055 approved these changes Feb 29, 2020

View reviewed changes

lina128 merged commit 0162a29 into tensorflow:master Feb 29, 2020

nsthorat mentioned this pull request Mar 4, 2020

Change test-integration to look for 'run-ci' instead of 'diff'. #2823

Merged

Add affected packages analysis. #2793

Add affected packages analysis. #2793

Uh oh!

Conversation

lina128 commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nsthorat left a comment

Choose a reason for hiding this comment

Uh oh!

dsmilkov left a comment

Choose a reason for hiding this comment

Uh oh!

lina128 commented Feb 27, 2020

Uh oh!

nsthorat left a comment

Choose a reason for hiding this comment

Uh oh!

lina128 commented Feb 28, 2020

Uh oh!

lina128 commented Feb 28, 2020

Uh oh!

dsmilkov left a comment

Choose a reason for hiding this comment

Uh oh!

pyu10055 left a comment

Choose a reason for hiding this comment

Uh oh!

nsthorat left a comment

Choose a reason for hiding this comment

Uh oh!

lina128 commented Feb 28, 2020

Uh oh!

nsthorat left a comment

Choose a reason for hiding this comment

Uh oh!

lina128 commented Feb 28, 2020

Uh oh!

lina128 commented Feb 28, 2020

Uh oh!

pyu10055 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lina128 commented Feb 26, 2020 •

edited

Loading