Comfy update and taesd and also medvram for a1111 #547
Conversation
|
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. |
|
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. |
|
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 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... |
|
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. |
docker-compose.yml
Outdated
| @@ -62,7 +62,7 @@ services: | |||
| build: ./services/comfy/ | |||
| image: sd-comfy:3 | |||
| git fetch && \ | ||
| git checkout ${BRANCH} && \ | ||
| git reset --hard ${SHA} && \ | ||
| pip install -r requirements.txt |
There was a problem hiding this comment.
please get this back in, the two times install makes updates at least 10x faster.
| ADD https://github.com/madebyollin/taesd/raw/main/taesd_decoder.pth \ | ||
| ${ROOT}/models/vae_approx/taesd_decoder.pth | ||
| ADD https://github.com/madebyollin/taesd/raw/main/taesdxl_decoder.pth \ | ||
| ${ROOT}/models/vae_approx/taesdxl_decoder.pth |
There was a problem hiding this comment.
these be overridden by the mounts on container startup?
There was a problem hiding this comment.
I did test it and it worked.
I believe the reason that this works, is because in the config https://github.com/AbdBarho/stable-diffusion-webui-docker/blob/master/services/comfy/extra_model_paths.yaml it only ever specifies subdirs of models. So, the models dir is never directly bindmounted (which would blow away these taesd decoders from models/vae_approx).
If user adds vae_approx/ to the extra_model_paths.yaml, then they could definitely add such a folder to mount and override these. Indeed this was the approach of my PR at first and the reason I came here to post it, but then I realized, well, why not remove this manual step of fetching these and have the dockerfile auto install them. The only risk is if the taesd project becomes deleted or these decoder models' paths get changed in the future.
There was a problem hiding this comment.
Isn't this something that should go into the download service? Seems more appropriate to me, and less likely to cause surprises.
There was a problem hiding this comment.
maybe? I am not really sure, most of the users want to use A1111, they maybe don't care about these files but they get downloaded regardless? not really sure...
There was a problem hiding this comment.
Fair enough. In that case, a download-comfy profile could be better?
There was a problem hiding this comment.
TAESD is useful for both comfy and a1111. I consider it to be an ideal compromise between overhead and quality, but afaik a1111 has a self installing option for it in its settings. My change is just to provide it for comfy.
Expanding the download profile to have a comfy specific one is reasonable, I guess but kind of calls into question the whole approach of having a download profile tbh, I mean I get why it's a thing, but it's pretty awkward to require users to have to deal with choosing what they're suppose to be running. For example: if the workflow is always to run docker compose up --profile download prior to docker compose up --profile auto, to ensure the large files to download are present, then shouldn't we just take that download logic and make it idempotent and then insert that into the auto and comfy services themselves?
There was a problem hiding this comment.
@unphased maybe it would have been a lot easier to know that each of the files is only ~5MB each, I guess it would be totally fine to add them to the download service.
I was worried they are maybe a couple of gigabytes each, and that would be a problem.
Regarding
make it idempotent and then insert that into the auto and comfy services themselves
That would be cool, the way we do it now is that we validate the checksums of the downloaded files, which takes a couple of minutes on a really beefy machine, and I would not want to run this on every startup.
The reason we validate the checksum is because the HF api drops the connection a lot and the downloads gets interrupted many times.
also because when I started this project, the files came from "unknown" resources, and made sense to validate them
maybe we can change this decision in the future, that would be another topic.
Agreed that sounds like an enhancement for the a1111 project. |
…esd-and-also-medvram-for-a1111
A group of small improvements that I found:
services/comfy/extra_model_paths.yaml, but then I realized I could just deliver the TAESD decoders via dockerfile.Update versions