-
-
Notifications
You must be signed in to change notification settings - Fork 3.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
feat: db migration #3595
feat: db migration #3595
Conversation
# Conflicts: # backend/apps/webui/models/functions.py # backend/apps/webui/routers/chats.py
# Conflicts: # backend/requirements.txt
…of-peewee BREAKING CHANGE/sqlalchemy instead of peewee
Fixed! |
@jonathan-rohde One thing we should test before merging this to dev/main is verifying update to 0.3.8 will work without any problem from other version like 0.2.0 when we didn't have supports for functions and etc. I'd greatly appreciate if you could verify this! |
@tjbck This will be most likely not working. The initial database migration of alembic is not throwing any errors during migration, if a table already exists. The existing table will just be ignored. However if between the older version (e.g. 0.2.0) a peewee-migration was adding new columns to existing tables, this change is not reflected in the alembic migration. One feature I have in mind which will cause such a conflict: oauth login. That was the reason why I flagged it as "breaking change" |
If restoring peewee migrations to alembic is all we need, that's what we'll have to do 😅 |
Perhaps we could add back in the old peewee migration code, and conditionally execute the peewee migration code only if |
I have been trying this out on an existing install from latest dev branch(then switching to this branch) using postgres database. Generally it seems okay, but I am experiencing "random" lockups, I haven't pinpointed taking any particular action to trigger this behavior yet Error Message on backend. This doesn't seem to recover without a restart of owui |
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.
@jonathan-rohde #3595 (comment) seems like it might be related to this change, I'm curious to know what the reason was behind this decision for opting-in for session factory instead of context manager. Just want to make sure that I won't be breaking anything if I were to update the code to use:
def get_db():
db = SessionLocal()
try:
yield db
finally:
db.close()
I appreciate the help as always!
The session factory has the advantage, that within one common context, for example a thread, you get the same session and you are able to chain the operations within one transaction. |
Extension of #3327