Skip to content
This repository was archived by the owner on Oct 17, 2021. It is now read-only.

Add Model.save(); Let loadModel() support IOHandlers #161

Merged
merged 5 commits into from
May 5, 2018

Conversation

caisq
Copy link
Contributor

@caisq caisq commented May 3, 2018

  • Model.save() uses IOHandler.save() to save model as artifacts.
  • Model.load() uses IOHandler.load() to get artifacts and construct
    a Model object.
  • Using dummy implementations of IOHandler in unit tests.

Also in this change:

  • Increase tfjs-core dependency version to 0.9.1.

Towards: tensorflow/tfjs#13


This change is Reviewable

* Model.save() uses IOHandler.save() to save model as artifacts.
* Model.load() uses IOHandler.load() to get artifacts and construct
  a Model object.
* Using dummy implementations of IOHandler in unit tests.

Towards: tensorflow/tfjs#13
Copy link
Contributor

@ericdnielsen ericdnielsen left a comment

Choose a reason for hiding this comment

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

Commented several places regarding the IOHandler|string conventions, since it didn't match what I was expecting from the design doc.

After this PR I feel we're in a weird place and we probably want to push more into it, or pull saving out.

As I understand it now we
a) still accept the old load string format, with no changes to behavior (even though I think we require changes to match the design doc)
b) don't accept strings on save (explicit error)

@@ -155,8 +155,8 @@ export class ModelExports {
subheading: 'Loading',
useDocsFrom: 'loadModelInternal'
})
static loadModel(modelConfigPath: string): Promise<Model> {
return loadModelInternal(modelConfigPath);
static loadModel(pathOrIOHandler: string|io.IOHandler): Promise<Model> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the design settled on is not backward compatible, so we'll need to make sure we bump the middle version number, right? (While we still accept strings like before, we require a prefix that we didn't in the past)

src/models.ts Outdated
* See the Python converter function `save_model()` for more details.
*
* It is also assumed that model weights can be accessed from relative
* paths described by the `paths` fields in weights manifest.
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting

src/models.ts Outdated
@@ -125,7 +190,7 @@ export interface ModelAndWeightsConfig {
* @returns A `Promise` of `Model`, with the topology and weights loaded.
*/
// TODO(cais): Add link to the core's documentation of `WeightManifestConfig`.
export async function loadModelInternal(modelConfigPath: string):
export async function loadModelFromPath(modelConfigPath: string):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at my earlier API comment, I think we're missing something here to support the scheme prefixed string sugaring. Or is this an interim solution that's accepting raw strings without scheme information?

src/models.ts Outdated
*
* // Load the model back and use it to do prediction, which should yield
* // the same result as the prediction result above.
* model2 = await tf.loadModel('localstorage://SavedByModelSaveCodeSnippet');
Copy link
Contributor

Choose a reason for hiding this comment

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

This shows the scheme I was expecting, but I don't think the string load side handles it yet?

@caisq
Copy link
Contributor Author

caisq commented May 4, 2018

Review status: 0 of 7 files reviewed at latest revision, 4 unresolved discussions.


src/exports.ts, line 158 at r1 (raw file):

Previously, ericdnielsen wrote…

I think the design settled on is not backward compatible, so we'll need to make sure we bump the middle version number, right? (While we still accept strings like before, we require a prefix that we didn't in the past)

This is backward compatible. Currently we support loadModel('path/to/model.json'), interpreting the string path as a relative HTTP path. With this PR, we'll continue supporting that, in addition to supporting loadModel(ioHandlerObject), such as loadModel(tf.io.browserLocalStorage('foo')). The support for URL-like strings such as loadModel('localstorage://foo') is in the works, but not implemented as of this PR. I'll get it implemented before the the actual release to be announced with the new save/load features. You can see that in the code snippet below.

Also, yes, the release will involve a minor (middle) version bump.


src/models.ts, line 193 at r1 (raw file):

Previously, ericdnielsen wrote…

Looking at my earlier API comment, I think we're missing something here to support the scheme prefixed string sugaring. Or is this an interim solution that's accepting raw strings without scheme information?

This is interim. Scheme-based URL-like string will be supported. The error thrown below is just a temporary NotImplementedError that will be removed before the release. It's not meant to be a permanent error.


src/models.ts, line 665 at r1 (raw file):

Previously, ericdnielsen wrote…

This shows the scheme I was expecting, but I don't think the string load side handles it yet?

Correct. I just opted to write the code snippet according the final state we want to be in at the release, even though this is not supported yet.


