Conversation
MISC This PR is a step towards modularization of the library so webgl and cpu backends can live in a directory that can just be dropped entirely. The goal with this PR is to break no existing API (including things like `tf.ENV.set`, `tf.ENV.global`). Unfortunately I can't really break this change up. High-level changes: - `ENGINE` is now the global singleton root object. This is the object that is now stuck on `window` or `process` in Node (it used to be `ENV`). - `ENGINE` owns all backend registration. - `ENV` (instance of `Environment`) lives as an instance on `ENGINE` so it is also a global singleton. Its sole purpose is flags and holding `global` (`window` in browser or `process` in Node). - Environment flags are now set up with a registry on `ENV`, which allows `Environment` to have 0 dependencies. This means code that evaluates an environment flag will not pull in the world. - Unit tests now rely on the normal library backend registration (instead of having custom registries for unit tests). - When a page loads the flow is: Engine creation, Environment creation, setting flags from the URL, Flag registration, Backend registration, and lazily backend initialization when the backend is asked for. New API: - `tf.backend()` gets the current backend instance. Not to be confused with `tf.getBackend()` which returns the current backend string (don't want to break this API). Removed flags: - `BACKEND`: Flags are cached which makes this flag invalid if you switch backends. You can get the current backend name with `tf.getBackend()`. - `EPSILON`: The value of this depends on the current backend. Since backends can switch, this flag can get stale. To get epsilon you should use `tf.backend().epsilon()`. - `TEST_EPSILON`: Similar reason as above. Now you can simply use `testEpsilon()`. Detailed changes: - When registering a backend the backend is not instantiated immediately. It is instantiated lazily when something needs the current backend or the user calls `tf.backend()`. This is because constructors of backends need environment flag evaluations, which need registration to happen so we must delay the initialization of the backend. This also means that we don't initialize possibly expensive backends that don't actually get used. - `ENGINE` never gets re-instantiated during unit tests, it just gets reset (which resets all the internal state). Between unit tests we never clear registries, just instantiation of those registries (for both flags as well as engine state). - `window.ENV` => `window._tfengine` as the global so we don't possibly collide with other libraries. - `backend_webgl` now lives in `webgl/` and `backend_cpu` lives in `cpu/`. The goal will be to drop those directories if we want to have a modular build. - Constraints of unit tests now take an explicit field for backend. Previously we relied on flags but we're removing anything backend related from flags. `CPU_ENVS` and `WEBGL_ENVS` tests were testing backend-specific things, and these tests will move into the respective `webgl/` and `cpu/` folders eventually. - Optimizer cleanup, removed scalars (this was because I removed 'EPSILON' and decided to just clean them up while there). - Remove safe mode. Nobody uses it. Linked against all packages, some changes required in other repos: tensorflow/tfjs-layers#512 tensorflow/tfjs-data#176 tensorflow/tfjs-node#238 Here is a union bundle with all changes from core as well as changes above: https://storage.googleapis.com/dljs-test/env-bundle/tf1.js And a codepen showing basic usage works with this bundle: https://codepen.io/anon/pen/bJbyPx Number of tests changes slightly because I'm moving tests around and deleting some. Verified all other packages don't have a change in number of tests that were run. Number of tests locally before (`yarn test`): 9639 of 9657 Number of tests locally after (`yarn test`): 9618 of 9636 More tests in node because some tests are moved to a regular describe where they are backend-agnostic. Number of tests locally before (`yarn test-node`): 3209 Number of tests locally after (`yarn test-node`): 3261
dsmilkov
left a comment
There was a problem hiding this comment.
Reviewed 3 of 5 files at r1, 1 of 1 files at r2, 6 of 6 files at r3.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @caisq, @dsmilkov, @nkreeger, and @nsthorat)
src/io/node_http_test.ts, line 24 at r3 (raw file):
// tslint:disable-next-line:no-require-imports const fetch = require('node-fetch');
Add a comment that you are using this just for making a response for a unit test
nsthorat
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @caisq and @nkreeger)
src/io/node_http_test.ts, line 24 at r3 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Add a comment that you are using this just for making a response for a unit test
Done
nkreeger
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @caisq and @nkreeger)
caisq
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 3 of 1 approvals obtained (waiting on @caisq and @nsthorat)
src/index.ts, line 51 at r4 (raw file):
success
success is not used anywhere else IIUC. Why not inline it in the line below?
src/nodejs_kernel_backend.ts, line 156 at r4 (raw file):
16|32
Can this magic number union be assigned to a named constant?
nsthorat
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 3 of 1 approvals obtained (waiting on @caisq)
src/index.ts, line 51 at r4 (raw file):
Previously, caisq (Shanqing Cai) wrote…
success
successis not used anywhere else IIUC. Why not inline it in the line below?
success is here just so it's clear :)
src/nodejs_kernel_backend.ts, line 156 at r4 (raw file):
Previously, caisq (Shanqing Cai) wrote…
16|32Can this magic number union be assigned to a named constant?
I did it this way to avoid yet another import / depdency (you'll get a compile error if you don't type this).
This change is