Skip to content

Add multi-entity observation support and researcher skill - #211

Merged
keyurva merged 6 commits into
datacommonsorg:mainfrom
keyurva:multi-entities
Jul 23, 2026
Merged

Add multi-entity observation support and researcher skill#211
keyurva merged 6 commits into
datacommonsorg:mainfrom
keyurva:multi-entities

Conversation

@keyurva

@keyurva keyurva commented Jul 22, 2026

Copy link
Copy Markdown
Contributor
  • Support backend entities map payload construction across all observation API requests
  • Add get_multi_entity_observations tool for retrieving multi-entity relationship indicators such as foreign aid flows, bilateral trade, and migration
  • Create data-commons-multi-entity-researcher skill detailing multi-entity research workflows and dual-table response parsing
  • Add multi-entity discovery routing hooks to standard researcher skills to seamlessly switch skills when multi-entity variables are discovered
  • Update server prompt with explicit DCID resolution guidelines and tool date options

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for multi-entity relationship statistical variables (such as bilateral trade or foreign aid flows) in the Data Commons MCP server. It adds a new get_multi_entity_observations tool, updates the payload structure of the existing observations endpoint, and provides detailed documentation and skill guides for the agent. The review feedback suggests adding defensive validation to ensure that all child entity expansion parameters are provided together rather than being partially specified and silently ignored, along with corresponding unit tests and defaulting the date filter parameter to a safe value like 'latest'.

Comment thread packages/datacommons-mcp/datacommons_mcp/agent_api_service.py Outdated
Comment thread packages/datacommons-mcp/tests/test_agent_api.py
@keyurva
keyurva requested a review from clincoln8 July 22, 2026 22:31

@clincoln8 clincoln8 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!!!!

entities: dict[str, list[str]],
parent_entity_property: str | None = None,
parent_entity_dcid: str | None = None,
child_entity_type: str | None = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long term thought: I wonder if eventually we'd support passing a property here too for non-place cases? Right now we only expand on containedInPlace

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can discuss later but my current thinking is we may not need to pass a property here. Note that the containedInPlace expansion happens in mixer. We don't pass any expansion property from here.

What I'm thinking starts with a new resolve tool but let's dig into this later. For now, "get in and get out" :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we update
https://github.com/datacommonsorg/agent-toolkit/blob/main/packages/datacommons-mcp/.env.sample#L50-L58
that it needs the "agent_api" subdir if using agent api?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"To use child entity expansion, all of 'parent_entity_property', "
"'parent_entity_dcid', and 'child_entity_type' must be provided."
)
entities_payload[parent_entity_property] = { # type: ignore[index]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If an LLM agent passes entities={"recipient": ["country/AFG"]} AND specifies parent_entity_property="recipient", this will silently overwrite entities_payload["recipient"].

Suggested change
entities_payload[parent_entity_property] = { # type: ignore[index]
if parent_entity_property in entities_payload:
raise ValueError( f"Property '{parent_entity_property}' cannot be specified in both 'entities' and child expansion parameters.")
entities_payload[parent_entity_property] = { # type: ignore[index]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

- `source_override` (optional, string): Filter by a specific data source provenance DCID.
- `date` (optional, string): Specific date (e.g., `"2024"`), `"all"` (for complete historical time series), or `"latest"` (default).
- `date_range_start` / `date_range_end` (optional, string): Date range boundaries.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Add small note like

Important: parent_entity_property, parent_entity_dcid, and child_entity_type are co-dependent. If requesting child expansion, all three must be specified together.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@keyurva keyurva left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

- `source_override` (optional, string): Filter by a specific data source provenance DCID.
- `date` (optional, string): Specific date (e.g., `"2024"`), `"all"` (for complete historical time series), or `"latest"` (default).
- `date_range_start` / `date_range_end` (optional, string): Date range boundaries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"To use child entity expansion, all of 'parent_entity_property', "
"'parent_entity_dcid', and 'child_entity_type' must be provided."
)
entities_payload[parent_entity_property] = { # type: ignore[index]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

entities: dict[str, list[str]],
parent_entity_property: str | None = None,
parent_entity_dcid: str | None = None,
child_entity_type: str | None = None,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can discuss later but my current thinking is we may not need to pass a property here. Note that the containedInPlace expansion happens in mixer. We don't pass any expansion property from here.

What I'm thinking starts with a new resolve tool but let's dig into this later. For now, "get in and get out" :)

@keyurva
keyurva merged commit cae2af3 into datacommonsorg:main Jul 23, 2026
13 of 14 checks passed
@keyurva
keyurva deleted the multi-entities branch July 23, 2026 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants