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

Adds semantic session manager #141

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

Conversation

justin-cechmanek
Copy link
Collaborator

@justin-cechmanek justin-cechmanek commented Apr 23, 2024

Here's the proposed class for LLM session management. It support recent or full conversation storage & retrieval, as well as relevance based conversation section retrieval.

Copy link
Collaborator

@tylerhutcherson tylerhutcherson left a comment

Choose a reason for hiding this comment

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

Left some feedback! Great start. From an interface design perspective, I'd think about what are the key public methods that the user will have access to in order to manipulate or read from the session state. this is what I was able to derive :)

  • conversation_history - loads full conversation history. What about just last 10 messages (the naive session retrieval approach)?
  • fetch_context - fetches session data based on semantic similarity to text input
  • load_previous_sessions - loads previous session data (how is this diff than the first method)
  • store - store exchanges into the db!

redisvl/extensions/sessionmanager/__init__.py Outdated Show resolved Hide resolved
redisvl/extensions/sessionmanager/session.py Outdated Show resolved Hide resolved
redisvl/extensions/sessionmanager/session.py Outdated Show resolved Hide resolved
redisvl/extensions/sessionmanager/session.py Outdated Show resolved Hide resolved
redisvl/extensions/sessionmanager/session.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 111 lines in your changes are missing coverage. Please review.

Project coverage is 83.07%. Comparing base (ce0f710) to head (3a117eb).

❗ Current head 3a117eb differs from pull request most recent head 5d4f34b. Consider uploading reports for the commit 5d4f34b to get more accurate results

Files Patch % Lines
redisvl/extensions/session_manager/session.py 0.00% 109 Missing ⚠️
redisvl/extensions/session_manager/__init__.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #141      +/-   ##
==========================================
- Coverage   88.60%   83.07%   -5.54%     
==========================================
  Files          29       31       +2     
  Lines        1667     1778     +111     
==========================================
  Hits         1477     1477              
- Misses        190      301     +111     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@tylerhutcherson tylerhutcherson left a comment

Choose a reason for hiding this comment

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

looking solid, left some more thoughts here

redisvl/extensions/session_manager/session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/session.py Outdated Show resolved Hide resolved
@justin-cechmanek justin-cechmanek marked this pull request as ready for review May 7, 2024 21:06
@justin-cechmanek justin-cechmanek changed the title draft PR to add semantic session manager Adds semantic session manager May 8, 2024
Copy link
Collaborator

@rbs333 rbs333 left a comment

Choose a reason for hiding this comment

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

Left a couple comments mainly for clarification and style. In general, looks good!

redisvl/extensions/session_manager/base_session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/base_session.py Outdated Show resolved Hide resolved

schema = IndexSchema.from_dict({"index": {"name": name, "prefix": prefix}})

schema.add_fields(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style comment: might make a function for creating index and adding fields outside the init

redisvl/extensions/session_manager/semantic_session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/standard_session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/standard_session.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tylerhutcherson tylerhutcherson left a comment

Choose a reason for hiding this comment

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

Left some comments too! Looking nice. Because we are introducing a new capability here into the library, we will need to discuss how to evaluate further and eventually roll this out.

redisvl/extensions/session_manager/base_session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/base_session.py Outdated Show resolved Hide resolved
redisvl/extensions/session_manager/semantic_session.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Session Management Feat.
4 participants