Skip to content
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

Merged
merged 34 commits into from
Jul 6, 2024
Merged

feat: db migration #3595

merged 34 commits into from
Jul 6, 2024

Conversation

tjbck
Copy link
Contributor

@tjbck tjbck commented Jul 2, 2024

Extension of #3327

@tjbck
Copy link
Contributor Author

tjbck commented Jul 3, 2024

Fixed!

@tjbck
Copy link
Contributor Author

tjbck commented Jul 3, 2024

@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!

@jonathan-rohde
Copy link
Contributor

@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"
In theory it should be possible to not create just one initial migration but recreate all the steps that peewee migration had. But it will be very tedious.

@tjbck
Copy link
Contributor Author

tjbck commented Jul 3, 2024

If restoring peewee migrations to alembic is all we need, that's what we'll have to do 😅

@tjbck
Copy link
Contributor Author

tjbck commented Jul 3, 2024

Perhaps we could add back in the old peewee migration code, and conditionally execute the peewee migration code only if migratehistory table is detected then bring the database up to the latest version, then remove (or rename) themigratehistory table to indicate the database has been successfully migrated?

@Peter-De-Ath
Copy link
Contributor

Peter-De-Ath commented Jul 3, 2024

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.
sqlalchemy.exc.TimeoutError: QueuePool limit of size 5 overflow 10 reached, connection timed out, timeout 30.00 (Background on this error at: https://sqlalche.me/e/20/3o7r)

This doesn't seem to recover without a restart of owui

@tjbck tjbck added the enhancement New feature or request label Jul 4, 2024
@tjbck tjbck added help wanted Extra attention is needed core core feature labels Jul 4, 2024
Copy link
Contributor Author

@tjbck tjbck left a 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!

@jonathan-rohde
Copy link
Contributor

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.
You will have more trouble doing it with a context manager. Although at the moment there are hardly any combined actions within one transaction in the peewee implementation.

@tjbck tjbck marked this pull request as ready for review July 6, 2024 06:46
@tjbck tjbck merged commit 8f6f766 into dev Jul 6, 2024
3 of 6 checks passed
@tjbck tjbck deleted the dev-migration branch July 9, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core core feature enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants