Move cpu backend out of tfjs-core #3008

tafsiri · 2020-04-01T17:12:26Z

Moves the CPU backend out of tfjs-core into its own package/top level folder.

tfjs-backend-cpu is a devDependency of core to enable tests that depend on CPU specifically or more generally on having some backend present. This does create a circular dep between packages with respect to test running. But there are some workarounds to make the two libs build (using require over import in some places).
tfjs-backend-cpu is a now a devDependency of most of the other packages as they relied on CPU backend for running tests.
This PR allows re-registration of a kernel for a given backend (overwriting the prev kernel config). This is because WebGL backend is still in core and registers kernels as a side effect and core is a dependency of tfjs-backend-cpu. Thus in tests some double registration happens. Once WebGL is out of core the old behaviour could be restored.
As far as dependency structure for testing, this PR tries to mirror the status quo. We can tune stuff around when tests get called for what in future.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

This change is

annxingyuan · 2020-04-06T17:54:23Z

tfjs-backend-cpu/src/backend_cpu.ts

@@ -3795,5 +3794,3 @@ export class MathBackendCPU extends KernelBackend {
    return buffer.toTensor().reshape(shape);
  }
 }
-
-ENGINE.registerBackend('cpu', () => new MathBackendCPU(), 1 /* priority */);


Currently I think the WebGL backend assumes there's a registered CPU backend for CPU forwarding, something to be keep in mind when we pull out WebGL.

shouldExecuteOnCPU does check that a CPU backend is present (via getCPUBackend), if not it falls back to the WebGL implementation. So I think that code path is covered.

annxingyuan · 2020-04-06T19:35:04Z

tfjs-core/src/index.ts

  webgl,
  tensor_util,
  slice_util,
  gather_util,
-  scatter_util
+  scatter_util,


Curious why you've changed it to export these explicitly rather than through backend_util?

No particular reason other than not noticing the export *s in backend_util. I was following the pattern of tensor_util, slice_util and gather_util (which was maybe wrong if they are not just for backends?).

I could move the ones that are only used by backends to be exported by backend util. I don't have a strong preference for the pattern used.

nsthorat

Reviewed 45 of 52 files at r1, 7 of 10 files at r2, 3 of 3 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, @nsthorat, and @tafsiri)

tfjs-backend-cpu/karma.conf.js, line 34 at r3 (raw file):

};

use only 1 blank line between these top-level scopes

tfjs-backend-cpu/package.json, line 39 at r3 (raw file):

    "rollup-plugin-terser": "~5.3.0",
    "rollup-plugin-visualizer": "~3.3.2",
    "shelljs": "~0.8.3",

i think you can remove this

tfjs-backend-cpu/scripts/test-ci.sh, line 26 at r3 (raw file):

  # Run the first karma separately so it can download the BrowserStack binary
  # without conflicting with others.
  yarn run-browserstack --browsers=bs_safari_mac,bs_ios_11

can we just use what we do here so this CL doesn't affect test coverage (all the other lines are restricted to webgl)?

https://github.com/tensorflow/tfjs/blob/master/tfjs-core/scripts/test-ci.sh#L32-L33

tfjs-converter/cloudbuild.yml, line 9 at r3 (raw file):

  args: ['install']

# Install tfjs-converter dependencies

how come this didnt fail before haha?

tfjs-core/src/global_util.ts, line 22 at r3 (raw file):

// resolved.
// tslint:disable-next-line:no-any
let globalNameSpace: {_tfGlobals: Map<string, any>};

how come you have to make this change in this PR? this code is really delicate and completely untested (this is all about 2 copies of tfjs being on the page which is really hard to test without doing it manually). you probably have a good reason, but just make sure we can verify that 2 copies of tfjs on a page doesn't break :)

tfjs-core/src/index.ts, line 116 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

No particular reason other than not noticing the export *s in backend_util. I was following the pattern of tensor_util, slice_util and gather_util (which was maybe wrong if they are not just for backends?).

I could move the ones that are only used by backends to be exported by backend util. I don't have a strong preference for the pattern used.

+1 to keep it in backend_util just so it doesn't pollute the top-level tf. (which you get a dropdown for in vscode) and is only used by backends

tfjs-core/src/index.ts, line 127 at r3 (raw file):

  erf_util,
  conv_util,
  log_util,

remove trailing comma

tfjs-core/src/index.ts, line 138 at r3 (raw file):

setOpHandler(ops);

// Export all kernel names / info

nit period

tfjs-core/src/kernel_registry.ts, line 140 at r3 (raw file):

        `The kernel '${kernelName}' for backend ` +
        `'${backendName}' is already registered`);
    // TODO(yassogba) make this an error again once WebGL is moved from core.

for my own understand why does this PR break?

tfjs-core/src/kernel_registry_test.ts, line 59 at r3 (raw file):

  // tslint:disable-next-line: ban
  xit('errors when registering the same kernel twice', () => {
    // TODO(yassogba) restore this once WebGL backend is out of core.

let's refactor this test so it's not dependent on the cpu backend (and for anything else like this)

you can make a new TestBackend here, register it, set it, make sure double kernel registry fails

tfjs-core/src/test_async_backends.ts, line 25 at r3 (raw file):

// tslint:disable-next-line: no-require-imports
require('@tensorflow/tfjs-backend-cpu');

can you add a comment about require here too

tfjs-core/src/test_async_backends.ts, line 45 at r3 (raw file):

// notation.
// tslint:disable-next-line:no-any
// const backend: any = new MathBackendCPU();

remove this and the tslint line above

tfjs-core/src/test_async_backends.ts, line 56 at r3 (raw file):

          `constrain your test with SYNC_BACKEND_ENVS`);
    }
    //@ts-ignore;

curious what this ts-ignore is for

tfjs-core/src/backends/webgl/kernels/Transpose_impl.ts, line 34 at r3 (raw file):

}

// todo(@yassogba) import this from cpu backend once that package is published.

nit TODO

also, it can depend on it via link:../ right? so it's not really when it's published it's when webgl moves out :)

sounds like we need a tracker for "things when webgl moves out" :)

tfjs-backend-cpu/src/backend_cpu.ts, line 18 at r3 (raw file):

 */

import * as tf from '@tensorflow/tfjs-core';

can we import exactly what we need from this? I noticed tf.real, which afaik is not amenable to tree shaking (correct me if this is wrong)

tfjs-backend-cpu/src/backend_cpu.ts, line 22 at r3 (raw file):

import {array_ops_util, axis_util, backend_util, broadcast_util, complex_util, concat_util, conv_util, erf_util, fused_util, gather_nd_util, log_util, scatter_nd_util, selu_util, slice_util, util} from '@tensorflow/tfjs-core';
import {BackendTimingInfo, DataStorage, DataType, DataValues, KernelBackend, NumericDataType, Rank, Scalar, ShapeMap, Tensor, Tensor1D, Tensor2D, Tensor3D, Tensor4D, Tensor5D, TensorBuffer, TypedArray, upcastType} from '@tensorflow/tfjs-core';
import {kernel_impls} from '@tensorflow/tfjs-core';

qq for my own understanding, does kernel_impls get tree shaken with this type of import?

tfjs-backend-cpu/src/backend_cpu_test.ts, line 24 at r3 (raw file):

import {describeWithFlags} from '@tensorflow/tfjs-core/dist/jasmine_util';
// tslint:disable-next-line: no-imports-from-dist
import {expectArraysClose, expectArraysEqual} from '@tensorflow/tfjs-core/dist/test_util';

test_util is part of the public API

tfjs-backend-cpu/src/backend_cpu_test_envs.ts, line 22 at r3 (raw file):

export const CPU_ENVS: Constraints = {
  predicate: testEnv => testEnv.backendName === 'cpu'

This is guaranteed now because these tests are only run from where you generate the environments, you can remove this constraint entirely and move everything to "ALL_ENVS"

nsthorat · 2020-04-07T00:54:06Z

Amazing work on this Yannick!

annxingyuan

Just left one question about exports in tfjs-core/src/index.ts!

dsmilkov

Great work Yannick!!

Reviewed 6 of 52 files at r1, 2 of 3 files at r3.
Reviewable status: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, and @tafsiri)

tfjs-backend-cpu/karma.conf.js, line 27 at r3 (raw file):

    // Ignore the import of the `worker_threads` package used in a core test
    // meant to run in node.
    exclude: ['worker_threads'],

