Async client #31

eliseygusev · 2022-11-03T09:48:29Z

No description provided.

github-actions · 2022-11-03T09:48:45Z

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

.pylintrc

dune_client/base_client.py

dune_client/client_async.py

dune_client/interface.py

bh2smith

I think this is coming along very well! Nice work here and nice code style. Your improvement suggestions are fantastic. Given that it is passing all the tests, I would say its ready for merge. Are there any e2e tests you think we might be missing?

Also, please take a look at the CLA thing

.gitignore

dune_client/base_client.py

dune_client/client_async.py

dune_client/interface.py

bh2smith · 2022-11-03T11:18:13Z

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 · 2022-11-03T11:32:28Z

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 · 2022-11-03T11:43:37Z

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 · 2022-11-04T22:07:32Z

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

eliseygusev · 2022-11-04T22:13:00Z

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 · 2022-11-04T22:15:59Z

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 · 2022-11-05T11:40:18Z

dune_client/base_client.py

+    def query_to_params(self, query: Query) -> Dict[str, dict]:
+        """Transforms Query objects to params to pass in API"""
+        return {
+            "query_parameters": {
+                p.key: p.to_dict()["value"] for p in query.parameters()
+            }
+        }


This method belongs on the query object (note that it does not make use of self) and has a single "query" attribute.

Yeah, that's true! Thanks for noticing. I guess we can move it to be a static method of Query dataclass?

Its not a static method of the query class (it takes a query as input -- i.e. self)

query.request_format()

or

query.as_request()

Not sure the best name

I like "request format"!

Resolved this and added unit-test for the functionality

dune_client/client_async.py

bh2smith · 2022-11-05T11:54:42Z

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 · 2022-11-05T12:00:07Z

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 · 2022-11-05T12:01:55Z

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 · 2022-11-07T14:17:52Z

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

eliseygusev#1

Would love to get this merged!

eliseygusev · 2022-11-07T14:19:58Z

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 <eliseygusev#1> Would love to get this merged! — Reply to this email directly, view it on GitHub <#31 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBEHL6IQGHRB6YDZVRDXQLWHEFRXANCNFSM6AAAAAARV6GMTA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Fix Async Errors

eliseygusev · 2022-11-08T13:39:16Z

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 · 2022-11-08T13:51:09Z

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

bh2smith · 2022-11-08T13:51:18Z

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 · 2022-11-08T13:52:05Z

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

bh2smith · 2022-11-08T13:53:24Z

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

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

eliseygusev · 2022-11-08T13:55:05Z

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 · 2022-11-08T13:56:25Z

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 · 2022-11-08T13:57:42Z

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 · 2022-11-08T14:00:56Z

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

Fix the failing lint! I will do the rest.

eliseygusev · 2022-11-08T14:05:03Z

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 · 2022-11-08T14:46:46Z

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

Thanks very much for your contribution!

bh2smith · 2022-11-15T23:54:08Z

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?

eliseygusev commented Nov 3, 2022

View reviewed changes

.pylintrc Outdated Show resolved Hide resolved

eliseygusev commented Nov 3, 2022

View reviewed changes

dune_client/base_client.py Show resolved Hide resolved

eliseygusev commented Nov 3, 2022

View reviewed changes

dune_client/client_async.py Outdated Show resolved Hide resolved

eliseygusev commented Nov 3, 2022

View reviewed changes

dune_client/interface.py Outdated Show resolved Hide resolved

eliseygusev mentioned this pull request Nov 3, 2022

Make AsyncDuneClient #28

Closed

bh2smith approved these changes Nov 3, 2022

View reviewed changes

.gitignore Show resolved Hide resolved

dune_client/base_client.py Show resolved Hide resolved

dune_client/client_async.py Outdated Show resolved Hide resolved

dune_client/interface.py Outdated Show resolved Hide resolved

Add AsyncDuneClient

3e47231

eliseygusev force-pushed the async-client branch 2 times, most recently from a67222e to 3e47231 Compare November 4, 2022 22:05

github-actions bot added a commit that referenced this pull request Nov 4, 2022

@eliseygusev has signed the CLA from Pull Request #31

806f220

bh2smith reviewed Nov 5, 2022

View reviewed changes

dune_client/client_async.py Outdated Show resolved Hide resolved

