Skip to content
This repository was archived by the owner on Sep 17, 2022. It is now read-only.

Update Node to 1.1.0 and update to reflect changes from core.#238

Merged
nsthorat merged 9 commits intomasterfrom
env
Apr 19, 2019
Merged

Update Node to 1.1.0 and update to reflect changes from core.#238
nsthorat merged 9 commits intomasterfrom
env

Conversation

@nsthorat
Copy link

@nsthorat nsthorat commented Mar 30, 2019


This change is Reviewable

nsthorat pushed a commit to tensorflow/tfjs-core that referenced this pull request Apr 4, 2019
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
Nikhil Thorat added 2 commits April 17, 2019 15:38
@nsthorat nsthorat changed the title WIP: Update Node with new environment changes from core. Update Node to 1.1.0 and update to reflect changes from core. Apr 18, 2019
@nsthorat nsthorat requested review from caisq, dsmilkov and nkreeger April 18, 2019 14:50
Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r1, 1 of 1 files at r2, 6 of 6 files at r3.
Reviewable status: :shipit: 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

Copy link
Author

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: 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

Copy link
Contributor

@nkreeger nkreeger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @caisq and @nkreeger)

Copy link
Contributor

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: 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?

Copy link
Author

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 3 of 1 approvals obtained (waiting on @caisq)


src/index.ts, line 51 at r4 (raw file):

Previously, caisq (Shanqing Cai) wrote…
success

success is 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|32

Can 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).

@nsthorat nsthorat merged commit 2b77782 into master Apr 19, 2019
@nsthorat nsthorat deleted the env branch April 19, 2019 14:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants