Allow passthrough save handler to be async. #1200
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @davidsoergel and @caisq)
src/io/passthrough.ts, line 47 at r1 (raw file):
Hmm... the fact that it wasn't a Promise to begin with seems to be an oversight. For consistency going forward, I suggest changing it to only Promise. This should be fine because it is not reflected in our documentation on js.tensorflow.org yet. How does that sound?
Same for withSaveHandler below.
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @davidsoergel)
src/io/passthrough.ts, line 47 at r1 (raw file):
Previously, caisq (Shanqing Cai) wrote…
Hmm... the fact that it wasn't a Promise to begin with seems to be an oversight. For consistency going forward, I suggest changing it to only Promise. This should be fine because it is not reflected in our documentation on js.tensorflow.org yet. How does that sound?
Same for withSaveHandler below.
Yep, it was just an oversight, and I agree Promise-only is even better-- done.
|
Oops airplane wifi seems to be blocking my git push. The fix will be in
once I have reliable net :)
…On Wed, Aug 1, 2018 at 8:28 PM David Soergel ***@***.***> wrote:
***@***.**** commented on this pull request.
*Reviewable <https://reviewable.io/reviews/tensorflow/tfjs-core/1200>*
status: 0 of 1 approvals obtained (waiting on @davidsoergel
<https://github.com/davidsoergel>)
------------------------------
*src/io/passthrough.ts, line 47 at r1
<https://reviewable.io/reviews/tensorflow/tfjs-core/1200#-LIsRmi74JWN76MvuZA3:-LIsfN8f7QSolwmDMhyD:b-wytiyz>
(raw file
<https://github.com/tensorflow/tfjs-core/blob/e72eeb28ab438745a420c6fd3d48cc6de5c6e6a1/src/io/passthrough.ts#L47>):*
*Previously, caisq (Shanqing Cai) wrote…*
Hmm... the fact that it wasn't a Promise to begin with seems to be an
oversight. For consistency going forward, I suggest changing it to only
Promise. This should be fine because it is not reflected in our
documentation on js.tensorflow.org yet. How does that sound?
Same for withSaveHandler below.
Yep, it was just an oversight, and I agree Promise-only is even better--
done.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1200 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABSPNQK9rfT57ODGY7-IirmVKfQgEOkTks5uMnHfgaJpZM4Vrhp0>
.
|
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq)
src/io/passthrough.ts, line 47 at r1 (raw file):
Previously, davidsoergel (David Soergel) wrote…
Yep, it was just an oversight, and I agree Promise-only is even better-- done.
Done.
|
FYI, there are some minor build issues: https://travis-ci.org/tensorflow/tfjs-core/jobs/411427788#L489 |
There was a problem hiding this comment.
Oops, done
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq)
Allow the PassthroughSaver to accept an async saveHandler. This just works since IOHandler.save() was already async.
FEATURE
This change is