Initial pass at PostgreSQL session management #1163
Conversation
|
Wanted to get the code up here in case there was some discussion on how to do things. Will look into the documentation tomorrow. |
|
Got a little stuck on the I took a stab at documentation. Open to feedback on structuring. Overall I wanted the postgres session to be in extensions because psycopg can be somewhat heavy on certain platforms. But I wasn't sure if the docs should go with extensions or the other memory docs. |
ff857ed to
be28fbc
Compare
|
Should be ready now. Have been running it in our testing for about a week now. |
be28fbc to
4498798
Compare
9f0d7a1 to
78aa642
Compare
|
Tests fixed. Missed the test changes I had made locally. |
ef1a02d to
7d392a1
Compare
| def __init__( | ||
| self, | ||
| session_id: str, | ||
| pool: AsyncConnectionPool, |
There was a problem hiding this comment.
Why you use a hole connection pool just for one session management? One connection will be enough.
There was a problem hiding this comment.
In my use case this is running in a service with many concurrent sessions. A shared connection pool is used between them. That's why you can initialize it from a connection string, or using an existing pool
There was a problem hiding this comment.
Okay, that is your way, it is not bad.
7d392a1 to
4eb7af4
Compare
* Add postgres_session.py
* Implements session management using PostgreSQL
* Accepts a pool as a default argument
* Additional class method for creation from a connection string
* Add optional-dependency for psycopg
Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
* Fix warning from opening a pool in the constructor: https://www.psycopg.org/psycopg3/docs/news_pool.html#psycopg-pool-3-2-2 * Add integration tests that can be run locally * Fix get_items with limit Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
4eb7af4 to
1bfcec4
Compare
| @@ -38,6 +38,7 @@ voice = ["numpy>=2.2.0, <3; python_version>='3.10'", "websockets>=15.0, <16"] | |||
| viz = ["graphviz>=0.17"] | |||
| litellm = ["litellm>=1.67.4.post1, <2"] | |||
| realtime = ["websockets>=15.0, <16"] | |||
| psycopg = ["psycopg[pool]>=3.2.9,<4"] | |||
There was a problem hiding this comment.
I'd recommend psycopg-binary so users don't need a compiler installed to use this https://pypi.org/project/psycopg-binary/
There was a problem hiding this comment.
Does this cover all platform/arch combinations? By not specifying binary, any user of this library should be able to choose binary if they'd like. If a binary doesn't exist for a platform then won't over specifying the dependency cause issues? (I have not experimented with the binary library)
| def __init__( | ||
| self, | ||
| session_id: str, | ||
| pool: AsyncConnectionPool, |
There was a problem hiding this comment.
It would be nice to have this as an optional parameter with the default set to None. It can then be created in the __init__ if the user doesn't pass it explicitly.
There was a problem hiding this comment.
How could it be created? That was the intention of the from_connection_string classmethod
|
@seratch @rm-openai - May I know the expected timeline for this release? |
|
We've accepted this SQLAlchemy based implementation, which supports not only PostgreSQL but any other RDBs: #1357 Thus, to avoid duplicated implementations in the built-in modules, let us close this pull request now. Anyone who prefers a simpler one can use this PostgreSQL implementation by copying the code in their projects. We truly appreciate your time and efforts here. Thank you so much again! |
|
Makes sense. Thanks @seratch |
Summary
Test plan
Tests were written mocking the postgres calls. This was tested locally against a docker postgres database, with a variety of input messages.
Issue number
Related to #745
Checks
make lintandmake format