add api, distributed setup #135
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alex-dixon
commented
Aug 12, 2024
src/ell/studio/config.py
Outdated
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
|
|
||
| def model_post_init(self, __context): |
There was a problem hiding this comment.
This should let us do Config() instead of Config.create() and have the same functionality
alex-dixon
commented
Aug 12, 2024
| # This intends to honor the default we had set in the CLI | ||
| storage_dir = os.getcwd() | ||
| # todo. better default? | ||
| self.storage_dir = os.getcwd() |
There was a problem hiding this comment.
I’m ending up with SQLite dbs in various places. Maybe we standardize on ell home as a default?
alex-dixon
commented
Aug 12, 2024
alex-dixon
commented
Aug 12, 2024
src/ell/studio/connection_manager.py
Outdated
There was a problem hiding this comment.
let me know if you want to restore this. Broadcast was replicated in the new “pub sub” interfaces. connection moved to the ws handler.
alex-dixon
commented
Aug 12, 2024
alex-dixon
commented
Aug 12, 2024
alex-dixon
commented
Aug 12, 2024
|
There’s an error in the api implementation where a parent invocation isn’t in the db at the time a child is being written. Investigating. |
|
I can't repro this in tests. Can't repro it with sqlite either. So I think it's something wrong with the rest api implementation, differences between sqlite and postgres, or a race condition that the api/postgres impl encounters but sqlite doesn't. |
|
Adding |
|
My big question here is can we have su[prot for both this server client version and one version where we actually still ahve sqlite? |
|
@MadcowD Yes :) I took care to preserve that. it should work just as it did before. the distributed implementation is completely optional but works the same |
|
we're gonna merge i swear to god |
|
From yesterday’s discussion:
|
|
OK, so what we should do here is ref factor studio to be a bit more self-contained in the package having a separate L_studio package and self containing all of the docker file related stuff in one directory so as to not confuse developers. That would make this a bit more clean |
|
Can try this weekend |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to provide a unified architecture for two primary use cases:
To support ell’s features across both, we add a pub sub implementation to enable realtime updates when ell is used with network databases.
A new api server allows the core library to delegate responsibility for writing lmps and traces in a distributed context. This also creates space for ell’s backend to grow, to the extent it may write to multiple databases or other backend services. In this pr, it writes to the configured database and broadcasts its writes to the pub sub implementation.
Studio has been updated to optionally subscribe to these broadcasts and republish them to websocket clients.
If this is ok here would be next steps:
trackI should probably mention I do not have mastery in Python. Overall things seem to work but more testing would be good.
Breaking changes