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

added forge first version #670

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

added forge first version #670

wants to merge 1 commit into from

Conversation

fg-uulm
Copy link

@fg-uulm fg-uulm commented Mar 4, 2024

Closes discussion item #658

@jsjolund
Copy link
Contributor

jsjolund commented Mar 4, 2024

Good job! It works great from my testing.

Some suggestions. You seem to have copied the entire services/AUTOMATIC1111/Dockerfile and just changed a few lines:

$ diff AUTOMATIC1111/Dockerfile forge/Dockerfile
15a16
> RUN . /clone.sh stable-diffusion-webui-assets https://github.com/AUTOMATIC1111/stable-diffusion-webui-assets.git 6f7db241d2f8ba7457bac5ca9753331f0c266917
32,34c33,35
<   git clone https://github.com/AUTOMATIC1111/stable-diffusion-webui.git && \
<   cd stable-diffusion-webui && \
<   git reset --hard cf2772fab0af5573da775e7437e6acdca424f26e && \
---
>   git clone https://github.com/lllyasviel/stable-diffusion-webui-forge.git && \
>   cd stable-diffusion-webui-forge && \
>   #git reset --hard cf2772fab0af5573da775e7437e6acdca424f26e && \
38c39
< ENV ROOT=/stable-diffusion-webui
---
> ENV ROOT=/stable-diffusion-webui-forge

This adds a lot of extra maintenance overhead. I would suggest instead modifying AUTOMATIC1111/Dockerfile to use Docker build arguments to specify for example which repo to clone. These arguments can then be added to docker-compose.yaml, to build two different images using the same Dockerfile.

Also, forge/clone.sh is an identical copy of AUTOMATIC1111/clone.sh. This could be a symbolic link instead.

Finally, forge/config.py is a copy of AUTOMATIC1111/config.py that just changes one line, the one with DEFAULT_FILEPATH. This should not be necessary, since this can be set as an input argument to config.py:

if __name__ == '__main__':
  if len(sys.argv) > 1:
    check_and_replace_config(*sys.argv[1:])
  else:
    check_and_replace_config(DEFAULT_FILEPATH)

@fg-uulm
Copy link
Author

fg-uulm commented Mar 5, 2024

@jsjolund I was wondering about the points you mentioned, and decided at some point to just go with the most simple, obvious solution to make it work somehow in the most isolated manner possible first.

I totally agree with your more DRY-ish approach however, but let me ask some questions before I continue to work on this further:

  1. About your symlink idea: This would work for Mac and Linux I guess, but I'm unsure about Win (not a Win user - there seems to be a recent mklink git equivalent, no idea if it is being used widely). Is Windows support relevant here?

  2. A more general question and idea: I decided to setup forge as a completely separate environment as per my personal preference, also isolating it from A1111 (which I've been using as main UI in the past).
    However, there's also an option to use forge as an additional git remote + branch of vanilla A1111 (see here: https://github.com/continue-revolution/sd-webui-animatediff/blob/forge/master/docs/how-to-use.md#you-have-a1111-and-you-know-git). As far as I understand, this seems to be used by people to share extensions, settings, etc. between A1111 and forge.
    In theory, it should be possible to adopt this approach by having the A1111 Dockerfile clone the forge files as well at build time, and implement a branch switch/checkout triggered from two compose profiles pointing to the same image. This would further minimize overhead, however as far as I can tell at the cost of a very much stronger coupling that I'd personally like to avoid (or at least have choice).

Looking forward to hear your thoughts on those two points!

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.

None yet

2 participants