Skip to content

Conversation

@pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented Aug 13, 2020

This would allow user to translate model weight file name to the correct URL.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@pyu10055 pyu10055 requested a review from lina128 August 13, 2020 21:52
Copy link
Collaborator

@lina128 lina128 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 @lina128 and @pyu10055)


tfjs-core/src/io/http.ts, line 39 at r1 (raw file):

  private readonly fetch: Function;
  private readonly weightUrlTranslationFunc:

Is there a better naming? What about weightUrlConverter?


tfjs-core/src/io/http.ts, line 221 at r1 (raw file):

    const fetchURLs: string[] = [];
    for await (const weightsGroup of weightsManifest) {

weightsManifest isn't async, why do we need await here?


tfjs-core/src/io/http.ts, line 224 at r1 (raw file):

      for await (const path of weightsGroup.paths) {
        if (this.weightUrlTranslationFunc != null) {
          const url = await this.weightUrlTranslationFunc(path);

This async call in the loop makes it call sequentially, can we trigger the call in parallel? Something like:

if (this.weightUrlTranslationFunc != null) {
    const translatedUrlPromises = [];
    weightsManifest.forEach(weightsGroup => {
        weightsGroup.paths.forEach(path => {
          translatedUrlPromises.push(this.weightUrlTranslationFunc(path));
    }

    fetchURLs.push(...await Promise.all(translatedUrlPromises));
}

tfjs-core/src/io/http_test.ts, line 810 at r1 (raw file):

          },
          requestInits);
      async function weightUrlTranslateFunc(weightFile: string):

Let's make this test more readable, weightFileRelativePath.


tfjs-core/src/io/http_test.ts, line 812 at r1 (raw file):

      async function weightUrlTranslateFunc(weightFile: string):
          Promise<string> {
        return 'auth_' + weightFile;

Fake a promise return, something like:

return Promise(resolve => setTimeout(resolve, 1, 'auth_' + weightFile));

tfjs-core/src/io/types.ts, line 469 at r1 (raw file):

  /**
   * An async function to translate weight file name to URL. By default we

Is it file name or file relative path? I think the doccomment should also describe where to find these values, something like, "the weight file relative paths are stored in model.json's weightsManifest.paths

Copy link
Collaborator Author

@pyu10055 pyu10055 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 @lina128)


tfjs-core/src/io/http.ts, line 39 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Is there a better naming? What about weightUrlConverter?

Done.


tfjs-core/src/io/http.ts, line 221 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

weightsManifest isn't async, why do we need await here?

Refactored according to your suggestion.


tfjs-core/src/io/http.ts, line 224 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

This async call in the loop makes it call sequentially, can we trigger the call in parallel? Something like:

if (this.weightUrlTranslationFunc != null) {
    const translatedUrlPromises = [];
    weightsManifest.forEach(weightsGroup => {
        weightsGroup.paths.forEach(path => {
          translatedUrlPromises.push(this.weightUrlTranslationFunc(path));
    }

    fetchURLs.push(...await Promise.all(translatedUrlPromises));
}

good suggestion, I have refactored the code in similar fashion, hope this way is more clear.


tfjs-core/src/io/http_test.ts, line 810 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Let's make this test more readable, weightFileRelativePath.

Done.


tfjs-core/src/io/http_test.ts, line 812 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Fake a promise return, something like:

return Promise(resolve => setTimeout(resolve, 1, 'auth_' + weightFile));

Done.


tfjs-core/src/io/types.ts, line 469 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Is it file name or file relative path? I think the doccomment should also describe where to find these values, something like, "the weight file relative paths are stored in model.json's weightsManifest.paths

It is filename, our model.json assume all files are in the same path.

Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Thank you Ping!

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128)

@pyu10055 pyu10055 merged commit c891e3b into master Aug 17, 2020
@pyu10055 pyu10055 deleted the http_weight_url_translate branch August 17, 2020 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants