Overhaul MCP tools, add metadata tool, and introduce skills#209
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Data Commons MCP server to support distinct 'agent_api' and 'local' modes, organizing tool instructions and introducing native FastMCP skills via the SkillsDirectoryProvider. It also adds new tools and service endpoints for retrieving variable metadata and child observations. The review feedback highlights critical improvements: defaulting the date parameter to None in get_observations and get_child_observations to ensure range queries are not inadvertently overridden, and upgrading the root fastmcp dependency to >=3.0.0 to match the package's requirement and prevent runtime import errors.
|
FYI - @carojahn |
| server_params=StdioServerParameters( | ||
| command="uv", | ||
| args=["run", "datacommons-mcp", "serve", "stdio"], | ||
| env={"DC_API_KEY": dc_api_key}, |
There was a problem hiding this comment.
This now conflicts with the updated eval fixtures. The fixtures expect agent API tools like get_child_observations, but this server is still launched without DC_USE_AGENT_API=true, so it exposes only the local tools (get_observations, search_indicators). Please add DC_USE_AGENT_API: "true" here or keep the fixtures on the local tool surface.
There was a problem hiding this comment.
Good call. Reverted the evals for now. We need a larger overhaul of evals, something that's on @shixiao-coder's radar in the medium term.
|
|
||
| Data Commons organizes data into two main structural hierarchies. Understanding these is key to choosing your place names and variables: | ||
|
|
||
| 1. **Topics (Variable Hierarchy)**: A taxonomy of categories (e.g., `Health` -> `Clinical Data` -> `Medical Conditions`). Topics contain sub-topics and individual variables. |
There was a problem hiding this comment.
should we also explain what is the relationship between variables, topics, and indicators because they are used across the skills.
There was a problem hiding this comment.
This is a starter template. @carojahn is actually working / testing the skill + server and tool instructions. We'll then use those prompts here.
Caro - pl consider Rohit's comments on this PR when finalizing the prompts.
| When researching statistics, always separate your work into three distinct phases to avoid context bloat: | ||
|
|
||
| 1. **Discovery (`search_indicators` or `search_child_indicators`)**: Use this to find candidate variables matching the user's concept. | ||
| 2. **Assessment (`get_variable_metadata`)**: Pass candidate variables and target locations to retrieve structural metadata, ensuring the dataset matches the required temporal range, granularity, and source trust. |
There was a problem hiding this comment.
is this for variable or indicator? If only for variable, how will agent distinguish between them?
There was a problem hiding this comment.
Only for variable. search_indicators returns the typeOf property which the agent uses to distinguish.
|
|
||
| 1. **Discovery (`search_indicators` or `search_child_indicators`)**: Use this to find candidate variables matching the user's concept. | ||
| 2. **Assessment (`get_variable_metadata`)**: Pass candidate variables and target locations to retrieve structural metadata, ensuring the dataset matches the required temporal range, granularity, and source trust. | ||
| 3. **Retrieval (`get_observations` or `get_child_observations`)**: Fetch the actual timeseries arrays once the variables and facets have been qualified. |
There was a problem hiding this comment.
Should we explain what facet and child observation is? Does paent-child refer to place hierarchy or soemthing else too, better to call out.
There was a problem hiding this comment.
Similar comment as earlier - Caro is working on this.
| @@ -0,0 +1,141 @@ | |||
| --- | |||
| name: data-commons-researcher | |||
There was a problem hiding this comment.
will this skill file be always loaded in mcp client context, whether it uses dc mpc or not?
There was a problem hiding this comment.
Just like any other skill, the frontmatter is always loaded, the content only when needed.
keyurva
left a comment
There was a problem hiding this comment.
Thanks for the review!
| @@ -0,0 +1,141 @@ | |||
| --- | |||
| name: data-commons-researcher | |||
There was a problem hiding this comment.
Just like any other skill, the frontmatter is always loaded, the content only when needed.
|
|
||
| Data Commons organizes data into two main structural hierarchies. Understanding these is key to choosing your place names and variables: | ||
|
|
||
| 1. **Topics (Variable Hierarchy)**: A taxonomy of categories (e.g., `Health` -> `Clinical Data` -> `Medical Conditions`). Topics contain sub-topics and individual variables. |
There was a problem hiding this comment.
This is a starter template. @carojahn is actually working / testing the skill + server and tool instructions. We'll then use those prompts here.
Caro - pl consider Rohit's comments on this PR when finalizing the prompts.
| When researching statistics, always separate your work into three distinct phases to avoid context bloat: | ||
|
|
||
| 1. **Discovery (`search_indicators` or `search_child_indicators`)**: Use this to find candidate variables matching the user's concept. | ||
| 2. **Assessment (`get_variable_metadata`)**: Pass candidate variables and target locations to retrieve structural metadata, ensuring the dataset matches the required temporal range, granularity, and source trust. |
There was a problem hiding this comment.
Only for variable. search_indicators returns the typeOf property which the agent uses to distinguish.
|
|
||
| 1. **Discovery (`search_indicators` or `search_child_indicators`)**: Use this to find candidate variables matching the user's concept. | ||
| 2. **Assessment (`get_variable_metadata`)**: Pass candidate variables and target locations to retrieve structural metadata, ensuring the dataset matches the required temporal range, granularity, and source trust. | ||
| 3. **Retrieval (`get_observations` or `get_child_observations`)**: Fetch the actual timeseries arrays once the variables and facets have been qualified. |
There was a problem hiding this comment.
Similar comment as earlier - Caro is working on this.
| server_params=StdioServerParameters( | ||
| command="uv", | ||
| args=["run", "datacommons-mcp", "serve", "stdio"], | ||
| env={"DC_API_KEY": dc_api_key}, |
There was a problem hiding this comment.
Good call. Reverted the evals for now. We need a larger overhaul of evals, something that's on @shixiao-coder's radar in the medium term.
Overview
This PR overhauls the Data Commons MCP server to simplify tool definitions, reduce errors, and introduce native skills.
Key Changes