chore(wren-ai-service): minor-updates #1265
Conversation
cyyeh
added
module/ai-service
|
Warning Rate limit exceeded@cyyeh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 55 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 systematically add an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AskService
participant Pipeline
participant PostProcessor
participant Engine
Client->>AskService: Send SQL request
AskService->>Pipeline: Invoke pipeline with engine_timeout=30
Pipeline->>PostProcessor: Call run(timeout=30)
PostProcessor->>Engine: Execute SQL with timeout=30
Engine-->>PostProcessor: Return result or error message
PostProcessor-->>Pipeline: Return processed data
Pipeline-->>AskService: Deliver final output
AskService-->>Client: Send response
Possibly related PRs
Suggested reviewers
Poem
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: 0
🧹 Nitpick comments (2)
wren-ai-service/src/core/engine.py (2)
53-53: Update function docstring to reflect new return type.The function's return type has changed from
Tuple[str, bool]toTuple[str, str], but this isn't documented.Add a docstring explaining the return values:
def add_quotes(sql: str) -> Tuple[str, str]: + """Add quotes to SQL identifiers. + + Args: + sql: SQL query string + + Returns: + Tuple[str, str]: (quoted_sql, error_message) + - If successful: (quoted_sql, "") + - If failed: ("", error_message) + """
61-63: Consider using None for success case instead of empty string.Using an empty string to indicate success is less explicit than using None. This would make the success/failure states more distinguishable.
- return "", str(e) + return None, str(e) - return quoted_sql, "" + return quoted_sql, None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
deployment/kustomizations/base/cm.yaml(1 hunks)docker/config.example.yaml(1 hunks)wren-ai-service/src/config.py(1 hunks)wren-ai-service/src/core/engine.py(1 hunks)wren-ai-service/src/globals.py(8 hunks)wren-ai-service/src/pipelines/generation/followup_sql_generation.py(5 hunks)wren-ai-service/src/pipelines/generation/relationship_recommendation.py(4 hunks)wren-ai-service/src/pipelines/generation/sql_breakdown.py(4 hunks)wren-ai-service/src/pipelines/generation/sql_correction.py(5 hunks)wren-ai-service/src/pipelines/generation/sql_expansion.py(5 hunks)wren-ai-service/src/pipelines/generation/sql_generation.py(5 hunks)wren-ai-service/src/pipelines/generation/utils/sql.py(9 hunks)wren-ai-service/src/pipelines/retrieval/sql_executor.py(5 hunks)wren-ai-service/src/web/v1/services/ask.py(3 hunks)wren-ai-service/src/web/v1/services/ask_details.py(1 hunks)wren-ai-service/src/web/v1/services/sql_expansion.py(3 hunks)wren-ai-service/tools/config/config.example.yaml(1 hunks)wren-ai-service/tools/config/config.full.yaml(1 hunks)
🔇 Additional comments (31)
wren-ai-service/src/config.py (1)
37-39: LGTM! Well-structured configuration addition.The engine_timeout configuration is logically placed under a new "engine config" section with a reasonable default value.
wren-ai-service/src/core/engine.py (1)
55-57: LGTM! Enhanced error handling with sqlglot.The addition of
error_level=sqlglot.ErrorLevel.RAISEensures consistent error handling.wren-ai-service/src/pipelines/retrieval/sql_executor.py (2)
30-31: LGTM! Consistent timeout propagation.The timeout parameter is properly propagated from DataFetcher to engine.execute_sql.
Also applies to: 39-40
69-70: LGTM! Well-structured configuration storage.The engine_timeout is properly stored in _configs for use in pipeline execution.
Also applies to: 76-78
wren-ai-service/src/pipelines/generation/sql_correction.py (2)
82-92: LGTM! Enhanced post-processing with timeout.The timeout parameter is properly integrated into the post-processing step.
114-115: LGTM! Consistent configuration pattern.The engine_timeout configuration follows the same pattern as in SQLExecutor.
Also applies to: 128-131
wren-ai-service/src/pipelines/generation/sql_expansion.py (2)
75-85: LGTM! Timeout parameter correctly integrated.The engine_timeout parameter is properly added to the post_process function and correctly passed to post_processor.run.
106-112: LGTM! Proper configuration handling.The engine_timeout parameter is correctly added to the constructor with a sensible default value and properly stored in _configs.
Also applies to: 125-128
wren-ai-service/src/pipelines/generation/sql_generation.py (2)
94-104: LGTM! Timeout parameter correctly integrated.The engine_timeout parameter is properly added to the post_process function and correctly passed to post_processor.run.
122-127: LGTM! Proper configuration handling.The engine_timeout parameter is correctly added to the constructor with a sensible default value and properly stored in _configs.
Also applies to: 140-142
wren-ai-service/src/pipelines/generation/followup_sql_generation.py (2)
106-116: LGTM! Timeout parameter correctly integrated.The engine_timeout parameter is properly added to the post_process function and correctly passed to post_processor.run.
134-139: LGTM! Proper configuration handling.The engine_timeout parameter is correctly added to the constructor with a sensible default value and properly stored in _configs.
Also applies to: 152-154
wren-ai-service/src/web/v1/services/ask_details.py (1)
126-127: LGTM! Improved error handling.The code now uses a more descriptive error_message instead of a boolean flag, providing better error context. The conditional logic correctly handles the error case by falling back to the original SQL when there's an error message.
wren-ai-service/src/pipelines/generation/sql_breakdown.py (3)
138-139: LGTM! Theengine_timeoutparameter is properly added to thepost_processfunction.The parameter is correctly passed to the
post_processor.runmethod, enhancing timeout control for SQL processing.
176-177: LGTM! Theengine_timeoutparameter is properly added to the class constructor.The default value of 30.0 seconds is reasonable for most SQL operations.
192-192: LGTM! Theengine_timeoutis properly stored in the configuration.The timeout value is correctly added to the
_configsdictionary for use during pipeline execution.wren-ai-service/src/pipelines/generation/relationship_recommendation.py (2)
173-173: LGTM! Theengine_timeoutparameter is properly added to the class constructor.The default value of 30.0 seconds aligns with other services.
185-187: LGTM! Theengine_timeoutis properly stored in the configuration.The timeout value is correctly added to the
_configsdictionary for use during pipeline execution.wren-ai-service/src/web/v1/services/sql_expansion.py (1)
110-110: LGTM! Error handling is improved with better error message propagation.The changes properly capture and propagate error messages from failed dry run results, enhancing error reporting.
Also applies to: 170-188, 208-209, 212-212
wren-ai-service/src/globals.py (1)
110-111: LGTM! Theengine_timeoutparameter is consistently added to all relevant services.The timeout configuration is properly propagated to all SQL processing services, ensuring consistent timeout behavior across the application.
Also applies to: 117-118, 121-122, 134-135, 146-147, 161-162, 170-171, 187-188, 191-192, 219-220, 237-238
wren-ai-service/src/web/v1/services/ask.py (3)
144-144: LGTM! Added error message tracking.The addition of the
error_messagevariable improves error handling by capturing specific error messages from failed SQL generation attempts.
361-394: Improved error handling with timeout distinction.The changes enhance error handling by:
- Distinguishing between timeout and other errors
- Skipping SQL correction for timeout errors
- Preserving error messages for better feedback
This aligns with the PR objective to improve SQL generation error handling.
415-415: Enhanced error reporting in response metadata.The error message is now properly propagated to the response metadata, improving error visibility.
Also applies to: 422-422
wren-ai-service/src/pipelines/generation/utils/sql.py (2)
32-32: Added timeout parameter with sensible default.The addition of the
timeoutparameter with a default value of 30.0 seconds aligns with the PR objective to support configurable engine timeouts.Also applies to: 92-92
61-63: Properly propagated timeout parameter.The timeout parameter is correctly propagated to the SQL execution methods, ensuring consistent timeout behavior throughout the pipeline.
Also applies to: 99-99
docker/config.example.yaml (1)
142-142: Added engine timeout configuration.The addition of
engine_timeout: 30in settings provides the configurable timeout functionality mentioned in the PR objectives.wren-ai-service/tools/config/config.full.yaml (1)
158-158: Consistent timeout configuration across files.The
engine_timeout: 30setting is consistently defined across configuration files, ensuring uniform behavior.wren-ai-service/tools/config/config.example.yaml (2)
98-99: SQL Answer Pipeline Engine Removal
The explicitengine: wren_uisetting for thesql_answerpipeline has been removed. This change streamlines the configuration in line with removing the unused engine. Please verify that downstream components correctly default or handle the absence of an explicit engine setting.
158-159: Engine Timeout Parameter Addition
The newengine_timeout: 30setting within the settings block provides a flexible mechanism for configuring engine operation timeouts. This update aligns well with the PR’s objectives for enhanced timeout management.deployment/kustomizations/base/cm.yaml (2)
132-134: SQL Answer Pipeline Engine Removal Consistency
In the pipelines section for thewren-ai-service-config, thesql_answerpipeline now omits the explicit engine parameter. This change is consistent with the removal reported in other configuration files. Please confirm that the intended default behavior is maintained across environments.
190-192: Engine Timeout Parameter in ConfigMap
The addition ofengine_timeout: 30in the settings section of the ConfigMap standardizes the timeout configuration for engine operations. This is a positive update that ensures consistent timeout handling across various service components.
Summary by CodeRabbit