just confirming that these these tests still get exported by core, so we have to ignore them?

tfjs-backend-cpu/karma.conf.js, line 74 at r3 (raw file):

      // For browserstack configs see:
      // https://www.browserstack.com/automate/node
      bs_chrome_mac: {

something to think about given the limited BS resources. Since now this only tests CPU, we could reduce the range of devices for CPU and have a wider range for webgl

nsthorat

Reviewable status: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, and @tafsiri)

tfjs-backend-cpu/karma.conf.js, line 74 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

something to think about given the limited BS resources. Since now this only tests CPU, we could reduce the range of devices for CPU and have a wider range for webgl

Na is going to do this in a follow up. She was waiting for this PR (I talked to her yesterday)

dsmilkov

Reviewed 15 of 52 files at r1, 3 of 10 files at r2, 1 of 3 files at r3.
Reviewable status: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, and @tafsiri)

tfjs-core/package.json, line 59 at r3 (raw file):

    "build": "node ./scripts/enumerate-tests.js && tsc",
    "build-npm": "./scripts/build-npm.sh",
    "build-deps-ci": "yarn build-cpu-backend",

curious, who calls "build-deps-ci"? is that called from a central nightly script?

tfjs-core/src/index.ts, line 102 at r3 (raw file):

import * as erf_util from './ops/erf_util';
import * as conv_util from './ops/conv_util';
import * as log_util from './log';

can you check if all of these import * as name, export {name} get tree-shaken?

lina128

Reviewable status: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, @nsthorat, and @tafsiri)

tfjs-backend-cpu/scripts/test-ci.sh, line 26 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can we just use what we do here so this CL doesn't affect test coverage (all the other lines are restricted to webgl)?

https://github.com/tensorflow/tfjs/blob/master/tfjs-core/scripts/test-ci.sh#L32-L33

+1

lina128 · 2020-04-07T16:37:40Z

tfjs-backend-cpu/package.json

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


Also need build-core-ci, similar to this: https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/package.json#L44
Because Core's build-ci is slightly different than build: https://github.com/tensorflow/tfjs/blob/master/tfjs-core/package.json#L55-L56

lina128 · 2020-04-07T16:38:52Z

tfjs-backend-cpu/cloudbuild.yml

+  dir: 'tfjs-backend-cpu'
+  id: 'build-core'
+  entrypoint: 'yarn'
+  args: ['build-core']


build-core-ci

lina128 · 2020-04-07T16:43:40Z

tfjs-converter/cloudbuild.yml

@@ -6,13 +6,22 @@ steps:
  id: 'yarn-common'
  args: ['install']

+# Install tfjs-converter dependencies


Why do we need this? This is already included in yarn test-ci

lina128 · 2020-04-07T16:45:03Z

tfjs-converter/package.json

@@ -57,9 +58,10 @@
    "build": "yarn gen-json --test && tsc",
    "build-ci": "yarn gen-json --test && tsc",
    "build-core": "cd ../tfjs-core && yarn && yarn build",
+    "build-backend-cpu": "cd ../tfjs-backend-cpu && yarn && yarn build",


Add another one for ci, build-backend-cpu-ci, even if it's the same. This is for consistency with other packages. Suggested by Daniel.

lina128 · 2020-04-07T16:49:09Z

tfjs-backend-cpu/scripts/test-ci.sh

+set -e
+
+yarn lint
+yarn build


yarn
yarn build-deps-ci
yarn build-ci
yarn lint

Example: https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/scripts/test-ci.sh#L14-L15

lina128 · 2020-04-07T16:56:48Z

tfjs-backend-cpu/package.json

+    "publish-local": "rimraf dist/ && yarn build && rollup -c && yalc push",
+    "publish-npm": "npm publish",
+    "lint": "tslint -p . -t verbose",
+    "test": "karma start",


We need to make sure test always include yarn && yarn build-deps, because we no longer rely on npm to manage dependency, we have to ensure the dependencies are available through this. example: https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/package.json#L52

lina128 · 2020-04-07T17:09:13Z

tfjs-data/package.json

@@ -44,9 +45,10 @@
    "build-core": "cd ../tfjs-core && yarn && yarn build",
    "build-core-ci": "cd ../tfjs-core && yarn && yarn build-ci",
    "build-layers": "cd ../tfjs-layers && yarn && yarn build",
