Add basic types and helper methods for model exporting

[caisq]

Towards tensorflow/tfjs#13

---

This change is 

[caisq]

cc @adarob as this touches the same files as #965

[ericdnielsen]

Can we include the name of the tensor here as well.

[ericdnielsen]

As above, please include the name fields as well.

[ericdnielsen]

Are there minification concerns here with constructor.name?

[davidsoergel]

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

---

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

Inc

LLC

---

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

const tensor

This shadows the imported tensor() function; can you use a different name?

---

Comments from Reviewable

[dsmilkov]

Really nice work. To give more context to our external contributions, can you add a short description in this PR for what the changes are and how you plan to use encodeTensors and decodeTensors from other places since they are exposed to public API?

---

Reviewed 7 of 10 files at r1.
Review status: 5 of 6 files reviewed at latest revision, 5 unresolved discussions.

---

src/index.ts, line 37 at r1 (raw file):

// tslint:disable-next-line:max-line-length
export {IOHandler, LoadHandler, ModelArtifacts, SaveHandler, SaveResult, WeightsManifestConfig, WeightsManifestEntry} from './io/types';
export {loadWeights} from './io/weights_loader';

add a newline here to separate serialization from optimizers.

---

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

 */

import {tensor} from '../index';

since this is not a test, avoid importing directly from index.

---

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

 * @throws Error: on unsupported tensor `dtype`.
 */
export async function encodeTensors(tensors: NamedTensorMap):

How about calling it something more descriptive like tensorsToBuffer() and the other one bufferToTensors() or do you want to keep its implementation detail hidden from the user?

---

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

 */
export async function encodeTensors(tensors: NamedTensorMap):
    Promise<[ArrayBuffer, WeightsManifestEntry[]]> {

instead of a tuple, it's better to return an object for readability. Perhaps:
{ buffer: ArrayBuffer, metadata: WeightsManifestEntry[]}

---

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

Float32Array|Int32Array|Uint8Array>

use the alias in types.ts: TypedArray

---

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

    const shape = spec.shape;

    let numel = 1;

util.sizeFromShape(shape)

---

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

 */
export function concatenateTypedArrays(
    xs: Array<Float32Array|Int32Array|Uint8Array>): ArrayBuffer {

use TypedArray in types.ts (alias for all these arrays)

---

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

    return null;
  }
  if (xs === undefined) {

when do null and undefined happen? Better to throw an error in that case and let the caller figure it out

---

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

x as any

strange that you need to cast to any

---

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

import {NamedTensorMap} from '../types';

import {concatenateTypedArrays, decodeTensors, encodeTensors} from './io_utils';

if these are exposed to the users, use public API (import from index)

---

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

  trainableOnly?: boolean;

  // TODO(cais): Add more fields if necessary.

no need for this TODO, since you can always add more fields everywhere ;)

---

Comments from Reviewable

[dsmilkov]

typo: external contributors*

---

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

---

Comments from Reviewable

[dsmilkov]

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

---

src/index.ts, line 36 at r1 (raw file):

export {decodeTensors, encodeTensors} from './io/io_utils';
// tslint:disable-next-line:max-line-length
export {IOHandler, LoadHandler, ModelArtifacts, SaveHandler, SaveResult, WeightsManifestConfig, WeightsManifestEntry} from './io/types';

just a note to double-check that it is necessary for all these types to be exposed to public api. Want to make sure we don't expose things that are meant to be implementation detail

---

Comments from Reviewable

[dsmilkov]

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

---

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

   * any). This is applicable only to server-based saving routes.
   */
  resposnes?: Response[];

s/resposnes/responses

---

Comments from Reviewable

[pyu10055]

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

---

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

encodeTensors

want to point out this method will only work for encode weights, the activation tensors could have multiple outputs, they are identified by the name + index. maybe better to rename to encodeWeights and decodeWeights?

---

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

 * @throws Error: on unsupported tensor `dtype`.
 */
export async function encodeTensors(tensors: NamedTensorMap):

Also, how different is this code from the loadWeights method, are there plan to merge those two?
I also want to raise attention about the PR by Adam in converter repo that supports weight quantization.

---

Comments from Reviewable

[caisq]

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

---

src/index.ts, line 36 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

just a note to double-check that it is necessary for all these types to be exposed to public api. Want to make sure we don't expose things that are meant to be implementation detail

Done double checking. These are all needed by implementation in tfjs-layer.

---

src/index.ts, line 37 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

add a newline here to separate serialization from optimizers.

Done.

---

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

Previously, davidsoergel (David Soergel) wrote…

Inc

LLC

Done making this change throughout src/io/

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

since this is not a test, avoid importing directly from index.

Done.

---

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

decodeWeights

Good point. In TensorFlow and TensorFlow.js, non-weight tensors generally don't have names. So decodeWeights and encodeWeights are more appropriate names here. Thanks. Done.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

How about calling it something more descriptive like tensorsToBuffer() and the other one bufferToTensors() or do you want to keep its implementation detail hidden from the user?

The encodeTensors method actually returns two things, a buffer and an array of specs. So tensorsToBuffers is not the most appropriate.

Considering Ping's comment, I'll rename the methods to encodeWeights and decodeWeights.

---

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

Previously, pyu10055 (Ping Yu) wrote…

Also, how different is this code from the loadWeights method, are there plan to merge those two?
I also want to raise attention about the PR by Adam in converter repo that supports weight quantization.

Yes, loadWeights should use decodeWeights (formerly decodeTensors). I added a TODO item.

I also added TODO items to handle quantization to these methods. cc @adarob

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

instead of a tuple, it's better to return an object for readability. Perhaps:
{ buffer: ArrayBuffer, metadata: WeightsManifestEntry[]}

Done. I'm using the field names data and specs, to be consistent with the fields weightData and weightSpecs in ModelArtifacts in type.ts.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

Float32Array|Int32Array|Uint8Array>

use the alias in types.ts: TypedArray

Done.

---

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

Previously, davidsoergel (David Soergel) wrote…

const tensor

This shadows the imported tensor() function; can you use a different name?

Good point. Done. Using t.

---

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

Previously, ericdnielsen wrote…

Can we include the name of the tensor here as well.

Done.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

util.sizeFromShape(shape)

Done.

---

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

Previously, ericdnielsen wrote…

As above, please include the name fields as well.

Done.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

use TypedArray in types.ts (alias for all these arrays)

Done.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

when do null and undefined happen? Better to throw an error in that case and let the caller figure it out

I removed the logic for null and undefined here and throw an error as you suggested.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

x as any

strange that you need to cast to any

This is because we are testing things that shouldn't happen in TypeScript because of type checking, i.e., things that can happen only in JavaScript.

---

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

Previously, ericdnielsen wrote…

Are there minification concerns here with constructor.name?

Good question. The input here is a subtype of TypedArray, i.e., a built-in type. So most likely, there will be no minification of the constructor.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

if these are exposed to the users, use public API (import from index)

Done.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

no need for this TODO, since you can always add more fields everywhere ;)

Done.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

s/resposnes/responses

Done.

---

Comments from Reviewable

[nsthorat]

Reviewed 2 of 8 files at r2.
Review status: 2 of 6 files reviewed at latest revision, 20 unresolved discussions.

---

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

Previously, caisq (Shanqing Cai) wrote…

Done.

I still see index

---

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

Previously, caisq (Shanqing Cai) wrote…

Done.

can you just use size instead of numel

---

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

/**
 * Encode a map from names to weight values as an ArrayBuffer, along with an
 * `Array` of `WeightsManifestEntry` as specification of the encoded weights.

would be good to mention how this is supposed to be symmetric to our other functionality. Would also be good to say this does not shard.

---

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

    let value: Tensor;
    if (dtype === 'float32') {
      bytes = numel * 4;

we have a map you can use:
/~https://github.com/tensorflow/tfjs-core/blob/master/src/weights_loader.ts#L33

---

Comments from Reviewable

[dsmilkov]

---

Reviewed 6 of 8 files at r2.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

---

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

  let totalByteLength = 0;
  for (const x of xs) {

use forEach since it's a list. This way the tsc won't need to compile this into es5 compatible code.

---

Comments from Reviewable

[caisq]

Review status: all files reviewed at latest revision, 16 unresolved discussions, all commit checks successful.

---

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

Previously, nsthorat (Nikhil Thorat) wrote…

I still see index

Done.

---

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

Previously, nsthorat (Nikhil Thorat) wrote…

can you just use size instead of numel

Done.

---

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

Previously, nsthorat (Nikhil Thorat) wrote…

would be good to mention how this is supposed to be symmetric to our other functionality. Would also be good to say this does not shard.

Done.

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

use forEach since it's a list. This way the tsc won't need to compile this into es5 compatible code.

Done.

---

Comments from Reviewable

[dsmilkov]

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

---

src/index.ts, line 67 at r3 (raw file):

// Second level exports.
export {environment, test_util, util};

move io here, so we have all second level exports together

---

Comments from Reviewable

[caisq]

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

---

src/index.ts, line 67 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

move io here, so we have all second level exports together

Done.

---

Comments from Reviewable