Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Add IOHandler subtype BrowserLocalStorage. #1003

Merged
merged 8 commits into from
Apr 30, 2018

Conversation

caisq
Copy link
Collaborator

@caisq caisq commented Apr 27, 2018

  • tf.io.browserLocalStorage() is the public factory function.
  • Adjust the interfaceSaveResult slightly: Remove the required
    success boolean field. Failures in saving and loading will always be
    thrown as Errors.
  • Add helper functions:
    • stringByteLength: required for calculating the byte length of
      the JSON model topology and weight specs.
    • arrayBufferToBase64String
    • base64StringToArrayBuffer
      The last two helper functions are required because local storage
      values cannot by binary arrays or ArrayBuffers.

Towards tensorflow/tfjs#13


This change is Reviewable

caisq added 5 commits April 26, 2018 21:39
* tf.io.browserLocalStorage() is the public factory function.
* Adjust SaveResult slightly: Remove the required success boolean
  field. Failures in saving and loading will always be thrown as
  `Error`s.
* Add helper functions:
  * strinbByteLength: required for calculating the byte length of
    the JSON model topology and weight specs.
  * arrayBufferToBase64String
  * base64StringToArrayBuffer
  The last two helper functions are required because local storage
  values cannot by binary arrays or `ArrayBuffer`s.

if (modelPath == null) {
throw new Error(
'For local storage, modelPath must not be null or undefined.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest using the same error string for both this Error and the next one. We don't people playing whack-a-mole. Following the discussion last year on TF errors, something like "Local Storage requires a modelPath that is no empty, underfined, nor null." Maybe add on a "Suggested fix/use: use a unique descriptive name, possibly including a version token such as 'supermodel-01'"

async save(modelArtifacts: ModelArtifacts): Promise<SaveResult> {
if (!(window && window.localStorage)) {
throw new Error(
'The current environment does not support local storage.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want an io.io_util.enumerateSupportedHandlers? To be able to provide the list of support handlers in the error message? In either case can we provide better details "LocalStorage requires a browser that supports XYZ, this program is not being run in a browser or not in a browser that supports that standard."

}

throw new Error(
`Failed to save model '${this.modelPath}' to local storage: ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

Does err contain any information that we can use to say why it failed? Do we need to do size checks before we attempt to write? Are there possible permissions issues we can pre-check?

@caisq
Copy link
Collaborator Author

caisq commented Apr 27, 2018

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


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

Previously, ericdnielsen wrote…

I would suggest using the same error string for both this Error and the next one. We don't people playing whack-a-mole. Following the discussion last year on TF errors, something like "Local Storage requires a modelPath that is no empty, underfined, nor null." Maybe add on a "Suggested fix/use: use a unique descriptive name, possibly including a version token such as 'supermodel-01'"

Done.


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

Previously, ericdnielsen wrote…

Do we want an io.io_util.enumerateSupportedHandlers? To be able to provide the list of support handlers in the error message? In either case can we provide better details "LocalStorage requires a browser that supports XYZ, this program is not being run in a browser or not in a browser that supports that standard."

Good point. I added a TODO item here to improve the helpfulness of this error message. Ideas include:

  1. Link to a page under js.tensorflow.org
  2. When we have the classes and logic ready, automatically determine what the supported IOHandlers are given the current environment and print them out.

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

Previously, ericdnielsen wrote…

Does err contain any information that we can use to say why it failed? Do we need to do size checks before we attempt to write? Are there possible permissions issues we can pre-check?

There is no API to check the browser local storage quota. Hence there are pages like this that figure this out for you by saving incrementally large artifacts: https://arty.name/localstorage.html

I added a few words here pointing to size quota as a likely cause.


Comments from Reviewable

@nsthorat
Copy link
Contributor

:lgtm_strong:

Nice work!


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


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

 * @returns A string that base64-encodes `buffer`.
 */
export function arrayBufferToBase64String(buffer: ArrayBuffer) {

return type


src/io/io_utils.ts, line 167 at r2 (raw file):

 */
export function arrayBufferToBase64String(buffer: ArrayBuffer) {
  return btoa(String.fromCharCode.apply(null, new Uint8Array(buffer)));

optional: use spread operator.

String.fromCharCode(...new Uint8Array(buffer));


src/io/io_utils.ts, line 176 at r2 (raw file):

 * @returns Decoded `ArrayBuffer`.
 */
export function base64StringToArrayBuffer(str: string) {

return type


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

Previously, caisq (Shanqing Cai) wrote…

There is no API to check the browser local storage quota. Hence there are pages like this that figure this out for you by saving incrementally large artifacts: https://arty.name/localstorage.html

I added a few words here pointing to size quota as a likely cause.

Can we also log the native err here?


src/io/local_storage.ts, line 67 at r2 (raw file):

  protected readonly paths: {[key: string]: string};

  constructor(modelPath: string) {

s/modelPath/modelName? you refer to this as name in error messages


src/io/local_storage.ts, line 116 at r2 (raw file):

      const modelArtifactsInfo: ModelArtifactsInfo = {
        dateSaved: new Date(),
        modelTopologyType: 'KerasJSON',

how will this get wired through? will this be a field in ModelArtifacts?


Comments from Reviewable

@caisq
Copy link
Collaborator Author

caisq commented Apr 30, 2018

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


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

Previously, nsthorat (Nikhil Thorat) wrote…

return type

Done.


src/io/io_utils.ts, line 167 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

optional: use spread operator.

String.fromCharCode(...new Uint8Array(buffer));

Hmm. The results seem different

btoa(String.fromCharCode.apply(null, new Uint8Array([1,2,3])))
// vs.
btoa(...new Uint8Array([1,2,3]))

Let's keep it this way.


src/io/io_utils.ts, line 176 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

return type

Done.


Comments from Reviewable

@nsthorat
Copy link
Contributor

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


src/io/io_utils.ts, line 167 at r2 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Hmm. The results seem different

btoa(String.fromCharCode.apply(null, new Uint8Array([1,2,3])))
// vs.
btoa(...new Uint8Array([1,2,3]))

Let's keep it this way.

Just for posterity (no need to update, just good for JS foo), the correct line is:

btoa(String.fromCharCode(...new Uint8Array(buffer)));


Comments from Reviewable

@caisq caisq merged commit 73756f8 into tensorflow:master Apr 30, 2018
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.

3 participants