mikebridge commented on code in PR #44266:
URL: https://github.com/apache/superset/pull/44266#discussion_r4033306556
##########
superset/mcp_service/semantic_layer/schemas.py:
##########
@@ -105,17 +110,35 @@ class ListMetricsRequest(BaseModel):
description="Filter to metrics from a specific semantic view.",
)
include_compatible_dimensions: bool = Field(
- default=True,
+ default=False,
description=(
- "When True, each metric includes its list of compatible
dimensions. "
- "Set to False to reduce response size when dimensions aren't
needed."
+ "Embed compatible dimensions only when explicitly requested. "
+ "Use get_compatible_dimensions for the full per-metric list. "
+ "When True, set page_size to at most 8."
),
)
page: int = Field(default=1, ge=1, description="1-based page number.")
page_size: int = Field(
- default=50, ge=1, le=500, description="Number of metrics per page."
+ default=25, ge=1, le=500, description="Number of metrics per page."
)
+ @model_validator(mode="after")
+ def validate_embedded_dimensions_page_size(self) -> "ListMetricsRequest":
+ """Reject embedded pages that risk exceeding the MCP response guard."""
+ if (
+ self.include_compatible_dimensions
+ and self.page_size > EMBEDDED_DIMENSIONS_MAX_PAGE_SIZE
+ ):
+ raise ValueError(
+ "Embedded compatible dimensions require "
+ f"page_size <= {EMBEDDED_DIMENSIONS_MAX_PAGE_SIZE}: each
external "
+ "metric's dimension list can consume roughly 1–2k tokens or
more, "
+ "and the MCP response guard rejects responses above ~25k
tokens. "
Review Comment:
Addressed in 52f018b3b12651d556f80a4dddd457a6d7d4578e. The error names
`MCP_RESPONSE_SIZE_CONFIG['token_limit']`, identifies ~25k as the default, and
explicitly says the fixed page cap does not guarantee every payload fits. The
agent instructions, get_table workflow and UPDATING.md also describe opt-in
dimensions and the configured guard accurately.
Added 10k/50k override cases: the guard factory honors each configured
limit, while request validation still accepts page 8 and rejects page 9 with
configuration-aware guidance. Both cases failed against the old wording; the
two affected suites pass 124 tests. Complete branch-file hooks and independent
final-source review pass. This is mocked/unit evidence, not a new live-provider
measurement; fresh CI on the published head remains pending.
##########
superset/mcp_service/semantic_layer/schemas.py:
##########
@@ -89,6 +89,11 @@ class MetricInfo(BaseModel):
# ---------------------------------------------------------------------------
+# Measured with 40 dimensions per metric: 20x40 = 42,336 tokens;
+# 10x40 = 21,186; 8x40 = 16,956 (18,284 fallback), against a 25,000 limit.
+EMBEDDED_DIMENSIONS_MAX_PAGE_SIZE = 8
Review Comment:
Agreed that these are independent controls. This follow-up deliberately
retains the conservative embedding cap of 8; increasing the configured token
limit does not relax it, and decreasing the limit can still make an otherwise
valid page exceed the runtime guard. The wording and upgrade note at
52f018b3b12651d556f80a4dddd457a6d7d4578e make that distinction explicit, and
override tests pin the cap's independence at 10k and 50k limits.
No dynamic/data-aware cap or universal fit guarantee is introduced. That
policy would need separate measurements and design rather than being inferred
from this wording fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
