LlamaIndex: Evaluate Ollama LLM backend with smaller models #1046
Conversation
|
Warning Rate limit exceeded@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 44 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)
""" WalkthroughThe changes refactor LLM configuration to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
topic/machine-learning/llama-index/test.py (1)
36-36: Action Required: Add fallback and default forllm_backendinconfigure_llmThe failures (
ValueError: Open AI API type not defined or invalid: None) occur becauseLLM_BACKENDisn’t set in yourenv.standalone, sollm_backendisNoneand falls through to theelsebranch. You can fix this by picking up the existingOPENAI_API_TYPEenv var (if present) and defaulting to"openai":– In
topic/machine-learning/llama-index/boot.py, replace the current assignmentllm_backend = os.getenv("LLM_BACKEND")with:
llm_backend = ( os.getenv("LLM_BACKEND") or os.getenv("OPENAI_API_TYPE") or "openai" )– Update the error message to reference
llm_backendrather thanopenai.api_type:- else: - raise ValueError(f"Open AI API type not defined or invalid: {openai.api_type}") + else: + raise ValueError(f"LLM backend undefined or invalid: {llm_backend}")This ensures:
• Tests continue to work when only
OPENAI_API_KEY(and optionallyOPENAI_API_TYPE) are defined inenv.standalone.
• New code paths (LLM_BACKEND) are still supported.
• A sensible default ("openai") prevents unintendedNonevalues.topic/machine-learning/llama-index/boot.py (1)
54-64: Missing embedding support for Ollama backend.The embedding model configuration only handles "openai" and "azure" backends, but doesn't provide embedding support for the new "ollama" backend. This could cause issues when the Ollama backend is used.
Consider adding Ollama embedding support or handling the case explicitly:
if llm_backend == "openai": embed_model = LangchainEmbedding(OpenAIEmbeddings(model=llm_model)) elif llm_backend == "azure": embed_model = LangchainEmbedding( AzureOpenAIEmbeddings( azure_endpoint=os.getenv("OPENAI_AZURE_ENDPOINT"), model=llm_model, ) ) + elif llm_backend == "ollama": + # Ollama doesn't have built-in embeddings, could use a fallback or raise an error + embed_model = None # Or implement Ollama-specific embedding solution else: embed_model = None
♻️ Duplicate comments (1)
topic/machine-learning/llama-index/env.standalone (1)
11-12: Verify the model name "gpt-4.1" is valid.Same issue as in
env.azure- please verify that "gpt-4.1" is a valid OpenAI model name. OpenAI typically uses names like "gpt-4" or "gpt-4-turbo".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
topic/machine-learning/llama-index/boot.py(1 hunks)topic/machine-learning/llama-index/demo_nlsql.py(1 hunks)topic/machine-learning/llama-index/env.azure(1 hunks)topic/machine-learning/llama-index/env.standalone(1 hunks)topic/machine-learning/llama-index/requirements.txt(1 hunks)topic/machine-learning/llama-index/test.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
topic/machine-learning/llama-index/env.standalone (1)
Learnt from: amotl
PR: #1038
File: application/open-webui/compose.yml:44-48
Timestamp: 2025-07-23T22:00:51.593Z
Learning: In the cratedb-examples repository, hard-coded credentials like "crate:crate" in Docker Compose files are acceptable for demonstration purposes to maintain simplicity and avoid unnecessary layers of indirection, even when flagged by security tools like Checkov.
🪛 GitHub Actions: LlamaIndex
topic/machine-learning/llama-index/test.py
[error] 36-36: Test 'test_nlsql' failed with ValueError: Open AI API type not defined or invalid: None
[error] 54-54: Test 'test_mcp' failed with ValueError: Open AI API type not defined or invalid: None
topic/machine-learning/llama-index/demo_nlsql.py
[error] 23-23: ValueError: Open AI API type not defined or invalid: None. This error occurs in the configure_llm() function when the OpenAI API type is not set or invalid.
🪛 Gitleaks (8.27.2)
topic/machine-learning/llama-index/env.standalone
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (10)
topic/machine-learning/llama-index/requirements.txt (1)
6-6: LGTM! Dependency addition supports new Ollama backend.The addition of
llama-index-llms-ollama<0.7correctly introduces the required dependency for Ollama LLM backend support, with a version constraint consistent with other llama-index packages in the project.topic/machine-learning/llama-index/env.azure (1)
1-3: LLM Model Name ValidatedThe additions in
topic/machine-learning/llama-index/env.azure(lines 1–3) correctly introduceLLM_BACKEND=azureandLLM_MODEL=gpt-4.1. The model name “gpt-4.1” is indeed an official OpenAI GPT-4 series variant. No further changes needed.topic/machine-learning/llama-index/test.py (1)
27-27: LGTM! Minor docstring improvement.The addition of "the" before "outcome" improves the grammatical clarity of the docstrings.
Also applies to: 45-45
topic/machine-learning/llama-index/demo_nlsql.py (1)
34-34: Removal of explicit tables parameter is safe
- Confirmed that
NLSQLTableQueryEnginedefaults to using the tables already scoped bySQLDatabase(include_tables=…), so commenting out thetablesargument has no adverse effect.- The CI is still failing on the existing
configure_llm()OpenAI API type error; please address that in a follow-up.topic/machine-learning/llama-index/env.standalone (2)
1-9: LGTM! Good examples of Ollama model configurations.The commented-out Ollama backend configuration provides helpful examples of various model options (phi4-mini, deepseek-r1, llama3.2, qwen3, gemma3) that users can easily uncomment and use for local LLM deployment.
13-13: Static analysis false positive - API key is clearly marked as invalid.The flagged API key contains "-invalid-" in the string and is commented out, making it clearly a placeholder for demonstration purposes. This aligns with the project's approach of using demo credentials for examples.
topic/machine-learning/llama-index/boot.py (4)
11-11: LGTM! Ollama import added correctly.The import for Ollama is properly added to support the new backend functionality.
28-33: LGTM! OpenAI configuration updated correctly.The refactoring to use environment variables for the OpenAI backend is implemented correctly, maintaining the same functionality while improving configurability.
34-42: LGTM! Azure configuration updated correctly.The Azure OpenAI configuration properly uses the environment variable for the model while maintaining all necessary Azure-specific parameters.
43-50: Ollama configuration looks good with appropriate timeout settings.The Ollama backend configuration includes sensible defaults:
temperature=0.0for consistent outputsrequest_timeout=120.0for reasonable timeout handlingkeep_alive=-1to keep the model loaded
`gemma3:1b` looks very promising with `NLSQLTableQueryEngine`.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
topic/machine-learning/llama-index/boot.py (2)
21-27: LGTM! Environment variable validation implemented.The validation logic properly addresses the past review comment concerns by checking for required environment variables and providing clear error messages.
57-57: LGTM! Error message correctly references the variable.The error message fix properly addresses the past review comment by referencing
llm_backendinstead of the oldopenai.api_type.
🧹 Nitpick comments (1)
topic/machine-learning/llama-index/boot.py (1)
59-69: Consider adding embedding model support for Ollama backend.The current implementation returns
Nonefor embedding models when using the Ollama backend, which may cause issues in downstream code that expects an embedding model.Consider adding Ollama embedding support or documenting this limitation:
+ elif llm_backend == "ollama": + # Note: Ollama embeddings could be added here if needed + # For now, returning None - ensure downstream code handles this + embed_model = None else: embed_model = NoneAlternatively, you could use a local embedding model that works well with Ollama:
+ elif llm_backend == "ollama": + from llama_index.embeddings.huggingface import HuggingFaceEmbedding + embed_model = HuggingFaceEmbedding(model_name="sentence-transformers/all-MiniLM-L6-v2")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
topic/machine-learning/llama-index/boot.py(1 hunks)topic/machine-learning/llama-index/demo_nlsql.py(1 hunks)topic/machine-learning/llama-index/env.azure(1 hunks)topic/machine-learning/llama-index/env.standalone(1 hunks)topic/machine-learning/llama-index/requirements.txt(1 hunks)topic/machine-learning/llama-index/test.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- topic/machine-learning/llama-index/demo_nlsql.py
🚧 Files skipped from review as they are similar to previous changes (3)
- topic/machine-learning/llama-index/requirements.txt
- topic/machine-learning/llama-index/test.py
- topic/machine-learning/llama-index/env.azure
🧰 Additional context used
🧠 Learnings (1)
topic/machine-learning/llama-index/env.standalone (1)
Learnt from: amotl
PR: #1038
File: application/open-webui/compose.yml:44-48
Timestamp: 2025-07-23T22:00:51.593Z
Learning: In the cratedb-examples repository, hard-coded credentials like "crate:crate" in Docker Compose files are acceptable for demonstration purposes to maintain simplicity and avoid unnecessary layers of indirection, even when flagged by security tools like Checkov.
🪛 Gitleaks (8.27.2)
topic/machine-learning/llama-index/env.standalone
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
topic/machine-learning/llama-index/env.standalone (3)
1-9: LGTM! Well-structured Ollama backend configuration.The commented configuration examples provide clear options for various Ollama models, including the target
gemma3:1bmodel mentioned in the PR objectives. The structure makes it easy to switch between different models for evaluation.
13-13: Static analysis false positive - API key is safe.The flagged API key is commented out and marked as "invalid", making it a placeholder for demonstration purposes. Based on retrieved learnings, demo credentials are acceptable in this repository.
10-13: Model Name Validation – No Action RequiredUpon verification,
gpt-4.1is an officially supported OpenAI model (introduced April 2025). No change to theLLM_MODELsetting is needed.Likely an incorrect or invalid review comment.
topic/machine-learning/llama-index/boot.py (2)
11-11: LGTM! Proper import for Ollama backend.The import is correctly added to support the new Ollama LLM backend functionality.
48-55: LGTM! Well-configured Ollama backend implementation.The Ollama LLM configuration follows best practices with:
- Appropriate temperature setting (0.0 for deterministic results)
- Reasonable timeout (120 seconds)
- Keep-alive setting (-1 for persistent connection)
This aligns with the PR objective of evaluating Ollama with smaller models.
gemma3:1blooks very promising when used with LlamaIndex'NLSQLTableQueryEngine. Validated with a basic Text-to-SQL job, it responds in approx. 8 seconds on a lousy CPU, while OpenAI GPT-4.1 via SaaS also takes up to 5 seconds.NLSQLTableQueryEnginefor generic use cases.