Skip to content

Conversation

@pythongosssss
Copy link
Member

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
Contributor

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
Contributor

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
@github-actions
Copy link
Contributor

(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
Contributor

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
adlerfaulkner pushed a commit to LucaLabsInc/ComfyUI that referenced this pull request Oct 16, 2025
* Add support for sqlite database

* fix
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.

7 participants