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

docker compose setup #162

Merged
merged 20 commits into from
Apr 11, 2024
Merged

docker compose setup #162

merged 20 commits into from
Apr 11, 2024

Conversation

LawyZheng
Copy link
Collaborator

  • build a docker image
  • support docker compose setup

Copy link
Contributor

@wintonzheng wintonzheng left a comment

Choose a reason for hiding this comment

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

LGTM overall. Thanks for building this!

Dockerfile Outdated
COPY --from=requirements-stage /tmp/requirements.txt /app/requirements.txt
RUN pip install --no-cache-dir --upgrade -r requirements.txt
RUN pip install --no-cache-dir streamlit
# Use following command, if you have network problem
Copy link
Contributor

Choose a reason for hiding this comment

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

what are these comments? can we delete these comments?

- BROWSER_TYPE=chromium-headful
- ENABLE_OPENAI=true
- OPENAI_API_KEY=<your_openai_key>
# If you want to use other LLM provide, like azure or anthropic
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# If you want to use other LLM provide, like azure or anthropic
# If you want to use other LLM provider, like azure and anthropic:

- ./videos:/data/videos
- ./har:/data/har
- ./.streamlit:/app/.streamlit
environment:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we point it to the .env file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If .env pointed, users need to run the docker-compose together with .env.
In this way, rookie users could run the single docker-compose.yml file.
And advanced users could point it to the .env by themselves.

Copy link
Collaborator Author

@LawyZheng LawyZheng Apr 7, 2024

Choose a reason for hiding this comment

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

Besides, if we change the settings in env, we are supposed to restart/recreate the container.
If we have an isolated env setting file, I think it's easy to forget to restart/recreate.
I think load from .env could be more useful if we want to share the same env between multiple containers, instead of running a single container

Comment on lines 42 to 43
depends_on:
- postgres
Copy link
Contributor

Choose a reason for hiding this comment

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

This wouldn't work well because this way it just waits for the container to be up. Instead, we should be waiting for postgres to be ready.

This eventually works out but until skyvern is up, the container will restart multiple times since it'll die without a healthy DB.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great, I have noticed it already.
But I don't have any idea how to make skyvern check the DB health hook in docker-compose.
Any good idea?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I just let skyvern restart on failure. haha

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it works but if someone checks out the logs or runs without -d it could get confusing 😄

Can you check this out and see if it works? I did something similar before, if this doesn't work we can hop on a call later?

https://stackoverflow.com/a/55835081

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It worked.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Kerem Yilmaz <kerem@skyvern.com>
@@ -101,6 +101,16 @@ Note: Our setup script does these two for you, but they are here for reference.
```
1. Navigate to `http://localhost:8501` in your browser to start using the UI

## Docker Compose setup
Copy link
Contributor

Choose a reason for hiding this comment

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

We should test the docker, (maybe even ask people in DC to test it out and provide feedback) and when it's all good this should be the primary way to start using Skyvern.

@ykeremy
Copy link
Contributor

ykeremy commented Apr 8, 2024

@LawyZheng the Skyvern container waits the postgres to actually be ready now!! 🎉

Stearmlit can't connect to the server though:

ConnectionError: HTTPConnectionPool(host='127.0.0.1', port=8000): Max retries exceeded with url: /api/v1/internal/tasks?page=1&page_size=15 (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0xffff5e1d8ad0>: Failed to establish a new connection: [Errno 111] Connection refused'))

LMK if you need help debugging.

@LawyZheng
Copy link
Collaborator Author

@ykeremy you'd better delete or move away the old secrets.toml in folder .streamlit.
If you are trying to use the old DB, you need to verify some things:

  1. the connect string in docker-compose is correct.
  2. need change the the host in secrets.toml to skyvern from 127.0.0.1/ or you can map a port 8000 to the local machine in the skyvern container

@LawyZheng
Copy link
Collaborator Author

My default docker-compose is prepared for uses who haven't do any setups yet.
And only streamlit port is exposed to the local machine network. postgres and skyvern are both in the docker network without exported.

@ykeremy
Copy link
Contributor

ykeremy commented Apr 8, 2024

Removing the old secrets.toml was the solution 🎉

There will be users who already set something up in their locals that would like to start using the dockerized solution. Let's think about how we should let people know about stuff like this. I'll add this to our discussions topics in tomorrow's meeting.

@andr0s
Copy link

andr0s commented Apr 9, 2024

btw this seems to be working, i've just tested it :) Thanks

@LawyZheng LawyZheng merged commit c0b4510 into main Apr 11, 2024
2 checks passed
@LawyZheng LawyZheng deleted the feature/support-docker-setup branch April 11, 2024 00:54
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