-
Notifications
You must be signed in to change notification settings - Fork 44.4k
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
Make the docker setup consistent with local #1199
Make the docker setup consistent with local #1199
Conversation
Thanks man! It's useful. |
@mikekelly There are conflicts now |
dc40016
to
5185086
Compare
5cc4ba3
to
c940762
Compare
c940762
to
62dadc2
Compare
62dadc2
to
22f6cf5
Compare
@mikekelly There are conflicts now |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
2 similar comments
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
22f6cf5
to
e938c59
Compare
I'm not experiencing this. What are the permissions on the files in that dir? ( |
You may want to ensure that your working dir is completely clean before using |
d5f2d7a
to
4549b97
Compare
@mikekelly the permissions are:
Also I tried using a fresh git clone, and just making the changes in this pull request and got same error. I'm running debian 11 and
|
@montanaflynn please can you try this branch (https://github.com/mikekelly/Auto-GPT/tree/docker-permissions-on-linux) in another completely fresh clone and see if it resolves your issue |
@mikekelly no permissions issues with that branch, although I did get this warning which I hadn't seen before:
|
4549b97
to
a1749d5
Compare
@montanaflynn I don't think that warning relates to this change |
a1749d5
to
6171920
Compare
6171920
to
cc48b09
Compare
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.
Looks pretty good. I'd like to request a few tweaks, nothing big though.
|
||
# Install Xvfb and other dependencies for headless browser testing |
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.
This comment was actually useful imo, can you add it back in some form?
- ".env:/app/.env" | ||
profiles: ["exclude-from-up"] | ||
- "./:/app" | ||
profiles: ["exclude-from-up"] # Run and attach to container instead with: `docker-compose run auto-gpt` |
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.
profiles: ["exclude-from-up"] # Run and attach to container instead with: `docker-compose run auto-gpt` | |
profiles: ["exclude-from-up"] # Run and attach to container instead with: `docker-compose run --rm auto-gpt` |
--rm
to prevent 1001 dead containers
env_file: | ||
- .env |
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.
Since redis
is started by default, it seems logical to also connect redis
by default. This will be overridden by any explicit settings in .env
.
env_file: | |
- .env | |
env_file: | |
- .env | |
environment: | |
MEMORY_BACKEND: ${MEMORY_BACKEND:-redis} | |
REDIS_HOST: ${REDIS_HOST:-redis} |
|
||
# Copy the application files | ||
COPY --chown=appuser:appuser autogpt/ ./autogpt | ||
COPY . . |
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.
We don't need all files in the container build. docker-compose.yml
contains a non-selective volume mount, that's fine because it is intended for local use. Please be more selective here though, since we will be shipping images using this config.
RUN wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - \ | ||
&& echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google-chrome.list \ | ||
&& apt-get update \ | ||
&& apt-get install -y chromium firefox-esr |
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.
Chromium is already installed by apt-get install chromium-driver
. This might entirely be replaced by apt-get install -y firefox-esr
.
RUN wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - \ | |
&& echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google-chrome.list \ | |
&& apt-get update \ | |
&& apt-get install -y chromium firefox-esr | |
RUN apt-get install -y firefox-esr |
RUN apt-get -y update | ||
RUN apt-get -y install git chromium-driver | ||
RUN apt-get update | ||
RUN apt-get install -y git chromium-driver wget gnupg2 libgtk-3-0 libdbus-glib-1-2 dbus-x11 xvfb ca-certificates |
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'd prefer keeping X11 and the Xvfb etc out of the docker image, as docker containers are headless anyways, and clearly state in the docs that only headless browsing is supported in the docker container. Headless browsing should work fine since #1473.
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
Closing in favor of #1843. Sorry, but we're trying to move quickly and there wasn't any activity on this PR. |
Background
The project structure in Docker was different from local setup for no apparent reason, this brings it in line and cleans things up.
Changes
apt-get update
andapt-get install
once eachapt-get update
andapt-get install
into separate steps so the former can hit docker cache when building/tmp
dir to adjust therequirements.txt
file to remove non Docker deps/app
dirTesting
Image built successfully and container ran successfully.
PR Quality Checklist