-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
unphased
wants to merge
5
commits into
AbdBarho:master
Choose a base branch
from
unphased:comfy-update-and-taesd-and-also-medvram-for-a1111
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+11
−15
Open
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
27c560c
what i have right now
unphased e8681cf
adjustments to remove my customizations
unphased 6a3fb0c
simplify the dockerfile
unphased c99a240
seems to work
unphased fc23291
Merge remote-tracking branch 'origin/master' into comfy-update-and-ta…
unphased File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
seems to work
commit c99a240ca59d727e0ca5b1ada9b736967827703c
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 todocker 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.
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
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.