Add tf.io.browserIndexedDB

[caisq]

for saving model artifacts to browser IndexedDB and loading them.

Towards: tensorflow/tfjs#13

---

This change is 

[arunikayadav42]

Hello all I am e beginner in open source and have a basic knowledge of javascript and want to get started with contributing to this project!Can somebody please help me?I also know a bit of NLP and deep learning.

[caisq]

@arunikayadav42 Great to see you are interested in contributing. Maybe you can take a look at the list of our issues with the "help wanted" tag and see which ones interest you. https://github.com/tensorflow/tfjs/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22

[nsthorat]

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

---

src/io/indexed_db.ts, line 35 at r2 (raw file):

  return new Promise<void>((resolve, reject) => {
    const deleteRequest = idbFactory.deleteDatabase(DATABASE_NAME);
    deleteRequest.onsuccess = () => {

simplify these:

deleteRequest.onsuccess = () => resolve();

---

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

function getIndexedDBFactory(): IDBFactory {
  if (typeof window === 'undefined') {

Can you add an environment flag for the environment? Maybe called "IS_NODE" so we don't have these checks sprinkled everywhere?

Add in 3 places:
https://github.com/tensorflow/tfjs-core/blob/master/src/environment.ts#L30
https://github.com/tensorflow/tfjs-core/blob/master/src/environment.ts#L53
https://github.com/tensorflow/tfjs-core/blob/master/src/environment.ts#L324

---

src/io/indexed_db.ts, line 55 at r2 (raw file):

  // tslint:disable-next-line:no-any
  const theWindow: any = window;
  return theWindow.indexedDB || theWindow.mozIndexedDB ||

not all browsers support this, should we throw if it's null?

---

src/io/indexed_db.ts, line 83 at r2 (raw file):

    return new Promise<SaveResult>((resolve, reject) => {
      const modelArtifactsInfo: ModelArtifactsInfo = {

this seems to be pretty common -- maybe pull out into a util.

---

src/io/indexed_db.ts, line 96 at r2 (raw file):

      // Create the schema.
      openRequest.onupgradeneeded = () => {

openRequest.onupgradeneeded = () => this.setUpDatabase(openRequest);

---

src/io/indexed_db.ts, line 113 at r2 (raw file):

          reject(putRequest.error);
        };
        tx.oncomplete = () => {

tx.oncomplete = () => db.close();

follow this convention elsewhere where you can as well, you dont need the {} for single-line body callbacks.

---

src/io/indexed_db.ts, line 127 at r2 (raw file):

      const openRequest = this.indexedDB.open(DATABASE_NAME, DATABASE_VERSION);

      openRequest.onupgradeneeded = () => {

openRequest.onupgradeneeded = () => this.setUpDatabase(openRequest);

---

src/io/indexed_db.ts, line 138 at r2 (raw file):

        const getRequest = store.get(this.modelPath);
        getRequest.onsuccess = () => {
          if (getRequest.result === undefined) {

double equals null

---

src/io/indexed_db.ts, line 149 at r2 (raw file):

          reject(getRequest.error);
        };
        tx.oncomplete = () => {

tx.oncomplete = () => db.close();

---

src/io/indexed_db.ts, line 153 at r2 (raw file):

        };
      };
      openRequest.onerror = (error) => {

openRequest.onerror = error => reject(openRequest.error);

---

src/io/indexed_db.ts, line 166 at r2 (raw file):

/**
 * Factory function for browser IndexedDB IOHandler.

Same comment here: "Creates a browser IndexedDB IOHandler for saving models."

---

Comments from Reviewable

[bileschi]

Review status: 0 of 3 files reviewed at latest revision, 11 unresolved discussions.

---

src/io/indexed_db.ts, line 59 at r2 (raw file):

      theWindow.shimIndexedDB;
}

Please add something analogous to the local_storage comment here.
/**

• IOHandler subclass: Browser IndexedDB.
• See the doc string to browserIndexedDB for more details.
  */

---

src/io/indexed_db.ts, line 161 at r2 (raw file):

 db.createObjectStore(OBJECT_STORE_NAME, {keyPath: 'modelPath'});

This method can fail. It seems it's ok if it fails because the objectStore already exists, but some of the other reasons should maybe be caught?
https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase/createObjectStores

---

Comments from Reviewable

[caisq]

Review status: 0 of 6 files reviewed at latest revision, 13 unresolved discussions.

---

src/io/indexed_db.ts, line 35 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

simplify these:

deleteRequest.onsuccess = () => resolve();

Done.

---

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

Previously, nsthorat (Nikhil Thorat) wrote…

Can you add an environment flag for the environment? Maybe called "IS_NODE" so we don't have these checks sprinkled everywhere?

Add in 3 places:
https://github.com/tensorflow/tfjs-core/blob/master/src/environment.ts#L30
https://github.com/tensorflow/tfjs-core/blob/master/src/environment.ts#L53
https://github.com/tensorflow/tfjs-core/blob/master/src/environment.ts#L324

I don't feel I'm familiar with environment.ts enough to be able to do it correctly. But I agree we should deduplicate and centralize these checks. I added a TODO item to get that done in a later PR, maybe with your help. Let me know whether that's okay.

---

src/io/indexed_db.ts, line 55 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

not all browsers support this, should we throw if it's null?

Good point. Done.

---

src/io/indexed_db.ts, line 59 at r2 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…

Please add something analogous to the local_storage comment here.
/**

• IOHandler subclass: Browser IndexedDB.
• See the doc string to browserIndexedDB for more details.
  */

Done.

---

src/io/indexed_db.ts, line 83 at r2 (raw file):

common
Good point. Done.

---

src/io/indexed_db.ts, line 96 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

openRequest.onupgradeneeded = () => this.setUpDatabase(openRequest);

Done.

---

src/io/indexed_db.ts, line 113 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

tx.oncomplete = () => db.close();

follow this convention elsewhere where you can as well, you dont need the {} for single-line body callbacks.

Done. Simplified all places like this in this file.

---

src/io/indexed_db.ts, line 127 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

openRequest.onupgradeneeded = () => this.setUpDatabase(openRequest);

Done.

---

src/io/indexed_db.ts, line 138 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

double equals null

Done.

---

src/io/indexed_db.ts, line 149 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

tx.oncomplete = () => db.close();

Done.

---

src/io/indexed_db.ts, line 153 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

openRequest.onerror = error => reject(openRequest.error);

Done.

---

src/io/indexed_db.ts, line 161 at r2 (raw file):

fail

If this method fails, it should throw an error and the error will surface in the BrowserIndexedDB.save and .load calls.

---

src/io/indexed_db.ts, line 166 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Same comment here: "Creates a browser IndexedDB IOHandler for saving models."

Done. Added the word "loading" as the returned IOHandler also supports loading.

---

Comments from Reviewable

[pyu10055]

Review status: 0 of 6 files reviewed at latest revision, 13 unresolved discussions.

---

src/io/indexed_db.ts, line 153 at r2 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Done.

still has braces

---

src/io/indexed_db.ts, line 75 at r3 (raw file):

!modelPath

!modelPath also check modelPath === null

---

src/io/indexed_db.ts, line 94 at r3 (raw file):

          getModelArtifactsInfoForKerasJSON(modelArtifacts);

      const openRequest = this.indexedDB.open(DATABASE_NAME, DATABASE_VERSION);

the database code for save and load method are quite similar, maybe better to refactor to a wrapper class that
takes in transaction type and onsuccess function.

---

src/io/indexed_db.ts, line 122 at r3 (raw file):

readwrite

will read be enough?

---

src/io/indexed_db.ts, line 127 at r3 (raw file):

        const getRequest = store.get(this.modelPath);
        getRequest.onsuccess = () => {
          if (getRequest.result == null) {

use ===

---

src/io/indexed_db_test.ts, line 135 at r3 (raw file):

async

since you are using done, is the async still needed?

---

Comments from Reviewable

[caisq]

Review status: 0 of 6 files reviewed at latest revision, 18 unresolved discussions.

---

src/io/indexed_db.ts, line 153 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

still has braces

Done.

---

src/io/indexed_db.ts, line 94 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

the database code for save and load method are quite similar, maybe better to refactor to a wrapper class that
takes in transaction type and onsuccess function.

Done. Thanks for the suggestion.

---

src/io/indexed_db.ts, line 127 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

use ===

Done. Using === undefined.

---

src/io/indexed_db_test.ts, line 135 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

async

since you are using done, is the async still needed?

Good point. async is no longer needed in these tests. Removed them.

---

Comments from Reviewable

[davidsoergel]

Reviewed 4 of 5 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

---

src/io/indexed_db.ts, line 29 at r4 (raw file):

tensorfowjs

typo

---

Comments from Reviewable

[caisq]

Review status: 5 of 6 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

---

src/io/indexed_db.ts, line 29 at r4 (raw file):

Previously, davidsoergel (David Soergel) wrote…

tensorfowjs

typo

Done.

---

Comments from Reviewable

[dsmilkov]

Nice work! Sorry for delay - things are a little busy. Left a few comments but nothing blocking, thus LGTM

---

Review status: 5 of 6 files reviewed at latest revision, 19 unresolved discussions.

---

src/io/browser_files.ts, line 39 at r4 (raw file):

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

todo's are easy to forget about. better to file an issue and link to it: e.g. TODO(tensorflow/tfjs#999): .../

---

src/io/indexed_db.ts, line 59 at r2 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Done.

Since we are not going to expose this class, I think there is no need for the jsdoc, because it's another thing to keep in sync if we rename things.

Also BrowserIndexedDB implements IOHandler is self-documenting and the jsdoc just repeats that.

---

src/io/indexed_db.ts, line 21 at r5 (raw file):

import {getModelArtifactsInfoForKerasJSON} from './io_utils';
import {IOHandler, ModelArtifacts, ModelArtifactsInfo, SaveResult} from './types';

remove extra newline

---

src/io/indexed_db.ts, line 44 at r5 (raw file):

  // TODO(cais): Use central environment flag when it's available.
  if (typeof window === 'undefined') {
    // TODO(cais): Add more info about what IOHandler subtypes are available.

remove todo and file an issue. this way we open the door to discussion and potential contributors that can tackle this problem.

---

src/io/indexed_db.ts, line 120 at r5 (raw file):

      openRequest.onsuccess = () => {
        const db = openRequest.result as IDBDatabase;

double checking IDBDatabase and IDBFactory are built-in types right?

---

src/io/indexed_db.ts, line 178 at r5 (raw file):

 *   which can be used with, e.g., `tf.Model.save`.
 */
export function browserIndexedDB(modelPath: string): BrowserIndexedDB {

since there are no special methods on specific IOHandlers (by design), let's return IOHandler here instead of BrowserIndexedDB and update @returns to just say it returns an IOHandler that stores data in IndexedDB and it should be used with tf.Model.save.

---

src/io/indexed_db_test.ts, line 25 at r5 (raw file):

import {deleteDatabase} from './indexed_db';

describe('IndexedDB', () => {

describeWithFlags('IndexedDB', CPU_ENVS, () => { ...} to constraint it to run only on cpu, otherwise this test will fail on node environment, Also no need to run this explicitly on webgl either. We will need to fix the other tests (e.g. localStorage) to also run only on cpu. See tensorflow/tfjs#279

---

src/io/local_storage.ts, line 38 at r5 (raw file):

export function purgeLocalStorageArtifacts(): string[] {
  // TODO(cais): Use central environment flag when it's available.
  if (!(window && window.localStorage)) {

typeof window === 'undefined'

---

Comments from Reviewable

[nsthorat]

---

Review status: 5 of 6 files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

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

Previously, caisq (Shanqing Cai) wrote…

I don't feel I'm familiar with environment.ts enough to be able to do it correctly. But I agree we should deduplicate and centralize these checks. I added a TODO item to get that done in a later PR, maybe with your help. Let me know whether that's okay.

Sounds good!

---

src/io/indexed_db.ts, line 138 at r2 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Done.

I still see triple equals undefined. Prefer double equals null.

---

src/io/indexed_db.ts, line 127 at r3 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Done. Using === undefined.

You shouldn't be using === undefined here (see comment above).

---

Comments from Reviewable

[caisq]

Review status: 5 of 6 files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.

---

src/io/indexed_db.ts, line 59 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Since we are not going to expose this class, I think there is no need for the jsdoc, because it's another thing to keep in sync if we rename things.

Also BrowserIndexedDB implements IOHandler is self-documenting and the jsdoc just repeats that.

It doesn't hurt to have the doc string here.

---

src/io/indexed_db.ts, line 75 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

!modelPath

!modelPath also check modelPath === null

=== null is covered in == null

---

src/io/indexed_db.ts, line 122 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

readwrite

will read be enough?

Using more restrictive permissions in the new helper function (databaseAction).

---

src/io/indexed_db.ts, line 127 at r3 (raw file):

=== undefined

OK. Sorry about the back and forth. I agree with Nikhil that == null is better here, because in the lines below we are going to access the modelTopologhy field of getRequest.result`.

---

src/io/indexed_db.ts, line 21 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

remove extra newline

Done.

---

src/io/indexed_db.ts, line 44 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

remove todo and file an issue. this way we open the door to discussion and potential contributors that can tackle this problem.

Filed. tensorflow/tfjs#282

I'll keep the TODO item here just so it's easy to understand what the issues is talking about.

---

src/io/indexed_db.ts, line 120 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

double checking IDBDatabase and IDBFactory are built-in types right?

They seem to be built-in types for TypeScript.

---

src/io/indexed_db.ts, line 178 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

since there are no special methods on specific IOHandlers (by design), let's return IOHandler here instead of BrowserIndexedDB and update @returns to just say it returns an IOHandler that stores data in IndexedDB and it should be used with tf.Model.save.

I agree. Done. Using the same pattern elsewhere as well.

---

src/io/indexed_db_test.ts, line 25 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

describeWithFlags('IndexedDB', CPU_ENVS, () => { ...} to constraint it to run only on cpu, otherwise this test will fail on node environment, Also no need to run this explicitly on webgl either. We will need to fix the other tests (e.g. localStorage) to also run only on cpu. See tensorflow/tfjs#279

Done.

---

src/io/local_storage.ts, line 38 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

typeof window === 'undefined'

Done.

---

Comments from Reviewable

[pyu10055]

---

Reviewed 2 of 5 files at r3, 6 of 6 files at r6.
Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

src/io/indexed_db.ts, line 75 at r3 (raw file):

Previously, caisq (Shanqing Cai) wrote…

=== null is covered in == null

according ts style guide, always use === http://go/tsstyle#equality-checks

---

Comments from Reviewable