Add IS_BROWSER to ENV; add scheme-based URL router to tf.io.* modules #1034

caisq · 2018-05-14T02:16:27Z

The currently supported schemes are:

localstorage:// for browser local storage
indexeddb:// for browser IndexedDB
downloads:// for browser file downloads
http:// and https:// fro browser HTTP requests

Since browserLocalStorage, browserIndexedDB and browserDownloads
are methods without any optional arguments and can be copmletely
replaced by the URL-like strings, they are removed from the tf.io.*
exports.

Also:

Export tf.io.getSaveHandler, tf.io.getLoadHandler
Export tf.io.registerSaveRouter and tf.io.registerLoadRouter

Fixes: tensorflow/tfjs#282
Towards: tensorflow/tfjs#13

This change is

The currently supported schemes are: - localstorage:// for browser local storage - indexeddb:// for browser IndexedDB - downloads:// for browser file downloads - http:// and https:// fro browser HTTP requests Since browserLocalStorage, browserIndexedDB and browserDownloads are methods without any optional arguments and can be copmletely replaced by the URL-like strings, they are removed from the tf.io.* exports. Also: - Export tf.io.getSaveHandler, tf.io.getLoadHandler - Export tf.io.registerSaveRouter and tf.io.registerLoadRouter

ericdnielsen · 2018-05-14T13:32:46Z

src/environment.ts