Comments from Reviewable

@caisq
Copy link
Contributor Author

caisq commented May 4, 2018

Review status: 0 of 7 files reviewed at latest revision, 4 unresolved discussions.


src/models.ts, line 126 at r1 (raw file):

Previously, ericdnielsen wrote…

formatting

Done.


Comments from Reviewable

@bileschi
Copy link
Contributor

bileschi commented May 4, 2018

:lgtm_strong:


Reviewed 2 of 7 files at r1.
Review status: 2 of 7 files reviewed at latest revision, 4 unresolved discussions.


src/models.ts, line 121 at r2 (raw file):

Python converter function

When referring to objects in other repositories, do we typically offer links?


src/models.ts, line 132 at r2 (raw file):

 as string

Just a question here. I thought TypeScript handled this type of inference automatically. Do these have any function beyond clarity to the reader (which is good enough purpose for this reviewer).


src/models.ts, line 673 at r2 (raw file):

   *   saving, such as byte sizes of the saved artifacts for the model's

Great job on the clarity of the documentation!!!


src/models_test.ts, line 387 at r2 (raw file):

artifiacts

nit sp.


Comments from Reviewable

@nsthorat
Copy link

nsthorat commented May 4, 2018

:lgtm_strong:


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


src/model_save_test.ts, line 21 at r2 (raw file):

    savedArtifacts: io.ModelArtifacts;

    constructor() {}

remove this line


src/model_save_test.ts, line 29 at r2 (raw file):

  }

  class EmptyIOHanlder implements io.IOHandler {

Handler


src/model_save_test.ts, line 30 at r2 (raw file):

  class EmptyIOHanlder implements io.IOHandler {
    constructor() {}

remove this line


src/models.ts, line 665 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Correct. I just opted to write the code snippet according the final state we want to be in at the release, even though this is not supported yet.

I wouldn't add this until we actually support it.


src/models.ts, line 132 at r2 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
 as string

Just a question here. I thought TypeScript handled this type of inference automatically. Do these have any function beyond clarity to the reader (which is good enough purpose for this reviewer).

+1 I think this should be inferred


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

    }
    model.loadWeights(
        io.decodeWeights(artifacts.weightData, artifacts.weightSpecs), false,

pull false / true out to variables for readability.


src/models.ts, line 630 at r2 (raw file):

  /**
   * Save the model to an IOHandler.

it's not really saving it to an IOHandler, it's using an IOHandler.

What about something more human like:

Saves the model configuration and weights.

And then go into detail in the next lines.


src/models.ts, line 690 at r2 (raw file):

        await io.encodeWeights(this.getNamedWeights(config));

    const modelConfig = this.toJSON(null, false);

name these params


src/engine/topology.ts, line 993 at r2 (raw file):

   */
  getWeights(trainableOnly = false): Tensor[] {
    return trainableOnly ? K.batchGetValue(this.trainableWeights) :

return K.batchGetValue(trainableOnly ? this.trainableWeights this.weights);


Comments from Reviewable

@caisq
Copy link
Contributor Author

caisq commented May 5, 2018

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


src/model_save_test.ts, line 21 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

remove this line

Done.


src/model_save_test.ts, line 30 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

remove this line

Done.


src/models.ts, line 665 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I wouldn't add this until we actually support it.

OK. I saved this on my computer. And I'll add it back later.


src/models.ts, line 121 at r2 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
Python converter function

When referring to objects in other repositories, do we typically offer links?

Good point. Unfortunately we don't have Python doc up on js.tensorflow.org yet. I added a link to a tutorial page on js.tensorflow.org and a link to keras doc instead.


src/models.ts, line 132 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

+1 I think this should be inferred

Done.


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

Previously, nsthorat (Nikhil Thorat) wrote…

pull false / true out to variables for readability.

Done.


src/models.ts, line 630 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

it's not really saving it to an IOHandler, it's using an IOHandler.

What about something more human like:

Saves the model configuration and weights.

And then go into detail in the next lines.

Good point. Done.


Comments from Reviewable

@caisq
Copy link
Contributor Author

caisq commented May 5, 2018

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


src/models.ts, line 690 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

name these params

Done.


src/models_test.ts, line 387 at r2 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…
artifiacts

nit sp.

Done.


src/engine/topology.ts, line 993 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

return K.batchGetValue(trainableOnly ? this.trainableWeights this.weights);

Good point. Done.


Comments from Reviewable

@caisq caisq merged commit 11903ba into tensorflow:master May 5, 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.

4 participants