-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[WIP] - SQL Model Data Layer #2469
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
base: main
Are you sure you want to change the base?
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.
I think the initial file should be the creation of the tables, right? So that new users do not have to problem to create the tables by themself.
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.
Today our SQL tables are stored in cameCase which is abnormal so my idea for the 1st migration would rename the problematic columns like threadId to thread_id. That would be the most "hands-off" migration. I have not actually done that here yet though.
Alternatively we could create brand new tables in a new database schema and use COPY INTO scripts.
I really don't think keeping these as camelCase would be good long term as we'd have to handle this into the future for all changes and it's not the right way to make SQL tables.
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.
I agree for the camelCase.
I suggest we add conditional statements in here to also support the creation of all tables, if the user not already created the tables himself or he was using chainlit data layers before.
This way, we can ensure that after running the initial database migration, all users use the same database tables, not depending if the user already used data layers in the past.
This PR is stale because it has been open for 14 days with no activity. |
@github-actions Leave this PR open |
How can I support in this pull request? I was on holiday and then almost forgot this PR @hayescode |
@niklasdiehm here's the thread on it in discord (can also be found in gh issues) https://discord.com/channels/1088038867602526210/1421299306404647012 I've been very busy at work lately and will probably continue to be. Sorry I couldn't make more progress here. I think we need to align on a design and structure then we can split into pieces with this PR being the target branch. I think sqlmodel would be great (FastAPI + SQLAlchemy + Pydantic) but I'm not married to it. Things like the "metadata" column are a reserved word and requires work arounds. What are your thoughts? We'll take all of the help we can get so whatever piece of this you'd like we welcome contributions on. Thanks! |
Caution
This is currently a mockup and incomplete. This has not been tested and will not run.
This is an initial draft for consolidating all SQL data layers using FastAPI's sqlmodel (Pydantic + SQL Alchemy).
For now I have created the new models in a new models/ directory and are duplicated from step.py, element.py, etc. The idea would be to delete those once this is ready.
The main reasons for this change are:
replaces #1365
Notes:
metadata_
instead, then aliased for proper database and API usage.