MCP Server Part 6: Format callback results for LLM consumption#3748
MCP Server Part 6: Format callback results for LLM consumption#3748
Conversation
|
Thank you for your contribution to Dash! 🎉 This PR is exempt from requiring a linked issue due to its labels. |
9b3063f to
7451883
Compare
0e54d74 to
8a41b76
Compare
7451883 to
5fbf497
Compare
8a41b76 to
4fb9d4a
Compare
5fbf497 to
b564279
Compare
4fb9d4a to
10a544b
Compare
b564279 to
e2aec3d
Compare
2acb39d to
04ca2fe
Compare
04ca2fe to
8f9d5a0
Compare
camdecoster
left a comment
There was a problem hiding this comment.
Looks good. I add a few minor suggestions.
| formatters are called per output property and may add additional | ||
| content items (images, markdown, etc.). | ||
| """ | ||
| content: list[Any] = [ |
There was a problem hiding this comment.
Would using a dict instead of a list make more sense here? Then you wouldn't have to iterate through the list down below (assuming you could figure out which formatter to use before calling it).
| not isinstance(returned_output_value, list) | ||
| or not returned_output_value |
There was a problem hiding this comment.
| not isinstance(returned_output_value, list) | |
| or not returned_output_value | |
| not returned_output_value | |
| or not isinstance(returned_output_value, list) |
| def _to_markdown_table(rows: list[dict], max_rows: int = MAX_ROWS) -> str: | ||
| """Render a list of row dicts as a markdown table.""" | ||
| columns = list(rows[0].keys()) | ||
| total = len(rows) |
There was a problem hiding this comment.
| total = len(rows) | |
| total_rows = len(rows) |
| lines: list[str] = [] | ||
| lines.append(f"*{total} rows \u00d7 {len(columns)} columns*") | ||
| lines.append("") | ||
| lines.append("| " + " | ".join(columns) + " |") |
There was a problem hiding this comment.
If wouldn't save you much, but you could avoid adding the outer |s and still comply with the markdown spec. But every token counts, right?
| ) | ||
|
|
||
| GENERIC_FIGURE = PropRole( | ||
| PLOTLY_FIGURE = PropRole( |
There was a problem hiding this comment.
Why is this change necessary?
| @classmethod | ||
| def format( | ||
| cls, output: MCPOutput, returned_output_value: Any | ||
| ) -> list[TextContent | ImageContent]: |
There was a problem hiding this comment.
Since only TextContent is added below, could you remove ImageContent here?
| ) -> list[TextContent | ImageContent]: | ||
| if not PLOTLY_FIGURE.matches(output.get("component_type"), output["property"]): | ||
| return [] | ||
| if not isinstance(returned_output_value, dict): |
There was a problem hiding this comment.
| if not isinstance(returned_output_value, dict): | |
| if ( | |
| not returned_output_value | |
| or not isinstance(returned_output_value, dict) | |
| ): |
|
|
||
|
|
||
| def _render_image(figure: Any) -> ImageContent | None: | ||
| """Render the figure as a base64 PNG ImageContent. |
There was a problem hiding this comment.
Typical for this PR:
| """Render the figure as a base64 PNG ImageContent. | |
| """ | |
| Render the figure as a base64 PNG ImageContent. |
Summary
PlotlyFigureResult— rendersGraph.figureoutputs as PNG images via kaleido, returned asImageContentDataFrameResult— rendersDataTable.dataandAgGrid.rowDataoutputs as markdown tables, returned asTextContentManual verification: