-
Notifications
You must be signed in to change notification settings - Fork 0
Feat dockerfile #1
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
base: main
Are you sure you want to change the base?
Conversation
|
Warning Rate limit exceeded@ArnabChatterjee20k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis change introduces a new embedding/LLM service infrastructure using Ollama. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.env (1)
1-1: Add a blank line at the end of the file.This file is missing a trailing newline, which is flagged by dotenv-linter and is a POSIX standard best practice.
_APP_EMBEDDING_MODELS=embeddinggemma +docker-compose.yml (2)
4-18: Add a health check to verify Ollama is responsive.Without a health check, the container may report as running even if the Ollama service is unresponsive. This is critical for ensuring reliable service availability.
services: ollama: build: context: . args: MODELS: ${_APP_EMBEDDING_MODELS:-embeddinggemma} OLLAMA_KEEP_ALIVE: 24h restart: unless-stopped + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:11434/api/tags"] + interval: 30s + timeout: 5s + retries: 3 + start_period: 10s ports:
1-18: Document the interaction between build-time model preloading and persistent volume caching.The Dockerfile preloads models during build (cached in layers), and docker-compose mounts a persistent volume for
/root/.ollama. IfMODELSchanges and the image is rebuilt, the old cached models in the volume won't be automatically cleaned up. Consider documenting this behavior and/or adding a note about when to rebuild vs. when to recreate volumes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env(1 hunks)docker-compose.yml(1 hunks)dockerfile(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env
[warning] 1-1: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
| for m in $MODELS; do \ | ||
| echo "Pulling model $m..."; \ | ||
| ollama pull $m || exit 1; \ | ||
| done && \ | ||
| pkill ollama |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Quote the $MODELS variable in the build-time model loop.
Unquoted variables risk word-splitting if model names contain spaces or special characters. Also use word-boundary matching in grep.
for m in $MODELS; do \
echo "Pulling model $m..."; \
- ollama pull $m || exit 1; \
+ ollama pull "$m" || exit 1; \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for m in $MODELS; do \ | |
| echo "Pulling model $m..."; \ | |
| ollama pull $m || exit 1; \ | |
| done && \ | |
| pkill ollama | |
| for m in $MODELS; do \ | |
| echo "Pulling model $m..."; \ | |
| ollama pull "$m" || exit 1; \ | |
| done && \ | |
| pkill ollama |
| EXPOSE 11434 | ||
|
|
||
| # On container start, quickly ensure models exist (no re-download unless missing) | ||
| ENTRYPOINT ["/bin/bash", "-c", "(sleep 2; for m in $MODELS; do ollama list | grep -q $m || ollama pull $m; done) & exec ollama $0"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the ENTRYPOINT command logic—$0 won't pass CMD correctly.
In the bash -c context, $0 refers to bash itself, not the CMD argument. This means exec ollama $0 passes "bash" to ollama (invalid), and CMD ["serve"] is never invoked. The background model check also won't block container startup if a pull fails, so the service may start with missing models.
-ENTRYPOINT ["/bin/bash", "-c", "(sleep 2; for m in $MODELS; do ollama list | grep -q $m || ollama pull $m; done) & exec ollama $0"]
+ENTRYPOINT ["/bin/bash", "-c", "for m in \"$MODELS\"; do ollama list | grep -qw \"$m\" || ollama pull \"$m\" || exit 1; done && exec ollama serve"]
CMD ["serve"]This change:
- Removes the
$0reference and explicitly usesserve. - Makes the model check synchronous so the container fails if models are unavailable.
- Adds word-boundary matching to grep (
-w) and quotes variables to handle model names safely. - Removes the unused CMD since the command is now fully specified in ENTRYPOINT.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dockerfile around line 22, the ENTRYPOINT uses bash -c with $0 (which is
bash) and runs the model-check in background so CMD isn't passed and failed
pulls are ignored; change ENTRYPOINT to run a synchronous shell script/command
that quotes $MODELS, iterates over each model, uses word-boundary grep (-w)
and/or exact matching, attempts ollama pull and exits non-zero on any pull
failure so startup fails if models are missing, then exec ollama serve
explicitly (remove reliance on $0 and remove the now-unused CMD).
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.