+    "build-backend-cpu": "cd ../tfjs-backend-cpu && yarn && yarn build",


yarn build-ci

lina128 · 2020-04-07T17:09:27Z

tfjs-data/package.json

-    "build-deps": "yarn build-core && yarn build-layers",
-    "build-deps-ci": "yarn build-core-ci && yarn build-layers-ci",
+    "build-deps": "yarn build-core && yarn build-layers && yarn build-backend-cpu",
+    "build-deps-ci": "yarn build-core-ci && yarn build-layers-ci && yarn build-backend-cpu",


yarn build-backend-cpu-ci

lina128 · 2020-04-07T17:09:46Z

tfjs-layers/package.json

@@ -41,9 +42,10 @@
    "build": "tsc",
    "build-ci": "tsc",
    "build-core": "cd ../tfjs-core && yarn && yarn build",
+    "build-backend-cpu": "cd ../tfjs-backend-cpu && yarn && yarn build",


yarn build-ci

lina128 · 2020-04-07T17:09:56Z

tfjs-layers/package.json

-    "build-deps": "yarn build-core",
-    "build-deps-ci": "yarn build-core-ci",
+    "build-deps": "yarn build-core && yarn build-backend-cpu",
+    "build-deps-ci": "yarn build-core-ci && yarn build-backend-cpu",


yarn build-backend-cpu-ci

lina128

Reviewable status: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, and @tafsiri)

tfjs-core/src/index.ts, line 102 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

can you check if all of these import * as name, export {name} get tree-shaken?

I was curious about that too, seems depending on bundler, here's a discussion: webpack/webpack#2713

tfjs-core/src/kernel_registry_test.ts, line 59 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

let's refactor this test so it's not dependent on the cpu backend (and for anything else like this)

you can make a new TestBackend here, register it, set it, make sure double kernel registry fails

+1. If we can remove the circular dependency, that will be great! Like the idea of having a TestBackend mock.

nsthorat

Reviewable status: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, and @tafsiri)

tfjs-backend-cpu/scripts/test-ci.sh, line 33 at r3 (raw file):

    "run-browserstack --browsers=bs_android_9" \
    "run-browserstack --browsers=bs_firefox_mac,bs_chrome_mac" \
    "run-browserstack --browsers=win_10_chrome" \

this isnt tested on CPU today

lina128 · 2020-04-07T19:45:41Z

Hi Yannick, you probably need to add build-backend-cpu to Node's build-deps too.

tafsiri · 2020-04-07T20:03:27Z

@lina128 Node shouldn't need to depend on backend-cpu to run its tests imo. It provides a backend that tests can be run against. If there are tests in core that specifically rely on CPU they should be constrained via a testEnv. With the tests passing earlier it looks like node does not depend on CPU (which is the expected behaviour imo)

tafsiri

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

tfjs-backend-cpu/cloudbuild.yml, line 21 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

build-core-ci

Done

tfjs-backend-cpu/karma.conf.js, line 34 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

use only 1 blank line between these top-level scopes

Done.

tfjs-backend-cpu/package.json, line 39 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

i think you can remove this

Done

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

Previously, lina128 (Na Li) wrote…

yarn
yarn build-deps-ci
yarn build-ci
yarn lint

Example: https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/scripts/test-ci.sh#L14-L15

In this case build-deps-ci should be done (and is done) in cloudbuild.yml, there is a cyclic dependency between core and cpu so we can't have the scripts call each other recursively.

updated yarn build to yarn build-ci

tfjs-backend-cpu/scripts/test-ci.sh, line 26 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

+1

I'm not sure I see how coverage has dropped. Here we do safari_mac, ios_11, android_9, firefox_mac, chrome_mac, win_10_chrome. Which seems to match what is in core (I adapted this from that file).

Am I missing something? Is it the flags? In this library we shouldn't need to set WebGL false as webgl should not be present.

Feel free to message me on chat if that would be faster.

tfjs-backend-cpu/scripts/test-ci.sh, line 33 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this isnt tested on CPU today

Ah okay, it wasn't coverage reduction. I've removed this. Though i think we probably should add this at some points as we streamline.

tfjs-converter/cloudbuild.yml, line 9 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

how come this didnt fail before haha?

