Skip to content
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

Comfy update and taesd and also medvram for a1111 #547

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

unphased
Copy link

@unphased unphased commented Jul 14, 2023

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

@PassiveLemon
Copy link
Contributor

PassiveLemon commented Jul 17, 2023

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.

@AbdBarho
Copy link
Owner

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.

@unphased
Copy link
Author

unphased commented Jul 22, 2023

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

@AbdBarho
Copy link
Owner

AbdBarho commented Jul 22, 2023

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.

@@ -62,7 +62,7 @@ services:
build: ./services/comfy/
image: sd-comfy:3
Copy link
Owner

Choose a reason for hiding this comment

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

please bump this number up

git fetch && \
git checkout ${BRANCH} && \
git reset --hard ${SHA} && \
pip install -r requirements.txt
Copy link
Owner

Choose a reason for hiding this comment

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

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
Copy link
Owner

Choose a reason for hiding this comment

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

these be overridden by the mounts on container startup?

Copy link
Author

@unphased unphased Jul 22, 2023

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Isn't this something that should go into the download service? Seems more appropriate to me, and less likely to cause surprises.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Fair enough. In that case, a download-comfy profile could be better?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

@AbdBarho AbdBarho Aug 30, 2023

Choose a reason for hiding this comment

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

@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.

@unphased
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants