feat: load NEMAR's dataset tools from its MCP server - #352
Conversation
The NEMAR assistant's two dataset tools were dead. Both called nemar.org/api/dataexplorer/datapipeline/..., which returns 404: the legacy dataexplorer site was retired and its URLs now redirect to nemar.org/dataset/<id>. So this replaces them rather than supplementing them. Adds an MCP client runtime. Two design points worth knowing before changing it: Discovery runs in a worker thread with its own loop, and that is not a workaround. Tools are assembled in CommunityAssistant.__init__, which is synchronous and which the FastAPI app calls from inside a running event loop, where asyncio.run raises. A dedicated thread is correct whether or not the caller has a loop. Invocation needs none of this: it is a plain coroutine. Mutation-checked, and with a bare asyncio.run the running-loop test fails while the plain synchronous one still passes. A session per call is the right shape for this server, not laziness. The NEMAR server is stateless by design: no session id, GET and DELETE on the endpoint are 405, and there is nothing to keep alive. Connect-call-close costs one round trip and removes every piece of lifecycle management a persistent client would need. This wraps the SDK directly rather than using langchain-mcp-adapters, which pins mcp<2.0.0 and so cannot negotiate the 2026-07-28 revision the server implements. The dependency goes in the server extra, next to langchain. Failure degrades and never breaks: an unreachable server yields an empty tool list and a log line, matching _load_plugin_tools' contract. A tool error comes back as TEXT rather than raised, because the server's refusals name the cap they hit or a public URL to read instead, and handing that sentence to the model lets it correct itself. The system prompt is rewritten around the six prefixed tools as a cost ladder, with correct nm/on dataset ids and nemar.org/dataset links, plus what to relay about the streaming copy: lossy is always true, effective_rate_hz below source_rate_hz means a downsampled view, zarr_verify_status null means not yet checked rather than wrong, and a non-empty filled_ranges means part of a window is not real signal. Tests are real, no mocks: a genuine MCPServer over Streamable HTTP on a real socket, driven by the real client through the real LangChain wrappers. Plus a wiring test that reads the shipped config.yaml and asserts the prompt names the tools the loader actually produces, and a network-marked tier that runs against https://mcp.nemar.org and passes.
CI is red, and none of it is this PRThree failures, all pre-existing infrastructure. Evidence for each, since "not my
The app is not broken -- I checked before assuming. Under My own lockfile change adds
What this PR was verified againstLocally, on the locked dependency set: 1839 passed, 21 skipped, with only the And the part that matters most: the |
1 participant
Replaces the NEMAR assistant's two dataset tools, which were dead, with tools
served by NEMAR's own MCP server.
search_nemar_datasetsandget_nemar_dataset_detailscallednemar.org/api/dataexplorer/datapipeline/.... That endpoint returns 404 --the legacy dataexplorer site was retired and its URLs now redirect to
nemar.org/dataset/<id>-- so both tools had been non-functional. The MCP servermaps onto them almost exactly (
search_datasets,describe_dataset) and addsfour more for what is inside a dataset.
Server side: nemarOrg/nemar-cli epic #1065, live at
mcp.nemar.orgsince v0.10.1.Anonymous, no credentials.
The two design points
Discovery runs in a worker thread, and it has to. Tools are assembled in
CommunityAssistant.__init__-- synchronous, and the FastAPI app calls it frominside a running event loop, where
asyncio.runraisesRuntimeError. There isno correct way to await from inside a live loop on the same thread, so discovery
gets a dedicated thread with its own loop, which behaves identically whether or
not the caller has one. Invocation needs none of this: it is a plain coroutine
LangChain awaits normally.
Mutation-checked rather than asserted: replacing the worker thread with a bare
asyncio.runmakestest_works_from_inside_a_running_event_loopfail whiletest_works_from_a_plain_synchronous_callerstill passes.A session per call is right for this server. It is stateless by design: no
session id,
GET/DELETEon the endpoint are 405, nothing to keep alive. Soconnect-call-close costs one round trip and removes all lifecycle management --
reconnection, liveness, and a shared object whose failure mode is every tool
breaking at once. The module docstring says this so nobody "optimizes" it into a
persistent connection that then needs the machinery back.
Direct SDK usage rather than
langchain-mcp-adapters, which pinsmcp<2.0.0andtherefore cannot negotiate the 2026-07-28 revision this server implements. The
wrapping is about sixty lines.
mcp>=2.2.0goes in theserverextra, next tolangchain.
Degradation, deliberately
An unreachable server yields
[]and a log line -- the same contract_load_plugin_toolsalready has. An assistant that cannot start because someoneelse's host is down is worse than one missing a few tools.
A tool error comes back as text, not an exception. These refusals are written
to be read: they name the cap that was exceeded, or the public URL to fetch
instead. Handing that sentence to the model lets it correct itself, where raising
would just end the turn.
Prompt
Rewritten around the six prefixed tools (
nemar_search_datasets, ...) as anexplicit cost ladder, with correct
nm/ondataset ids instead of OpenNeurodsaccessions and
nemar.org/dataset/{id}instead of the retireddataexplorer/detail?dataset_id=. It also tells the model what to relay about thestreaming copy:
lossyis always true, aneffective_rate_hzbelowsource_rate_hzmeans the user is looking at a downsampled view,zarr_verify_status: nullmeans "not yet checked" rather than "wrong", and anon-empty
filled_rangesmeans part of a window is not real signal.Tests: real, no mocks
tests/test_tools/test_mcp_client.pystands up a genuineMCPServeroverStreamable HTTP on a real socket and drives it with the real client through the
real LangChain wrappers. 14 tests: discovery, name prefixing, the server's own
inputSchemapassed through verbatim, argument round-tripping, defaults,multi-block text joining, refusal-as-text, schema rejection, both event-loop
cases, and three degradation paths.
tests/test_assistants/test_nemar_mcp_wiring.pycovers the gap a client testcannot see: it reads the shipped
config.yamland asserts the prompt namesthe tools the loader actually produces. A prefix change or a config typo would
otherwise leave every client test green and the assistant broken.
Both files have a
network-marked tier that runs againsthttps://mcp.nemar.organd passes: the six tool names, a real search returning real datasets, and a
refusal arriving as readable text.
Gates
uv run pytest -m "not network": 1839 passed, 21 skipped.ruff check src testsand
ruff format --checkclean.mypy src/tools/mcp_client.pyclean. Pre-commithooks pass.
One unrelated failure locally,
test_version_shows_version, which asserts onun-colored output and passes under
TERM=dumb; it is a Rich-highlighting artifactof a local TTY, touches no file in this branch, and CI has no TTY.