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

MessageCache and LightNode #59

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

MessageCache and LightNode #59

wants to merge 20 commits into from

Conversation

MHHukiewitz
Copy link
Member

Reopened with smaller scope from #31. Depends on #54.

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
Copy link
Member Author

I started using v1 posts instead of v0 posts. I wonder why we did not earlier, as it seems a cleaner, albeit minimalistic, endpoint for getting posts. There should be no problem in changing back to v0 posts though.

@MHHukiewitz
Copy link
Member Author

MHHukiewitz commented Sep 27, 2023

A few points on my ToDo list:

  • DomainNode: Should keep a synchronized cache with a subset of all Aleph messages.
    • Only allow messages to be posted, that fall under the scope of the Domain, meaning they would not be filtered by given initialization parameters.
    • Should remove messages from the cache, if they not reappear as confirmed in the watch_messages stream, after some pre-defined timeout.
  • Make BaseAlephClient and BaseAuthenticatedAlephClient pure interfaces.
  • Add interfaces for UserSessionSync and AuthenticatedUserSessionSync
    • Let MessageCache implement UserSessionSyncInterface instead of BaseAlephClient, as all provided methods can be sync.
    • Implement DomainNode.__enter__() with a DomainNodeSync class.
  • Add parameter classes like in Refactor: Filters were messy with duplication #58 for all of the above.

Copy link
Contributor

@odesenfans odesenfans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed ~70% of it, some changes and questions.

src/aleph/sdk/conf.py Show resolved Hide resolved
src/aleph/sdk/models.py Outdated Show resolved Hide resolved
src/aleph/sdk/node/__init__.py Outdated Show resolved Hide resolved
src/aleph/sdk/node/__init__.py Outdated Show resolved Hide resolved
src/aleph/sdk/node/__init__.py Outdated Show resolved Hide resolved
src/aleph/sdk/node/__init__.py Outdated Show resolved Hide resolved
src/aleph/sdk/node/__init__.py Outdated Show resolved Hide resolved
src/aleph/sdk/node/__init__.py Outdated Show resolved Hide resolved
src/aleph/sdk/node/common.py Outdated Show resolved Hide resolved
tests/integration/itest_forget.py Outdated Show resolved Hide resolved
@MHHukiewitz MHHukiewitz changed the title MessageCache and DomainNode MessageCache and LightNode Oct 25, 2023
@MHHukiewitz
Copy link
Member Author

Implemented all of the suggested changes by @odesenfans, highlights:

  • Broadcasted messages are now being polled up to 50 seconds for their status every 5 seconds. If they fail to become processed, they are removed from the cache.
  • Validating that the database schema is the same as defined in the SDK's model, raising an error and asking the user to delete or migrate the existing DB.
  • Aggregates are now correctly being aggregated and materialized in an Aggregate table.
  • Forget messages are now taken into consideration and they delete entries in the cache.
  • File downloads are now cached, too.

@MHHukiewitz MHHukiewitz force-pushed the mhh-domain-node branch 3 times, most recently from a146f5a to 0e3102f Compare October 25, 2023 21:51
@MHHukiewitz MHHukiewitz reopened this Nov 13, 2023
@MHHukiewitz MHHukiewitz dismissed odesenfans’s stale review November 13, 2023 13:26

Addressed issues, but Olivier can't review them anymore.

Copy link

Failed to retrieve llama text: ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))

@MHHukiewitz
Copy link
Member Author

Failed to retrieve llama text: ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))

Context size not enough, I rate this PR manually as BLACK

@MHHukiewitz MHHukiewitz added the BLACK This PR has critical implications and must be reviewed by a senior engineer. label Nov 13, 2023
@MHHukiewitz MHHukiewitz removed the request for review from odesenfans November 17, 2023 11:08
A LightNode can synchronize on a subset, or domain, of aleph.im messages. It relies on the MessageCache, which manages a message database with peewee.
…ust `create_program` methods on `LightNode`
nesitor
nesitor previously approved these changes Feb 1, 2024
@hoh
Copy link
Member

hoh commented Feb 1, 2024

This PR adds +1,937 and -16 lines of code to the project.

What is the impact of this PR on the test coverage ?

@MHHukiewitz
Copy link
Member Author

What is the impact of this PR on the test coverage ?

It actually improved relatively, as the new code has more coverage than the existing one.

Antonyjin and others added 12 commits February 8, 2024 19:50
ssl:True [SSLCertVerificationError: (1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate

when using the function: AuthenticatedAlephHttpClient
I searched on the internet for a way to solve this problem, but all the commands/advice given didn't work.

So I thought it would be a good idea to give the user the option of specifying a specific SSL certificate if they wish.

This worked in my case and gave me the option of continuing to use the SDK provided by Aleph.
I had the following problem:

ssl:True [SSLCertVerificationError: (1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate

when using the function: AuthenticatedAlephHttpClient
I searched on the internet for a way to solve this problem, but all the commands/advice given didn't work.

So I thought it would be a good idea to give the user the option of specifying a specific SSL certificate if they wish.

This worked in my case and gave me the option of continuing to use the SDK provided by Aleph.
Co-authored-by: Mike Hukiewitz <70762838+MHHukiewitz@users.noreply.github.com>
The problem was with the connector type:

src/aleph/sdk/client/http.py:55:25 : error : Incompatible types in assignment (expression has type "UnixConnector", variable has type "Optional[TCPConnector]")

I have therefore declared the connector as a union of possible BaseConnector or None types. This means it can be any aiohttp BaseConnector or None.
Users could not easily import or migrate accounts using their mnemonic representation.

Solution: Add a static method `from_mnemonic` on the `ETHAccount` class.

Discussion: This is a first step and this behaviour can be extended to more chains in the future.
Problem: Instances don't use program encodings. This argument was left from refactoring.

Solution: Drop the argument.
Problem: Users of the SDK could not create instances with a specific payment method such as token streams.

Solution: Add a new argument, `payment`, to `AuthenticatedAlephClient` and use it in the "Instance" messages generated.

The argument is optional and defaults to "hold" on "ETH" for backward compatibility.

Discussion:
The argument is added just after mandatory arguments so it can be made mandatory in the future. This may however break backward compatibility with existing code that does not call the function using keywords arguments.
Problem: too strict aleph-message dependency

Solution: loosen it to accept compatible versions to 0.4.2

Co-authored-by: mhh <mike.hukiewitz@robotter.ai>
Problem: too strict aleph-message dependency

Solution: loosen it to accept compatible versions to 0.4.2

Co-authored-by: Hugo Herter <git@hugoherter.com>
@@ -1,6 +1,6 @@
from pkg_resources import DistributionNotFound, get_distribution

from aleph.sdk.client import AlephHttpClient, AuthenticatedAlephHttpClient
from aleph.sdk.client import AlephHttpClient, AuthenticatedAlephHttpClient, LightNode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current discussion with @hoh : we might want to use another name for this class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLACK This PR has critical implications and must be reviewed by a senior engineer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants