Docker Deployment For Kelvin Evaluator#825
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Docker-deployable “Kelvin Evaluator” component (image + compose services) and updates backend logic/config so evaluator workers can run inside containers and communicate using an internal base URL.
Changes:
- Introduces an
evaluatorDocker image target and an entrypoint that runs evaluator image builds before starting workers. - Extends
docker-compose.ymlwith evaluator scheduler/CPU/GPU worker services and a Docker socket TCP proxy. - Updates backend utilities for Docker-internal URL building and evaluator job temp directory handling; updates CI to build/push the evaluator image.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| evaluator/evaluator-entrypoint.sh | New evaluator entrypoint that builds evaluator sub-images, then runs manage.py commands. |
| docker-compose.yml | Adds evaluator services and docker socket proxy; adds internal base URL env var; adjusts pull policies. |
| common/utils.py | Adds API_INTERNAL_BASEURL handling with a production safety guard. |
| common/evaluate.py | Disables TLS verification in DEBUG and changes evaluation temp dir base. |
| Dockerfile | Switches build base image approach; adds evaluator target with Docker tooling and entrypoint. |
| .github/workflows/ci.yml | Builds, uploads, loads, and pushes the new kelvin-evaluator image in CI/deploy. |
| .env.example | Documents evaluator-related env vars and API_INTERNAL_BASEURL. |
| .dockerignore | Expands ignore patterns for cleaner Docker build contexts. |
Comments suppressed due to low confidence (1)
docker-compose.yml:15
pull_policy: neverfor theappservice prevents pulling prebuilt images from GHCR and can cause prod deployments to use stale images or fail when the image tag isn’t present locally. If the deployment flow relies on${APP_IMAGE_TAG}(as the comment suggests), this should remainalways(or be configurable via an env var) rather than hard-coded tonever.
image: "ghcr.io/mrlvsb/kelvin:${APP_IMAGE_TAG:-latest}" # Interpolation for Deployment Service
pull_policy: always
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
common/utils.py
Outdated
| # If the URL is the default Docker-internal one, only use it in DEBUG mode. | ||
| # This prevents Production from accidentally using the internal container hostname | ||
| # instead of the public domain, unless explicitly forced. | ||
| if base_uri == "https://nginx" and not settings.DEBUG: |
There was a problem hiding this comment.
This needs more explanation somewhere.
There was a problem hiding this comment.
Ok, I think that I understand the issue now. There are a few things that are not ideal here:
- We require HTTPS in nginx in local deployment (but this is more related to the
s.verify = Falsething, not this, though it is also related to this - We set the default value of the internal API to
nginx; it is not clear if that is indeed a good default, especially given that we don't really expect to run the evaluators in the same Docker network as the web in production. - This function looks like it should be used for any general URL generation, but in reality it is only used for evaluators.
So I would suggest this:
- Put
API_INTERNAL_BASEURL=https://nginxto.env.example, so that it is used by default in local Docker deployment, without it being specified indocker-compose.yml - Remove the
:-https://nginxdefault indocker-compose.yml; make the variable required - Move the variable loading to
settings.py, an environment variable shouldn't be accessed randomly in one of Kelvin's functions, but rather be centralized in the website configuration - Rename the variable e.g. to
EVALUATION_LINK_BASEURL - Only use the variable when generating an URL for evaluators (including LLM evaluation), not for "normal" public URL links, such as for e-mails. I think that this actually already happens, as the non-evaluator part of Kelvin uses
request.build_absolute_uridirectly. So it is enough to renamebuild_absolute_urito e.g.build_evaluation_download_uri, or something like that, to make it clear for what it should (and shouldn't!) be used.
There was a problem hiding this comment.
Changed mostly based on your suggestions, please review it, if it is acceptable
docker-compose.yml
Outdated
| - KELVIN__HOST_URL=${KELVIN__HOST_URL} | ||
| # - Defaults to 'https://nginx' for local docker development (to fix loopback ref to 127.0.0.1) | ||
| # - IGNORED by app if value is 'https://nginx' AND DEBUG=False | ||
| - API_INTERNAL_BASEURL=${API_INTERNAL_BASEURL:-https://nginx} |
There was a problem hiding this comment.
Added more description
3cba5e3 to
592a9bd
Compare
common/utils.py
Outdated
| # If the URL is the default Docker-internal one, only use it in DEBUG mode. | ||
| # This prevents Production from accidentally using the internal container hostname | ||
| # instead of the public domain, unless explicitly forced. | ||
| if base_uri == "https://nginx" and not settings.DEBUG: |
There was a problem hiding this comment.
Ok, I think that I understand the issue now. There are a few things that are not ideal here:
- We require HTTPS in nginx in local deployment (but this is more related to the
s.verify = Falsething, not this, though it is also related to this - We set the default value of the internal API to
nginx; it is not clear if that is indeed a good default, especially given that we don't really expect to run the evaluators in the same Docker network as the web in production. - This function looks like it should be used for any general URL generation, but in reality it is only used for evaluators.
So I would suggest this:
- Put
API_INTERNAL_BASEURL=https://nginxto.env.example, so that it is used by default in local Docker deployment, without it being specified indocker-compose.yml - Remove the
:-https://nginxdefault indocker-compose.yml; make the variable required - Move the variable loading to
settings.py, an environment variable shouldn't be accessed randomly in one of Kelvin's functions, but rather be centralized in the website configuration - Rename the variable e.g. to
EVALUATION_LINK_BASEURL - Only use the variable when generating an URL for evaluators (including LLM evaluation), not for "normal" public URL links, such as for e-mails. I think that this actually already happens, as the non-evaluator part of Kelvin uses
request.build_absolute_uridirectly. So it is enough to renamebuild_absolute_urito e.g.build_evaluation_download_uri, or something like that, to make it clear for what it should (and shouldn't!) be used.
evaluator/evaluator-entrypoint.sh
Outdated
|
|
||
| # Run the image builder to ensure all required images are present | ||
| # Skip image build if running as scheduler (detected via --with-scheduler arg) | ||
| if [[ "$*" != *"--with-scheduler"* ]]; then |
There was a problem hiding this comment.
--with-scheduler doesn't tell anything about whether the evaluator will need the images or not.
Let's just inline the whole command into docker-compose.yml, seems like the simplest solution without doing similar hacks.
…ronment configurations
2b00528 to
f0695f2
Compare
Kobzol
left a comment
There was a problem hiding this comment.
Thank you, left one comment.
| # or solve issues with socket permissions | ||
| docker_proxy: | ||
| container_name: kelvin_docker_proxy | ||
| profiles: [ prod,evaluator_cpu,evaluator_cuda ] |
There was a problem hiding this comment.
Hmm, this means that the Docker proxy will be running on the main server, even if there will be no evaluators there. Seems safer to just not do that.
| profiles: [ prod,evaluator_cpu,evaluator_cuda ] | |
| profiles: [ evaluator_cpu, evaluator_cuda ] |
There was a problem hiding this comment.
But it is required for deployment service :D
Kobzol
left a comment
There was a problem hiding this comment.
Ok, let's try. Thank you.
EVALUATOR_CPU_REPLICAS)EVALUATOR_CUDA_REPLICAS)EVALUATOR_REDIS__HOSTEVALUATOR_REDIS__PORTAPI_INTERNAL_BASEURL)