chore(wren-ai-service): fix intent classification#1074
Conversation
cyyeh
added
module/ai-service
WalkthroughThe pull request introduces enhancements to the intent classification pipeline's time handling and configuration management. The changes primarily focus on updating the Changes
Sequence DiagramsequenceDiagram
participant User
participant AskService
participant IntentClassification
participant TimeService
User->>AskService: Submit query
AskService->>TimeService: Get current time
TimeService-->>AskService: Return current time
AskService->>IntentClassification: Run classification with query and configuration
IntentClassification->>IntentClassification: Process query with time context
IntentClassification-->>AskService: Return classified intent
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)wren-ai-service/tests/pytest/services/mocks.py (1)
The newly introduced Would you like to generate tests that verify different scenarios based on the 🪧 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 (1)
wren-ai-service/src/pipelines/generation/intent_classification.py (1)
231-231: Optional configuration parameter added toprompt.When
configurationisNone,show_current_time(configuration.timezone)can fail. Safeguard by providing a fallback or default timezone.def prompt( query: str, construct_db_schemas: list[str], prompt_builder: PromptBuilder, history: Optional[AskHistory] = None, - configuration: Configuration | None = None, + configuration: Optional[Configuration] = None, ) -> dict: ... + timezone = configuration.timezone if configuration and configuration.timezone else "UTC" return prompt_builder.run( ... - current_time=show_current_time(configuration.timezone), + current_time=show_current_time(timezone), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ai-service/src/pipelines/generation/intent_classification.py(7 hunks)wren-ai-service/src/web/v1/services/ask.py(1 hunks)
🔇 Additional comments (7)
wren-ai-service/src/pipelines/generation/intent_classification.py (6)
16-16: New import for time-based functionality.
The addition of show_current_time expands the time-awareness capability. Good job ensuring the pipeline can handle time context.
18-18: Configuration import introduced.
Importing Configuration here aligns well with the introduction of configuration-driven features. This provides a flexible approach for timezones and other parameterization.
107-107: Template updated with Current Time placeholder.
Injecting the current time into the prompt template is a cohesive way to make the classification time-aware. Consider verifying the formatting of the displayed time if end-users will see it directly.
241-241: Potential edge case: configuration.timezone is None.
As recommended above, provide a default timezone to prevent errors when configuration.timezone is not set.
327-331: run method signature updated to integrate configuration object.
This change is beneficial for customizing time-based or other pipeline parameters. Confirm that callers now provide the correct configuration.
340-340: configuration usage in _pipe.execute.
Providing configuration ensures the pipeline’s newly introduced time logic is effective. No issues spotted.
wren-ai-service/src/web/v1/services/ask.py (1)
182-182: Passing configuration to intent_classification pipeline.
This addition is consistent with the changes in intent_classification.py. Great job ensuring the pipeline receives configuration data, including timezone settings.
paopa
left a comment
paopa
left a comment
There was a problem hiding this comment.
LGTM. have a nitpick comment only, we could ignore it in this PR tho.
| query=query, | ||
| db_schemas=construct_db_schemas, | ||
| query_history=previous_query_summaries, | ||
| current_time=show_current_time(configuration.timezone), |
There was a problem hiding this comment.
nitpick: the timezone in configuration class had a function to convert to current time, we might use it instead of this one.
2 participants
add time info to intent classification
testing result: https://cloud.langfuse.com/project/clsxfcv7i000038pix6dvd1y9/traces/e3aa58ab-b57e-4ed6-ab83-5773a5d68618?timestamp=2024-12-30T05:13:34.419Z&observation=335b3e61-9ce3-4de9-bede-8d45f227c6a9
Summary by CodeRabbit
New Features
Improvements