perf: lazy mcp SDK import + tool-discovery mtime cache + browser_tool import diet
Three cold-start cuts, measured with .venv python, PYTHONPATH=worktree,
median of 3-5 fresh subprocesses:
1. tools/mcp_oauth.py — availability now via importlib.util.find_spec("mcp");
SDK classes (OAuthClientProvider et al.) import lazily on first use via
_ensure_sdk_loaded(). Module-level names kept as None placeholders so the
test-patch surface (patch.object(mcp_oauth, "OAuthClientProvider", ...))
still works. import tools.mcp_oauth: 242ms -> 56ms (mcp SDK no longer
loaded at import time).
2. tools/registry.py — discover_builtin_tools AST scan memoized in an
mtime_ns+size-keyed disk cache at ~/.hermes/cache/tool_discovery_cache.json
(atomic write via utils.atomic_json_write, best-effort/never raises;
corrupt or missing cache -> full rescan + rewrite; per-file stat mismatch
-> rescan just that file). Scan of 100 files: 158ms cold -> 3ms warm.
3. tools/browser_tool.py — top-level `import requests` and
agent.auxiliary_client.call_llm moved to lazy first-use (PEP 562
__getattr__ preserves patch("tools.browser_tool.requests.get") and
patch("tools.browser_tool.call_llm") surfaces; internal call sites go
through _lazy_call_llm which reads module globals so patches are honored).
Entry-point imports (median ms, before -> after):
import model_tools 392 -> 245 (warm discovery cache)
import cli 152 -> 151 (unchanged; cli doesn't hit these paths)
import gateway.run 234 -> 230
Functional verification:
- get_tool_definitions(quiet_mode=True) under temp HERMES_HOME: identical
sorted 30-tool name set before vs after (empty diff).
- Discovery cache: delete cache -> 158ms, second run -> 3ms; corrupt cache
-> clean full rescan; touching one file -> single-file rescan (12ms).
- Tests green: tests/tools/test_registry.py, all test_mcp_oauth*,
test_mcp_dashboard_oauth, test_mcp_tool_401_handling, all
tests/tools/test_browser*.py, tests/hermes_cli/test_mcp_{config,startup,
dashboard_oauth}.py, test_skills_tool_discovery_cache.py.
This commit is contained in:
+37
-5
@@ -61,10 +61,8 @@ import sys
|
||||
import tempfile
|
||||
import threading
|
||||
import time
|
||||
import requests
|
||||
from typing import Dict, Any, Optional, List, Tuple, Union
|
||||
from pathlib import Path
|
||||
from agent.auxiliary_client import call_llm
|
||||
from agent.redact import redact_cdp_url
|
||||
from hermes_constants import (
|
||||
agent_browser_runnable,
|
||||
@@ -75,6 +73,38 @@ from utils import env_int, is_truthy_value
|
||||
from hermes_cli.config import DEFAULT_CONFIG, cfg_get
|
||||
from hermes_cli._subprocess_compat import windows_hide_flags
|
||||
|
||||
|
||||
def __getattr__(name: str):
|
||||
"""Lazy module attributes (PEP 562) — import diet for cold start.
|
||||
|
||||
``requests`` (~40 ms) and ``agent.auxiliary_client.call_llm`` (~65 ms)
|
||||
are only needed on specific code paths, so they load on first use. The
|
||||
module-level names are preserved for the test-patch surface
|
||||
(``patch("tools.browser_tool.requests.get")`` /
|
||||
``patch("tools.browser_tool.call_llm")``): first attribute access imports
|
||||
the real object and binds it into module globals.
|
||||
"""
|
||||
if name == "requests":
|
||||
import requests as _requests
|
||||
|
||||
globals()["requests"] = _requests
|
||||
return _requests
|
||||
if name == "call_llm":
|
||||
from agent.auxiliary_client import call_llm as _call_llm
|
||||
|
||||
globals()["call_llm"] = _call_llm
|
||||
return _call_llm
|
||||
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
|
||||
|
||||
|
||||
def _lazy_call_llm(*args, **kwargs):
|
||||
"""Invoke ``call_llm`` through module globals so test patches of
|
||||
``tools.browser_tool.call_llm`` are honored, importing lazily otherwise."""
|
||||
fn = globals().get("call_llm")
|
||||
if fn is None:
|
||||
fn = __getattr__("call_llm")
|
||||
return fn(*args, **kwargs)
|
||||
|
||||
# Browser-specific tool keys passed through to the agent-browser subprocess
|
||||
# AFTER credential stripping. agent-browser is a Node process loading npm
|
||||
# deps; handing it the full operator keyring (#29157 / GHSA-m4m8-xjp4-5rmm)
|
||||
@@ -433,6 +463,8 @@ def _resolve_cdp_override(cdp_url: str) -> str:
|
||||
version_url = discovery_url.rstrip("/") + "/json/version"
|
||||
|
||||
try:
|
||||
import requests # lazy — shared module object, test patches still apply
|
||||
|
||||
response = requests.get(version_url, timeout=10)
|
||||
response.raise_for_status()
|
||||
payload = response.json()
|
||||
@@ -2770,7 +2802,7 @@ def _extract_relevant_content(
|
||||
model = _get_extraction_model()
|
||||
if model:
|
||||
call_kwargs["model"] = model
|
||||
response = call_llm(**call_kwargs)
|
||||
response = _lazy_call_llm(**call_kwargs)
|
||||
extracted = (response.choices[0].message.content or "").strip()
|
||||
if not extracted:
|
||||
# _truncate_snapshot stores its own pointer (dedupes to the same
|
||||
@@ -4333,7 +4365,7 @@ def browser_vision(question: str, annotate: bool = False, task_id: Optional[str]
|
||||
call_kwargs["model"] = vision_model
|
||||
# Try full-size screenshot; on size-related rejection, downscale and retry.
|
||||
try:
|
||||
response = call_llm(**call_kwargs)
|
||||
response = _lazy_call_llm(**call_kwargs)
|
||||
except Exception as _api_err:
|
||||
from tools.vision_tools import (
|
||||
_is_image_size_error, _resize_image_for_vision, _RESIZE_TARGET_BYTES,
|
||||
@@ -4349,7 +4381,7 @@ def browser_vision(question: str, annotate: bool = False, task_id: Optional[str]
|
||||
data_url = _resize_image_for_vision(
|
||||
screenshot_path, mime_type="image/png")
|
||||
call_kwargs["messages"][0]["content"][1]["image_url"]["url"] = data_url
|
||||
response = call_llm(**call_kwargs)
|
||||
response = _lazy_call_llm(**call_kwargs)
|
||||
else:
|
||||
raise
|
||||
|
||||
|
||||
+74
-12
@@ -60,20 +60,70 @@ logger = logging.getLogger(__name__)
|
||||
# Lazy imports -- MCP SDK with OAuth support is optional
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_OAUTH_AVAILABLE=False
|
||||
try:
|
||||
from mcp.client.auth import OAuthClientProvider
|
||||
from mcp.shared.auth import (
|
||||
OAuthClientInformationFull,
|
||||
OAuthClientMetadata,
|
||||
OAuthMetadata,
|
||||
OAuthToken,
|
||||
)
|
||||
# Availability is detected WITHOUT importing the mcp SDK (which costs
|
||||
# ~170 ms at module load). The actual classes are imported lazily on first
|
||||
# use via _ensure_sdk_loaded(); the module-level names below are kept as
|
||||
# placeholders so tests can patch them (patch.object requires the attribute
|
||||
# to exist on the module).
|
||||
import importlib.util as _importlib_util
|
||||
|
||||
_OAUTH_AVAILABLE=True
|
||||
except ImportError:
|
||||
_OAUTH_AVAILABLE = _importlib_util.find_spec("mcp") is not None
|
||||
if not _OAUTH_AVAILABLE:
|
||||
logger.debug("MCP OAuth types not available -- OAuth MCP auth disabled")
|
||||
|
||||
# Lazily-bound SDK names (rebound by _ensure_sdk_loaded on first use).
|
||||
# Annotated ``Any`` so quoted type annotations elsewhere in the file remain
|
||||
# valid for static checkers while the runtime value starts as None.
|
||||
OAuthClientProvider: Any = None
|
||||
OAuthClientInformationFull: Any = None
|
||||
OAuthClientMetadata: Any = None
|
||||
OAuthMetadata: Any = None
|
||||
OAuthToken: Any = None
|
||||
|
||||
# Cache of the real SDK classes so a test that temporarily patches one of the
|
||||
# module-level names (and restores it to None afterwards) doesn't strand the
|
||||
# module in a broken state.
|
||||
_SDK_CLASSES: dict[str, Any] = {}
|
||||
_SDK_LOAD_FAILED = False
|
||||
|
||||
|
||||
def _ensure_sdk_loaded() -> bool:
|
||||
"""Import the MCP SDK OAuth classes on first use and bind module globals.
|
||||
|
||||
Returns True when the SDK classes are available. Module-level names that
|
||||
have been replaced (e.g. patched by tests) are left untouched; only names
|
||||
that are currently ``None`` are (re)bound to the real SDK classes.
|
||||
"""
|
||||
global _SDK_LOAD_FAILED, _OAUTH_AVAILABLE
|
||||
if _SDK_LOAD_FAILED:
|
||||
return False
|
||||
if not _SDK_CLASSES:
|
||||
try:
|
||||
from mcp.client.auth import OAuthClientProvider as _Provider
|
||||
from mcp.shared.auth import (
|
||||
OAuthClientInformationFull as _InfoFull,
|
||||
OAuthClientMetadata as _ClientMeta,
|
||||
OAuthMetadata as _Meta,
|
||||
OAuthToken as _Token,
|
||||
)
|
||||
except ImportError:
|
||||
_SDK_LOAD_FAILED = True
|
||||
_OAUTH_AVAILABLE = False
|
||||
logger.debug("MCP OAuth types not available -- OAuth MCP auth disabled")
|
||||
return False
|
||||
_SDK_CLASSES.update(
|
||||
OAuthClientProvider=_Provider,
|
||||
OAuthClientInformationFull=_InfoFull,
|
||||
OAuthClientMetadata=_ClientMeta,
|
||||
OAuthMetadata=_Meta,
|
||||
OAuthToken=_Token,
|
||||
)
|
||||
g = globals()
|
||||
for _name, _cls in _SDK_CLASSES.items():
|
||||
if g.get(_name) is None:
|
||||
g[_name] = _cls
|
||||
return True
|
||||
|
||||
try:
|
||||
from pydantic import AnyUrl
|
||||
except ImportError:
|
||||
@@ -405,6 +455,8 @@ class HermesTokenStorage:
|
||||
data = _read_json(self._tokens_path())
|
||||
if data is None:
|
||||
return None
|
||||
if OAuthToken is None and not _ensure_sdk_loaded():
|
||||
return None
|
||||
# Hermes records an absolute wall-clock ``expires_at`` alongside the
|
||||
# SDK's serialized token (see ``set_tokens``). On read we rewrite
|
||||
# ``expires_in`` to the remaining seconds so the SDK's downstream
|
||||
@@ -466,6 +518,8 @@ class HermesTokenStorage:
|
||||
data = _read_json(self._client_info_path())
|
||||
if data is None:
|
||||
return None
|
||||
if OAuthClientInformationFull is None and not _ensure_sdk_loaded():
|
||||
return None
|
||||
try:
|
||||
return OAuthClientInformationFull.model_validate(data)
|
||||
except (ValueError, TypeError, KeyError) as exc:
|
||||
@@ -492,6 +546,8 @@ class HermesTokenStorage:
|
||||
data = _read_json(self._meta_path())
|
||||
if data is None:
|
||||
return None
|
||||
if OAuthMetadata is None and not _ensure_sdk_loaded():
|
||||
return None
|
||||
try:
|
||||
return OAuthMetadata.model_validate(data)
|
||||
except (ValueError, TypeError, KeyError) as exc:
|
||||
@@ -1139,6 +1195,8 @@ def _build_client_metadata(cfg: dict) -> "OAuthClientMetadata":
|
||||
raise ValueError(
|
||||
"_configure_callback_port() must be called before _build_client_metadata()"
|
||||
)
|
||||
if OAuthClientMetadata is None:
|
||||
_ensure_sdk_loaded()
|
||||
client_name = cfg.get("client_name", "Hermes Agent")
|
||||
scope = cfg.get("scope")
|
||||
redirect_uri = _resolve_redirect_uri(cfg, port)
|
||||
@@ -1171,6 +1229,8 @@ def _maybe_preregister_client(
|
||||
client_id = cfg.get("client_id")
|
||||
if not client_id:
|
||||
return
|
||||
if OAuthClientInformationFull is None:
|
||||
_ensure_sdk_loaded()
|
||||
port = cfg["_resolved_port"]
|
||||
redirect_uri = _resolve_redirect_uri(cfg, port)
|
||||
|
||||
@@ -1263,7 +1323,9 @@ def build_oauth_auth(
|
||||
An ``OAuthClientProvider`` instance, or None if the MCP SDK lacks
|
||||
OAuth support.
|
||||
"""
|
||||
if not _OAUTH_AVAILABLE:
|
||||
if not _OAUTH_AVAILABLE or (
|
||||
OAuthClientProvider is None and not _ensure_sdk_loaded()
|
||||
):
|
||||
logger.warning(
|
||||
"MCP OAuth requested for '%s' but SDK auth types are not available. "
|
||||
"Install with: pip install 'mcp>=1.26.0'",
|
||||
|
||||
+80
-7
@@ -65,14 +65,48 @@ def _module_registers_tools(module_path: Path) -> bool:
|
||||
|
||||
|
||||
def discover_builtin_tools(tools_dir: Optional[Path] = None) -> List[str]:
|
||||
"""Import built-in self-registering tool modules and return their module names."""
|
||||
"""Import built-in self-registering tool modules and return their module names.
|
||||
|
||||
The per-file AST scan (:func:`_module_registers_tools`) costs ~145 ms over
|
||||
~100 files on a warm cache, so verdicts are memoized on disk keyed by
|
||||
``(mtime_ns, size)``. A file whose mtime_ns+size match the cached entry is
|
||||
trusted without re-reading; any mismatch (or a corrupt/missing cache file)
|
||||
falls back to a fresh scan for that file. The cache write is best-effort
|
||||
and atomic, so concurrent processes can race harmlessly.
|
||||
"""
|
||||
tools_path = Path(tools_dir) if tools_dir is not None else Path(__file__).resolve().parent
|
||||
module_names = [
|
||||
f"tools.{path.stem}"
|
||||
for path in sorted(tools_path.glob("*.py"))
|
||||
if path.name not in {"__init__.py", "registry.py", "mcp_tool.py"}
|
||||
and _module_registers_tools(path)
|
||||
]
|
||||
|
||||
cache = _load_discovery_cache()
|
||||
fresh_cache: Dict[str, list] = {}
|
||||
cache_dirty = False
|
||||
|
||||
module_names: List[str] = []
|
||||
for path in sorted(tools_path.glob("*.py")):
|
||||
if path.name in {"__init__.py", "registry.py", "mcp_tool.py"}:
|
||||
continue
|
||||
abs_path = str(path.resolve())
|
||||
try:
|
||||
st = path.stat()
|
||||
stat_key = (st.st_mtime_ns, st.st_size)
|
||||
except OSError:
|
||||
continue
|
||||
cached = cache.get(abs_path)
|
||||
if (
|
||||
isinstance(cached, (list, tuple))
|
||||
and len(cached) == 3
|
||||
and (cached[0], cached[1]) == stat_key
|
||||
):
|
||||
registers = bool(cached[2])
|
||||
else:
|
||||
registers = _module_registers_tools(path)
|
||||
cache_dirty = True
|
||||
fresh_cache[abs_path] = [stat_key[0], stat_key[1], registers]
|
||||
if registers:
|
||||
module_names.append(f"tools.{path.stem}")
|
||||
|
||||
# Drop entries for files that no longer exist; rewrite only when changed.
|
||||
if cache_dirty or set(fresh_cache) != set(cache):
|
||||
_save_discovery_cache(fresh_cache)
|
||||
|
||||
imported: List[str] = []
|
||||
for mod_name in module_names:
|
||||
@@ -84,6 +118,45 @@ def discover_builtin_tools(tools_dir: Optional[Path] = None) -> List[str]:
|
||||
return imported
|
||||
|
||||
|
||||
def _discovery_cache_path() -> Optional[Path]:
|
||||
"""Path of the tool-discovery verdict cache, or None if unresolvable."""
|
||||
try:
|
||||
# Deferred import keeps tools/registry.py a no-deps leaf at module
|
||||
# import time (hermes_constants itself is stdlib-only, so no cycle).
|
||||
from hermes_constants import get_hermes_home
|
||||
|
||||
return Path(get_hermes_home()) / "cache" / "tool_discovery_cache.json"
|
||||
except Exception:
|
||||
return None
|
||||
|
||||
|
||||
def _load_discovery_cache() -> Dict[str, list]:
|
||||
"""Read the discovery cache; any error → empty dict (full scan)."""
|
||||
path = _discovery_cache_path()
|
||||
if path is None:
|
||||
return {}
|
||||
try:
|
||||
with open(path, "r", encoding="utf-8") as fh:
|
||||
data = json.load(fh)
|
||||
return data if isinstance(data, dict) else {}
|
||||
except (OSError, ValueError):
|
||||
return {}
|
||||
|
||||
|
||||
def _save_discovery_cache(cache: Dict[str, list]) -> None:
|
||||
"""Best-effort atomic write of the discovery cache. Never raises."""
|
||||
path = _discovery_cache_path()
|
||||
if path is None:
|
||||
return
|
||||
try:
|
||||
from utils import atomic_json_write # stdlib+yaml only; no cycle
|
||||
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
atomic_json_write(path, cache, indent=0)
|
||||
except Exception as e:
|
||||
logger.debug("Could not write tool discovery cache %s: %s", path, e)
|
||||
|
||||
|
||||
class ToolEntry:
|
||||
"""Metadata for a single registered tool."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user