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

Add passthrough IOHandlers to save and load models to and from memory #1148

Merged
merged 8 commits into from
Jul 10, 2018

Conversation

davidsoergel
Copy link
Member

@davidsoergel davidsoergel commented Jul 10, 2018

Add passthrough IOHandlers that allow users to work with the tf.io.ModelArtifacts in-memory serialization format.

Partly addresses tensorflow/tfjs#495

FEATURE


This change is Reviewable

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @davidsoergel, @nsthorat, @caisq, and @dsmilkov)


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

{}

Do you want to make this {}|ArrayBuffer, so that it is general enough to include FrozenModels?


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

 modelTopology

The ModelArtifacts interface actually allows modelTopology to be missing, in which case the artifacts are weights-only. I don't think it'll be too difficult to support that here.


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

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

Is new Promise ... necessary here? Can you simply do return {modelTopology: this.modelTopology} etc.?


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

this.weightData === undefined

Should this handle the case of this.weightData === null as well?


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

Creates an IOHandler that passes saved model artifacts to a callback.

Can you add a code example?


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

export function withHandler(
    saveHandler: (artifacts: ModelArtifacts) => SaveResult): IOHandler {

I think it would be better to call it "withSaveHandler" so that it is clearer whether this is for saving or loading.


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

555

Is it possible to not hard-code these numbers here to prevent possible future maintenance cost?

Copy link
Member Author

@davidsoergel davidsoergel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq, @nsthorat, and @dsmilkov)


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

Previously, caisq (Shanqing Cai) wrote…
{}

Do you want to make this {}|ArrayBuffer, so that it is general enough to include FrozenModels?

You bet, good catch


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

Previously, caisq (Shanqing Cai) wrote…
 modelTopology

The ModelArtifacts interface actually allows modelTopology to be missing, in which case the artifacts are weights-only. I don't think it'll be too difficult to support that here.

Done.


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

Previously, caisq (Shanqing Cai) wrote…
return new Promise<ModelArtifacts>((resolve, reject) => {

Is new Promise ... necessary here? Can you simply do return {modelTopology: this.modelTopology} etc.?

Oops, I overlooked the paste artifact, thanks


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

Previously, caisq (Shanqing Cai) wrote…
this.weightData === undefined

Should this handle the case of this.weightData === null as well?

Yep. What a ridiculous language :) I still get confused about {null, undefined} x {==, ===}. Anyway I think '!= null' covers both cases.


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

Previously, caisq (Shanqing Cai) wrote…
Creates an IOHandler that passes saved model artifacts to a callback.

Can you add a code example?

It seemed to me that the boilerplate for a minimal example overwhelmed the point here. Is this OK (eliding the details via '...')?


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

Previously, caisq (Shanqing Cai) wrote…
export function withHandler(
    saveHandler: (artifacts: ModelArtifacts) => SaveResult): IOHandler {

I think it would be better to call it "withSaveHandler" so that it is clearer whether this is for saving or loading.

Done.


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

Previously, caisq (Shanqing Cai) wrote…
555

Is it possible to not hard-code these numbers here to prevent possible future maintenance cost?

Done.

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Great. Thanks!

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Great

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @caisq, @nsthorat, and @dsmilkov)

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @caisq, @nsthorat, and @dsmilkov)

@davidsoergel davidsoergel merged commit dd7b00e into master Jul 10, 2018
@davidsoergel davidsoergel deleted the blob-io branch July 10, 2018 14:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants