search

duneanalytics / dune-client

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

Async client #31

Closed eliseygusev closed 1 year ago

github-actions[bot] commented 1 year ago

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

bh2smith commented 1 year ago

It looks like the e2e tests fail because your PR is not allow to make use of project secrets. I will run this locally and check it out.

bh2smith commented 1 year ago

Ran the e2e tests quickly and they are all failing with:

TypeError: 'coroutine' object is not subscriptable
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_cancel_execution - TypeError: 'coroutine' object is not subscriptable
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_endpoints - TypeError: 'coroutine' object is not subscriptable
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_get_status - TypeError: 'coroutine' object is not subscriptable
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_internal_error - TypeError: 'coroutine' object is not subscriptable
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_invalid_api_key_error - TypeError: 'coroutine' object is not subscriptable
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_invalid_job_id_error - AttributeError: 'coroutine' object has no attribute 'get'
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_parameters_recognized - TypeError: 'coroutine' object is not subscriptable
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_query_not_found_error - TypeError: 'coroutine' object is not subscriptable
FAILED tests/e2e/test_async_client.py::TestDuneClient::test_refresh - TypeError: 'coroutine' object is not subscriptable

Also this

sys:1: RuntimeWarning: coroutine 'ClientResponse.json' was never awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x1114c9d50>
Unclosed connector
connections: ['[(<aiohttp.client_proto.ResponseHandler object at 0x1115fdec0>, 2.528666034)]']
connector: <aiohttp.connector.TCPConnector object at 0x1114c9d10>
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x1113c9b50>
Unclosed connector
connections: ['[(<aiohttp.client_proto.ResponseHandler object at 0x1114584b0>, 2.839456806)]']
connector: <aiohttp.connector.TCPConnector object at 0x111484ed0>
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x111605b10>
Unclosed connector
connections: ['[(<aiohttp.client_proto.ResponseHandler object at 0x11166af30>, 3.121304255)]']
connector: <aiohttp.connector.TCPConnector object at 0x111605ad0>
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x111600590>
Unclosed connector
connections: ['[(<aiohttp.client_proto.ResponseHandler object at 0x1114ae210>, 3.394400219)]']
connector: <aiohttp.connector.TCPConnector object at 0x111630c50>
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x111630d90>
Unclosed connector
connections: ['[(<aiohttp.client_proto.ResponseHandler object at 0x1114aea60>, 3.531035455)]']
connector: <aiohttp.connector.TCPConnector object at 0x111614d90>
bh2smith commented 1 year ago

It would also be nice to see a usage test:

Like 

async_dune.refresh(query1, query2, query3)  <- Did you provide an interface to query many?

and also some test that demonstrates the maximum number of parallel execution limitations.

eliseygusev commented 1 year ago

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

eliseygusev commented 1 year ago

Fixed the issue with the tests failing -- forgot that ClientResponse().json() outputs the coroutine itself. I still have issues testing this locally as I don't have the Dune API key (and this key can't be easily acquired).

Didn't add any new functionality so far. I think it's better to leave it for future updates (as this PR already is kinda big).

eliseygusev commented 1 year ago

async_dune.refresh(query1, query2, query3) <- Did you provide an interface to query many?

as for this issue. I think the more convenient way would be for the consumer to do something like asyncio.gather(*[async_dune.refresh(query) for query in queries]) This way the consumer has all the control around executing and limitations

bh2smith commented 1 year ago

Here is the failing assertion error details:

            assert (
                self._base_url.origin() == self._base_url
            ), "Only absolute URLs without path part are supported"

and the values are:

> self._base_url 
https://api.dune.com/api/v1 
> self._base_url.origin()
https://api.dune.com

I have also fixed all the lint, format and type errors on my side. Not sure if I can push them here (or if you even want me to)

eliseygusev commented 1 year ago

I have also fixed all the lint, format and type errors on my side. Not sure if I can push them here (or if you even want me to)

Thanks a lot for helping! Fixing the mistakes. Sorry that you have to manually run the pipeline, I just can not test this locally, so such mistakes occur... I would push code formatting together with tests fixes

bh2smith commented 1 year ago

I think I could separate the pipeline a bit (partition lint format type instead of having them run sequentially so you can at least see all of the issues from a single push). Note that you should be able to run all but e2e checks on from your own side (i.e. without an API_KEY)

bh2smith commented 1 year ago

Hows this going @eliseygusev - Have you seen the PR with Fixes to make this pass CI?

https://github.com/eliseygusev/dune-client/pull/1

Would love to get this merged!

eliseygusev commented 1 year ago

Yeah, I’ve seen the PR. Would update the code either today in the evening or tomorrow. Thanks for your support!

On Mon, 7 Nov 2022 at 15:18, Benjamin Smith @.***> wrote:

Hows this going @eliseygusev https://github.com/eliseygusev - Have you seen the PR with Fixes to make this pass CI?

eliseygusev#1 https://github.com/eliseygusev/dune-client/pull/1

Would love to get this merged!

— Reply to this email directly, view it on GitHub https://github.com/cowprotocol/dune-client/pull/31#issuecomment-1305684131, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBEHL6IQGHRB6YDZVRDXQLWHEFRXANCNFSM6AAAAAARV6GMTA . You are receiving this because you were mentioned.Message ID: @.***>

eliseygusev commented 1 year ago

Ok, so merged your request. Thanks a lot for it! But i'm kind of lost about whether there is something left to fix before merging?

eliseygusev commented 1 year ago

Fixed the linters but not sure whats happening with the tests. Is it permissions' things or something is failing in the code?

bh2smith commented 1 year ago

I think its all good. We can merge and publish a beta release from there. I won't have time to test it out this week though, but at least it will be available.

bh2smith commented 1 year ago

It is a permission thing, your branch does not have access to our secrets.

bh2smith commented 1 year ago

but you can also run these lint checks on your own side before pushing:

black ./ && pylint dune_client && mypy dune_client --strict
eliseygusev commented 1 year ago

but you can also run these lint checks on your own side before pushing:

black ./ && pylint dune_client && mypy dune_client --strict

yeah, sorry, my bad. always forget about it. can I add Makefile with commands for linters and tests so that one can just make lint to make the code pass linters?

bh2smith commented 1 year ago

yeah, sorry, my bad. always forget about it. can I add Makefile with commands for linters and tests so that one can just make lint to make the code pass linters?

That's great, but in a separate PR.

eliseygusev commented 1 year ago

That's great, but in a separate PR.

Ok, will add it soon. Is there anything else in this PR left to do on my side? Or you will just merge and publish by yourself?

bh2smith commented 1 year ago

Is there anything else in this PR left to do on my side?

Fix the failing lint! I will do the rest.

eliseygusev commented 1 year ago

Fix the failing lint! I will do the rest.

I wasn't sure why mypy failed in CI. I didnt touch interface.py and locally mypy --strict works. However, I think I did fix it with the addition of abc.abstract_method should be good now

bh2smith commented 1 year ago

Has been published under https://pypi.org/project/dune-client/0.2.0b0/

Thanks very much for your contribution!

bh2smith commented 1 year ago

I am trying to use this async client and keep running into these errors


RuntimeError: Timeout context manager should be used inside a task
2022-11-16 00:49:21,368 ERROR asyncio Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x106ba64a0>

I have tried putting a close session after the refresh call, like so

response = await self.dune.refresh(query, ping_frequency=10)
await self.dune.close_session()

but still get them. Would love if you could show me how to use this from an external package and maybe make it somehow more convenient for external users.

I have written some elementary "async" wrapper code on the regular DuneClient like:

async def fetch(self, query: Query) -> list[DuneRecord]:
    response = await asyncio.to_thread(
        self.dune.refresh, query, ping_frequency=10
    )

(which could probably be pushed deeper into the _post and _get methods), but it works just fine without all this "close_session" stuff. Is there something wrong with this simplified approach?