Skip to content

Add support for sqlite database #8444

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

Merged
merged 2 commits into from
Jun 11, 2025
Merged

Add support for sqlite database #8444

merged 2 commits into from
Jun 11, 2025

Conversation

pythongosssss
Copy link
Collaborator

Cut down version of #8432, to just contain the database setup code.

@robinjhuang robinjhuang added the Core Core team dependency label Jun 6, 2025
@pythongosssss pythongosssss marked this pull request as ready for review June 6, 2025 18:51
@bigcat88
Copy link
Contributor

bigcat88 commented Jun 8, 2025

Suggestions:

  1. Use WAL mode from start in case the database is SQLite
  2. Use async from start to not rewrite code in future (even for SQLite)

Some code can be taken from here:

https://github.com/Visionatrix/Visionatrix/blob/ea4caf0a30f0a0c5a21d440be802cd2e624ffcf3/visionatrix/database.py#L210-L243

@bigcat88
Copy link
Contributor

bigcat88 commented Jun 8, 2025

Also It would be great to be able to turn off the default database entirely - or better yet, to introduce a set of well-defined asynchronous persistence interfaces. That way, anyone building on ComfyUI (or simply wanting to swap in a different database backend) could implement those interfaces and have their chosen storage engine used transparently.
Moreover, a plugin‐based system for this feature feels much more in line with ComfyUI’s modular design; in its current form the PR seems geared more toward a Comfy-Desktop design. (I may be off-base here, as I haven’t seen all of the internal discussion)

@webfiltered
Copy link
Collaborator

or simply wanting to swap in a different database backend

Is there an issue with the current ORM impl. that would prevent this? Adding a custom abstraction layer feels pretty impractical to maintain, at present.

@bigcat88
Copy link
Contributor

bigcat88 commented Jun 8, 2025

Is there an issue with the current ORM impl. that would prevent this? Adding a custom abstraction layer feels pretty impractical to maintain, at present.

As far as I know - no, you are absolutely right about not having to build an additional layer over SqlAlchemy. I probably didn't describe it quite accurately. By backends I meant SqlAlchemy engines, they are called backends in some places.


def get_db_path():
url = args.database_url
if url.startswith("sqlite:///"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the ^sqlite:/// check? I assume the ORM will have an appropriate fit if it's misconfigured.

@bigcat88 Hadn't noticed this - disregard my earlier comment!

@yoland68 yoland68 added the Run-CI-Test This is an administrative label to tell the CI to run full automatic testing on this PR now. label Jun 11, 2025
Copy link

(Automated Bot Message) CI Tests are running, you can view the results at https://ci.comfy.org/?branch=8444%2Fmerge

@comfyanonymous comfyanonymous merged commit 50c605e into master Jun 11, 2025
17 of 21 checks passed
@webfiltered
Copy link
Collaborator

n.b. Merged as-is to unblock work, and flagged for follow-up / testing regarding allowing other DBs.

@comfyanonymous comfyanonymous deleted the pysssss-database branch July 8, 2025 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Core team dependency Run-CI-Test This is an administrative label to tell the CI to run full automatic testing on this PR now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants