Add tf.io.browserHTTPRequest #1030

caisq · 2018-05-09T21:01:15Z

Allows model artifacts to be sent via a multipart/form-data
HTTP request

This change is

* Allows model artifacts to be sent via a multipart/form-data HTTP request Towards: tensorflow/tfjs#13

davidsoergel · 2018-05-11T15:04:03Z

Here's a first set of comments.

Broadly I'm confused about the expected usage pattern for this and IOHandler generally (sorry I missed the prior reviews). It seems unlikely to me that users want to specify a location once and then write to it repeatedly (overwriting, presumably). But the API seems specifically designed for that use case, and the code requires some extra complexity to handle it safely. What am I missing?

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

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

import {IOHandler, ModelArtifacts, SaveResult, WeightsManifestConfig} from './types';

export class BrowserHTTPRequest implements IOHandler {

Does this need to be exported, given that users should use the factory function anyway?

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

    if (modelArtifacts.modelTopology instanceof ArrayBuffer) {
      throw new Error(
          'HTTPReqwuests.save() does not support saving model topology ' +

typo, but also I think you meant BrowserHTTPRequest.save()

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

    }

    return new Promise<SaveResult>(async (resolve, reject) => {

Unless I'm misunderstanding something, this is adding an unnecessary layer of Promise, and mixing async/await syntax with callback syntax is confusing in any case. I think you can just delete this line and make the corresponding minor tweaks below.

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

    return new Promise<SaveResult>(async (resolve, reject) => {
      const requestInit =

Don't shadow the field name

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

      const requestInit =
          Object.assign({method: this.DEFAULT_METHOD}, this.requestInit);
      if (requestInit.body == null) {

But if the user passed in a body, then each call to save() uses the same one (because Object.assign makes a shallow copy). Thus the formData.append calls below will accumulate artifacts in the same body.

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

      };

      const formData = requestInit.body as FormData;

I think you could avoid the cast here by preparing formData in the constructor and/or in the conditional above where its type is unambiguous.

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

      if (response.status === 200) {
        resolve({

return

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

        });
      } else {
        reject(response);

throw

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

 * The following Python code snippet based on the
 * [flask](https://github.com/pallets/flask) server framework implements a
 * server that can receive the request. Upon receiving the model artifacrts

typo

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

BrowserHTTPRequest

The declared return type is IOHandler; the concrete class is an implementation detail that the caller doesn't care about.

Comments from Reviewable

caisq · 2018-05-11T18:30:14Z

As David and I discussed offline, this API that involves calling tf.io.browserHTTPRequest is fine in that

most users will call a simpler API that doesn't involve tf.io.browserHTTPRequest, namely model.save('http://my-server/upload-model'). tf.io.browserHTTPRequest` is used only when the user needs to control additional request parameters such as method, headers and credentials.
there are indeed cases in which a user wants to load/save multiple times from the same object as returned by tf.io.browserHTTPRequest.

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

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