-
Notifications
You must be signed in to change notification settings - Fork 47
Introduce the LLM session manager classes #141
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
Conversation
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 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 inputload_previous_sessions
- loads previous session data (how is this diff than the first method)store
- store exchanges into the db!
Codecov ReportAttention: Patch coverage is
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. |
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.
looking solid, left some more thoughts here
d9ccb26
to
c20d23e
Compare
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 couple comments mainly for clarification and style. In general, looks good!
|
||
schema = IndexSchema.from_dict({"index": {"name": name, "prefix": prefix}}) | ||
|
||
schema.add_fields( |
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.
Style comment: might make a function for creating index and adding fields outside the 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.
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.
…alidation to field names
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.
Great work. Left two small notes, but after we address those, let's get this merged!
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.
Looking good left a few thoughts. Since this PR is pretty large and involved it might be worth using 15 minutes at the end of standup to make sure we all really get it before merge otherwise probably pretty much there
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.
Great work -- pulling this into main and we can scope a few updates over the next few weeks before our next release.
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.