-
Notifications
You must be signed in to change notification settings - Fork 9
LlamaIndex and llm: Evaluate gemma3:1b with Ollama
#1050
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
Conversation
WalkthroughThis set of changes updates documentation, configuration, and code for the LlamaIndex-based NLSQLTableQueryEngine demo, focusing on improved environment variable handling for LLM backends (OpenAI GPT and Ollama). It also introduces new prompt instruction files, updates requirements, adds a test block for the Gemma model, and removes explicit table filtering in SQLDatabase instantiation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DemoScript
participant EnvVars
participant LLMBackend (OpenAI/Ollama)
participant SQLDatabase
User->>DemoScript: Run NLSQLTableQueryEngine demo
DemoScript->>EnvVars: Read LLM backend config (API keys, model, base_url)
DemoScript->>LLMBackend (OpenAI/Ollama): Initialize backend with config
DemoScript->>SQLDatabase: Instantiate (now with all tables)
User->>DemoScript: Input natural language question
DemoScript->>LLMBackend (OpenAI/Ollama): Send prompt/instruction
LLMBackend (OpenAI/Ollama)->>DemoScript: Return SQL query
DemoScript->>SQLDatabase: Execute SQL query
SQLDatabase->>DemoScript: Return results
DemoScript->>LLMBackend (OpenAI/Ollama): Synthesize response (optional)
DemoScript->>User: Present answer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ 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 (
|
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: 1
🧹 Nitpick comments (1)
topic/machine-learning/llama-index/demo_nlsql.py (1)
31-34: LGTM! Increased flexibility for multi-table scenarios.Removing the table filtering allows the demo to work with databases containing multiple tables, which is more realistic and flexible. The preserved commented code provides a clear reference for how to restrict to specific tables when needed.
Consider documenting this trade-off: while removing table filtering increases flexibility, it may impact performance with large databases and could make LLM responses less focused. You might want to add a comment explaining when to use table filtering:
sql_database = SQLDatabase( engine_crate, + # Uncomment to restrict to specific tables for better performance/focus: #include_tables=[os.getenv("CRATEDB_TABLE_NAME")], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
topic/machine-learning/llama-index/README.md(1 hunks)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.standalone(1 hunks)topic/machine-learning/llm/requirements.txt(1 hunks)topic/machine-learning/llm/sql-request.txt(1 hunks)topic/machine-learning/llm/sql-response.txt(1 hunks)topic/machine-learning/llm/test.xsh(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: llm
topic/machine-learning/llm/test.xsh
[error] 40-40: SyntaxError: Invalid syntax at line 40 with assignment '$OLLAMA_HOST=http://100.127.86.113:11434/'.
🔇 Additional comments (8)
topic/machine-learning/llm/requirements.txt (1)
3-3: Confirm dependency addition and monitoring recommendationThe
llm-ollama<0.14constraint is consistent with existing patterns and safely allows patch/minor updates. According to PyPI, the latest stable release is 0.12.0, and no known vulnerabilities have been reported to date. However, sincellm-ollamacurrently lacks a published security policy, please:
- Continue to monitor the package for new releases and advisories.
- Consider tightening the upper bound (e.g.,
llm-ollama<0.13) if you prefer locking to the latest known stable release.No changes are strictly required at this time.
topic/machine-learning/llm/sql-response.txt (1)
1-3: LGTM! Clean and focused prompt instruction.This prompt instruction file provides clear guidance for LLMs to synthesize responses from query results. The placeholder "Query:" line appropriately indicates where query results will be inserted during runtime.
topic/machine-learning/llama-index/boot.py (1)
51-51: LGTM! Environment variable integration follows established pattern.The addition of the
base_urlparameter usingos.getenv("OLLAMA_BASE_URL", "")is correctly implemented and consistent with other environment variable usage in the file. The empty string default is appropriate as the Ollama client will use its default URL when not specified.topic/machine-learning/llama-index/env.standalone (2)
3-4: LGTM! Good configuration examples for different deployment scenarios.The two OLLAMA_BASE_URL options provide helpful examples for both cloud (RunPod proxy) and local deployment scenarios.
11-12: LGTM! Relevant model options for Text-to-SQL tasks.The addition of
sqlcoder:7bandduckdb-nsql:7bmodels is excellent as these are specialized for SQL generation tasks, which aligns perfectly with the NLSQL demo's purpose.topic/machine-learning/llama-index/README.md (1)
99-113: Excellent documentation improvements for LLM backend configuration.The added configuration examples clearly demonstrate how to set up both OpenAI GPT and Ollama backends, with practical examples for different deployment scenarios (runpod, local). This directly supports the PR objective of evaluating the gemma3:1b model and provides users with clear setup instructions.
topic/machine-learning/llm/test.xsh (1)
35-48: Well-structured test for Gemma model Text-to-SQL capability.The test follows the established pattern and includes appropriate assertions for validating SQL query generation. The use of the sql-request.txt fragment and system prompt with table schema provides proper context for the model.
topic/machine-learning/llm/sql-request.txt (1)
1-15: Comprehensive SQL generation instructions.The instruction file provides clear guidance for CrateDB SQL query generation, addressing key best practices like column validation, table qualification, and proper output formatting. This will help ensure consistent and accurate SQL generation across different LLM models.
Gemma3 works well for basic Text-to-SQL at least.
surister
left a 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.
lgtm
About
It looks like
gemma3:1bcan do Text-to-SQL reasonably, at least on basic inquiries, when using good instructions, in this case from LlamaIndex, coming from LangChain.