docs(mcp): document all 7 generate_chart chart_type values, fix stale config comment#41599
Conversation
aminghadersohi
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41599 +/- ##
==========================================
- Coverage 64.79% 64.77% -0.02%
==========================================
Files 2740 2739 -1
Lines 153135 152912 -223
Branches 35127 35029 -98
==========================================
- Hits 99221 99048 -173
+ Misses 52017 51964 -53
- Partials 1897 1900 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The generate_chart tool's docstring only described chart_type='xy' and 'table', even though the schema has supported 'pie', 'pivot_table', 'mixed_timeseries', 'handlebars', and 'big_number' since apache#38375/apache#38402/apache#38403. LLM clients calling chart_type='line' or 'bar' (a 'kind' value, not a chart_type) hit the raw Pydantic discriminator error and looped through retries until timeout. Lists all 7 valid chart_type values with their required fields, adds a natural-language lookup table, and adds a pie chart example.
MCP_TOOL_SEARCH_CONFIG has defaulted include_schemas to True since apache#39732 (to keep discriminated-union chart configs visible to LLMs without a second round trip), but the comment above it still described include_schemas=False as the default.
aminghadersohi
force-pushed
the
amin/sc-106049/mcp-generate-chart-docstring
branch
from
4ce61b7 to
27df454
Compare
There was a problem hiding this comment.
Pull request overview
Updates MCP documentation to reduce chart-creation retries by making generate_chart’s supported chart_type discriminator values explicit and by correcting a stale comment about tool search schema inclusion defaults.
Changes:
- Expand
generate_chartdocstring to document all supportedchart_typediscriminator values and required fields, plus add a natural-language lookup and a pie example. - Update
MCP_TOOL_SEARCH_CONFIGcomment to reflect thatinclude_schemasdefaults toTrue(so discriminated-union schemas remain visible insearch_tools).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset/mcp_service/mcp_config.py | Refreshes include_schemas/summary-mode documentation to match current defaults. |
| superset/mcp_service/chart/tool/generate_chart.py | Improves generate_chart docstring to list all valid chart_type values and provide clearer guidance/examples. |
| IMPORTANT: The 'chart_type' field in the config is a DISCRIMINATOR that determines | ||
| which chart configuration schema to use. It MUST be included and MUST match the | ||
| other fields in your configuration: | ||
| other fields in your configuration. There are exactly 7 valid chart_type values | ||
| — NOT 'line', 'bar', 'mixed_timeseries', etc. Those are 'kind' values WITHIN | ||
| chart_type='xy' (see below), not chart_type values themselves: |
|
The docstring in To resolve this, the docstring should be updated to clarify that superset/mcp_service/chart/tool/generate_chart.py |
Code Review Agent Run #6e6727Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
SUMMARY
The
generate_chartMCP tool's docstring only documentedchart_type='xy'andchart_type='table', even though the underlying schema (ChartConfigdiscriminated union) has supported 5 additional chart types since #38375, #38402, and #38403:pie,pivot_table,mixed_timeseries,handlebars, andbig_number.Because the docstring is the primary signal an LLM client sees before calling the tool, models reaching for natural chart-type names like
'line','bar', or'pie'directly as thechart_typevalue (instead of as akindsub-field ofchart_type='xy', or simply being unaware'pie'was a valid discriminator at all) would hit the raw Pydantic discriminator validation error and loop through retries until timing out.This PR only touches docstrings/comments — no schema, validation, or behavior changes:
generate_chartdocstring now lists all 7 validchart_typevalues with their required fieldskind(line/bar/area/scatter) is a sub-field ofchart_type='xy', not a chart_type itselfMCP_TOOL_SEARCH_CONFIGthat describedinclude_schemas=Falseas the default; it has defaulted toTruesince fix(mcp): restore typed ChartConfig in tool schemas for LLM visibility #39732 (to keep discriminated-union chart configs visible to LLMs without a second round trip)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — docstring/comment-only change, not user-facing UI.
TESTING INSTRUCTIONS
ruff format --check/ruff checkpass on both changed filestests/unit_tests/mcp_service/chart/tool/test_generate_chart.pysuite is unaffected since only documentation (not schema or logic) changedADDITIONAL INFORMATION