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.

Sign up for GitHub

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
Open

Comfy update and taesd and also medvram for a1111 #547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
27c560c
what i have right now
unphased Jul 14, 2023
e8681cf
adjustments to remove my customizations
unphased Jul 14, 2023
6a3fb0c
simplify the dockerfile
unphased Jul 14, 2023
c99a240
seems to work
unphased Jul 14, 2023
fc23291
Merge remote-tracking branch 'origin/master' into comfy-update-and-ta…
unphased Aug 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ services:
build: ./services/AUTOMATIC1111
image: sd-auto:63
environment:
- CLI_ARGS=--allow-code --medvram --xformers --enable-insecure-extension-access --api
- CLI_ARGS=--allow-code --xformers --enable-insecure-extension-access --api

auto-cpu:
<<: *automatic
Expand Down Expand Up @@ -62,7 +62,7 @@ services:
build: ./services/comfy/
image: sd-comfy:4
environment:
- CLI_ARGS=
- CLI_ARGS=--preview-method=taesd


comfy-cpu:
Expand Down
22 changes: 9 additions & 13 deletions services/comfy/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,24 @@ ENV DEBIAN_FRONTEND=noninteractive PIP_PREFER_BINARY=1

RUN apt-get update && apt-get install -y git && apt-get clean

RUN --mount=type=cache,target=/root/.cache/pip \
--mount=type=bind,from=xformers,source=/wheel.whl,target=/xformers-0.0.21-cp310-cp310-linux_x86_64.whl \
pip install /xformers-0.0.21-cp310-cp310-linux_x86_64.whl

ENV ROOT=/stable-diffusion
RUN --mount=type=cache,target=/root/.cache/pip \
git clone https://github.com/comfyanonymous/ComfyUI.git ${ROOT} && \
cd ${ROOT} && \
git checkout master && \
git reset --hard 884ea653c8d6fe19b3724f45a04a0d74cd881f2f && \
git reset --hard 91ed2815d542c96fdad75edba2205140de3cbba6 && \
pip install -r requirements.txt


RUN --mount=type=cache,target=/root/.cache/pip \
--mount=type=bind,from=xformers,source=/wheel.whl,target=/xformers-0.0.21-cp310-cp310-linux_x86_64.whl \
pip install /xformers-0.0.21-cp310-cp310-linux_x86_64.whl


WORKDIR ${ROOT}

ARG BRANCH=master SHA=8607c2d42d10b0108de02528e813cc703e58813f
RUN --mount=type=cache,target=/root/.cache/pip \
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 •
edited
Loading

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 •
edited
Loading

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.


# add info
COPY . /docker/
Expand Down