-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support Postgres for custom data layer #825
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.
Great effort here, and I'd like to take a closer look at this later, here are a couple points from my automated review:
Code is fairly complex, and could use some functional refactoring and perhaps a bit more documentation on necessary env or config requirements. There are no tests. I would think a basic sanity check would be needed here, but I'm not sure what the team considers best practices here.
sql_upsert, sql_select, and sql_delete could use a helper function for the duplicated code. I also have a flag for security considerations against SQL injection. Not sure the risk/effort to support parameterized queries.
Database calls should be wrapped with error handling, not just logging. Have you checked in the literalAI SDK to see how the team handles failures currently?
TODOs shouldn't be in a PR. They need to be completed, or removed prior to approval.
Run this through ruff --autofix as well.
Keep up the good work!
@hayescode would it make sense to use SQLAlchemy to allow different SQL engines it supports? I'm working on a similar feature and opted for that route. |
@dahifi thanks for your review!
I don't see tests on the other data layers, are you talking about the github workflows?
This code is already parameterized and based on my reading of the documentation currently guards against SQL injection. Could you share some details about the security flag you speak of?
sql_upsert, sql_select, and sql_delete all have try/except blocks in addition to the logging similiar to the literal SDK. I'm not sure I understand.
Fair. I moreso put those TODO as an FYI to reviewers that I know this code can be improved in various ways, but imo is un-necessary for merging as many are 'nice-to-haves'. I'll remove those comments now.
I'll look into this. If this is about the mypy test failures i'm running that through GPT4 right now.
Thanks again for looking this over and for your feedback! |
@nixent I actually started with SQLAlchemy but ran into a blocker but can't remember exactly what it was but |
@nixent ya know I recently upgraded my SQL alchemy version (had to use different version because I was also using it for Databricks) and it seems to work now. I'm going to close this PR for now and try to use that instead because I agree that would be a better solution and people could plug in their own dialects. Great feedback! |
@nixent i just refactored the whole thing using SQLAlchemy 2.0.28 and it WORKS! I dunno what the deal was before. Thanks for your comment inspiring this. I'll try to get the PR out tomorrow, just need to do some more testing and fix these mypy issues. Cheers! |
Hey ! Thanks a lot for this, it's very interesting and useful ! I have some kind of bug, I'm not sure if it comes from chainlit, your code or something I did, but maybe you guys can help : Basically, when you leave the current conversation (conversation A) by clicking on a past chat, then refresh the page and go back to conversation A by clicking on it in the past chats section and resuming, the messages are wrongly ordered, the user messages all come first, then all the AI messages follow. Furthermore, they are not always ordered the way they were typed in. This doesn't happen when you leave the chat session by using new chat or refreshing the page. I assume this has something to do with the way the messages are ordered when inserting them or retrieving them from postgres, but I'm not sure. Any idea where this might come from ? How can I adress this ? Thanks a lot guys ! Here is what I mean (the user messages are in the wrong order) : |
Are you on the latest code? I also saw this but fixed it (I think). I was sorting by createdAt, but it shoulda been |
I'm using the 394 lines version of the code currently in the main branch. I tried to change the request code near line 140 from
to
but I still have issues with messages ordering. Is that what you meant ? Also, it would seem after several tests that user and ai messages are not necessarily bundled. It really just seems like when you click a past chat and then refresh, it messes up the order of whatever conversation you were having before the past chat. |
This PR is dead look at #836 |
@KillianLachx , I'm also facing this conversation order issue for my custom data layer for azure cosmos db. Can advise how do you resolved the issue? |
Overview
Test System
How to configure
Run this SQL DDL in your postgres database
DDL
Add
POSTGRES_URI
environment variable in your.env
fileAdd this to your
app.py
Optional: Blob Storage
app.py