bh2smith reviewed Nov 5, 2022

View reviewed changes

dune_client/client_async.py Outdated Show resolved Hide resolved

lint, format type check

d264064

bh2smith mentioned this pull request Nov 5, 2022

Fix Async Errors eliseygusev/dune-client#1

Merged

bh2smith added 3 commits November 5, 2022 14:12

fixed all but one test

02de04d

fix execute with params - was missing post data

a51d355

remove dead code

2a03fc2

Merge pull request #1 from cowprotocol/fix-async-errors

6b57e96

Fix Async Errors

eliseygusev added 2 commits November 8, 2022 14:44

Add test for Query.request_format

00f9cca

Pylint+black

9d9f2b7

Abstract method to DuneInterface

9f45bd9

bh2smith merged commit 7271822 into duneanalytics:main Nov 8, 2022

github-actions bot locked and limited conversation to collaborators Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async client #31

Async client #31

eliseygusev commented Nov 3, 2022

github-actions bot commented Nov 3, 2022 •

edited

Loading

bh2smith left a comment •

edited

Loading

bh2smith commented Nov 3, 2022

bh2smith commented Nov 3, 2022 •

edited

Loading

bh2smith commented Nov 3, 2022

eliseygusev commented Nov 4, 2022

eliseygusev commented Nov 4, 2022

eliseygusev commented Nov 4, 2022

bh2smith Nov 5, 2022

eliseygusev Nov 5, 2022

bh2smith Nov 5, 2022 •

edited

Loading

eliseygusev Nov 8, 2022

eliseygusev Nov 8, 2022

bh2smith commented Nov 5, 2022

eliseygusev commented Nov 5, 2022

bh2smith commented Nov 5, 2022

bh2smith commented Nov 7, 2022

eliseygusev commented Nov 7, 2022 via email

eliseygusev commented Nov 8, 2022

eliseygusev commented Nov 8, 2022

bh2smith commented Nov 8, 2022

bh2smith commented Nov 8, 2022

bh2smith commented Nov 8, 2022

eliseygusev commented Nov 8, 2022

bh2smith commented Nov 8, 2022

eliseygusev commented Nov 8, 2022

bh2smith commented Nov 8, 2022

eliseygusev commented Nov 8, 2022

bh2smith commented Nov 8, 2022

bh2smith commented Nov 15, 2022 •

edited

Loading

Async client #31

Async client #31

Conversation

eliseygusev commented Nov 3, 2022

github-actions bot commented Nov 3, 2022 • edited Loading

bh2smith left a comment • edited Loading

Choose a reason for hiding this comment

bh2smith commented Nov 3, 2022

bh2smith commented Nov 3, 2022 • edited Loading

bh2smith commented Nov 3, 2022

eliseygusev commented Nov 4, 2022

eliseygusev commented Nov 4, 2022

eliseygusev commented Nov 4, 2022

bh2smith Nov 5, 2022

Choose a reason for hiding this comment

eliseygusev Nov 5, 2022

Choose a reason for hiding this comment

bh2smith Nov 5, 2022 • edited Loading

Choose a reason for hiding this comment

eliseygusev Nov 8, 2022

Choose a reason for hiding this comment

eliseygusev Nov 8, 2022

Choose a reason for hiding this comment

bh2smith commented Nov 5, 2022

eliseygusev commented Nov 5, 2022

bh2smith commented Nov 5, 2022

bh2smith commented Nov 7, 2022

eliseygusev commented Nov 7, 2022 via email

eliseygusev commented Nov 8, 2022

eliseygusev commented Nov 8, 2022

bh2smith commented Nov 8, 2022

bh2smith commented Nov 8, 2022

bh2smith commented Nov 8, 2022

eliseygusev commented Nov 8, 2022

bh2smith commented Nov 8, 2022

eliseygusev commented Nov 8, 2022

bh2smith commented Nov 8, 2022

eliseygusev commented Nov 8, 2022

bh2smith commented Nov 8, 2022

bh2smith commented Nov 15, 2022 • edited Loading

github-actions bot commented Nov 3, 2022 •

edited

Loading

bh2smith left a comment •

edited

Loading

bh2smith commented Nov 3, 2022 •

edited

Loading

bh2smith Nov 5, 2022 •

edited

Loading

bh2smith commented Nov 15, 2022 •

edited

Loading