aleph-im / aleph-sdk-python

Python SDK library for the Aleph.im network
MIT License
3 stars 5 forks source link

Feature: SQLite-based Message Cache #31

Closed MHHukiewitz closed 1 year ago

MHHukiewitz commented 1 year ago

Problem

Some apps require access to the same messages over and over again. This puts unnecessary strain on the CCN API and increases the latency of apps using Aleph Messages.

Solution

This pull request proposes a solution by introducing a message cache implemented with Peewee and an SQLite database in the Aleph SDK. The message cache effectively stores frequently accessed messages, eliminating the need for repetitive CCN API calls. With the message cache in place, apps utilizing the Aleph SDK will experience enhanced efficiency and responsiveness in accessing messages.

MHHukiewitz commented 1 year ago

After introducing the AlephClientInterface, I get a mypy error, that I really don't get: https://github.com/aleph-im/aleph-sdk-python/actions/runs/5071082316/jobs/9107016057?pr=31

Even though I am exactly copying the signature of the watch_messages function:

    async def watch_messages(
        self,
        message_type: Optional[MessageType] = None,
        content_types: Optional[Iterable[str]] = None,
        refs: Optional[Iterable[str]] = None,
        addresses: Optional[Iterable[str]] = None,
        tags: Optional[Iterable[str]] = None,
        hashes: Optional[Iterable[str]] = None,
        channels: Optional[Iterable[str]] = None,
        chains: Optional[Iterable[str]] = None,
        start_date: Optional[Union[datetime, float]] = None,
        end_date: Optional[Union[datetime, float]] = None,
    ) -> AsyncIterable[AlephMessage]:

mypy raises:

src/aleph/sdk/cache.py:370:5: error: Return type "AsyncIterable[Any]" of "watch_messages" incompatible with return type "Coroutine[Any, Any, AsyncIterable[Any]]" in supertype "AlephClientInterface" [override] src/aleph/sdk/client.py:708:5: error: Return type "AsyncIterable[Any]" of "watch_messages" incompatible with return type "Coroutine[Any, Any, AsyncIterable[Any]]" in supertype "AlephClientInterface" [override]

MHHukiewitz commented 1 year ago

Looking into snapshot testing to figure out how to mock API responses.

MHHukiewitz commented 1 year ago

Thanks @hoh, this helps prioritizing on which points work on next for me.

MHHukiewitz commented 1 year ago

@hoh I'd like to point out that this line:

if isinstance(messages, AlephMessage):

is making tests fail in Python versions =< 3.9. The alternative is making comparisons with every single Aleph message type (PostMessage, AggregateMessage, ...) which I think might be a bit ridiculous.

As Python progressed already to 3.11 now, can it be expected of people wanting to use our newest SDK versions to use at least Python 3.10?

hoh commented 1 year ago

@hoh I'd like to point out that this line:

if isinstance(messages, AlephMessage):

is making tests fail in Python versions =< 3.9. The alternative is making comparisons with every single Aleph message type (PostMessage, AggregateMessage, ...) which I think might be a bit ridiculous.

The solution seems to be using isinstance(data, typing.get_args(Constant))

https://stackoverflow.com/a/73874277

As Python progressed already to 3.11 now, can it be expected of people wanting to use our newest SDK versions to use at least Python 3.10?

Not as long as we support Debian 11, which ships with Python 3.9. The SDK should work on as many environments as possible, and keep doing so for a while. The situation is different for PyAleph since it is only expected to be deployed in containers and already uses Python 3.11.

MHHukiewitz commented 1 year ago

So, resolved code duplication and other issues as pointed out by @hoh.

I made some breaking changes on the interface of the Client, which should have been done anyways (like changing the return type of get_posts() to a PostsResponse type, similar to get_messages() -> MessagesResponse. I hope this does not impact the review, splitting those changes in distinct PRs would be a chore. I would be OK with reviewing the changes together, if needed.

After extensive testing, whereby we are now reaching 61% test coverage, I am convinced that this branch is reliable and can be publicized in a future minor release.

MHHukiewitz commented 1 year ago

Closed for now, as it will be broken down into smaller PRs