-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: various breaking bugs with local LLM implementation and postgres docker. #1355
Conversation
And to go back to fix 1, changing the uri in config works for memgpt run, but not memgpt server. Will try to find the issue tonight. It seems to resave the config file with 5324 in place of 8888. |
@@ -379,29 +380,33 @@ def list_data_sources(self): | |||
unique_data_sources = session.query(self.db_model.data_source).filter(*self.filters).distinct().all() | |||
return unique_data_sources | |||
|
|||
def query_date(self, start_date, end_date, offset=0, limit=None): | |||
def query_date(self, start_date, end_date, limit=None, offset=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.
small question - is there any reason why this line is a diff? (args got swapped?)
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 fixes what I reference in 2.1 above. The order of arguments is different from that of the callers, unless I'm interpreting the intent of what is supposed to be returned. Maybe a previous version used the keywords and order didn't matter, but the current callers of these functions don't so order does matter.
Ok, seems that when I push new commits to my fork, it automatically gets added here. I guess I should have created a new branch or something. See #1362 . This commit makes it so that the server only overrides the config file settings for the postgres uri if, and only if, environmental variables are set to provide one. It feels like a hacky solution, but it minimizes risk for potential unwanted behavior with other parts of memgpt that use settings.memgpt_pg_uri. Currently, settings.memgpt_pg_uri always returns a value and if the environment variables are not set, it returns a default memgpt/localhost/5432 uri. This then overrides the config file values and it then saves the config file with the default. Bad behavior. |
Tests should be OK on main (probs have a bug in our tests @sarahwooders). Thank you for the PR @madgrizzle !! |
Please describe the purpose of this pull request.
This is a series of fixes to allow local LLMs to work properly.
edit: I realized while driving into work how to work around this so am removing "Fix 1". I think adding a note in the docs that if you are using docker, during the configuration process you need to edit the default entry for the postgres connection string to change 5324 to 8888 like below:
? Enter postgres connection string (e.g. postgresql+pg8000://{user}:{password}@{ip}:5432/{database}): postgresql+pg8000://memgpt:memgpt@localhost:8888/memgpt
Regardless, Fixes 2 and 3 are very important to make local LLMs actually work.
Fix 1- db/run_postgres.sh:It's hard for me to believe I'm the only one with this problem, but for whatever reason, 8888:5324 did not work with docker file. I don't know where the 8888 comes from so I set it to 5324:5324 and it works.
How to test Fix 1Try to start PostGres docker using db/run_postgres.sh and then start memgpt to see it connect. Fails without it.
Fix 2
See Issue db query_text and query_date is broken due to mismatched arguments (and other issue) #1347
Fix 2.1. Change the order of arguments to fix the mismatch. I think it would be better to be consistent with names of the arguments, but this was the simplest change.
Fix 2.2. Added filters to exclude the system prompts and tool from conversation searches.
Fix 2.3. Fix, at least for PostGres, the format of the date in query_date SQL command. See query_date fails with 'operator does not exist: timestamp with time zone >= character varying' #1343
How to test Fix 2
I find asking an LLM with some conversational history to explicitly perform a certain search will cause tend to make it actually call the function. So telling it, "Perform a conversation_search for my name" will cause it to search on "name" as the query. Without fix 2.1, it would return too many responses (more than a page of 5 messages). Fix 2.2 eliminates the system prompt (which is vitally important) from being returned. With fix 2.3, you should be able to ask for a conversation_search_date without it erroring out.
Fix 3
-memgpt/data_types.py & memgpt/memory.py
See #1281
I don't know if this is a good fix, but it resolves the issue. Seems that the transition to Message objects wasn't fully implemented and left some things broken for local LLMs.
How to test Fix 3
Have the LLM perform a conversation_search and it should work with the fix. Otherwise it will fail.
Have you tested this PR?
Yes, this is the not the intended final result (I wanted a summary, but not to update core memory), but it shows it actually works and doesn't error out:
Related issues or PRs
#1347 #1343 #1281
Is your PR over 500 lines of code?
Nope
Additional context
Would appreciate feedback if something needs to be changed.