-
Notifications
You must be signed in to change notification settings - Fork 44.4k
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
Add the ability to capture and load snapshots of memory and message history #192
Conversation
@@ -114,10 +114,10 @@ def chat_with_ai( | |||
) | |||
|
|||
# Update full message history | |||
full_message_history.append( | |||
message_history.append( |
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 this a significant or required change?
@@ -178,14 +177,14 @@ def get_hyperlinks(url): | |||
|
|||
def commit_memory(string): | |||
_text = f"""Committing memory with string "{string}" """ | |||
mem.permanent_memory.append(string) | |||
memory.append(string) |
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.
Nice, the stuff in this area makes sense to me.
class DataStore(): | ||
|
||
def persist_message_history(self, id): | ||
return True |
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.
What about return False on everything for the abstract base class? Or if errors are involved, throwing the NotImplementedError?
message_history.set_history(f["message_history"]) | ||
return True | ||
except Exception as e: | ||
return e |
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 function returns either True
or Error
. Both a consistent boolean or an actual exception is better if you ask me.
@@ -248,6 +252,9 @@ def parse_arguments(): | |||
parser.add_argument('--speak', action='store_true', help='Enable Speak Mode') | |||
parser.add_argument('--debug', action='store_true', help='Enable Debug Mode') | |||
parser.add_argument('--gpt3only', action='store_true', help='Enable GPT3.5 Only Mode') | |||
parser.add_argument('--enable-snapshots', action='store_true', help='Enable Snapshots') | |||
parser.add_argument('--snapshot-path', type=str, default=None, required=False, help='Path for the snapshot directory') | |||
parser.add_argument('--snapshot-id', type=str, default=None, required=False, help='ID of the snapshot to load') |
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 all looks good right here
Can you rebase it on the current master? |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
Related to:
#9 #122 #124 #125
Primary discussion here:
#124 (comment)
TODO:
Tests
Code review
Improve code quality