@@ -30,6 +30,8 @@ export enum Type {
 export interface Features {
  // Whether to enable debug mode.
  'DEBUG'?: boolean;
+  // Whether we are in a node.js (i.e., non-browser) environment.
+  'IS_NODE'?: boolean;


This seems a little worrisome to me. We have IS_NODE here and BACKEND below, shouldn't IS_NODE be derivable from BACKEND?

If this were a "List of features that current backend supports, then I would expect to not see the BACKEND token in this list, and IS_NODE might be ok.

ericdnielsen · 2018-05-14T13:34:36Z

src/environment.ts

@@ -302,6 +304,8 @@ export class Environment {
  private evaluateFeature<K extends keyof Features>(feature: K): Features[K] {
    if (feature === 'DEBUG') {
      return false;
+    } else if (feature === 'IS_NODE') {


Given this stanza it seems like it should be IS_BROWSER not IS_NODE. You're checking if the window object exists, not whether we authoritatively know that we're in node.

ericdnielsen · 2018-05-14T13:34:59Z

src/io/browser_files.ts

 // tslint:enable:max-line-length

 const DEFAULT_FILE_NAME_PREFIX = 'model';
 const DEFAULT_JSON_EXTENSION_NAME = '.json';
 const DEFAULT_WEIGHT_DATA_EXTENSION_NAME = '.weights.bin';

-class BrowserDownloads implements IOHandler {
+export class BrowserDownloads implements IOHandler {
  private readonly modelTopologyFileName: string;
  private readonly weightDataFileName: string;
  private readonly jsonAnchor: HTMLAnchorElement;
  private readonly weightDataAnchor: HTMLAnchorElement;

  constructor(fileNamePrefix?: string) {
    // TODO(cais): Use central environment flag when it's available.


remove this todo?

ericdnielsen · 2018-05-14T13:35:55Z

src/io/browser_files.ts

@@ -233,6 +240,20 @@ class BrowserFiles implements IOHandler {
  }
 }

+const URL_SCHEME = 'downloads://';


Can/should this be a property of the IOHandler?

ericdnielsen · 2018-05-14T13:36:51Z

src/io/browser_http.ts

@@ -100,6 +107,25 @@ class BrowserHTTPRequest implements IOHandler {
  //   See: https://github.com/tensorflow/tfjs/issues/290
 }

+const URL_SCHEMES = ['http://', 'https://'];


Property of the Handler?

ericdnielsen · 2018-05-14T13:37:32Z

src/io/indexed_db.ts

@@ -40,7 +44,7 @@ export async function deleteDatabase(): Promise<void> {
 function getIndexedDBFactory(): IDBFactory {
  // TODO(cais): Use central environment flag when it's available.


Remove this todo.

ericdnielsen · 2018-05-14T13:39:07Z

src/io/io.ts

 export {
-  browserDownloads,


I think we might still want to export these for symmetry purpose. I think its useful to the mental model to expose the non-sugared version that matches the pattern that the non-sugared handlers use.

ericdnielsen · 2018-05-14T13:40:31Z

src/io/router_registry.ts

+      }
+    }
+    if (validHandlers.length === 0) {
+      return null;


Why null? This feels like it should also be an error?

nsthorat · 2018-05-14T14:36:50Z

Review status: 0 of 12 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

src/environment.ts, line 34 at r1 (raw file):

Previously, ericdnielsen wrote…

This seems a little worrisome to me. We have IS_NODE here and BACKEND below, shouldn't IS_NODE be derivable from BACKEND?

If this were a "List of features that current backend supports, then I would expect to not see the BACKEND token in this list, and IS_NODE might be ok.

re point 1: It's not, they have different semantics. You could be in Node and use a JS CPU backend.

src/environment.ts, line 307 at r1 (raw file):

Previously, ericdnielsen wrote…

Given this stanza it seems like it should be IS_BROWSER not IS_NODE. You're checking if the window object exists, not whether we authoritatively know that we're in node.

+1 this is a good point. There are other headless settings (react native, etc)

src/io/browser_files.ts, line 277 at r1 (raw file):

 *
 * ```js
 * const saveResult = await model.save('downloads://mymodel');

maybe make downloads singular so this is in line with the other prefixes

src/io/browser_files_test.ts, line 107 at r1 (raw file):

  });

  it('Explicit file name prefix, with existing anchors', async done => {

dont use async here and below, async tests dont work

src/io/browser_http.ts, line 110 at r1 (raw file):

Previously, ericdnielsen wrote…

Property of the Handler?

+1

src/io/io.ts, line 28 at r1 (raw file):

export {
  browserDownloads,

what happened to these?

src/io/local_storage_test.ts, line 111 at r1 (raw file):

  it('Save artifacts succeeds', async done => {
    const testStartDate = new Date();
    const handler = tf.io.browserLocalStorage('FooModel');

In general I prefer importing the public facing API from unit tests (it helps you understand what the API looks like) -- can we do that here?

src/io/router_registry.ts, line 22 at r1 (raw file):

export type IORouter = (url: string) => IOHandler;

export class IORouterRegistry {

just for my understanding, we need this registry because we want the node IO handlers to not live in this repository?

src/io/router_registry.ts, line 23 at r1 (raw file):

export class IORouterRegistry {
  // Singletone instance.

Singleton

src/io/router_registry.ts, line 24 at r1 (raw file):

export class IORouterRegistry {
  // Singletone instance.
  private static instance: IORouterRegistry = null;

no need to initialize to null

src/io/router_registry.ts, line 88 at r1 (raw file):

      IOHandler {
    const validHandlers: IOHandler[] = [];
    for (const router of handlerType === 'load' ?

use forEach for arrays

src/io/router_registry_test.ts, line 48 at r1 (raw file):

    // tslint:disable:no-any
    tempRegistryInstance = (IORouterRegistry as any).instance;
    (IORouterRegistry as any).instance = null;

reaching into the private variable is a code smell, maybe expose a reset() method

Comments from Reviewable

caisq · 2018-05-14T17:35:35Z

Thanks for the review, Eric and Nikhil.

Review status: 0 of 12 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

src/environment.ts, line 34 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

re point 1: It's not, they have different semantics. You could be in Node and use a JS CPU backend.

+1 to Nikhil. So I'll leave it as is. But I'll change it from iS_NODE to IS_BROWSER. See below.

src/environment.ts, line 307 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

+1 this is a good point. There are other headless settings (react native, etc)

Replaced IS_NODE with IS_BROWSER. Thanks.

src/io/browser_files.ts, line 43 at r1 (raw file):

Previously, ericdnielsen wrote…

remove this todo?

Done.

src/io/browser_files.ts, line 243 at r1 (raw file):

Previously, ericdnielsen wrote…

Can/should this be a property of the IOHandler?

Done throughout indexed_db.ts, local_storage.ts, browser_files.ts and browser_http.ts.

src/io/browser_files.ts, line 277 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

maybe make downloads singular so this is in line with the other prefixes

I think plural makes more sense, because for both tf.Model and FrozenModel, this will end up being two or three files.

src/io/browser_files_test.ts, line 107 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

dont use async here and below, async tests dont work

Done.

src/io/browser_http.ts, line 110 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

+1

Done.

src/io/indexed_db.ts, line 45 at r1 (raw file):

Previously, ericdnielsen wrote…

Remove this todo.

Done.

src/io/io.ts, line 28 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

what happened to these?

See PR description. With these URL schemes, Model.save() and tf.loadModel() can work with only a string argument for browser downloads, local storage and indexedDB. So there is no point keeping these around. They don't have any optional fields, unlike the browserHTTPRequest being kept. I want to follow the principle of "there should be only one way to do it". If we add options to browserDownloads, browserLocalStorage in the future, we can add them back to the exports.

src/io/local_storage_test.ts, line 111 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

In general I prefer importing the public facing API from unit tests (it helps you understand what the API looks like) -- can we do that here?

Done using the router registry.

src/io/router_registry.ts, line 22 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

just for my understanding, we need this registry because we want the node IO handlers to not live in this repository?

Yes: it is for tfjs-node. But it can also be used by users to define custom URL schemes.

src/io/router_registry.ts, line 23 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Singleton

Done.

src/io/router_registry.ts, line 24 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

no need to initialize to null

Done.

src/io/router_registry.ts, line 97 at r1 (raw file):

Previously, ericdnielsen wrote…

Why null? This feels like it should also be an error?

I think returning null make it a little easier for clients (i.e., tfjs-layers in this case) to use. Clients can just test for null, instead of wrapping things in a try-catch block.

But actually coming to think about it, I think in this case it should just return an empty array. And in case there are >1 handlers available, return them all as an array. This was the API is cleaner: it always returns arrays and it delegates the responsibility of deciding what to do when there is 0, 1, or more handlers available to clients. Done.

src/io/router_registry_test.ts, line 48 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

reaching into the private variable is a code smell, maybe expose a reset() method

I considered that. But here we not only want to reset the registry, but also want to restore the state back to its original state once the tests are done. For doing that, doing it the way here feels better than adding two methods. This is just a test file.

Comments from Reviewable

nsthorat · 2018-05-14T20:02:50Z

Review status: 0 of 12 files reviewed at latest revision, 16 unresolved discussions.

src/environment.ts, line 33 at r2 (raw file):

  // Whether to enable debug mode.
  'DEBUG'?: boolean;
  // Whether we are in a node.js (i.e., non-browser) environment.

update this comment

Comments from Reviewable

caisq · 2018-05-14T20:08:10Z

Review status: 0 of 12 files reviewed at latest revision, 17 unresolved discussions, some commit checks pending.

src/environment.ts, line 33 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

update this comment

Done.

Comments from Reviewable

caisq added 2 commits May 13, 2018 22:13

Add doc strings to router_registry.ts

8e84f93

caisq requested review from nsthorat and dsmilkov May 14, 2018 02:31

Fix doc string for local_storage.ts

b94fcf6

caisq requested a review from ericdnielsen May 14, 2018 13:26

ericdnielsen reviewed May 14, 2018

View reviewed changes

response to review comments

dec828c

caisq added 2 commits May 14, 2018 13:36

Merge branch 'master' into iohandler-routers

8631433

fix linter error

2a2c43a

ericdnielsen approved these changes May 14, 2018

View reviewed changes

caisq changed the title ~~Add IS_NODE to ENV; add scheme-based URL router to tf.io.* modules~~ Add IS_BROWSER to ENV; add scheme-based URL router to tf.io.* modules May 14, 2018

Update doc string

c105753

caisq merged commit 1bfac1d into tensorflow:master May 14, 2018

caisq deleted the iohandler-routers branch May 14, 2018 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add IS_BROWSER to ENV; add scheme-based URL router to tf.io.* modules #1034

Add IS_BROWSER to ENV; add scheme-based URL router to tf.io.* modules #1034

Uh oh!

caisq commented May 14, 2018 •

edited

Loading

Uh oh!

ericdnielsen May 14, 2018

Uh oh!

ericdnielsen May 14, 2018

Uh oh!

ericdnielsen May 14, 2018

Uh oh!

ericdnielsen May 14, 2018

Uh oh!

ericdnielsen May 14, 2018

Uh oh!

ericdnielsen May 14, 2018

Uh oh!

ericdnielsen May 14, 2018

Uh oh!

ericdnielsen May 14, 2018

Uh oh!

nsthorat commented May 14, 2018

Uh oh!

caisq commented May 14, 2018

Uh oh!

nsthorat commented May 14, 2018

Uh oh!

caisq commented May 14, 2018

Uh oh!

Uh oh!

		@@ -40,7 +44,7 @@ export async function deleteDatabase(): Promise<void> {
		function getIndexedDBFactory(): IDBFactory {
		// TODO(cais): Use central environment flag when it's available.

Add IS_BROWSER to ENV; add scheme-based URL router to tf.io.* modules #1034

Add IS_BROWSER to ENV; add scheme-based URL router to tf.io.* modules #1034

Uh oh!

Conversation

caisq commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsthorat commented May 14, 2018

Uh oh!

caisq commented May 14, 2018

Uh oh!

nsthorat commented May 14, 2018

Uh oh!

caisq commented May 14, 2018

Uh oh!

Uh oh!

caisq commented May 14, 2018 •

edited

Loading