Fix presubmit change detection for non-Bazel packages (#6390)

Fix incorrect presubmit change detection between Bazel and non-Bazel packages. Fixes #6384
tensorflow · May 10, 2022 · 8a8b7e0 · 8a8b7e0
1 parent 4206957
commit 8a8b7e0
Show file tree

Hide file tree

Showing 12 changed files with 107 additions and 112 deletions.
diff --git a/package.json b/package.json
@@ -13,6 +13,7 @@
     "@rollup/plugin-node-resolve": "^11.2.1",
     "@types/argparse": "^1.0.38",
     "@types/jasmine": "~3.0.0",
+    "@types/js-yaml": "^4.0.5",
     "@types/long": "4.0.1",
     "@types/mkdirp": "^0.5.2",
     "@types/node": "^12.7.5",
@@ -63,8 +64,8 @@
   "scripts": {
     "lint": "tslint -p tsconfig_tslint.json",
     "test-packages-ci": "yarn generate-cloudbuild-for-packages && ./scripts/run-build.sh",
-    "generate-cloudbuild-for-packages": "./scripts/generate_cloudbuild_for_packages.js",
-    "test-generate-cloudbuild": "jasmine run scripts/generate_cloudbuild_test.js",
+    "generate-cloudbuild-for-packages": "ts-node -s ./scripts/generate_cloudbuild_for_packages.ts",
+    "test-generate-cloudbuild": "cd scripts && node --require ts-node/register ../node_modules/jasmine/bin/jasmine.js run generate_cloudbuild_test.ts",
     "test-run-flaky": "jasmine run scripts/run_flaky_test.js",
     "release": "ts-node -s ./scripts/release.ts",
     "release-tfjs": "ts-node -s ./scripts/release-tfjs.ts",

diff --git a/scripts/bazel_packages.ts b/scripts/bazel_packages.ts
@@ -0,0 +1,4 @@
+export const BAZEL_PACKAGES = new Set([
+  'tfjs-core', 'tfjs-backend-cpu', 'tfjs-tflite', 'tfjs-converter',
+  'tfjs-backend-webgl', 'tfjs-backend-webgpu', 'tfjs-layers', 'tfjs-data'
+]);
diff --git a/scripts/cloudbuild_e2e_expected.yml b/scripts/cloudbuild_e2e_expected.yml
@@ -62,23 +62,6 @@ steps:
     waitFor:
       - bazel-tests
       - yarn-link-package-core
-  - name: gcr.io/learnjs-174218/release
-    dir: tfjs-converter
-    entrypoint: yarn
-    id: yarn-tfjs-converter
-    args:
-      - install
-    waitFor:
-      - yarn-common
-      - yarn-link-package
-  - name: gcr.io/learnjs-174218/release
-    id: create-pips-tfjs-converter
-    entrypoint: bash
-    args:
-      - ./tfjs-converter/scripts/create_python_pips.sh
-    waitFor:
-      - yarn-common
-      - yarn-link-package
   - name: gcr.io/learnjs-174218/release
     dir: tfjs-backend-wasm
     entrypoint: yarn
@@ -117,8 +100,6 @@ steps:
     waitFor:
       - yarn-common
       - yarn-link-package
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
   - name: gcr.io/learnjs-174218/release
     dir: tfjs
     entrypoint: yarn
@@ -129,8 +110,6 @@ steps:
       - yarn-tfjs
       - yarn-common
       - yarn-link-package
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
   - name: gcr.io/learnjs-174218/release
     dir: tfjs
     entrypoint: yarn
@@ -141,8 +120,6 @@ steps:
       - yarn-tfjs
       - yarn-common
       - yarn-link-package
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
   - name: gcr.io/learnjs-174218/release
     dir: tfjs-node
     entrypoint: yarn
@@ -218,8 +195,6 @@ steps:
       - yarn-tfjs
       - build-tfjs
       - lint-tfjs
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
       - yarn-tfjs-node
       - build-addon-tfjs-node
       - build-tfjs-node
@@ -246,8 +221,6 @@ steps:
       - yarn-tfjs
       - build-tfjs
       - lint-tfjs
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
       - yarn-tfjs-node
       - build-addon-tfjs-node
       - build-tfjs-node

diff --git a/scripts/cloudbuild_tfjs_node_expected.yml b/scripts/cloudbuild_tfjs_node_expected.yml
@@ -62,23 +62,6 @@ steps:
     waitFor:
       - bazel-tests
       - yarn-link-package-core
-  - name: gcr.io/learnjs-174218/release
-    dir: tfjs-converter
-    entrypoint: yarn
-    id: yarn-tfjs-converter
-    args:
-      - install
-    waitFor:
-      - yarn-common
-      - yarn-link-package
-  - name: gcr.io/learnjs-174218/release
-    id: create-pips-tfjs-converter
-    entrypoint: bash
-    args:
-      - ./tfjs-converter/scripts/create_python_pips.sh
-    waitFor:
-      - yarn-common
-      - yarn-link-package
   - name: gcr.io/learnjs-174218/release
     dir: tfjs-backend-wasm
     entrypoint: yarn
@@ -117,8 +100,6 @@ steps:
     waitFor:
       - yarn-common
       - yarn-link-package
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
   - name: gcr.io/learnjs-174218/release
     dir: tfjs
     entrypoint: yarn
@@ -129,8 +110,6 @@ steps:
       - yarn-tfjs
       - yarn-common
       - yarn-link-package
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
   - name: gcr.io/learnjs-174218/release
     dir: tfjs
     entrypoint: yarn
@@ -141,8 +120,6 @@ steps:
       - yarn-tfjs
       - yarn-common
       - yarn-link-package
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
   - name: gcr.io/learnjs-174218/release
     dir: tfjs-node
     entrypoint: yarn
@@ -232,8 +209,6 @@ steps:
       - yarn-tfjs
       - build-tfjs
       - lint-tfjs
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
       - yarn-tfjs-node
       - build-addon-tfjs-node
       - build-tfjs-node
@@ -260,8 +235,6 @@ steps:
       - yarn-tfjs
       - build-tfjs
       - lint-tfjs
-      - yarn-tfjs-converter
-      - create-pips-tfjs-converter
       - yarn-tfjs-node
       - build-addon-tfjs-node
       - build-tfjs-node

diff --git a/scripts/generate_cloudbuild.js → scripts/generate_cloudbuild.ts b/scripts/generate_cloudbuild.js → scripts/generate_cloudbuild.ts
@@ -1,4 +1,3 @@
-
 // Copyright 2020 Google LLC. All Rights Reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -14,13 +13,16 @@
 // limitations under the License.
 // =============================================================================
 
-const yaml = require('js-yaml');
-const fs = require('fs');
-const path = require('path');
-const {printTable} = require('console-table-printer');
+import {printTable} from 'console-table-printer';
+import * as fs from 'fs';
+import * as yaml from 'js-yaml';
+import * as path from 'path';
+
+import {BAZEL_PACKAGES} from './bazel_packages';
 
-const DEPENDENCY_GRAPH =
-    JSON.parse(fs.readFileSync('scripts/package_dependencies.json'));
+
+const DEPENDENCY_GRAPH = JSON.parse(
+  fs.readFileSync(path.join(__dirname, 'package_dependencies.json'), 'utf8'));
 
 // This is a reverse dependencies graph. Each entry in the graph lists the
 // packages that depend on it.
@@ -33,10 +35,14 @@ const DEPENDENCY_ORDER = topologicalSort(DEPENDENCY_GRAPH);
 // Steps to exclude from cloudbuild files.
 const EXCLUDE_STEPS = new Set(['build-deps', 'yarn-common']);
 
+type Graph<V extends Iterable<string> = Iterable<string>> = {
+  [node: string]: V
+}
+
 /**
  * Verify that an object is a valid graph.
  */
-function verifyGraph(graph) {
+function verifyGraph(graph: Graph) {
   const nodes = new Set(Object.keys(graph));
   for (const [node, edges] of Object.entries(graph)) {
     for (const edge of edges) {
@@ -51,9 +57,9 @@ function verifyGraph(graph) {
 /**
  * Transpose a directed graph i.e. reverse the direction of the edges.
  */
-function transposeGraph(graph) {
+function transposeGraph(graph: Graph) {
   verifyGraph(graph);
-  const transposed = {};
+  const transposed: Graph<Set<string>> = {};
   for (const [nodeName, connectedNodes] of Object.entries(graph)) {
     for (const connectedNode of connectedNodes) {
       if (!transposed[connectedNode]) {
@@ -75,12 +81,12 @@ function transposeGraph(graph) {
  * Returns a list of graph nodes such that, by following edges,
  * you can only move forward in the list, not backward.
  */
-function topologicalSort(graph) {
+function topologicalSort(graph: Graph) {
   // We can't use a standard sorting algorithm because
   // often, two packages won't have any dependency relationship
   // between each other, meaning they are incomparable.
   verifyGraph(graph);
-  const sorted = [];
+  const sorted: string[] = [];
 
   while (sorted.length < Object.keys(graph).length) {
     // Find nodes not yet in 'sorted' that have edges
@@ -104,7 +110,7 @@ function topologicalSort(graph) {
       throw new Error('Dependency graph has a cycle.');
     }
 
-    for (node of emptyNodes) {
+    for (let node of emptyNodes) {
       sorted.push(node);
     }
   }
@@ -115,7 +121,7 @@ function topologicalSort(graph) {
  * Find all subnodes in the subgraph generated by taking the transitive
  * closure at `node`.
  */
-function findSubgraph(node, graph, subnodes = new Set()) {
+function findSubgraph(node: string, graph: Graph, subnodes = new Set()) {
   const directSubnodes = graph[node];
   if (directSubnodes) {
     for (const directSubnode of directSubnodes) {
@@ -132,7 +138,7 @@ function findSubgraph(node, graph, subnodes = new Set()) {
 /**
  * Find the transitive closure of dependencies of the given packages.
  */
-function findDeps(packages) {
+function findDeps(packages: Iterable<string>) {
   return new Set(
       [...packages]
           .map(packageName => findSubgraph(packageName, DEPENDENCY_GRAPH))
@@ -144,20 +150,39 @@ function findDeps(packages) {
  * set of packages that include at least one of the given packages in
  * their transitive closure of dependencies.
  */
-function findReverseDeps(packages) {
+function findReverseDeps(packages: Iterable<string>) {
   return new Set([
     ...packages
   ].map(packageName => findSubgraph(packageName, REVERSE_DEPENDENCY_GRAPH))
                      .reduce((a, b) => [...a, ...b], []));
 }
 
+interface CloudbuildStep {
+  name: string,
+  id: string,
+  waitFor?: string[],
+  secretEnv?: string[];
+}
+
+interface CloudbuildSecret {
+  kmsKeyName: string,
+  secretEnv: {
+    [index: string]: string,
+  }
+}
+
+export interface CloudbuildYaml {
+  steps: CloudbuildStep[],
+  secrets: CloudbuildSecret[],
+}
+
 /**
  * Construct a cloudbuild.yml file that does the following:
  * 1. Builds all the dependencies of `packages`
  * 2. Builds and tests all the packages in `packages`
  * 3. Builds and tests all the reverse dependnecies of `packages`
  */
-function generateCloudbuild(packages, print = true) {
+export function generateCloudbuild(packages: Iterable<string>, print = true) {
   // Make sure all packages are declared in package_dependencies.json.
   const allPackages = new Set(Object.keys(DEPENDENCY_GRAPH));
   for (const packageName of packages) {
@@ -182,21 +207,28 @@ function generateCloudbuild(packages, print = true) {
     // Log what will be built and tested
     const buildTestTable = [];
     for (const packageName of allPackages) {
+      const bazel = BAZEL_PACKAGES.has(packageName);
+      const bazelStr = 'bazel      '; // Spaces for left alignment
       buildTestTable.push({
         'Package': packageName,
-        'Will Build': toBuild.has(packageName) ? '✔' : '',
-        'Will Test': toTest.has(packageName) ? '✔' : ''
+        'Will Build': bazel ? bazelStr : toBuild.has(packageName) ? '✔' : '',
+        'Will Test': bazel ? bazelStr : toTest.has(packageName) ? '✔' : '',
       });
     }
     printTable(buildTestTable);
   }
 
   // Load all the cloudbuild files for the packages
   // that need to be built or tested.
-  const packageCloudbuildSteps = new Map();
+  const packageCloudbuildSteps = new Map<string, Set<CloudbuildStep>>();
   for (const packageName of new Set([...toBuild, ...toTest])) {
-    const doc = yaml.safeLoad(
-        fs.readFileSync(path.join(packageName, 'cloudbuild.yml')));
+    if (BAZEL_PACKAGES.has(packageName)) {
+      // Do not build or test Bazel packages. The bazel-tests step does this.
+      continue;
+    }
+    const doc = yaml.load(
+      fs.readFileSync(path.join(__dirname, '../', packageName,
+                                'cloudbuild.yml'), 'utf8')) as CloudbuildYaml;
     packageCloudbuildSteps.set(packageName, new Set(doc.steps));
   }
 
@@ -257,11 +289,12 @@ function generateCloudbuild(packages, print = true) {
   }
 
   // Load the general cloudbuild config
-  baseCloudbuild =
-      yaml.safeLoad(fs.readFileSync('scripts/cloudbuild_general_config.yml'));
+  const baseCloudbuild =
+    yaml.load(fs.readFileSync(path.join(
+      __dirname, 'cloudbuild_general_config.yml'), 'utf8')) as CloudbuildYaml;
 
   // Include yarn-common as the first step.
-  const steps = [...baseCloudbuild.steps];
+  const steps = [...baseCloudbuild.steps] as CloudbuildStep[];
 
   // Arrange steps in dependency order
   for (const packageName of DEPENDENCY_ORDER) {
@@ -294,12 +327,10 @@ function generateCloudbuild(packages, print = true) {
   return baseCloudbuild;
 }
 
-function isTestStep(id) {
+function isTestStep(id: string) {
   return id.includes('test');
 }
 
-function makeStepId(id, packageName) {
+function makeStepId(id: string, packageName: string) {
   return `${id}-${packageName}`;
 }
-
-exports.generateCloudbuild = generateCloudbuild;
diff --git a/scripts/generate_cloudbuild_for_packages.js → scripts/generate_cloudbuild_for_packages.ts b/scripts/generate_cloudbuild_for_packages.js → scripts/generate_cloudbuild_for_packages.ts
@@ -18,7 +18,7 @@
 
 const {findPackagesWithDiff, allPackages} =
     require('./find_packages_with_diff.js');
-const {generateCloudbuild} = require('./generate_cloudbuild.js');
+const {generateCloudbuild} = require('./generate_cloudbuild');
 const {ArgumentParser} = require('argparse');
 const yaml = require('js-yaml');
 const fs = require('fs');