I suspect that there are some dependencies between our cloudbuild.ymls that may end up building the necessary thing a later test may need. In this case since it was a new dependency nothing happened to build it. That is my suspicion.

tfjs-converter/cloudbuild.yml, line 9 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

Why do we need this? This is already included in yarn test-ci

I was getting error of package not found without this. I think there are other scripts (e.g. test-snippets) that needs the dependencies installed. I think relying on test-ci makes it more confusing when there are multiple targets that rely on this.

Another problem with relying on test-ci to install deps/build dependencies, is there can sometimes be race conditions between different targets and these are better captures in cloudbuild.yml. (consider build scripts that call rimraf dist.

tfjs-converter/package.json, line 61 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

Add another one for ci, build-backend-cpu-ci, even if it's the same. This is for consistency with other packages. Suggested by Daniel.

Done. FWIW I think this we should make the 'build-ci' pattern the exception rather than the rule.

tfjs-core/package.json, line 59 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

curious, who calls "build-deps-ci"? is that called from a central nightly script?

Good catch this should be called by build-ci. Updated.

tfjs-core/src/global_util.ts, line 22 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

how come you have to make this change in this PR? this code is really delicate and completely untested (this is all about 2 copies of tfjs being on the page which is really hard to test without doing it manually). you probably have a good reason, but just make sure we can verify that 2 copies of tfjs on a page doesn't break :)

I didn't change it for _tfEngine (other than moving a helper function to this file) see getOrMakeEngine in file above.

However I did introduce this to make kernelRegistry/gradRegistry global. Was running into the same problem that engine originally experienced with being module scoped and not truly global.

I do think we should move _tfEngine to use this function, but wasn't sure if we considered _tfEngine public at all so decided to introduce a more generic utility but leave the old code path.

tfjs-core/src/index.ts, line 116 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

+1 to keep it in backend_util just so it doesn't pollute the top-level tf. (which you get a dropdown for in vscode) and is only used by backends

Done for the ones I added. For the existing ones, if I move them will do that in a followup PR.

tfjs-core/src/index.ts, line 127 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

remove trailing comma

Done

tfjs-core/src/index.ts, line 138 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

nit period

Done

tfjs-core/src/kernel_registry.ts, line 140 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

for my own understand why does this PR break?

Because index.js (of core) gets imported twice, it will try and re-register kernels (because those are a side effect). Once webgl is out that side effect wont be there.

tfjs-core/src/kernel_registry_test.ts, line 59 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

+1. If we can remove the circular dependency, that will be great! Like the idea of having a TestBackend mock.

Done

Na: The circular dependency can't go away until all tests that involve tensors are skippable in core. Ultimately since we want things like gradient tests to run as part of changes to core I don't think we will be able to have real-backend free tests in core.

tfjs-core/src/test_async_backends.ts, line 25 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you add a comment about require here too

Done

tfjs-core/src/test_async_backends.ts, line 45 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

remove this and the tslint line above

Done

tfjs-core/src/test_async_backends.ts, line 56 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

curious what this ts-ignore is for

the backend[name] does not compile. It says it can't index into KernelBackend.

tfjs-core/src/backends/webgl/kernels/Transpose_impl.ts, line 34 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

nit TODO

also, it can depend on it via link:../ right? so it's not really when it's published it's when webgl moves out :)

sounds like we need a tracker for "things when webgl moves out" :)

Yep. Those two things are simultaneous in my mind. But yeah this should say "once this package is published".

Updated for clarity.

tfjs-data/package.json, line 48 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

yarn build-ci

Added build-backend-cpu-ci

tfjs-data/package.json, line 51 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

yarn build-backend-cpu-ci

Done

tfjs-layers/package.json, line 45 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

yarn build-ci

added build-backend-cpu-ci

tfjs-layers/package.json, line 48 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

yarn build-backend-cpu-ci

done

tfjs-backend-cpu/src/backend_cpu.ts, line 18 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can we import exactly what we need from this? I noticed tf.real, which afaik is not amenable to tree shaking (correct me if this is wrong)

Yes. But I would do that in a later change, since this is just geared to moving stuff out.

Imports like this are somewhat amenable to tree-shaking (it depends on what is in that import). I'd rather do stuff like that once webgl is out and we can test against bundling actual programs in a more systematic manner.

