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.

Sign up for GitHub

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

Jump to bottom

Run logdetective server in a container #51

Merged
merged 4 commits into from
Aug 13, 2024
Merged

Run logdetective server in a container #51

merged 4 commits into from
Aug 13, 2024

Conversation

TomasTomecek
Copy link
Collaborator

No description provided.

jpodivin
jpodivin reviewed Aug 6, 2024
View reviewed changes
docker-compose.yaml Outdated Show resolved Hide resolved
Containerfile Outdated Show resolved Hide resolved
docker-compose.yaml Show resolved Hide resolved
jkonecny12
jkonecny12 reviewed Aug 7, 2024
View reviewed changes
Containerfile Outdated Show resolved Hide resolved
@TomasTomecek
Copy link
Collaborator Author

@jpodivin @jkonecny12 thanks for your reviews!

Please check again :)

jpodivin
jpodivin requested changes Aug 8, 2024
View reviewed changes
docker-compose.yaml Outdated Show resolved Hide resolved
docker-compose.yaml Show resolved Hide resolved
docker-compose.yaml Show resolved Hide resolved
.env Show resolved Hide resolved
@jkonecny12
Copy link
Collaborator

I would recommend to move all the container stuff (Containerfile, .env and docker-compose.yml) to the separate directory. Especially the .env in the repo root might be harder to find out why it is there.

@TomasTomecek
Copy link
Collaborator Author

I would recommend to move all the container stuff (Containerfile, .env and docker-compose.yml) to the separate directory.

Can you please elaborate? You are essentially asking me to rewrite this PR without providing substantial argument why.

Especially the .env in the repo root might be harder to find out why it is there.

I found out pretty quickly why it's there:

$ git grep '\.env'
docker-compose.yaml:    env_file: .env
docker-compose.yaml:    env_file: .env

@TomasTomecek @jpodivin @jkonecny12
compose: utilize .env
f46d125 
so we can store ports, hosts, paths in one place and just reference it

also simplifies the Containerfile

there are no models being downloaded anymore, you need to supply them
using $MODELS_PATH - this directory is bindmounted inside the llama-cpp
container

Signed-off-by: Tomas Tomecek <[email protected]>
Co-authored-by: Jiri Podivin <[email protected]>
Co-authored-by: Jiri Konecny <[email protected]>
@jkonecny12
Copy link
Collaborator

It was just my comment (and you can say wish) to keep main directory clean and use subdirectories to separate usage.
Ideally I would like to have a project in the repo root to contain only Makefile (if required) and dot files or configuration files. Everything what could be moved should be moved.

However, I don't insist on that, it's fine in a way as it is. For that reason it was only a comment not review :). Feel free to merge it as it is.

@TomasTomecek
do multiple containerization improvements
4eb5c1e 
1. align port numbers b/w code, composefile and .env
2. require PORT env vars to be nonempty
3. containerfile→build: specify containerfile path
4. fixup interpolation within .env

address review comments from @jpodivin

Signed-off-by: Tomas Tomecek <[email protected]>
@TomasTomecek
compose: drop LLM_CPP_SERVER_ADDRESS
3b9e186 
so we don't interpolate inside .env which doesn't work correctly on all
compose implmentations:

```
[server]    | requests.exceptions.ConnectionError:
HTTPConnectionPool(host='$%7bllama_cpp_host%7d', port=8000): Max retries
exceeded with url: /v1/completions (Caused by
NewConnectionError('<urllib3.connection.HTTPConnection object at
0x7ff685f4bad0>: Failed to establish a new connection: [Errno -2] Name
or service not known'))
```
@jpodivin jpodivin self-requested a review August 13, 2024 14:24
@TomasTomecek TomasTomecek merged commit b8d701d into fedora-copr:main Aug 13, 2024
2 checks passed
@TomasTomecek TomasTomecek deleted the containerize branch August 13, 2024 14:27
@TomasTomecek TomasTomecek mentioned this pull request Aug 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpodivin jpodivin jpodivin approved these changes

@jkonecny12 jkonecny12 Awaiting requested review from jkonecny12

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TomasTomecek @jkonecny12 @jpodivin