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

Improve docker setup & config #1843

Merged
merged 16 commits into from
Apr 24, 2023

Conversation

Pwuts
Copy link
Member

@Pwuts Pwuts commented Apr 16, 2023

Background

The current Dockerfile and docker-compose file seem to have unnecessary convolution of configuration and don't contain an optimal configuration out of the box. Also, it is not consistent with the README. This PR attempts to fix that.

This PR either conflicts with or deprecates PRs #1830 and #1199

Changes

  • Merge apt-get actions into one layer
  • Uncouple app user and python dependencies: install packages globally instead of in /home/appuser
  • Create appuser with UID 1000 to match probable permissions on Linux host
  • Change workdir to /app (as documented in the README) which is a widely used standard, making it easier to work with for new users
  • Improve/fix docker-compose config
    • Use Redis container as memory backend by default
    • Remove unused (and after this PR, redundant) volume mount to /app
    • Mount ./logs/, ./auto_gpt_workspace/ and ./ai_settings.yaml in /app

Documentation

New configuration is consistent with current configuration; no update needed.

Test Plan

No Python code is changed, so testing succeeds if the container builds and runs properly after these changes, which it does.

PR Quality Checklist

  • My pull request is atomic and focuses on a single change. (as far as I understand this checkbox)
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

@Pwuts Pwuts marked this pull request as ready for review April 16, 2023 10:51
nponeccop
nponeccop previously approved these changes Apr 16, 2023
Dockerfile Outdated Show resolved Hide resolved
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 17, 2023
@github-actions
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Apr 17, 2023
@github-actions
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@ntindle
Copy link
Member

ntindle commented Apr 18, 2023

May be worth adding
Chrome for selenium in docker to the compose file

https://hub.docker.com/r/selenium/standalone-chrome/

See here for examples

https://github.com/SeleniumHQ/docker-selenium

@Pwuts
Copy link
Member Author

Pwuts commented Apr 18, 2023

@ntindle currently, Chrome and Firefox along with some other tools are installed in the docker container, see Dockerfile. I'm not convinced this is the way, but also don't have time to dive into it further atm.

Do you think it is worth it to use standalone selenium containers instead of running headless browsers in the auto-gpt container?

@ntindle
Copy link
Member

ntindle commented Apr 18, 2023

IMO using the selenium headless versions with their drivers makes a lot of sense. Otherwise we’d basically need to duplicate the work they’ve done and will continue to do when things change

@Pwuts
Copy link
Member Author

Pwuts commented Apr 18, 2023

Could you make a PR to implement that as soon as this one merges? Would be nice :)

@ntindle
Copy link
Member

ntindle commented Apr 18, 2023

Can do

@Pwuts Pwuts added the docker label Apr 18, 2023
@Pwuts
Copy link
Member Author

Pwuts commented Apr 19, 2023

@mikekelly care to review?

@mikekelly
Copy link
Contributor

Quite a bit of this is covering ground from #1199 and will conflict with it

Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@Pwuts Pwuts self-assigned this Apr 23, 2023
@Pwuts Pwuts requested a review from ntindle April 23, 2023 21:17
@ntindle
Copy link
Member

ntindle commented Apr 23, 2023

This combined with a smoke test of the docker container in CI would be top notch

@Pwuts
Copy link
Member Author

Pwuts commented Apr 23, 2023

@ntindle this PR fixes docker; a draft for a smoke test job is in #3059

ntindle
ntindle previously approved these changes Apr 23, 2023
@github-actions github-actions bot added size/xl and removed size/l labels Apr 24, 2023
@Pwuts Pwuts requested a review from ntindle April 24, 2023 13:08
@p-i- p-i- merged commit 9c60eec into Significant-Gravitas:master Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

browse_website is broken on Apple M1 & M1 Max due to chromedriver mismatch
6 participants