duneanalytics / dune-client

A framework for interacting with Dune Analytics' officially supported API service
Apache License 2.0
85 stars 22 forks source link

feat: add async support for csv and dataframe methods #56

Closed gosuto-inzasheru closed 1 year ago

gosuto-inzasheru commented 1 year ago

opening this as a draft to get some feedback.

closes #54

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

gosuto-inzasheru commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

gosuto-inzasheru commented 1 year ago

im a bit out of depth here.

in the basic client, the requests' response is converted to a binary stream using BytesIO:

return ExecutionResultCSV(data=BytesIO(response.content))

however, the asyncio variant of our session returns a StreamReader type response. although a stream, it cannot be handled by pandas' read_csv. currently i was able to solve this as such:

return ExecutionResultCSV(data=BytesIO(await response.content.read(-1)))

but is this really the most efficient way to convert the asynchronous response to something pandas can read?

gosuto-inzasheru commented 1 year ago

client.py versus client_async.py: https://www.diffchecker.com/tXrNtPqY/

bh2smith commented 1 year ago

I would be willing to merge as is and we can tag a new release after a few other of the pending/incoming PRs are landed. Just let me know

gosuto-inzasheru commented 1 year ago

@eliseygusev care to comment on the async aspect here maybe?

gosuto-inzasheru commented 1 year ago

i will resolve conflicts and update for updates made to client in #53

bh2smith commented 1 year ago

Were you still planning on resolving conflicts here?

gosuto-inzasheru commented 1 year ago

yes will do, just also have a day job :(

gosuto-inzasheru commented 1 year ago

should be good to merge @bh2smith!

gosuto-inzasheru commented 1 year ago

@bh2smith what is in the way of merging this? i think waiting will just creates more conflicts down the road for other prs...

bh2smith commented 1 year ago

Ya we can merge this... but the performance tier is still failing. Its fine though (aw you explained)

bh2smith commented 1 year ago

Could you also review #59 --- @gosuto-inzasheru? Then I can tag a new release.