mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-27 21:05:34 +00:00
`jarvis serve` constructed every heavy component inline (engine discovery + instrumentation, telemetry, memory, agent manager, per-agent tools) and then called `SystemBuilder(config).build()` a second time inside the scheduler block purely to feed `AgentExecutor.set_system()`. That second build re-discovered and re-connected the engine, re-instrumented it, re-resolved tools, re-opened the configured channel and re-created the agent manager — ~30-40s of fully redundant startup work (the headline remaining cost in #263 after engine probes were parallelised and the version check moved off the hot path in #470). Fix: assemble the executor's `JarvisSystem` from the components already built inline instead of rebuilding from scratch. `AgentExecutor` only reads `engine`, `model`, `config`, `memory_backend`, `tool_executor`, `session_store` and `channel_backend` off the system; all are wired here. The memory backend is now constructed just before the scheduler block (it was built later) so the executor's system can reference it, and the primary agent's resolved tool list is reused to build the scheduler's `ToolExecutor` (preserving the MCP-discovered-tool pool the executor reads via `tool_executor._tools`). `skill_manager` / the learning orchestrator are only consumed by the orchestrator's `system.ask()` path, which the executor never invokes, so they are intentionally omitted. Tests: new `tests/cli/test_serve_single_build.py` patches `SystemBuilder.build` and asserts it is never called during `jarvis serve` startup, and that the executor still receives a system exposing `tool_executor` / `session_store` / `memory_backend` (plus engine/model/config). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jon Saad-Falcon
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
176ed3029b
commit
4218258486
+73
-19
@@ -261,6 +261,11 @@ def serve(
|
||||
# Resolve agent
|
||||
agent = None
|
||||
agent_key = agent_name or config.server.agent
|
||||
# Tool instances resolved for the primary agent are reused below to build
|
||||
# the scheduler's ToolExecutor — avoiding a second full SystemBuilder.build()
|
||||
# (which would re-discover the engine, re-resolve tools, re-open the channel,
|
||||
# etc.). See the scheduler block near the bottom of this function (#263).
|
||||
resolved_tools: list = []
|
||||
if agent_key:
|
||||
try:
|
||||
import openjarvis.agents # noqa: F401
|
||||
@@ -328,6 +333,8 @@ def serve(
|
||||
|
||||
if tools:
|
||||
agent_kwargs["tools"] = tools
|
||||
# Reuse these for the scheduler's ToolExecutor (#263).
|
||||
resolved_tools = tools
|
||||
|
||||
if getattr(agent_cls, "accepts_tools", False):
|
||||
agent_kwargs["max_turns"] = config.agent.max_turns
|
||||
@@ -461,6 +468,24 @@ def serve(
|
||||
# Create app
|
||||
from openjarvis.server.app import create_app
|
||||
|
||||
# Set up memory backend for context injection. Built before the scheduler
|
||||
# block so the executor's JarvisSystem can reference it (#263).
|
||||
memory_backend = None
|
||||
if config.agent.context_from_memory:
|
||||
try:
|
||||
import openjarvis.tools.storage # noqa: F401
|
||||
from openjarvis.core.registry import MemoryRegistry
|
||||
|
||||
mem_key = config.memory.default_backend
|
||||
if MemoryRegistry.contains(mem_key):
|
||||
memory_backend = MemoryRegistry.create(
|
||||
mem_key,
|
||||
db_path=config.memory.db_path,
|
||||
)
|
||||
console.print(" Memory: [cyan]active[/cyan]")
|
||||
except Exception as exc:
|
||||
logger.debug("Memory backend init failed: %s", exc)
|
||||
|
||||
# Set up agent manager
|
||||
agent_manager = None
|
||||
if config.agent_manager.enabled:
|
||||
@@ -500,9 +525,55 @@ def serve(
|
||||
event_bus=bus,
|
||||
trace_store=_trace_store,
|
||||
)
|
||||
from openjarvis.system import SystemBuilder
|
||||
# Reuse the components already built inline above instead of a
|
||||
# second full SystemBuilder.build() — the original double-build
|
||||
# re-discovered the engine, re-instrumented it, re-resolved tools,
|
||||
# re-opened the channel and re-created the agent manager, costing
|
||||
# ~30-40s on top of an already-paid startup (#263). The executor
|
||||
# only reads engine/model/config/memory_backend/tool_executor/
|
||||
# session_store/channel_backend from the system (see
|
||||
# AgentExecutor), all of which are wired here.
|
||||
from openjarvis.sessions.session import SessionStore
|
||||
from openjarvis.system import JarvisSystem
|
||||
from openjarvis.tools._stubs import ToolExecutor
|
||||
|
||||
system = SystemBuilder(config).build()
|
||||
_sched_session_store = None
|
||||
if config.sessions.enabled:
|
||||
try:
|
||||
from pathlib import Path as _SchedPath
|
||||
|
||||
_sched_session_store = SessionStore(
|
||||
db_path=_SchedPath(config.sessions.db_path).expanduser(),
|
||||
max_age_hours=config.sessions.max_age_hours,
|
||||
consolidation_threshold=(
|
||||
config.sessions.consolidation_threshold
|
||||
),
|
||||
)
|
||||
except Exception as exc:
|
||||
logger.debug("Scheduler session store init failed: %s", exc)
|
||||
|
||||
_sched_tool_executor = (
|
||||
ToolExecutor(resolved_tools, bus) if resolved_tools else None
|
||||
)
|
||||
|
||||
system = JarvisSystem(
|
||||
config=config,
|
||||
bus=bus,
|
||||
engine=engine,
|
||||
engine_key=engine_name,
|
||||
model=model_name,
|
||||
agent=agent,
|
||||
agent_name=agent_key or "",
|
||||
tools=resolved_tools,
|
||||
tool_executor=_sched_tool_executor,
|
||||
memory_backend=memory_backend,
|
||||
telemetry_store=telem_store,
|
||||
trace_store=_trace_store,
|
||||
session_store=_sched_session_store,
|
||||
capability_policy=sec.capability_policy,
|
||||
agent_manager=agent_manager,
|
||||
agent_executor=executor,
|
||||
)
|
||||
executor.set_system(system)
|
||||
|
||||
agent_scheduler = AgentScheduler(
|
||||
@@ -522,23 +593,6 @@ def serve(
|
||||
except Exception as exc:
|
||||
logger.debug("Agent scheduler init failed: %s", exc)
|
||||
|
||||
# Set up memory backend for context injection
|
||||
memory_backend = None
|
||||
if config.agent.context_from_memory:
|
||||
try:
|
||||
import openjarvis.tools.storage # noqa: F401
|
||||
from openjarvis.core.registry import MemoryRegistry
|
||||
|
||||
mem_key = config.memory.default_backend
|
||||
if MemoryRegistry.contains(mem_key):
|
||||
memory_backend = MemoryRegistry.create(
|
||||
mem_key,
|
||||
db_path=config.memory.db_path,
|
||||
)
|
||||
console.print(" Memory: [cyan]active[/cyan]")
|
||||
except Exception as exc:
|
||||
logger.debug("Memory backend init failed: %s", exc)
|
||||
|
||||
# --- Channel Gateway: API key, sessions, ChannelBridge ---
|
||||
import os as _os
|
||||
|
||||
|
||||
@@ -0,0 +1,210 @@
|
||||
"""Regression tests for #263 — ``jarvis serve`` must build the system once.
|
||||
|
||||
serve.py used to construct all heavy components inline and then call
|
||||
``SystemBuilder(config).build()`` a second time inside the scheduler block,
|
||||
re-discovering the engine, re-instrumenting it, re-resolving tools, re-opening
|
||||
the channel and re-creating the agent manager — ~30-40s of redundant work.
|
||||
|
||||
These tests pin the fix:
|
||||
|
||||
1. ``SystemBuilder.build`` is never called during ``jarvis serve`` startup
|
||||
(the duplicate build is gone).
|
||||
2. The ``AgentExecutor`` still receives a system exposing the attributes it
|
||||
actually reads: ``tool_executor``, ``session_store``, ``memory_backend``,
|
||||
plus ``engine`` / ``model`` / ``config``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from click.testing import CliRunner
|
||||
|
||||
from openjarvis.cli import cli
|
||||
|
||||
pytest.importorskip("fastapi")
|
||||
pytest.importorskip("uvicorn")
|
||||
|
||||
# ``openjarvis.cli.serve`` as an attribute resolves to the click *command*
|
||||
# (re-exported on the package); grab the real module to monkeypatch its globals.
|
||||
serve_mod = importlib.import_module("openjarvis.cli.serve")
|
||||
|
||||
|
||||
def _fake_engine() -> MagicMock:
|
||||
engine = MagicMock()
|
||||
engine.list_models.return_value = ["test-model"]
|
||||
engine.health.return_value = True
|
||||
engine.name = "mock"
|
||||
return engine
|
||||
|
||||
|
||||
def _repopulate_registries() -> None:
|
||||
"""Re-run the @register decorators wiped by the autouse conftest fixture.
|
||||
|
||||
The tool/memory modules are import-cached, so a plain ``import`` inside
|
||||
serve.py is a no-op after the registries are cleared per-test. Reload the
|
||||
individual submodules so ToolRegistry/MemoryRegistry are populated exactly
|
||||
as they would be on a fresh process — otherwise serve would resolve an
|
||||
empty tool list and no memory backend, masking the very wiring under test.
|
||||
"""
|
||||
import importlib
|
||||
import sys
|
||||
|
||||
import openjarvis.agents # noqa: F401
|
||||
import openjarvis.tools # noqa: F401
|
||||
import openjarvis.tools.storage # noqa: F401
|
||||
from openjarvis.core.registry import (
|
||||
AgentRegistry,
|
||||
MemoryRegistry,
|
||||
ToolRegistry,
|
||||
)
|
||||
|
||||
if not AgentRegistry.keys():
|
||||
for mod_name in list(sys.modules):
|
||||
if mod_name.startswith("openjarvis.agents.") and not mod_name.endswith(
|
||||
"_stubs"
|
||||
):
|
||||
try:
|
||||
importlib.reload(sys.modules[mod_name])
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
if not ToolRegistry.keys():
|
||||
for mod_name in list(sys.modules):
|
||||
if (
|
||||
mod_name.startswith("openjarvis.tools.")
|
||||
and not mod_name.endswith("_stubs")
|
||||
and not mod_name.endswith("agent_tools")
|
||||
):
|
||||
try:
|
||||
importlib.reload(sys.modules[mod_name])
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
if not MemoryRegistry.keys():
|
||||
for mod_name in list(sys.modules):
|
||||
if mod_name.startswith(
|
||||
"openjarvis.tools.storage."
|
||||
) and not mod_name.endswith("_stubs"):
|
||||
try:
|
||||
importlib.reload(sys.modules[mod_name])
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
def _run_serve(tmp_path, monkeypatch, *, build_spy, set_system_spy):
|
||||
"""Invoke ``jarvis serve`` with all heavy/blocking pieces stubbed out.
|
||||
|
||||
Returns the CliRunner result. The server is never actually started
|
||||
(``uvicorn.run`` is a no-op) and no real engine is contacted.
|
||||
"""
|
||||
from openjarvis.core.config import JarvisConfig
|
||||
|
||||
_repopulate_registries()
|
||||
|
||||
config = JarvisConfig()
|
||||
# Keep the scheduler block alive (it owns the executor wiring under test)
|
||||
# while pointing every store at the temp dir.
|
||||
config.agent_manager.enabled = True
|
||||
config.agent_manager.db_path = str(tmp_path / "agents.db")
|
||||
config.sessions.enabled = True
|
||||
config.sessions.db_path = str(tmp_path / "sessions.db")
|
||||
config.memory.db_path = str(tmp_path / "memory.db")
|
||||
config.telemetry.enabled = False
|
||||
config.traces.enabled = False
|
||||
config.channel.enabled = False
|
||||
config.skills.enabled = False
|
||||
config.server.host = "127.0.0.1"
|
||||
config.server.port = 8123
|
||||
# Resolve a model without contacting a real engine / discovery.
|
||||
config.intelligence.default_model = "test-model"
|
||||
|
||||
engine = _fake_engine()
|
||||
|
||||
monkeypatch.setattr(serve_mod, "load_config", lambda *a, **k: config)
|
||||
monkeypatch.setattr(serve_mod, "get_engine", lambda *a, **k: ("mock", engine))
|
||||
monkeypatch.setattr(serve_mod, "discover_engines", lambda *a, **k: {})
|
||||
monkeypatch.setattr(serve_mod, "discover_models", lambda *a, **k: {})
|
||||
|
||||
# setup_security returns its own context; pass the engine straight through
|
||||
# so we don't need real guardrails wired up.
|
||||
sec = MagicMock()
|
||||
sec.engine = engine
|
||||
sec.capability_policy = None
|
||||
sec.audit_logger = None
|
||||
monkeypatch.setattr("openjarvis.security.setup_security", lambda *a, **k: sec)
|
||||
|
||||
with (
|
||||
patch(
|
||||
"openjarvis.system.builder.SystemBuilder.build",
|
||||
build_spy,
|
||||
),
|
||||
patch(
|
||||
"openjarvis.agents.executor.AgentExecutor.set_system",
|
||||
set_system_spy,
|
||||
),
|
||||
patch("uvicorn.run", lambda *a, **k: None),
|
||||
):
|
||||
return CliRunner().invoke(cli, ["serve"], catch_exceptions=False)
|
||||
|
||||
|
||||
def test_serve_does_not_call_systembuilder_build(tmp_path, monkeypatch):
|
||||
"""The redundant second full build is gone (#263)."""
|
||||
build_spy = MagicMock(
|
||||
side_effect=AssertionError(
|
||||
"SystemBuilder.build() must not run during `jarvis serve` startup "
|
||||
"— it is the duplicate build #263 removed."
|
||||
)
|
||||
)
|
||||
set_system_spy = MagicMock()
|
||||
|
||||
result = _run_serve(
|
||||
tmp_path,
|
||||
monkeypatch,
|
||||
build_spy=build_spy,
|
||||
set_system_spy=set_system_spy,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
build_spy.assert_not_called()
|
||||
|
||||
|
||||
def test_executor_receives_required_system_attrs(tmp_path, monkeypatch):
|
||||
"""The executor still gets a system exposing the attributes it reads.
|
||||
|
||||
AgentExecutor reads engine/model/config/memory_backend/tool_executor/
|
||||
session_store off ``self._system``; the de-dup must not strip any of them.
|
||||
"""
|
||||
build_spy = MagicMock()
|
||||
captured: dict = {}
|
||||
|
||||
def _capture_set_system(self, system): # noqa: ANN001
|
||||
captured["system"] = system
|
||||
# Preserve real behaviour so the executor is usable afterwards.
|
||||
self._system = system
|
||||
|
||||
result = _run_serve(
|
||||
tmp_path,
|
||||
monkeypatch,
|
||||
build_spy=build_spy,
|
||||
set_system_spy=_capture_set_system,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
# Built once, from the inline components — not via SystemBuilder.build().
|
||||
build_spy.assert_not_called()
|
||||
|
||||
system = captured.get("system")
|
||||
assert system is not None, "executor.set_system was never called"
|
||||
|
||||
# Correctness constraint from the verifier: these must survive the de-dup.
|
||||
assert system.tool_executor is not None
|
||||
assert system.session_store is not None
|
||||
assert system.memory_backend is not None
|
||||
|
||||
# And the basics the executor resolves engine/model from.
|
||||
assert system.engine is not None
|
||||
assert system.model == "test-model"
|
||||
assert system.config is not None
|
||||
Reference in New Issue
Block a user