tfjs-backend-cpu/src/backend_cpu.ts, line 22 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

qq for my own understanding, does kernel_impls get tree shaken with this type of import?

Yep it can be (since that file exports just pure functions). As above down the line we may have to make adjustments based on how well our different bundlers behave, but those should all be internal/non-breaking changes.

tfjs-backend-cpu/src/backend_cpu_test.ts, line 24 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

test_util is part of the public API

Fixed.

Quick q. Why don't we export jasmine_util? (for example under the test_util namespace or something).

tfjs-backend-cpu/src/backend_cpu_test_envs.ts, line 22 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

This is guaranteed now because these tests are only run from where you generate the environments, you can remove this constraint entirely and move everything to "ALL_ENVS"

Good catch. My understanding of this comment is that we don't need this because it was only used in this package, and thus is equivalent to no constraint.

tafsiri

Thanks for the review. This is updated (synced with master) and ready for re-review if you want to make sure I've captured your comments. I'll leave this open for a bit as a quick once over would be useful.

Reviewed 12 of 17 files at r4, 9 of 12 files at r6.
Reviewable status: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, and @nsthorat)

tfjs-backend-cpu/karma.conf.js, line 27 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

just confirming that these these tests still get exported by core, so we have to ignore them?

Yep!

tfjs-backend-cpu/package.json, line 49 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

Also need build-core-ci, similar to this: https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/package.json#L44
Because Core's build-ci is slightly different than build: https://github.com/tensorflow/tfjs/blob/master/tfjs-core/package.json#L55-L56

Done.

tfjs-backend-cpu/package.json, line 55 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

We need to make sure test always include yarn && yarn build-deps, because we no longer rely on npm to manage dependency, we have to ensure the dependencies are available through this. example: https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/package.json#L52

Done. Using build-core directly as the relationship between this backend and core should be made as explicit as possible.

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

Previously, tafsiri (Yannick Assogba) wrote…

In this case build-deps-ci should be done (and is done) in cloudbuild.yml, there is a cyclic dependency between core and cpu so we can't have the scripts call each other recursively.

updated yarn build to yarn build-ci

@lina128 just highlighting this comment since it's quite different from other packages.

tfjs-backend-cpu/scripts/test-ci.sh, line 26 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

I'm not sure I see how coverage has dropped. Here we do safari_mac, ios_11, android_9, firefox_mac, chrome_mac, win_10_chrome. Which seems to match what is in core (I adapted this from that file).

Am I missing something? Is it the flags? In this library we shouldn't need to set WebGL false as webgl should not be present.

Feel free to message me on chat if that would be faster.

Done (below)

tfjs-core/src/index.ts, line 102 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

I was curious about that too, seems depending on bundler, here's a discussion: webpack/webpack#2713

Thats a pretty old issue. Generally my earlier experiments found these can be tree shaken modulo the presence of side effects. More targetted testing will come as we get closer to focusing on low level tree shakeability. If we end up needing to make big changes to support a particular bundler, the 3.x release train can incorporate that work.

lina128

Reviewable status: complete! 3 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, @nsthorat, and @tafsiri)

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

Previously, tafsiri (Yannick Assogba) wrote…

@lina128 just highlighting this comment since it's quite different from other packages.

We had to keep build in test-ci because previously integration tests also use test-ci, which is unaware of cloudbuild.yml. See PR description here: #2913

Now we remove that integration, we can put steps back to cloudbuild.yml.

Thanks for pointing this out, I will look into refactor this after the PR is merged.

tfjs-converter/cloudbuild.yml, line 9 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

I was getting error of package not found without this. I think there are other scripts (e.g. test-snippets) that needs the dependencies installed. I think relying on test-ci makes it more confusing when there are multiple targets that rely on this.

