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

Refactor IO-related code to use the new LoadOptions interface #1554

Merged
merged 7 commits into from
Feb 28, 2019

Conversation

caisq
Copy link
Collaborator

@caisq caisq commented Feb 8, 2019

  • Refactor several IO-related functions to use the new LoadOptions interface.
  • Move monitorProgress to the io-specific source directory.

This change is Reviewable

@caisq caisq changed the title Add LoadOptions and other tf.io-related refactoring [DO NOT REVIEW OR MERGE YET] Add LoadOptions and other tf.io-related refactoring Feb 8, 2019
@caisq caisq changed the title [DO NOT REVIEW OR MERGE YET] Add LoadOptions and other tf.io-related refactoring Refactor IO-related code to use the new LoadOptions interface Feb 13, 2019
@caisq caisq requested review from nsthorat, dsmilkov and pyu10055 and removed request for nsthorat and pyu10055 February 13, 2019 21:51
@nsthorat
Copy link
Contributor

Ideally we get this in this week before 1.0, is that right?

@nsthorat
Copy link
Contributor

(let me know when to review)

@caisq
Copy link
Collaborator Author

caisq commented Feb 26, 2019

@nsthorat There are a bunch of conflicts to resolve. I'll let you know when it's ready.

@caisq
Copy link
Collaborator Author

caisq commented Feb 26, 2019

@nsthorat @dsmilkov @pyu10055 ready for review.

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.

Reviewed 4 of 7 files at r1, 3 of 3 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @caisq, @dsmilkov, @nsthorat, and @pyu10055)


src/io/progress.ts, line 39 at r2 (raw file):

  let resolvedPromise = 0;

  function registerMonitor(promise: Promise<{}>) {

for these private functions can you create them like this so it's clear they're not top-level:

const registerMonitor = promise => ...

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r1, 1 of 3 files at r2.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @caisq, @dsmilkov, and @pyu10055)


src/io/browser_http.ts, line 448 at r2 (raw file):

 * 'model.weights.bin') will be appended to the body. If `requestInit` has a
 * `body`, an Error will be thrown.
 * @param weightPathPrefix Optional, this specifies the path prefix for weight

update jsdocs

Copy link
Collaborator Author

@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: :shipit: complete! 2 of 1 approvals obtained (waiting on @pyu10055)


src/io/browser_http.ts, line 448 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

update jsdocs

Done.


src/io/progress.ts, line 39 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

for these private functions can you create them like this so it's clear they're not top-level:

const registerMonitor = promise => ...

Done.

@caisq caisq merged commit 6625e66 into tensorflow:master Feb 28, 2019
@caisq caisq deleted the load-option branch February 28, 2019 04:40
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.

3 participants