-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Feature: Redis Session Management #1247
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
base: main
Are you sure you want to change the base?
Conversation
@seratch @rm-openai would you mind taking a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few general comments. I think supporting redis is a great addition for many developers. I would like @rm-openai to make the final decision on whether we should have this though!
May I ask you to resolve the uv.lock conflicts? |
@seratch sure thing, that is done! |
@seratch tests are failing due to missing I think once that is done, tests will pass thanks for the reviews and the patience :) |
That's weird. This project has many tests with LiteLLM etc. Perhaps, the uv.lock in this PR needs updates to have redis dep for tests. |
exactly, that was the issue. should be good now. |
@seratch tests are passing locally, should be good to go now. would you mind giving one final pass? |
Need to resolve python 3.9 test failures and mypy errors. |
done! |
good news @seratch , all checks are passing 🚀 |
@@ -0,0 +1,1899 @@ | |||
{"dryrun":false,"job":"Tests/build-docs ","jobID":"build-docs","level":"info","matrix":{},"msg":"⭐ Run Set up job","step":"Set up job","stepid":["--setup-job"],"time":"2025-07-29T11:56:59+02:00"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you delete this file?
@@ -102,268 +97,3 @@ async def pop_item(self) -> TResponseInputItem | None: | |||
async def clear_session(self) -> None: | |||
"""Clear all items for this session.""" | |||
... | |||
|
|||
|
|||
class SQLiteSession(SessionABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some people may import this class from agents.memory.session; for backward-compatibility, can you import of the class in this file? Adding a simple comment mentioning the necessity of backward compatibility would be appreciated too.
@rm-openai This PR looks almost good to me; if you have any feedback, sharing them here would be appreciated |
@@ -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"] | |||
redis = ["redis[hiredis]>=6.2.0"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't include hiredis
here. Only redis is needed, and requiring hiredis
could cause issues on certain systems.
current_time = time.time() # Use float for higher precision | ||
|
||
# Check if session already exists | ||
exists = await client.exists(self.session_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a possible race here if 2 clients tried to create the same session at once? Does the client.exist
and client.hset
need to be in a multi/exec/watch?
a hash to store session metadata. | ||
""" | ||
|
||
def __init__( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense here to have the constructor take a redis client, and a classmethod for from_url
. Because if someone already has a redis client, they may want to reuse that. See here
no, client.exist and client.hset are atomic.
…On Fri, Aug 1, 2025, 8:51 PM Aidan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/agents/memory/providers/redis.py
<#1247 (comment)>
:
> + if self._redis_client is None:
+ self._redis_client = redis.from_url(
+ self.redis_url,
+ db=self.db,
+ decode_responses=True,
+ retry_on_error=[redis.BusyLoadingError, redis.ConnectionError],
+ retry_on_timeout=True,
+ )
+ return self._redis_client
+
+ async def _ensure_session_exists(self, client: redis.Redis) -> None:
+ """Ensure session metadata exists in Redis."""
+ current_time = time.time() # Use float for higher precision
+
+ # Check if session already exists
+ exists = await client.exists(self.session_key)
Is there a possible race here if 2 clients tried to create the same
session at once? Does the client.exist and client.hset need to be in a
multi/exec/watch?
—
Reply to this email directly, view it on GitHub
<#1247 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEPZYXJOLO3C5HJOXMJZQD3LOZKJAVCNFSM6AAAAACCLMU4FCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTAOBQGI2DCMBRHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
can you ellaborate on which issues?
in terms of the benefits - from redis python client readme:
“Hiredis is a C library maintained by the core Redis team. Pieter Noordhuis
was kind enough to create Python bindings. Using Hiredis can provide up to
a 10x speed improvement in parsing responses from the Redis server."
…On Fri, Aug 1, 2025, 8:48 PM Aidan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pyproject.toml
<#1247 (comment)>
:
> @@ -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"]
+redis = ["redis[hiredis]>=6.2.0"]
This shouldn't include hiredis here. Only redis is needed, and requiring
hiredis could cause issues on certain systems.
—
Reply to this email directly, view it on GitHub
<#1247 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEPZYWEMC2TKY2RS7RFW5T3LOZBPAVCNFSM6AAAAACCLMU4FCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTAOBQGIZTKNJWGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Someone may not want to use hiredis. Technically the requirement of this sdk is that the redis library exists. It does not depend on hireds. Requiring a user to use hiredis is overly prescriptive. |
they are not called together atomically. Is it an issue if client.hset is called twice? |
that sounds more like your opinion rather than an argument. I see no reason
to take hiredis out, only benefits.
…On Fri, Aug 1, 2025, 10:25 PM Aidan ***@***.***> wrote:
*artificial-aidan* left a comment (openai/openai-agents-python#1247)
<#1247 (comment)>
can you ellaborate on which issues? in terms of the benefits - from redis
python client readme: “Hiredis is a C library maintained by the core Redis
team. Pieter Noordhuis was kind enough to create Python bindings. Using
Hiredis can provide up to a 10x speed improvement in parsing responses from
the Redis server."
… <#m_2085295882385902969_>
On Fri, Aug 1, 2025, 8:48 PM Aidan *@*.*> wrote: @.** commented on this
pull request. ------------------------------ In pyproject.toml <#1247
(comment)
<#1247 (comment)>>
: > @@ -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"] +redis =
["redis[hiredis]>=6.2.0"] This shouldn't include hiredis here. Only redis
is needed, and requiring hiredis could cause issues on certain systems. —
Reply to this email directly, view it on GitHub <#1247 (review)
<#1247 (review)>>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ADEPZYWEMC2TKY2RS7RFW5T3LOZBPAVCNFSM6AAAAACCLMU4FCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTAOBQGIZTKNJWGA
. You are receiving this because you authored the thread.Message ID: *@*
.***>
Someone may not want to use hiredis. Technically the requirement of this
sdk is that the redis library exists. It does not depend on hireds.
Requiring a user to use hiredis is overly prescriptive.
—
Reply to this email directly, view it on GitHub
<#1247 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEPZYRXFO6WIKAM6TWWPET3LPEKRAVCNFSM6AAAAACCLMU4FCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNBVGY4DINRXGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Why we plan to store all the function tool calls in the message history? Shouldn't it be only user query and assistant reponse in message conversation key. While keeping all the history of function tool calls in another metakey as this is increasing the context window when session is provided in Runner. We need to have moving context window with limit set to 10 pairs that can be customizable from runner. This will greatly enhance llm response times, rate limits. |
It is not my opinion. There are platforms where a user may not want to be required to use hiredis. To pick a random example, This is not an argument about whether or not |
Add Redis Session Support and Restructure Memory Module
Context
I've fully switched to OpenAI Agents SDK at my company and I was looking for a distributed-systems friendly session management mechanism. I wasn't sure at the beginning if creating a
RedisSession
would defeat the "lightweight" purpose of OpenAI Agents SDK, so initially I've published openai-agents-redis on PyPi. It follows theSession
protocol and works quite similarly toSQLLiteSession
- but withredis
as the backend.Since I think the community would benefit from having this additional option for session management, I'm creating this PR - which incorporates the work I've done to support Redis for session management within OpenAI Agents SDK.
🚀 Overview
This PR introduces Redis-based session storage for the OpenAI Agents SDK and restructures the memory module for better organization and extensibility. The changes provide developers with a distributed, scalable alternative to SQLite sessions while maintaining full backward compatibility.
🎯 Key Features
✨ New Redis Session Implementation
RedisSession
: Full-featured Redis-based session storage with TTL supportRedisSessionManager
: Connection pooling and session management🏗️ Memory Module Restructuring
agents.memory.providers
📁 Changes Summary
New Files
src/agents/memory/providers/redis.py
- Redis session implementation (345 lines)src/agents/memory/providers/sqlite.py
- Extracted SQLite session (278 lines)src/agents/memory/providers/__init__.py
- Provider module initializationtests/test_redis_session.py
- Comprehensive test suite (553 lines, 33 tests)Modified Files
src/agents/memory/__init__.py
- Updated exports for new structuresrc/agents/memory/session.py
- Cleaned up to contain only protocol definitionsdocs/sessions.md
- Documentation updatepyproject.toml
- Added Redis dependency🔧 Technical Implementation
Redis Session Features
Key Capabilities
🧪 Testing
Test Coverage (33 tests)
RedisSession
classRedisSessionManager
classTest Features
Test Results
$ uv run pytest tests/test_redis_session.py -v ============================== 33 passed in 0.07s ==============================
📚 Documentation Updates
sessions.md
🔮 Future Considerations
This provider-based architecture enables:
📋 Checklist