Another problem with relying on test-ci to install deps/build dependencies, is there can sometimes be race conditions between different targets and these are better captures in cloudbuild.yml. (consider build scripts that call rimraf dist.

Since you changed to build-ci, which includes build snippets. I think this issue should go away. But no worries, I can look into it after this PR is merged.

lina128

LGTM!

tafsiri added 4 commits March 30, 2020 12:18

move the CPU backend to a top level package

96c2eb4

add test runner to backend cpu

5f82f28

small refactor

ee82ec2

add backend cpu to cloudbuild

10e01b5

googlebot added the cla: yes label Apr 1, 2020

tafsiri added 25 commits April 1, 2020 13:25

fix file perms

549f151

fix test-snippets

1423b74

add secrets

9616a99

save

94f068a

save

cd1f2ce

cleanup

6cb5546

disable cpu testEnv in nightly of core.

57cff9d

add cpu backend as dev dep of converter

1c56c99

save

136be4a

save

2f9cfe6

save

5adc5c1

exclude web worker test from backend test suite

18dc1ca

converter cloudbuild, run yarn install

8801964

add backend-cpu as devDep in data and layers

e0d78c6

typo

0a8f8a8

typo

843acc6

fix

d0864c6

fix build deps

4631eeb

save

102ef4b

save

50a1f59

typo

d70fd2e

save

77388f8

save

7f541a9

make sure cpu backend is built in ci

3bc1bc1

restore engine test

a21d48f

annxingyuan reviewed Apr 6, 2020

View reviewed changes

nsthorat approved these changes Apr 7, 2020

View reviewed changes

annxingyuan approved these changes Apr 7, 2020

View reviewed changes

dsmilkov approved these changes Apr 7, 2020

View reviewed changes

nsthorat approved these changes Apr 7, 2020

View reviewed changes

dsmilkov approved these changes Apr 7, 2020

View reviewed changes

lina128 reviewed Apr 7, 2020

View reviewed changes

nsthorat approved these changes Apr 7, 2020

View reviewed changes

tafsiri added 2 commits April 7, 2020 15:32

code review fixes

ba1b18f

save

7a81d4b

save

d887b8d

tafsiri commented Apr 7, 2020

View reviewed changes

tafsiri added 4 commits April 7, 2020 16:32

Merge branch 'master' into move-cpu-backend

9ead647

fix imports

31350ee

move pool utils

6e6834a

update scripts

a4b5e41

tafsiri commented Apr 7, 2020

View reviewed changes

lina128 requested changes Apr 7, 2020

View reviewed changes

lina128 approved these changes Apr 7, 2020

View reviewed changes

tafsiri added 2 commits April 8, 2020 10:10

Merge branch 'master' into move-cpu-backend

21b2c4f

kick build

0ad2ae2

tafsiri changed the title ~~Move cpu backend~~ Move cpu backend out of tfjs-core Apr 8, 2020

tafsiri merged commit b72893a into master Apr 8, 2020

tafsiri deleted the move-cpu-backend branch April 8, 2020 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move cpu backend out of tfjs-core #3008

Move cpu backend out of tfjs-core #3008

tafsiri commented Apr 1, 2020 •

edited

Loading

annxingyuan Apr 6, 2020

tafsiri Apr 6, 2020

annxingyuan Apr 6, 2020

tafsiri Apr 6, 2020

nsthorat left a comment

nsthorat commented Apr 7, 2020

annxingyuan left a comment

dsmilkov left a comment

nsthorat left a comment

dsmilkov left a comment

lina128 left a comment

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 Apr 7, 2020

lina128 left a comment

nsthorat left a comment

lina128 commented Apr 7, 2020

tafsiri commented Apr 7, 2020

tafsiri left a comment

tafsiri left a comment

lina128 left a comment

lina128 left a comment

Move cpu backend out of tfjs-core #3008

Move cpu backend out of tfjs-core #3008

Conversation

tafsiri commented Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsthorat left a comment

Choose a reason for hiding this comment

nsthorat commented Apr 7, 2020

annxingyuan left a comment

Choose a reason for hiding this comment

dsmilkov left a comment

Choose a reason for hiding this comment

nsthorat left a comment

Choose a reason for hiding this comment

dsmilkov left a comment

Choose a reason for hiding this comment

lina128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lina128 left a comment

Choose a reason for hiding this comment

nsthorat left a comment

Choose a reason for hiding this comment

lina128 commented Apr 7, 2020

tafsiri commented Apr 7, 2020

tafsiri left a comment

Choose a reason for hiding this comment

tafsiri left a comment

Choose a reason for hiding this comment

lina128 left a comment

Choose a reason for hiding this comment

lina128 left a comment

Choose a reason for hiding this comment

tafsiri commented Apr 1, 2020 •

edited

Loading