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

Fixed, improved, and unified Docker environment #5474

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

WolframRavenwolf
Copy link
Contributor

@WolframRavenwolf WolframRavenwolf commented Feb 9, 2024

Checklist:

Fixed and improved Docker environment

  • Unified Dockerfile:

    • Moved GPU_CHOICE from RUN to ARG instruction so all the Dockerfiles could be unified into a single Dockerfile
    • Added CLI_ARGS to start_linux.sh command (very important fix, without it CLI_ARGS are ignored)
    • Set HOME as global instead of process-specific envvar (so it's also set when entering the container manually)
    • Removed redundant WORKDIR (we've already changed into it)
    • Deleted old Dockerfiles (now obsolete because of the unified Dockerfile)
  • Composefiles:

    • Fixed APP_UID variable default (added missing colon)
    • Added sensible container_name ("text-generation-webui" instead of default "text-generation-webui-text-generation-webui-1")
    • Added sensible hostname ("text-generation-webui" instead of random ID)
    • Added restart policy "unless-stopped" (so container can be kept running or stopped properly when starting detached)
  • Envfile-Example:

    • Added PYTORCH_KERNEL_CACHE_PATH pointing at cache dir to fix "UserWarning: Specified kernel cache directory could not be created! This disables kernel caching."
    • Added APP_RUNTIME_UID to APP_RUNTIME_GID (so you can not only define the group, but also the user for access to mounted volumes)
  • Docs:

    • Create symlinks (fixed) and missing dirs (so they won't be created by Docker as root)
  • .gitignore

    • Added .dockerignore

WolframRavenwolf and others added 8 commits February 9, 2024 19:26
- Moved GPU_CHOICE from RUN to ARG instruction so all the Dockerfiles could be unified into a single Dockerfile
- Added CLI_ARGS to start_linux.sh command (very important fix, without it CLI_ARGS are ignored)
- Set HOME as global instead of process-specific envvar (so it's also set when entering the container manually)
- Removed redundant WORKDIR (we've already changed into it)
Added PYTORCH_KERNEL_CACHE_PATH pointing at cache dir to fix "UserWarning: Specified kernel cache directory could not be created! This disables kernel caching."
Removed trailing slashes from cache paths for consistency
Create symlinks and missing dirs
Added APP_RUNTIME_UID to APP_RUNTIME_GID (so you can not only define the group, but also the user for access to mounted volumes)
Added .dockerignore to .gitignore
@oobabooga oobabooga deleted the branch oobabooga:dev February 17, 2024 21:53
@oobabooga oobabooga closed this Feb 17, 2024
@oobabooga oobabooga reopened this Feb 17, 2024
@jtara1
Copy link

jtara1 commented Feb 19, 2024

Tested and it works for me. I can now define and use CLI_ARGS="--listen-host 0.0.0.0 --listen" as an env var set in my .env

@oobabooga
Copy link
Owner

Could you merge the dev branch?

Copy link

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Some feedback to consider.

While I've not looked at the the files in full there's likely a bunch of duplication (at least from the repeated diffs still present) that could be unified to single files.

@@ -47,4 +47,5 @@ wandb
/docker-compose.yaml
/docker-compose.yml
/Dockerfile
.dockerignore

Choose a reason for hiding this comment

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

Why would you want to ignore a .dockerignore?

Choose a reason for hiding this comment

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

Actually... what is with ignoring these other Docker files prior to this PR? 🤔

FROM ubuntu:22.04
WORKDIR /builder

ARG GPU_CHOICE=${GPU_CHOICE:-A} # A: NVIDIA, B: AMD, C: APPLE, D: INTEL, N: NONE (CPU)

Choose a reason for hiding this comment

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

This and other ARG should just be the value assignment.

Why are they using var assignment?

Suggested change
ARG GPU_CHOICE=${GPU_CHOICE:-A} # A: NVIDIA, B: AMD, C: APPLE, D: INTEL, N: NONE (CPU)
# A: NVIDIA, B: AMD, C: APPLE, D: INTEL, N: NONE (CPU)
ARG GPU_CHOICE=A

You can override that default with --build-arg GPU_CHOICE=B, there is no need to try interpolate anything here like ENV.

ARG TORCH_CUDA_ARCH_LIST="${TORCH_CUDA_ARCH_LIST:-3.5;5.0;6.0;6.1;7.0;7.5;8.0;8.6+PTX}"
ARG BUILD_EXTENSIONS="${BUILD_EXTENSIONS:-}"
ARG APP_UID="${APP_UID:-6972}"
ARG APP_GID="${APP_GID:-6972}"

WORKDIR /builder

Choose a reason for hiding this comment

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

What is the point of this directive? You don't use /builder in the next RUN and the next WORKDIR that follows changes the location again.

RUN --mount=type=cache,target=/var/cache/apt,sharing=locked,rw \
apt update && \
apt install --no-install-recommends -y git vim build-essential python3-dev pip bash curl && \
rm -rf /var/lib/apt/lists/*
WORKDIR /home/app/
RUN git clone https://github.com/oobabooga/text-generation-webui.git
RUN git clone https://github.com/oobabooga/text-generation-webui.git

Choose a reason for hiding this comment

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

A Dockerfile for a project it resides in should really just be sourcing the context of the repo already available.

Since this is already what the image has been doing, I'd defer changing this until a follow-up PR, but you should build from the repo with the repo root as the build context. The benefit is you can go to any tag/commit of the repo from that point onwards and build the image if needed. It's better for automation in CI too.

Choose a reason for hiding this comment

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

You've got a WORKDIR /home/app followed by the clone and then WORKDIR /home/app/text-generation-webui to change directories.

Suggested change
RUN git clone https://github.com/oobabooga/text-generation-webui.git
WORKDIR /home/app/text-generation-webui
RUN git clone https://github.com/oobabooga/text-generation-webui.git .

Remove the other two WORKDIR surrounding that suggestion, they're redundant.

# set umask to ensure group read / write at runtime
CMD umask 0002 && export HOME=/home/app/text-generation-webui && ./start_linux.sh
CMD umask 0002 && ./start_linux.sh $CLI_ARGS

Choose a reason for hiding this comment

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

Suggested change
CMD umask 0002 && ./start_linux.sh $CLI_ARGS
ENTRYPOINT [ "/home/app/text-generation-webui/start_linux.sh" ]

Now CLI_ARGS would just be provided as the command (CMD):

docker run --rm -it image-name $CLI_ARGS

In compose.yaml the command field can set the CLI args to use, or you can use the Docker Compose ENV interpolation from a .env file to also use ${CLI_ARGS} as a value (kinda redundant though).

env_file: .env
hostname: text-generation-webui

Choose a reason for hiding this comment

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

Why?

Comment on lines +28 to +29
APP_GID: ${APP_GID:-6972}
APP_UID: ${APP_UID:-6972}

Choose a reason for hiding this comment

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

Are these actually used for anything? Nothing appears to reference them: https://github.com/search?q=repo%3Aoobabooga%2Ftext-generation-webui%20APP_UID&type=code

If they're only meant to change the runtime user, this should just be done with the standard user: uid:gid compose setting or docker run --user uid:gid CLI arg.

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