stable-diffusion-webui-docker icon indicating copy to clipboard operation
stable-diffusion-webui-docker copied to clipboard

Comfy update and taesd and also medvram for a1111

Open unphased opened this issue 2 years ago • 5 comments

A group of small improvements that I found:

  • ComfyUI bump version to current master
  • Reduce weird repetitive steps in ComfyUI dockerfile (curious what the reason for checking out two commits was, previously. I kept the same reset --hard procedure but it's also not clear why not checkout the commit, or even just leave it at master, though that may break oneday but we will need to keep bumping it to keep it updated)
  • Automate setting up TAESD preview, which seems like a no brainer default setting as it is a superior preview compared to the default. Regarding this, at first I set up a new entry in services/comfy/extra_model_paths.yaml, but then I realized I could just deliver the TAESD decoders via dockerfile.
  • I took out the medvram flag from the a1111 args, I think we're keeping the instructions in wiki, so if we go ahead with this change we should add a note in the wiki saying if you have limited vram to manually add this flag. Because I find on my 3090's the use of medvram reduces speed significantly (16 it/s down to 10). This goes without saying for everything, but I'm cool with taking this change out if you think having the flag is a better default.

Update versions

  • comfy: https://github.com/comfyanonymous/ComfyUI/commit/91ed2815d542c96fdad75edba2205140de3cbba6

unphased avatar Jul 14 '23 11:07 unphased

The multiple checkouts are to reduce the time from having to download the repositories and run installs. When simply just bumping the version, you change the 2nd checkout so it keeps the previous layer and all it has to do is download the current changes upstream. It doesn't matter so much in Comfy because it isn't super large but it's a generally nice to have thing and I would recommend leaving it in there.

PassiveLemon avatar Jul 17 '23 03:07 PassiveLemon

Thank you.

I wonder how much this conflicts with #451, if we can get the older one merged that would be great. Then this one can come directly after.

AbdBarho avatar Jul 22 '23 06:07 AbdBarho

Cool. Also I had another idea, which I just came up with, probably we can assume for nvidia users that nvidia-smi will be available, since we're going to be immediately trying to use the nvidia hardware. So then we can actually decide whether to set a flag like --medvram based on what amount of vram is available that we can directly query from nvidia-smi! Do you like this idea?

On the other hand, this being a docker setup maybe already implicitly selects for power users, ones who would prefer manually specifying flags like this...

unphased avatar Jul 22 '23 06:07 unphased

I don't think we can make all people happy, the problem is, a lot of users of this repo are not very tech savy, and are trying to use SD on their old laptops and stuff, which is why I had the medvram per default.

Maybe removing it is a good idea, maybe it is not, we can try for a week or two and see if we get bug reports.

As for the automatic decision, sound cool, but I would not want to implement it here, this should be in theory a functionality of the UI itself.

AbdBarho avatar Jul 22 '23 06:07 AbdBarho

I don't think we can make all people happy, the problem is, a lot of users of this repo are not very tech savy, and are trying to use SD on their old laptops and stuff, which is why I had the medvram per default.

Maybe removing it is a good idea, maybe it is not, we can try for a week or two and see if we get bug reports.

As for the automatic decision, sound cool, but I would not want to implement it here, this should be in theory a functionality of the UI itself.

Agreed that sounds like an enhancement for the a1111 project.

unphased avatar Jul 22 '23 06:07 unphased