Fix docs to not mention tf.io.browserDownloads(), since it's not exported. #1169
Conversation
There was a problem hiding this comment.
Thanks for the PR, David. Just to make sure I understand: for clients of the public API, they can just use the 'downloads://' URL scheme to access the functionality. Is the reason why you need it exported related to some non-public usage of this?
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @caisq)
There was a problem hiding this comment.
Oh, I see: since downloads:// does the same thing, we don't strictly need this--but we should instead update the docstring at https://github.com/tensorflow/tfjs-core/blob/master/src/io/browser_files.ts#L265.
On the other hand, it looks like tf.io.browserHTTPRequest() is exposed on purpose because it allows for fine-grained control via a RequestInit argument. Similarly, the argument to tf.io.withSaveHandler() can't be expressed as a URI. I understand that browserDownloads has no potential additional argument, but still the asymmetry is a little concerning. Basically, if I'm able to use any IOHandler as an argument to model.save(), I'd expect to be able to use all of them. That argument would apply to BrowserIndexedDB, BrowserLocalStorage, etc. too, but exposing all of those does seem like unnecessary overkill. So, to return to the first hand, the rule seems to be that we should insist on the URI form if that is sufficient, and allow an explicit IOHandler only in cases where it is really necessary. Is that right?
@bileschi I agree about the test, but will wait pending clarification of what we want the public API to be.
Reviewable status:
There was a problem hiding this comment.
David, some IOHandlers have optional arguments that allow fine-grained control, such as browserHTTPRequest. But currently browserDownloads happens to not one of those. The only control it gives the client is the file-name prefix. Fine-grained options may be added to it in the future, at which time we should definitely export this.
But I'm inclined toward not exporting it because it would make it possible to do the same thing in two different ways. I agree that the documentation should be fixed. I don't feel too strongly about this. So I'm going to LGTM this PR and let you decide whether to export this symbol or to fix the doc.
Reviewable status:
There was a problem hiding this comment.
+1 to Shanqing's reply. I'm fine either way , thus LGTM, but definitely would be great to fix the jsdoc in this PR while you are at it.
Reviewed 1 of 1 files at r1.
Reviewable status:
There was a problem hiding this comment.
OK, fine with me: I reverted the original change, so now instead this PR only changes the docstring to not mention tf.io.browserDownloads(). I know it's poor form to submit a change that is completely different from the one that was approved, but I think the prior discussion covers it well enough.
Reviewable status:
davidsoergel
changed the title
Description
The browserDownloads symbol was not exported in io/io.ts; this corrects the oversight and so makes
tf.io.browserDownloads()work as already documented.BUG
For repository owners only:
Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY
For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md
This change is