-
Notifications
You must be signed in to change notification settings - Fork 349
[Feature] New API to discover, fetch, and call tools from server extensions #1521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
2fbfbf6
d44f68c
f6a5d42
56b1bfd
715e76e
f4fe448
24116cd
9b23590
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
import importlib | ||
from itertools import starmap | ||
|
||
import jsonschema | ||
from tornado.gen import multi | ||
from traitlets import Any, Bool, Dict, HasTraits, Instance, List, Unicode, default, observe | ||
from traitlets import validate as validate_trait | ||
|
@@ -13,6 +14,37 @@ | |
from .config import ExtensionConfigManager | ||
from .utils import ExtensionMetadataError, ExtensionModuleNotFound, get_loader, get_metadata | ||
|
||
# probably this should go in it's own file? Not sure where though | ||
MCP_TOOL_SCHEMA = { | ||
"type": "object", | ||
"properties": { | ||
"name": {"type": "string"}, | ||
"description": {"type": "string"}, | ||
"inputSchema": { | ||
"type": "object", | ||
"properties": { | ||
"type": {"type": "string", "enum": ["object"]}, | ||
"properties": {"type": "object"}, | ||
"required": {"type": "array", "items": {"type": "string"}}, | ||
}, | ||
"required": ["type", "properties"], | ||
}, | ||
"annotations": { | ||
"type": "object", | ||
"properties": { | ||
"title": {"type": "string"}, | ||
"readOnlyHint": {"type": "boolean"}, | ||
"destructiveHint": {"type": "boolean"}, | ||
"idempotentHint": {"type": "boolean"}, | ||
"openWorldHint": {"type": "boolean"}, | ||
}, | ||
"additionalProperties": True, | ||
}, | ||
}, | ||
"required": ["name", "inputSchema"], | ||
"additionalProperties": False, | ||
} | ||
|
||
|
||
class ExtensionPoint(HasTraits): | ||
"""A simple API for connecting to a Jupyter Server extension | ||
|
@@ -97,6 +129,29 @@ def module(self): | |
"""The imported module (using importlib.import_module)""" | ||
return self._module | ||
|
||
@property | ||
def tools(self): | ||
"""Structured tools exposed by this extension point, if any.""" | ||
loc = self.app or self.module | ||
if not loc: | ||
return {} | ||
|
||
tools_func = getattr(loc, "jupyter_server_extension_tools", None) | ||
if not callable(tools_func): | ||
return {} | ||
|
||
tools = {} | ||
try: | ||
definitions = tools_func() | ||
for name, tool in definitions.items(): | ||
# not sure if we should just pick MCP schema or make the schema something the user can pass | ||
jsonschema.validate(instance=tool["metadata"], schema=MCP_TOOL_SCHEMA) | ||
tools[name] = tool | ||
except Exception as e: | ||
# not sure if this should fail quietly, raise an error, or log it? | ||
print(f"[tool-discovery] Failed to load tools from {self.module_name}: {e}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using Before we do that, though, we'll need to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #1522. If/when that gets merged, we can rebase your PR to use the logger created there. |
||
return tools | ||
|
||
def _get_linker(self): | ||
"""Get a linker.""" | ||
if self.app: | ||
|
@@ -443,6 +498,22 @@ def load_all_extensions(self): | |
for name in self.sorted_extensions: | ||
self.load_extension(name) | ||
|
||
def get_tools(self) -> Dict[str, Any]: | ||
"""Aggregate tools from all extensions that expose them.""" | ||
all_tools = {} | ||
|
||
for ext_name, ext_pkg in self.extensions.items(): | ||
if not ext_pkg.enabled: | ||
continue | ||
|
||
for point in ext_pkg.extension_points.values(): | ||
for name, tool in point.tools.items(): # <— new property! | ||
Abigayle-Mercer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if name in all_tools: | ||
raise ValueError(f"Duplicate tool name detected: '{name}'") | ||
all_tools[name] = tool | ||
|
||
return all_tools | ||
|
||
async def start_all_extensions(self): | ||
"""Start all enabled extensions.""" | ||
# Sort the extension names to enforce deterministic loading | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, let's not require MCP here. Maybe MCP is the default (for now), but we should let server extensions arrive with their own schema that the server can validate against.
Maybe the returned value of the
jupyter_server_extension_tools
hook is a tuple with the tool definition and the schema to validate against?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good — so just to clarify, you're thinking one validation schema per extension (covering all of its exposed tools), rather than per tool?
Also, should we assume each tool will include both metadata and a callable? If so, does that remove the need to build something like a server-exposed endpoint for tool execution? I’m thinking specifically of my run function prototype: https://github.com/Abigayle-Mercer/list_ai_tools/blob/main/list_ai_tools/list_ai_tools.py#L103, which takes structured tool calls and executes them dynamically.
Just wondering if you envision those callables staying internal to the server, or whether we’d eventually want something like POST /api/tools/:name to run them.