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

fix: Update Dockerfile #815

Closed
wants to merge 1 commit into from
Closed

fix: Update Dockerfile #815

wants to merge 1 commit into from

Conversation

Freytes
Copy link
Contributor

@Freytes Freytes commented Dec 3, 2024

The last line of the docker file needs to be:

CMD ["tail", "-f", "/dev/null"]

Or else it runs the container twice and fails because it's already running on port 3000

@shakkernerd shakkernerd changed the title Update Dockerfile fix: Update Dockerfile Dec 3, 2024
@lalalune
Copy link
Member

lalalune commented Dec 3, 2024

@HashWarlock i know you set this up like this for a reason, wanna just take a look and confirm

@odilitime
Copy link
Collaborator

What actually starts it now though?

@Freytes
Copy link
Contributor Author

Freytes commented Dec 3, 2024

The last line which was modified enters the container once completed.

@rarepepi
Copy link
Contributor

rarepepi commented Dec 3, 2024

I believe you are running the container using docker-compose which is also running a command on the container, I recommend removing that line from the compose file, not sending the Dockerfile to nothing at /dev/null

@Freytes I'd revert the Dockerfile change and leave as it was on origin/main and instead make this PR remove line 3 from the docker-compose file:

Remove command: ["pnpm", "start"] in docker-compose.yaml

This will solve your problem while also allowing the Dockerfile to deploy to many clouds such as AWS seamlessly. @odilitime

@HashWarlock
Copy link
Collaborator

HashWarlock commented Dec 3, 2024

What actually starts it now though?

Right now the code for pnpm docker is here https://github.com/ai16z/eliza/blob/main/scripts/docker.sh . This was implemented originally by @oberlinstands and probably had a different dev workflow bc the eliza was not built and only the packages were installed. This is why the /dev/null may have been required bc the dev needed to build eliza then pnpm start inside the container.

In the package.json file we can see pnpm docker will execute this script for all 3 docker commands build, run and bash.

"docker:build": "bash ./scripts/docker.sh build",
"docker:run": "bash ./scripts/docker.sh run",
"docker:bash": "bash ./scripts/docker.sh bash",
"docker:start": "bash ./scripts/docker.sh start",
"docker": "pnpm docker:build && pnpm docker:run && pnpm docker:bash",

https://github.com/ai16z/eliza/blob/438c1f1400e365510cae9c19dfc35ca4f663512d/package.json#L20C10-L20C78

If checking the docker run script, we can see that the command is executed based on this logic

 # Start building the docker run command
CMD="docker run --platform linux/amd64 -p 3000:3000 -d"

# Add base mounts
for mount in "${BASE_MOUNTS[@]}"; do
   CMD="$CMD -v \"$(pwd)/$mount\""
done

# Add package mounts
for package in "${PACKAGES[@]}"; do
   CMD="$CMD -v \"$(pwd)/packages/$package/src:/app/packages/$package/src\""
done

# Add core types mount separately (special case)
CMD="$CMD -v \"$(pwd)/packages/core/types:/app/packages/core/types\""

# Add container name and image
CMD="$CMD --name eliza eliza"

# Execute the command
eval $CMD
;;

For the last command docker bash, then we see that this. may be the culprit since it will try run

# Check if the container is running before executing bash
if [ "$(docker ps -q -f name=eliza)" ]; then
   docker exec -it eliza bash
else
   echo "Container 'eliza' is not running. Please start it first."
   exit 1
fi
;;

If I run a test on pnpm docker, I will see this result:
image

I think the change we are looking for is to remove bash command from the pnpm docker package script & let the docker image be built and run. If there are errors whenever this happens, then execute pnpm docker:bash to debug the docker image & find out what the problems are.

AbdulYahya added a commit to m-mohamed/project-2501 that referenced this pull request Dec 5, 2024
Initially removed "command" from tee services  (read this comment for
more context on why:
elizaOS#815 (comment))

Without deep-diving into this, I can't at a glance tell the difference
between having/not having it in the compose file, so for the time-being
I've decided to keep it in to maintain a better semblance with the main
eliza repo.
@lalalune
Copy link
Member

Is this good to go?

@rarepepi
Copy link
Contributor

@lalalune I believe the original PR opener was trying to revert back to an old version where the Docker image wasn't running anything because they are using docker-compose which would double run the start command.

I had previously gotten a PR merged #796 to fix the Docker setup to work better in Cloud environments like AWS ECS by adding a new non-interactive flag to the start script. I think the solution here would be to remove the double running of a command in the docker-compose file and leave the Docker image as is or else it would break Cloud running environments.

I could fix this PR and make sure all 3 ways of running with Docker work:

  1. Cloud Deploy Images
  2. Docker Compose
  3. docker package.json script

@lalalune
Copy link
Member

@lalalune I believe the original PR opener was trying to revert back to an old version where the Docker image wasn't running anything because they are using docker-compose which would double run the start command.

I had previously gotten a PR merged #796 to fix the Docker setup to work better in Cloud environments like AWS ECS by adding a new non-interactive flag to the start script. I think the solution here would be to remove the double running of a command in the docker-compose file and leave the Docker image as is or else it would break Cloud running environments.

I could fix this PR and make sure all 3 ways of running with Docker work:

  1. Cloud Deploy Images
  2. Docker Compose
  3. docker package.json script

that would be great!

@rarepepi
Copy link
Contributor

@lalalune I created #1139 to merge into new develop branch to fix docker-compose file issue from original poster

@odilitime
Copy link
Collaborator

#1139 has replaced this PR. closing it.

@odilitime odilitime closed this Dec 17, 2024
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.

5 participants