chore(wren-ai-service): improve chart generation #1101
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 9 minutes and 37 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 (3)
WalkthroughThe pull request introduces a new chart type called "multi_line" across multiple files in the Wren AI service. This change involves updating system prompts, chart generation instructions, and type definitions to support multi-line charts. The modifications primarily focus on expanding the chart type options in various components of the chart generation and adjustment systems, ensuring consistent support for the new chart visualization type. Changes
Sequence DiagramsequenceDiagram
participant User
participant ChartGeneration
participant DataAnalyst
participant ChartAdjustment
User->>ChartGeneration: Request chart with data
ChartGeneration->>DataAnalyst: Generate chart schema
DataAnalyst-->>ChartGeneration: Return multi-line chart schema
ChartGeneration->>ChartAdjustment: Propose chart adjustment
ChartAdjustment->>DataAnalyst: Refine chart if needed
DataAnalyst-->>ChartAdjustment: Return adjusted multi-line chart
ChartAdjustment->>User: Present final chart
Possibly related PRs
Suggested labels
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: 3
🧹 Nitpick comments (2)
wren-ai-service/src/pipelines/generation/chart_adjustment.py (1)
37-41: Consider the impact of the 20-word limit on reasoning quality.While conciseness is good, the strict 20-word limit for reasoning might be too restrictive for complex visualizations, especially for multi-line charts that compare multiple metrics.
Consider adjusting the limit based on the chart type's complexity or removing the strict word limit in favor of a guideline for conciseness.
wren-ai-service/src/pipelines/generation/chart_generation.py (1)
27-28: LGTM! Consider clarifying the reasoning length requirement.The simplified prompt is more focused and clearer. However, there's a minor inconsistency between "concise and easy-to-understand reasoning" in line 28 and the "20 words" limit mentioned in the instructions below.
Consider updating line 28 to explicitly mention the word limit:
-Besides, you need to give a concise and easy-to-understand reasoning within 20 words to describe why you provide such vega-lite schema. +Besides, you need to give a concise and easy-to-understand reasoning (within 20 words) to describe why you provide such vega-lite schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wren-ai-service/src/pipelines/generation/chart_adjustment.py(1 hunks)wren-ai-service/src/pipelines/generation/chart_generation.py(1 hunks)wren-ai-service/src/pipelines/generation/utils/chart.py(5 hunks)wren-ai-service/src/web/v1/services/chart.py(1 hunks)wren-ai-service/src/web/v1/services/chart_adjustment.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest
- GitHub Check: pytest
- GitHub Check: Analyze (go)
- GitHub Check: build-image
🔇 Additional comments (5)
wren-ai-service/src/web/v1/services/chart.py (1)
67-69: LGTM! Type definition updated for multi-line chart support.The addition of "multi_line" to the chart_type Literal is consistent with the PR objective.
wren-ai-service/src/web/v1/services/chart_adjustment.py (2)
80-82: LGTM! ChartAdjustmentResult updated for multi-line support.The addition of "multi_line" to the chart_type options maintains consistency with ChartResult.
17-19: LGTM! ChartAdjustmentOption updated for multi-line support.The addition of "multi_line" to the chart_type options is consistent.
✅ Verification successful
Implementation is consistent across the codebase
The "multi_line" chart type is properly supported in all relevant files, including services, pipelines, and utilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that ChartAdjustmentOption usage is updated across the codebase rg -l "ChartAdjustmentOption.*chart_type.*=.*" | xargs rg "multi_line"Length of output: 947
wren-ai-service/src/pipelines/generation/chart_adjustment.py (1)
28-30: Improved prompt clarity and conciseness.The updated prompt is more direct and focused on the essential requirements.
wren-ai-service/src/pipelines/generation/utils/chart.py (1)
61-66: LGTM! Clear documentation for multi-line chart requirements.The requirements and use cases for multi-line charts are well documented.
| chart_type: Literal[ | ||
| "line", "multi_line", "bar", "pie", "grouped_bar", "stacked_bar", "area", "" | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding schema validation for multi-line charts.
While the chart_type has been updated, there's no corresponding schema validation class for multi-line charts like there is for other chart types (LineChartSchema, BarChartSchema, etc.).
Consider adding a MultiLineChartSchema class to validate the specific requirements of multi-line charts, including the transform section:
class MultiLineChartSchema(ChartSchema):
class MultiLineChartMark(BaseModel):
type: Literal["line"] = Field(default="line")
class MultiLineChartTransform(BaseModel):
fold: list[str]
as_: list[str] = Field(alias="as")
class MultiLineChartEncoding(BaseModel):
x: TemporalChartEncoding | ChartSchema.ChartEncoding
y: ChartSchema.ChartEncoding
color: ChartSchema.ChartEncoding
mark: MultiLineChartMark
transform: list[MultiLineChartTransform]
encoding: MultiLineChartEncoding| 7. Multi Line Chart | ||
| - Vega-Lite Spec: | ||
| { | ||
| "$schema": "https://vega.github.io/schema/vega-lite/v5.json", | ||
| "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>, | ||
| "data": { | ||
| "values": [ | ||
| {"Date": "2022-01-01", "readCount": 100, "clickCount": "10"}, | ||
| {"Date": "2022-01-02", "readCount": 200, "clickCount": "30"}, | ||
| {"Date": "2022-01-03", "readCount": 300, "clickCount": "20"}, | ||
| {"Date": "2022-01-04", "readCount": 400, "clickCount": "40"} | ||
| ] | ||
| }, | ||
| "mark": {"type": "line"}, | ||
| "transform": [ | ||
| { | ||
| "fold": ["readCount", "clickCount"], | ||
| "as": ["Metric", "Value"] | ||
| } | ||
| ], | ||
| "encoding": { | ||
| "x": {"field": "Date", "type": "temporal", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>}, | ||
| "y": {"field": "Value", "type": "quantitative", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>}, | ||
| "color": {"field": "Metric", "type": "nominal", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>} | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed in the multi-line chart example.
- The
clickCountvalues are strings instead of numbers, which may cause type issues. - The
transformsection usage isn't documented in the instructions.
Apply these improvements:
{
"data": {
"values": [
- {"Date": "2022-01-01", "readCount": 100, "clickCount": "10"},
- {"Date": "2022-01-02", "readCount": 200, "clickCount": "30"},
- {"Date": "2022-01-03", "readCount": 300, "clickCount": "20"},
- {"Date": "2022-01-04", "readCount": 400, "clickCount": "40"}
+ {"Date": "2022-01-01", "readCount": 100, "clickCount": 10},
+ {"Date": "2022-01-02", "readCount": 200, "clickCount": 30},
+ {"Date": "2022-01-03", "readCount": 300, "clickCount": 20},
+ {"Date": "2022-01-04", "readCount": 400, "clickCount": 40}
]
},Also, add documentation for the transform section in the instructions:
- Multi Line Chart
- Use When: Displaying trends over continuous data, especially time.
- Data Requirements:
- One temporal or ordinal variable (x-axis).
- Two or more quantitative variables (y-axis and color).
+ - Implementation Notes:
+ - Uses `transform` with `fold` to combine multiple metrics into a single series
+ - The folded metrics are distinguished using the color encoding
- Example: Tracking monthly click rate and read rate over a year.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 7. Multi Line Chart | |
| - Vega-Lite Spec: | |
| { | |
| "$schema": "https://vega.github.io/schema/vega-lite/v5.json", | |
| "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>, | |
| "data": { | |
| "values": [ | |
| {"Date": "2022-01-01", "readCount": 100, "clickCount": "10"}, | |
| {"Date": "2022-01-02", "readCount": 200, "clickCount": "30"}, | |
| {"Date": "2022-01-03", "readCount": 300, "clickCount": "20"}, | |
| {"Date": "2022-01-04", "readCount": 400, "clickCount": "40"} | |
| ] | |
| }, | |
| "mark": {"type": "line"}, | |
| "transform": [ | |
| { | |
| "fold": ["readCount", "clickCount"], | |
| "as": ["Metric", "Value"] | |
| } | |
| ], | |
| "encoding": { | |
| "x": {"field": "Date", "type": "temporal", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>}, | |
| "y": {"field": "Value", "type": "quantitative", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>}, | |
| "color": {"field": "Metric", "type": "nominal", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>} | |
| } | |
| } | |
| 7. Multi Line Chart | |
| - Use When: Displaying trends over continuous data, especially time. | |
| - Data Requirements: | |
| - One temporal or ordinal variable (x-axis). | |
| - Two or more quantitative variables (y-axis and color). | |
| - Implementation Notes: | |
| - Uses `transform` with `fold` to combine multiple metrics into a single series | |
| - The folded metrics are distinguished using the color encoding | |
| - Example: Tracking monthly click rate and read rate over a year. | |
| - Vega-Lite Spec: | |
| { | |
| "$schema": "https://vega.github.io/schema/vega-lite/v5.json", | |
| "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>, | |
| "data": { | |
| "values": [ | |
| {"Date": "2022-01-01", "readCount": 100, "clickCount": 10}, | |
| {"Date": "2022-01-02", "readCount": 200, "clickCount": 30}, | |
| {"Date": "2022-01-03", "readCount": 300, "clickCount": 20}, | |
| {"Date": "2022-01-04", "readCount": 400, "clickCount": 40} | |
| ] | |
| }, | |
| "mark": {"type": "line"}, | |
| "transform": [ | |
| { | |
| "fold": ["readCount", "clickCount"], | |
| "as": ["Metric", "Value"] | |
| } | |
| ], | |
| "encoding": { | |
| "x": {"field": "Date", "type": "temporal", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>}, | |
| "y": {"field": "Value", "type": "quantitative", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>}, | |
| "color": {"field": "Metric", "type": "nominal", "title": <TITLE_IN_LANGUAGE_PROVIDED_BY_USER>} | |
| } | |
| } |
| Please provide your chain of thought reasoning, chart type and the vega-lite schema in JSON format. | ||
|
|
||
| {{ | ||
| "reasoning": <REASON_TO_CHOOSE_THE_SCHEMA_IN_STRING_FORMATTED_IN_LANGUAGE_PROVIDED_BY_USER>, | ||
| "chart_type": "line" | "bar" | "pie" | "grouped_bar" | "stacked_bar" | "area" | "", | ||
| "chart_type": "line" | "multi_line" | "bar" | "pie" | "grouped_bar" | "stacked_bar" | "area" | "", |
There was a problem hiding this comment.
💡 Codebase verification
Missing MultiLineChartSchema implementation for the new "multi_line" chart type
The "multi_line" option has been added to ChartGenerationResults.chart_type, but the corresponding schema implementation is missing:
- No MultiLineChartSchema class exists in the codebase
- "multi_line" is not included in ChartSchema.ChartType's supported types
- Other chart types (line, bar, pie, etc.) all have their corresponding schema implementations
🔗 Analysis chain
Verify schema updates and consider documenting chart types.
The addition of "multi_line" chart type aligns with the PR objectives. Let's verify the schema updates and consider documenting the chart types.
Let's verify that the new chart type is properly supported in related files:
Consider adding docstrings or comments to document:
- The purpose and use case for each chart type
- Required data format for each chart type
- Example schema for the new "multi_line" chart type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ChartGenerationResults and related files are updated
echo "Checking for ChartGenerationResults definition..."
rg -A 10 "class ChartGenerationResults" src/
echo "Checking for other occurrences of chart types..."
rg "chart_type.*=.*(?:line|multi_line|bar|pie|grouped_bar|stacked_bar|area)" src/
echo "Checking for any potential missed chart type validations..."
rg "chart_type.*:.*(?:str|Literal)" src/
Length of output: 653
Script:
#!/bin/bash
# Check schema and chart type validations with correct path
echo "Checking for ChartGenerationResults definition..."
rg -A 10 "class ChartGenerationResults" wren-ai-service/src/
echo "Checking for other occurrences of chart types..."
rg "chart_type.*=.*(?:line|multi_line|bar|pie|grouped_bar|stacked_bar|area)" wren-ai-service/src/
echo "Checking for chart type validations..."
rg "chart_type.*:.*(?:str|Literal)" wren-ai-service/src/
# Additional check for any schema or type definitions
echo "Checking for schema definitions..."
rg -A 5 "schema.*=.*{" wren-ai-service/src/
Length of output: 2528
Script:
#!/bin/bash
# Check for MultiLineChartSchema definition and usage
echo "Checking for MultiLineChartSchema..."
rg -A 5 "MultiLineChartSchema" wren-ai-service/src/
echo "Checking for chart schema types..."
rg -A 5 "class.*ChartSchema" wren-ai-service/src/
Length of output: 4958
There was a problem hiding this comment.
@cyyeh Thanks for fixing the issue! I can confirm that the MultiLineChartSchema has been properly implemented with all necessary components. Great work!
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for MultiLineChartSchema implementation
echo "=== Checking for MultiLineChartSchema class ==="
rg -A 10 "class MultiLineChartSchema"
echo -e "\n=== Checking for ChartType enum/literal with multi_line ==="
rg "ChartType.*=.*multi_line"
echo -e "\n=== Checking for MultiLineChartSchema usage ==="
rg "MultiLineChartSchema"
Length of output: 1642
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| chart_type: Literal[ | ||
| "line", "bar", "pie", "grouped_bar", "stacked_bar", "area", "multi_line", "" | ||
| ] |
There was a problem hiding this comment.
Curiously, why did we leave a empty string in types?
|
There are still some parts missing according to the Ai code review, I will add them later so I make the PR as draft |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wren-ai-service/src/pipelines/generation/utils/chart.py (1)
11-11: Maintain consistent capitalization in chart type names.For consistency with other chart types in the list, "Multi line chart" should be lowercase.
-Chart types: Bar chart, Line chart, Multi line chart, Area chart, Pie chart, Stacked bar chart, Grouped bar chart +Chart types: Bar chart, Line chart, Multi line chart, Area chart, Pie chart, Stacked bar chart, Grouped bar chart
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ai-service/src/pipelines/generation/utils/chart.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pytest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
wren-ai-service/src/pipelines/generation/utils/chart.py (4)
61-69: LGTM! Clear and comprehensive guidelines.The guidelines for multi-line charts are well-structured, including both usage requirements and implementation notes that help users understand the transform section.
225-250: LGTM! Previous review feedback has been addressed.The multi-line chart example is well-implemented with proper numeric values and a documented transform section.
318-334: LGTM! Schema validation has been implemented as suggested.The MultiLineChartSchema class properly validates all components of a multi-line chart, including the transform section.
406-408: LGTM! Chart type literal updated consistently.The "multi_line" type has been properly added to the chart_type literal, maintaining a logical ordering.
if chart is invalid, the chart type is empty string |
can you mention the thing in codebase, i think it will good for members to know, thanks! |
add multi-line chart
Summary by CodeRabbit
New Features
Improvements
Technical Updates