Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: binary fetch type #1786

Closed
hamilton opened this issue May 1, 2019 · 10 comments · Fixed by #2550
Closed

proposal: binary fetch type #1786

hamilton opened this issue May 1, 2019 · 10 comments · Fixed by #2550
Assignees

Comments

@hamilton
Copy link
Contributor

hamilton commented May 1, 2019

Inspired by the thread on #1686. It is very common to load binary data (pickled objects, arrow data frames, etc.) that need to get put through Uint8Array, that it might make sense to have an additional fetch type called bytes or binary that automatically does it so users don't have to deal with an additional implementation detail.

This would be as simple as just wrapping fetch(url).then(r=>arrayBuffer()).then(data=>new Uint8Array(data)) most likely.

This would be very simple to implement and reduce the amount of cognitive overhead in figuring out how to get one step further w/ binary data in Iodide.

@mdboom @bcolloran @wlach thoughts here appreciated.

thanks for the idea @vadimkantorov

@bcolloran
Copy link
Contributor

proposal sgtm hamilton!

qq for @mdboom -- in your pyodide blog post you mention that there is automatic conversion from python bytes to js UInt8ClampedArray -- would UInt8ClampedArray preferable for some reason? (i don't know the difference)

i think the "bytes" terminology might be better than "binary" to maintain that parallelism. @mdboom do happen to know if "bytes" is used this way in other languages?

https://hacks.mozilla.org/2019/04/pyodide-bringing-the-scientific-python-stack-to-the-browser/

@mdboom
Copy link
Contributor

mdboom commented May 1, 2019

The UInt8ClampedArray is required be HTML canvas to display pixels. But I don't think the distinction matters much here -- both forms convert implicitly and efficiently from Javascript to Python.

The term bytes would be consistent with Python's use of the term, so I'm happy with that.

@vadimkantorov
Copy link

@hamilton Maybe after pyodide/pyodide#407 this is not needed :) Though bytes still reads more intuitive than arrayBuffer (even in case arrayBuffer conversion to memoryview is fixed)

@hamilton
Copy link
Contributor Author

hamilton commented May 1, 2019

@vadimkantorov possible, but not every language has this affordance (including JS), so we'll still likely implement. Glad to see pyodide/pyodide#407 might unblock your notebook.

@wlach
Copy link
Contributor

wlach commented Nov 18, 2019

Looked at this at the pyodide sprint with @imranarrifin. It seems like this is more or less solved by #407, going to close

@wlach wlach closed this as completed Nov 18, 2019
@vadimkantorov
Copy link

@wlach Changeset in https://github.com/iodide-project/iodide/pull/407/files seems to have no mention of open_url or other fetching functionality

@vadimkantorov
Copy link

@wlach Ah, ok, just bad link in your comment: it links a PR407 instead of an issue407

@wlach
Copy link
Contributor

wlach commented Nov 18, 2019

Sorry! Should not have closed this, the history here is quite confusing as we already have the "arrayBuffer" fetch type.

It seems like this issue is still valid, though it is definitely far from being a "good first issue" as it involves unwinding a fair bit of internals on the client side. I think as a strawman, we should go with bytes here as a fetch type.

@wlach
Copy link
Contributor

wlach commented Nov 27, 2019

@imranariffin is interested in taking this one on

@imranariffin
Copy link
Collaborator

My apologies on the silence from my part on this issue, it's been pretty busy for me the past two weeks but I do plan to come back to this over this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants