AlUlkesh / stable-diffusion-webui-images-browser

an images browse for stable-diffusion-webui
626 stars 111 forks source link

Add flag to offload database operations to a different location #223

Closed valyrie97 closed 1 year ago

valyrie97 commented 1 year ago

Simple additional flag to copy / backup the sqlite database to a different location. The related issue is that sqlite requires filesystem permissions that are not always present (for example on many network sharing protocols). this PR alleviates that concern by operating on the database locally, and copying it back to the in-repo database when transactions are written.

AlUlkesh commented 1 year ago

Hi @valyrie97,

Thanks for the PR. I think if we change this area, I'd rather have the option to set a local path for the database, instead of copying it back and forth.

So I would see this workflow:

What do you think? Any problems from your view? Could you implement that? If not I'll look into it.

valyrie97 commented 1 year ago

Yes and no, in different ways. My intention with this code was actually entirely to be able to store the database on a nas, and so, copying it locally doesnt solve my problem. say for example, i wish to completely reinstall my server, and keep the installation and database. i can just trash the machine, reinstall the OS, and load up the webui like nothing changed.

However, I did also consider the re-routing of the databse to a different location as a different feature, and simply got this working for my own use case, and thought to send it in. Im genuinely quite happy to go forth with implementing that as well. Just doesnt solve my specific problem (which may not be in scope for the project, and if so, so be it)

as for whether it should be a CLI or setting in SD, I think both have viable needs. for myself, I used a CLI, as previously the databse would not even load in to have a webui to change settings... I also treat most of the servers i run as throwaways, so CLI / ENVs just end up working better for scripting. However, plenty of people have their single installations and would love to probably see these options in the webui.

In conclusion, i see a matrix of 4 features here: tmp database offloading and original database relocating, both in CLI and in-webui settings. all of which i think is fairly trivial and could be within this PR.

I hope that kindof spells more of where my brain was at, as i kindof just threw this at the wall after messing around in my local install :)

Edit: I didnt clarify. Yes, I would be happy to implement any of that set of features, as i was already toying around with these ideas in making these changes.

AlUlkesh commented 1 year ago

Yes, thanks a lot for the clarification. In that case I would suggest the following course of action:

Let's merge this PR as is, so the workaround is in place as soon as possible.

Whenever you have time, I'd be happy to get more changes that answer the other issues.

That being said, I have a few small notes about the current PR:

valyrie97 commented 1 year ago

@AlUlkesh great catches, Ill get those sent in ASAP, and start working on the other features :)

valyrie97 commented 1 year ago

To your note about the multiple connection methods, I normalized the connections to use a single transaction context, across all the operations. Im not by no means a python programmer, but it seemed to be the simplest solution to add some consistency and the least complexity. Hope thats alright.

If this looks good and gets pulled in ill get started on the settings ui and relocating database stuff

AlUlkesh commented 1 year ago

Oh wow, that solution with contextmanager is very elegant. I like it!

Does this version run on your end? I get this on startup:

  File "C:\tools\Ai\stable-diffusion-webui\extensions\stable-diffusion-webui-images-browser\scripts\image_browser.py", line 460, in cache_exif
    conn, cursor = wib_db.transaction_begin()
AttributeError: module 'scripts.wib.wib_db' has no attribute 'transaction_begin'

I guess some code in the main module also needs to be changed?

valyrie97 commented 1 year ago

Hahaha, yeah i totally was focused on the db module i never looked at the other one, and forgot to restart webui. i get the same. theres plenty of transactions i guess in the main file. simple enough to migrate them to transactions

AlUlkesh commented 1 year ago

Thanks again! I esp. really like the addition of contextmanager. 😃

valyrie97 commented 1 year ago

@AlUlkesh Of course! also i had that conn => cursor commit ready and i just forgot to push haha Moving onto other features :)