chore(wren-ai-service): minor-updates#1253
Conversation
cyyeh
added
module/ai-service
WalkthroughThis update introduces a new model, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant AS as AskService
participant PG as Pipeline Generator
U->>AS: Submit query
AS->>AS: Check allow_sql_generation_reasoning flag
alt Reasoning Enabled
AS->>PG: Trigger SQL generation with reasoning plan
PG-->>AS: Return SQL result with reasoning
else
AS->>PG: Trigger SQL generation without reasoning
PG-->>AS: Return SQL result
end
AS->>U: Return response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🔭 Outside diff range comments (2)
wren-ai-service/tools/config/config.example.yaml (1)
59-59: Missing Configuration Option:allow_sql_generation_reasoning
According to the PR objectives, a new configuration option (allow_sql_generation_reasoningwith a default value of True) should be added to the configuration. It is currently missing from this file. Please add the option in the appropriate section (for example, under a new settings block or within the llm configuration if that is where it logically belongs).deployment/kustomizations/base/cm.yaml (1)
190-200: Missing Configuration Option:allow_sql_generation_reasoningin Settings
The PR objectives mention the addition of a new configuration option,allow_sql_generation_reasoning(defaulting to True), yet it is absent from the settings section in this ConfigMap’sconfig.yaml. Please consider adding this option under the settings block to ensure consistency with the PR requirements.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 200-200: no new line character at the end of file
(new-line-at-end-of-file)
🧹 Nitpick comments (3)
wren-ai-service/src/web/v1/services/ask.py (1)
278-303: Consider adding a comment explaining the SQL generation reasoning flow.The response handling for SQL generation reasoning is well-implemented. Consider adding a brief comment explaining when and why SQL generation reasoning might be skipped to improve maintainability.
Add this comment before line 273:
+# Skip SQL generation reasoning if disabled in config or if results are already available if ( not self._is_stopped(query_id) and not api_results and self._allow_sql_generation_reasoning ):wren-ai-service/tools/config/config.example.yaml (1)
36-36: YAML Indentation Note
Static analysis reports a potential indentation issue on line 36. Although the list item undermodels:is generally indented by two spaces, please double-check that this indentation meets your project’s YAML style guidelines or adjust your YAMLlint configuration if needed.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 36-36: wrong indentation: expected 0 but found 2
(indentation)
deployment/kustomizations/base/cm.yaml (1)
171-171: Trailing Spaces Detected
Line 171 contains trailing spaces afterlitellm_embedder.text-embedding-3-large. Removing these ensures better adherence to YAML style standards. For example, apply the following diff:- embedder: litellm_embedder.text-embedding-3-large + embedder: litellm_embedder.text-embedding-3-large🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 171-171: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
deployment/kustomizations/base/cm.yaml(3 hunks)docker/.env.example(1 hunks)docker/config.example.yaml(3 hunks)wren-ai-service/demo/run_sql.py(1 hunks)wren-ai-service/src/config.py(1 hunks)wren-ai-service/src/globals.py(1 hunks)wren-ai-service/src/pipelines/generation/sql_generation.py(3 hunks)wren-ai-service/src/pipelines/generation/sql_generation_reasoning.py(1 hunks)wren-ai-service/src/pipelines/generation/utils/sql.py(0 hunks)wren-ai-service/src/web/v1/services/ask.py(3 hunks)wren-ai-service/tools/config/config.example.yaml(4 hunks)wren-ai-service/tools/config/config.full.yaml(4 hunks)
💤 Files with no reviewable changes (1)
- wren-ai-service/src/pipelines/generation/utils/sql.py
🧰 Additional context used
🪛 YAMLlint (1.35.1)
docker/config.example.yaml
[error] 123-123: trailing spaces
(trailing-spaces)
deployment/kustomizations/base/cm.yaml
[error] 171-171: trailing spaces
(trailing-spaces)
wren-ai-service/tools/config/config.example.yaml
[warning] 36-36: wrong indentation: expected 0 but found 2
(indentation)
[error] 141-141: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (16)
wren-ai-service/src/config.py (1)
34-36: LGTM! Well-structured configuration addition.The new configuration option is appropriately placed in the generation config section with a clear default value.
wren-ai-service/src/pipelines/generation/sql_generation_reasoning.py (1)
28-29: LGTM! Enhanced reasoning plan structure.The new requirements improve the clarity of the reasoning plan by:
- Ensuring SQL is not included in the reasoning plan
- Enforcing a numbered step format with explicit reasoning
wren-ai-service/src/pipelines/generation/sql_generation.py (2)
49-52: LGTM! Clean template update.The conditional rendering of the REASONING PLAN section is well-implemented and maintains backward compatibility.
64-64: LGTM! Proper type hints for optional parameter.The sql_generation_reasoning parameter is correctly typed as
str | Nonewith a default value of None in both prompt() and run() methods.Also applies to: 143-143
wren-ai-service/src/globals.py (1)
124-125: LGTM! Proper configuration integration.The allow_sql_generation_reasoning parameter is correctly passed from settings to AskService initialization.
wren-ai-service/src/web/v1/services/ask.py (2)
102-114: LGTM! Clean constructor changes.The addition of the
allow_sql_generation_reasoningparameter with a default value ofTruemaintains backward compatibility while adding configuration flexibility.
147-147: LGTM! Well-structured control flow changes.The addition of the
sql_generation_reasoningvariable and the modified condition properly integrate the new configuration option into the existing control flow.Also applies to: 273-277
wren-ai-service/demo/run_sql.py (1)
69-69: Verify the increased query limit impact.The query limit has been increased from 10 to 50 records. While this provides more comprehensive results, ensure that:
- The increased data volume aligns with the UI's rendering capabilities
- Memory usage remains within acceptable bounds
- Response times stay reasonable
✅ Verification successful
Verified: The increased limit is consistent with overall configuration limits.
- The updated limit of 50 in
wren-ai-service/demo/run_sql.pyis modest compared to other demo and production settings (e.g., 100 indemo/utils.pyand 500 indemo/app.py).- No conflicting configuration or evidence of performance/memory issues was found in the codebase.
- It is advisable to test the UI and performance in practice, but current configurations indicate that retrieving 50 records is in line with the intended design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this limit aligns with other configuration limits rg -A 1 "limit|max_" | grep -v "max_tokens\|max_completion_tokens"Length of output: 56989
docker/.env.example (1)
29-29: Verify the development version naming convention.The version has been changed to a development version with a future date (February 2025). While this might be intentional for testing, please ensure this aligns with your versioning strategy.
docker/config.example.yaml (1)
23-30: LGTM! New model configuration looks good.The new model
o3-mini-2025-01-31is properly configured with appropriate parameters including reasoning effort settings.wren-ai-service/tools/config/config.full.yaml (2)
5-30: LGTM! Model configurations are consistent.The model configurations, including the new
o3-mini-2025-01-31, match those inconfig.example.yaml.
34-39: Verify embedder configuration consistency.The embedder configuration matches
config.example.yaml. Please ensure all environment variables and endpoints are properly set up across different deployment environments.✅ Verification successful
Embedder configuration consistency verified.
- The
config.full.yamlfile matchesconfig.example.yaml, with identical values forprovider,model,api_base,api_key_name, andtimeout.- Other configuration examples (ollama, groq, deepseek, google ai studio, azure) intentionally differ to suit their respective environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Compare embedder configurations across files fd -e yaml -e yml | xargs rg "provider: litellm_embedder" -A 5Length of output: 5262
wren-ai-service/tools/config/config.example.yaml (2)
23-30: New Model Addition:o3-mini-2025-01-31
The new model entry is added correctly with appropriate keys and values. Please verify that the parameters (e.g. usingmax_completion_tokensandreasoning_effort) align with the expected API behavior.
33-40: Embedder Provider Update
The embedder section has been updated to uselitellm_embedderwith the new API key name (EMBEDDER_OPENAI_API_KEY) and timeout. This is consistent with the intended provider change.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 36-36: wrong indentation: expected 0 but found 2
(indentation)
deployment/kustomizations/base/cm.yaml (2)
72-79: New Model Addition in ConfigMap
The new modelo3-mini-2025-01-31is integrated into the llm models list with the necessary properties. Ensure that this new entry is fully supported by downstream services.
82-89: Embedder Configuration Update
The embedder configuration now useslitellm_embedderand updates the API key toEMBEDDER_OPENAI_API_KEY. This update is consistent with the changes in the tools config.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wren-ai-service/src/pipelines/generation/intent_classification.py (2)
241-258: Consider enhancing error handling in post_process.The current error handling catches all exceptions and returns a default TEXT_TO_SQL intent. This could mask specific issues and make debugging harder.
Consider this improvement:
except Exception: + logger.exception("Failed to parse intent classification results") + if not classify_intent.get("replies"): + raise ValueError("No replies from intent classifier") + if not classify_intent.get("replies")[0]: + raise ValueError("Empty reply from intent classifier") return { "intent": "TEXT_TO_SQL", "rephrased_question": "", "reasoning": "", "db_schemas": construct_db_schemas, }
269-277: Document the model configuration structure.The
INTENT_CLASSIFICAION_MODEL_KWARGSconfiguration would benefit from documentation explaining the JSON schema structure and its purpose.Consider adding a docstring:
INTENT_CLASSIFICAION_MODEL_KWARGS = { + # Configuration for the LLM to ensure structured JSON output + # following the IntentClassificationResult schema "response_format": { "type": "json_schema", "json_schema": { "name": "intent_classification", "schema": IntentClassificationResult.model_json_schema(), }, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ai-service/src/pipelines/generation/intent_classification.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: validate-pull-request-title
- GitHub Check: pytest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
wren-ai-service/src/pipelines/generation/intent_classification.py (1)
34-34: Verify handling of time/date formats in user queries.The change improves the logic by only adding time/date formats when relevant, preventing unnecessary format additions in non-temporal queries.
Let's verify the handling of time/date formats in user queries:
✅ Verification successful
Time/date conditional handling verified.
The changes in the intent classification pipeline correctly restrict time/date formatting to queries that include temporal expressions, thereby preventing unnecessary modifications to non-temporal questions. The test cases (e.g., in wren-ai-service/tests/pytest/services/test_semantics_description.py) and multiple similar implementations across the repository confirm that the updated logic behaves as expected.
- wren-ai-service/src/pipelines/generation/intent_classification.py: Verifies that time/date formatting is only applied when relevant.
- Test cases: Indicate proper handling of temporal queries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for test cases and examples involving time/date formats # to ensure proper handling of temporal queries. # Test 1: Look for test cases involving time/date queries rg -l "test.*time|test.*date" --type py # Test 2: Look for example queries with temporal expressions rg -i "last.*|next.*|previous.*|today|tomorrow|yesterday" --type py # Test 3: Search for datetime parsing or formatting logic rg -i "datetime|strftime|strptime|timestamp" --type pyLength of output: 14300
2 participants
Summary by CodeRabbit
New Features
Enhancements