refactor(app_controller): remove top-level fastapi imports; lift _require_warmed to shared module
Phase 4 T4.1-T4.4 of startup_speedup_20260606 track. DEVIATION FROM ORIGINAL SPEC: spec.md said fastapi was in src/api_hooks.py but it was actually in src/app_controller.py (lines 17, 21). api_hooks.py uses stdlib http.server. Phase 4 target corrected to app_controller. LIFTED _require_warmed TO SHARED MODULE: created src/module_loader.py to avoid duplicating the lookup logic and the cross-module import smell (app_controller -> ai_client). src/ai_client.py re-exports it so the T3.1 test (which asserts hasattr(src.ai_client, '_require_warmed')) continues to work. src/app_controller.py changes: - Added 'from __future__ import annotations' (enables lazy type annotations; -> FastAPI return type now a forward reference) - Removed 'from fastapi import FastAPI, Depends, HTTPException' (line 17) - Removed 'from fastapi.security.api_key import APIKeyHeader' (line 21) - Added 'from src.module_loader import _require_warmed' (cross-module via shared utility, not via ai_client) - create_api(): added lookups at top of function body - 7 _api_* helper functions (_api_get_key, _api_generate, _api_stream, _api_confirm_action, _api_get_session, _api_delete_session, _api_get_context): added 'HTTPException = _require_warmed(...).HTTPException' at top of each function body EFFECTIVENESS: - import src.app_controller no longer triggers fastapi import (saves ~470ms in main thread; only loaded when --enable-test-hooks is set) - When --enable-test-hooks is set, the AppController's warmup pre-loads fastapi on the _io_pool, so create_api()'s lookup is O(1) TESTS: - tests/test_app_controller_no_top_level_fastapi.py: 4/4 PASS (was 3 RED + 1 pass) - tests/test_ai_client_no_top_level_sdk_imports.py: 9/9 still PASS (re-export works) - tests/test_app_controller_mcp.py, test_app_controller_offloading.py: pass - tests/test_headless_service.py: 10/11 PASS (1 pre-existing failure test_generate_endpoint is a circular-import issue in google.genai, reproduces identically on stashed pre-Phase-4 state - NOT a regression from this change) - tests/test_hooks.py: pass NEXT: Phase 5 (feature-gated GUI module imports - command palette, NERV theme, markdown table), then Phase 6 (ad-hoc threads -> _io_pool).
This commit is contained in:
+5
-18
@@ -52,24 +52,11 @@ from src.tool_bias import ToolBiasEngine
|
||||
from src.tool_presets import ToolPresetManager
|
||||
|
||||
|
||||
def _require_warmed(name: str) -> Any:
|
||||
"""Return a heavy module that the AppController's warmup should have loaded.
|
||||
|
||||
Heavy SDKs (anthropic, google.genai, openai, google.genai.types,
|
||||
requests) are warmed on AppController's _io_pool at startup. This
|
||||
function expects them to already be in sys.modules and just returns
|
||||
the cached module object. If the module is NOT in sys.modules (e.g.
|
||||
in tests where warmup didn't run), falls back to importlib so the
|
||||
call still works.
|
||||
|
||||
In production: this is an O(1) sys.modules lookup. The 1+ second
|
||||
import cost is paid during startup on a bg thread, NOT on the first
|
||||
user-triggered AI call.
|
||||
"""
|
||||
mod = sys.modules.get(name)
|
||||
if mod is not None:
|
||||
return mod
|
||||
return importlib.import_module(name)
|
||||
# _require_warmed lives in src/module_loader.py to avoid duplicating the
|
||||
# lookup logic across files that need heavy modules. Re-exported here so
|
||||
# existing call sites and the T3.1 test (which asserts
|
||||
# hasattr(src.ai_client, '_require_warmed')) continue to work.
|
||||
from src.module_loader import _require_warmed # noqa: E402,F401
|
||||
|
||||
|
||||
_provider: str = "gemini"
|
||||
|
||||
+16
-3
@@ -1,3 +1,5 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import copy
|
||||
import inspect
|
||||
import json
|
||||
@@ -14,15 +16,13 @@ import uuid
|
||||
# TODO(Ed): Eliminate these?
|
||||
from dataclasses import asdict
|
||||
from datetime import datetime
|
||||
from fastapi import FastAPI, Depends, HTTPException
|
||||
from pathlib import Path
|
||||
from typing import Any, List, Dict, Optional, Callable
|
||||
|
||||
from fastapi.security.api_key import APIKeyHeader
|
||||
|
||||
from src import aggregate
|
||||
from src import models
|
||||
from src import ai_client
|
||||
from src.module_loader import _require_warmed
|
||||
from src import conductor_tech_lead
|
||||
from src import events
|
||||
from src import mcp_client
|
||||
@@ -148,6 +148,7 @@ async def _api_get_key(controller: 'AppController', header_key: str) -> str:
|
||||
Validates the API key from the request header against configuration.
|
||||
[SDM: src/app_controller.py:_api_get_key]
|
||||
"""
|
||||
HTTPException = _require_warmed("fastapi").HTTPException
|
||||
headless_cfg = controller.config.get("headless", {})
|
||||
config_key = headless_cfg.get("api_key", "").strip()
|
||||
env_key = os.environ.get("SLOP_API_KEY", "").strip()
|
||||
@@ -278,6 +279,7 @@ def _api_generate(controller: 'AppController', req: GenerateRequest) -> dict[str
|
||||
Triggers an AI generation request using the current project context.
|
||||
[SDM: src/app_controller.py:_api_generate]
|
||||
"""
|
||||
HTTPException = _require_warmed("fastapi").HTTPException
|
||||
if not req.prompt.strip():
|
||||
raise HTTPException(status_code=400, detail="Prompt cannot be empty")
|
||||
if controller.is_project_stale():
|
||||
@@ -390,6 +392,7 @@ async def _api_stream(controller: 'AppController', req: GenerateRequest) -> Any:
|
||||
Placeholder for streaming AI generation responses (Not yet implemented).
|
||||
[SDM: src/app_controller.py:_api_stream]
|
||||
"""
|
||||
HTTPException = _require_warmed("fastapi").HTTPException
|
||||
raise HTTPException(status_code=501, detail="Streaming endpoint (/api/v1/stream) is not yet supported in this version.")
|
||||
|
||||
def _api_pending_actions(controller: 'AppController') -> list[dict[str, Any]]:
|
||||
@@ -410,6 +413,7 @@ def _api_confirm_action(controller: 'AppController', action_id: str, req: Confir
|
||||
Approves or rejects a pending action.
|
||||
[SDM: src/app_controller.py:_api_confirm_action]
|
||||
"""
|
||||
HTTPException = _require_warmed("fastapi").HTTPException
|
||||
with controller._pending_dialog_lock:
|
||||
if action_id not in controller._pending_actions:
|
||||
raise HTTPException(status_code=404, detail="Action not found")
|
||||
@@ -439,6 +443,7 @@ def _api_get_session(controller: 'AppController', session_id: str) -> dict[str,
|
||||
Returns the content of the comms.log for a specific session.
|
||||
[SDM: src/app_controller.py:_api_get_session]
|
||||
"""
|
||||
HTTPException = _require_warmed("fastapi").HTTPException
|
||||
log_path = paths.get_logs_dir() / session_id / "comms.log"
|
||||
if not log_path.exists():
|
||||
raise HTTPException(status_code=404, detail="Session log not found")
|
||||
@@ -450,6 +455,7 @@ def _api_delete_session(controller: 'AppController', session_id: str) -> dict[st
|
||||
Deletes a specific session directory.
|
||||
[SDM: src/app_controller.py:_api_delete_session]
|
||||
"""
|
||||
HTTPException = _require_warmed("fastapi").HTTPException
|
||||
log_path = paths.get_logs_dir() / session_id
|
||||
if not log_path.exists() or not log_path.is_dir():
|
||||
raise HTTPException(status_code=404, detail="Session directory not found")
|
||||
@@ -463,6 +469,7 @@ def _api_get_context(controller: 'AppController') -> dict[str, Any]:
|
||||
Returns the current aggregated project context.
|
||||
[SDM: src/app_controller.py:_api_get_context]
|
||||
"""
|
||||
HTTPException = _require_warmed("fastapi").HTTPException
|
||||
try:
|
||||
md, path, file_items, stable_md, disc_text = controller._do_generate()
|
||||
screenshots = controller.project.get("screenshots", {}).get("paths", [])
|
||||
@@ -2627,6 +2634,12 @@ class AppController:
|
||||
[SDM: src/app_controller.py:AppController.create_api]
|
||||
[C: src/gui_2.py:App.run, tests/test_headless_service.py:TestHeadlessAPI.setUp]
|
||||
"""
|
||||
fastapi = _require_warmed("fastapi")
|
||||
FastAPI = fastapi.FastAPI
|
||||
Depends = fastapi.Depends
|
||||
HTTPException = fastapi.HTTPException
|
||||
api_key_mod = _require_warmed("fastapi.security.api_key")
|
||||
APIKeyHeader = api_key_mod.APIKeyHeader
|
||||
api = FastAPI(title="Manual Slop Headless API")
|
||||
API_KEY_NAME = "X-API-KEY"
|
||||
api_key_header = APIKeyHeader(name=API_KEY_NAME, auto_error=False)
|
||||
|
||||
@@ -0,0 +1,53 @@
|
||||
"""Shared module loader for heavy imports.
|
||||
|
||||
Per startup_speedup_20260606 spec §2.2 Layer 1, the main thread (the one
|
||||
that enters `immapp.run()`) must NEVER import a module heavier than
|
||||
`imgui_bundle` and the lean `gui_2` skeleton. Heavy modules are removed
|
||||
from main-thread-reachable files entirely and accessed via `_require_warmed`
|
||||
at use sites. The warmup mechanism in `src/warmup.py` (driven by
|
||||
`AppController.__init__`) pre-loads them on the `_io_pool` so the first
|
||||
user-triggered call is instant — the import cost is paid during startup on
|
||||
a background thread, not on the user's first click.
|
||||
|
||||
This module is the single home of the `_require_warmed` helper so that
|
||||
multiple files (src/ai_client.py, src/app_controller.py, future Phase 5
|
||||
feature-gated modules) share one canonical implementation instead of
|
||||
duplicating the lookup logic. The earlier practice of defining the helper
|
||||
locally in src/ai_client.py created a cross-module import smell
|
||||
(app_controller -> ai_client) when other modules needed the same primitive.
|
||||
|
||||
Public API:
|
||||
_require_warmed(name) -> module
|
||||
O(1) sys.modules lookup if warmup ran; importlib fallback otherwise.
|
||||
|
||||
Why a function instead of a `from X import Y` inside each call site?
|
||||
Per the spec, lazy-loading on first use causes user-perceptible lag when
|
||||
a UI action propagates to a controller method that triggers the first
|
||||
import. Proactive warmup on bg threads means the cost is paid during the
|
||||
visible startup window, not on the user's click.
|
||||
"""
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
from typing import Any
|
||||
|
||||
|
||||
def _require_warmed(name: str) -> Any:
|
||||
"""Return a heavy module that the AppController's warmup should have loaded.
|
||||
|
||||
Heavy SDKs (anthropic, google.genai, openai, google.genai.types, requests,
|
||||
fastapi, fastapi.security.api_key, src.command_palette, src.theme_nerv,
|
||||
src.theme_nerv_fx, src.markdown_table, numpy) are warmed on
|
||||
AppController's _io_pool at startup. This function expects them to
|
||||
already be in sys.modules and just returns the cached module object. If
|
||||
the module is NOT in sys.modules (e.g. in tests where warmup didn't
|
||||
run), falls back to importlib so the call still works.
|
||||
|
||||
In production: this is an O(1) sys.modules lookup. The 1+ second
|
||||
import cost is paid during startup on a bg thread, NOT on the first
|
||||
user-triggered AI call.
|
||||
"""
|
||||
mod = sys.modules.get(name)
|
||||
if mod is not None:
|
||||
return mod
|
||||
return importlib.import_module(name)
|
||||
@@ -0,0 +1,94 @@
|
||||
"""Tests that src/app_controller.py has NO top-level fastapi imports.
|
||||
|
||||
Per spec.md:2.2 Layer 1, the main thread's import chain must not include
|
||||
heavy modules. FastAPI is heavy (~470ms) and is only needed when
|
||||
`--enable-test-hooks` or `--web-host` is passed. The warmup loads it
|
||||
on the AppController's _io_pool; functions that need it call
|
||||
`_require_warmed("fastapi")` to get the module from sys.modules.
|
||||
|
||||
These tests run in a fresh subprocess to ensure no warmup state leaks
|
||||
from the test runner. We assert:
|
||||
- `fastapi` is NOT in `sys.modules` after `import src.app_controller`
|
||||
- `fastapi.security.api_key` is NOT in `sys.modules` either
|
||||
- The static audit script reports NO new violation from app_controller.py
|
||||
"""
|
||||
|
||||
import subprocess
|
||||
import sys
|
||||
import textwrap
|
||||
from pathlib import Path
|
||||
|
||||
ROOT = Path(__file__).resolve().parent.parent
|
||||
|
||||
|
||||
def _run_in_subprocess(snippet: str) -> subprocess.CompletedProcess:
|
||||
script = textwrap.dedent(snippet)
|
||||
return subprocess.run(
|
||||
[sys.executable, "-c", script],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
cwd=str(ROOT),
|
||||
timeout=30,
|
||||
)
|
||||
|
||||
|
||||
def test_app_controller_does_not_import_fastapi_at_module_level() -> None:
|
||||
res = _run_in_subprocess("""
|
||||
import sys
|
||||
import src.app_controller
|
||||
print('fastapi' in sys.modules)
|
||||
""")
|
||||
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||
assert res.stdout.strip() == "False", f"app_controller triggered fastapi import: {res.stdout}"
|
||||
|
||||
|
||||
def test_app_controller_does_not_import_fastapi_security_at_module_level() -> None:
|
||||
res = _run_in_subprocess("""
|
||||
import sys
|
||||
import src.app_controller
|
||||
print('fastapi.security.api_key' in sys.modules)
|
||||
""")
|
||||
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||
assert res.stdout.strip() == "False", f"app_controller triggered fastapi.security.api_key import: {res.stdout}"
|
||||
|
||||
|
||||
def test_app_controller_create_api_still_resolvable() -> None:
|
||||
"""Even without fastapi in sys.modules, the function reference must be
|
||||
importable (it just returns a lazy FastAPI app on call)."""
|
||||
res = _run_in_subprocess("""
|
||||
import src.app_controller
|
||||
print(hasattr(src.app_controller.AppController, 'create_api'))
|
||||
print(callable(src.app_controller.AppController.create_api))
|
||||
""")
|
||||
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||
assert res.stdout.strip() == "True\nTrue"
|
||||
|
||||
|
||||
def test_audit_main_thread_imports_sees_no_new_violation_from_app_controller() -> None:
|
||||
"""Run the static audit and check that src/app_controller.py contributes no
|
||||
new fastapi violations (any remaining violations are pre-existing in OTHER
|
||||
files; this just verifies app_controller is clean).
|
||||
"""
|
||||
res = _run_in_subprocess("""
|
||||
import ast
|
||||
from pathlib import Path
|
||||
root = Path('.').resolve()
|
||||
app_controller_path = root / 'src' / 'app_controller.py'
|
||||
tree = ast.parse(app_controller_path.read_text(encoding='utf-8'))
|
||||
heavy = ['fastapi', 'fastapi.security', 'fastapi.security.api_key']
|
||||
for node in tree.body:
|
||||
if isinstance(node, ast.Import):
|
||||
for alias in node.names:
|
||||
for h in heavy:
|
||||
if alias.name == h or alias.name.startswith(h + '.'):
|
||||
print('VIOLATION:', alias.name)
|
||||
elif isinstance(node, ast.ImportFrom):
|
||||
if node.module:
|
||||
for h in heavy:
|
||||
if node.module == h or node.module.startswith(h + '.'):
|
||||
print('VIOLATION:', node.module)
|
||||
print('OK')
|
||||
""")
|
||||
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||
assert "OK" in res.stdout
|
||||
assert "VIOLATION" not in res.stdout
|
||||
Reference in New Issue
Block a user