Repository navigation
fix(mcp): preserve deduplicated chart tool input schemas - #44575
Conversation
Code Review Agent Run #67be96Actionable Suggestions - 0Additional Suggestions - 4
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 |
github-actions
Bot
added
doc
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Measured, reproducible, and roughly a 49% cut on all three tools — and the handoff note on line ranges is genuinely useful for the two follow-ups that touch this file next. Two things before this is ready. 1. Removing Please measure the full inventory before and after, not just the three. If some non-chart tool regresses, that is worth knowing now rather than from a client. The ticket asked for per-tool inventory budgets; the inventory is more than the chart tools. 2. Three CI failures need attribution. Nothing else from me. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44575 +/- ##
===========================================
+ Coverage 66.14% 81.05% +14.91%
===========================================
Files 2950 2954 +4
Lines 177666 178105 +439
Branches 41177 41233 +56
===========================================
+ Hits 117513 144364 +26851
+ Misses 57649 31036 -26613
- Partials 2504 2705 +201
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:
|
Remove empty parentheses from @pytest.mark.asyncio() to match ruff PT023 (pytest-incorrect-mark-parentheses-style) style requirements. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Code Review Agent Run #bd9356Actionable Suggestions - 0Review 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 |
|
Measured all 72 registered tools, not only chart tools, comparing the pre-change serializer at Totals (sum of individual entries): 531,433 → 348,099 bytes (−34.50%); 126,042 → 81,354 tokens (−35.45%). Four tools shrink, two are unchanged, and 66 grow. Every increase is identified by its positive deltas below. The additional large win outside the original three is The increases total +12,210 bytes / +3,421 tokens across those 66 tools. The largest individual increase is Added Full inventory: every tool, including all 66 increases
CI: Re-read the rollup at The test-only follow-up head |
Code Review Agent Run #76f91cActionable Suggestions - 0Additional Suggestions - 1
Filtered 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hey Amin, thanks for taking this on.
The core change is right, and it fixes more than the size table suggests. On f8f293d2 the compacted chart schemas still carry 62 $ref pointers coming from discriminator.mapping that point at definitions the same pass deleted, and the inlining also dropped default values (get_instance_info loses "default": {} on request). On this branch both are gone, every $ref resolves locally, and the raw tool.parameters that call_tool validates against are byte for byte identical to master for all 72 tools. I am holding at request changes only for the two test infrastructure points I left inline: six budget entries ship with zero headroom and will turn an unrelated docstring edit into a red build, and the token budgets pull a CDN download into tests/unit_tests. Both look cheap to fix and I am happy to dig in with you on either.
What I ran, all on 90fe2661, in a clean 3.11 venv built from requirements/development.txt:
pytest tests/unit_tests/mcp_service: 5204 passed, 3 skipped.- The three tool execution suites from your testing instructions: 178 passed, matching your number exactly.
- Your two new test files against
f8f293d2: 83 failed, so the new cases really do pin the fix rather than an invariant. - Dumped the serialized search entry and the raw
parametersfor all 72 tools on both commits and diffed them. Descriptions, top level properties,requiredandadditionalPropertiesare unchanged everywhere, no$defsname collides across the inventory, and there are 0 dangling references on this branch against 62 onf8f293d2. - Measured the real
search_toolspayload shape: five chart tools in one result go from 403,142 to 207,871 bytes, while five list tools go from 18,139 to 19,538. 66 of 72 tools get larger, about 9% in aggregate for the non chart tools, and 4 get much smaller. That trade is clearly worth it for the reference fidelity, but it is the opposite direction from what the description implies, so it may be worth a line there.
CI is fully green on this head, including pre-commit, check-python-deps and python-dependency-liccheck.
Code Review Agent Run #5672dfActionable Suggestions - 0Additional Suggestions - 2
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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Amin, thanks for turning both of these around so fast, and for the detail in the replies.
I re-ran the exact probes that produced my change request, and both are genuinely fixed rather than worked around, so I am dismissing it. There is one doc NIT left inline that the Bito run also spotted, and it does not hold up the approval.
What I ran on b5a91a3f:
- The docstring probe that failed before. Changing
Get chart preview by ID or UUID.toGet a chart preview by ID or UUID.now measures 3302 bytes against a 3400 byte budget andtest_tool_inventory_size[get_chart_preview]passes. I then measured every entry: minimum slack is exactly 100 bytes, the 68 non chart tools all land between 100 and 199, and the chart caps are untouched (49,531 and 49,230 against 50,000; 53,395 and 52,409 against 55,000). - The offline probe. With
tiktokenshimmed to raise on import, an emptyTIKTOKEN_CACHE_DIRand every proxy variable pointed at a dead port, both inventory files give 155 passed, and the cache directory is still empty afterwards so nothing was fetched.tiktokenandregexare gone frompyproject.tomland the lockfile. - The 72 tool dump again. The serialized search entries on this head are byte for byte identical to
90fe2661for all 72 tools, and the rawtool.parametersare byte for byte identical tof8f293d2, so the_create_search_result_serializersimplification changed nothing a client sees. 0 dangling references here against 62 onf8f293d2. - Byte budgets alone still reject the regression. The tokenizer free tests against
f8f293d2give 83 failed, at 97,013 and 104,163 and 96,581 and 102,352 bytes against the 50,000 and 55,000 caps, so removing the token assertions cost no coverage. test_tool_search_transform.pyplus both inventory files: 209 passed. The three tool execution suites from your testing instructions: 178 passed. My own fulltests/unit_tests/mcp_servicerun got killed by machine load at 64% with no failures in what ran, so for that one I am leaning on the greenunit-tests (current)job rather than claiming a local pass.
CI is green on this head, 46 checks passing and none failing.
Code Review Agent Run #2b8f77Actionable Suggestions - 0Review 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 |
CI merges master 96d31ee (apache#44128), whose deleted_state descriptions explain edit/restore eligibility and omitted EXTRA_EDITORS_RESOLVER/guest editorship. These are legitimate caller-facing caveats, not redundant schema expansion, so preserve them rather than trim information to fit a stale budget. Approved b5a91a3 and head 24812ac both serialize list_charts at 4,946 bytes and list_dashboards at 4,419. The docs-only include_schemas example contributes zero bytes. Master's descriptions grow from 333 to 536 bytes (+203) and 342 to 549 (+207), yielding 5,149 and 4,626 in CI's 8273636 merge. Raise only these limits to 5,300 and 4,800 using the existing ceil(bytes/100)*100+100 policy. Retain complete-schema parity checks; no production field or description changes. Validation: MCP unit suite: 5,205 passed, 4 skipped on branch; 5,246 passed, 4 skipped on CI merge tree with this fix. Pre-commit passed for all branch-changed files, including mypy.
Code Review Agent Run #4bd281Actionable Suggestions - 0Review 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 |
Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com> (cherry picked from commit 1e08d61)
SUMMARY
Make chart-tool argument schemas retrievable without expanding shared models or weakening their contract.
The search serializer recursively inlined every reference and removed
$defs. This duplicated chart submodels, discarded nullable alternatives and reference siblings, and replaced recursive references with unconstrained objects. Remove that lossy pass: retain the original definitions, references, discriminator mappings, nullable unions, and constraints. Existing title/output-schema stripping and description limits remain unchanged. Authorization, sanitization, and response-size guards are untouched.The legacy
compact_schemassetting continues to select the default description limit; documentation explains reference resolution relative to each tool'sinputSchema. Search ranking and description-truncation behavior are outside this change.Chart tools shrink, but 66 of 72 tools grow, with non-chart tools growing by about 9% in aggregate, as gabotorresruiz measured in his review.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI change. Measured the actual registered Apache inventory entries, including tool metadata, using compact JSON (
ensure_ascii=False,separators=(',', ':')), UTF-8 byte counts, and tiktoken 0.14.0 / cl100k_base. Tokens are BPE client estimates, not Claude-tokenizer counts; no bytes/token conversion is used.Reference duplication accounts for approximately half the inventory size. Preserving the complete contract does not meet the proposed 50,000-byte target for
update_chart: its regression budget is explicitly 55,000 bytes, not a raised production cap. The other two budgets are 50,000 bytes; each token budget is independently 20,000 tokens.A local fixture using the existing gateway renderer, MCP Tool canonicalization and one-definition pagination also validates delivery overhead:
All three fit the existing 100KB/25k-token delivery limits individually, with no required constraint removed.
TESTING INSTRUCTIONS
Using the development dependencies (including the pinned test-only tokenizer):
server.pymakes all three size regression cases fail (97,013 / 104,163 / 96,581 bytes).uvx pre-commit run --filesacross every branch-changed file; all applicable hooks pass, including mypy, pylint, and ruff.ADDITIONAL INFORMATION