refactor(models): remove top-level pydantic import; lazy pydantic via PEP 562 __getattr__
Sub-track 2A of startup_speedup_20260606: clears 1 of 61 main-thread audit violations (pydantic in src/models.py).
Removed top-level 'from pydantic import BaseModel' (line 50) and the two static class definitions (GenerateRequest, ConfirmRequest). Replaced with PEP 562 module-level __getattr__ that materializes the pydantic classes on first access via pydantic.create_model() + _require_warmed('pydantic').
Pattern matches the lazy-proxy convention from sub-tracks 5A (command_palette), 5B (theme_nerv), 5C (markdown_table), 5D (gui_2 dead imports).
Result:
- pydantic NOT in sys.modules after 'import src.models' (verified via subprocess test)
- GenerateRequest and ConfirmRequest are accessible via 'from src.models import X' (proxy triggers pydantic import + caches class in globals())
- Pydantic validation works: GenerateRequest() raises ValidationError on missing 'prompt'
- Audit script: 60 violations (was 61)
- Existing test_project_switch_persona_preset.py: 8/9 pass; the 1 failure is the pre-existing ui_global_preset_name issue (unrelated)
Files changed:
- src/models.py: removed 1 import, 2 class defs; added 2 factory fns + 1 __getattr__
- tests/test_models_no_top_level_pydantic.py: new (7 tests; all pass)
Per user instruction, all implementation work is performed by the Tier 2 tech lead directly. The 'sub-track 2A' naming follows the sub-track 2 (audit violations) parent in the track plan.
This commit is contained in:
+31
-10
@@ -47,7 +47,6 @@ import tomllib
|
|||||||
from dataclasses import dataclass, field
|
from dataclasses import dataclass, field
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any, Dict, List, Optional, Union
|
from typing import Any, Dict, List, Optional, Union
|
||||||
from pydantic import BaseModel
|
|
||||||
|
|
||||||
from src.paths import get_config_path
|
from src.paths import get_config_path
|
||||||
|
|
||||||
@@ -209,16 +208,38 @@ def parse_history_entries(history_strings: list[str], roles: list[str]) -> list[
|
|||||||
|
|
||||||
#region: Pydantic Models
|
#region: Pydantic Models
|
||||||
|
|
||||||
class GenerateRequest(BaseModel):
|
def _create_generate_request() -> type:
|
||||||
prompt: str
|
from src.module_loader import _require_warmed
|
||||||
auto_add_history: bool = True
|
pydantic = _require_warmed("pydantic")
|
||||||
temperature: float | None = None
|
return pydantic.create_model(
|
||||||
top_p: float | None = None
|
"GenerateRequest",
|
||||||
max_tokens: int | None = None
|
prompt=(str, ...),
|
||||||
|
auto_add_history=(bool, True),
|
||||||
|
temperature=(float | None, None),
|
||||||
|
top_p=(float | None, None),
|
||||||
|
max_tokens=(int | None, None),
|
||||||
|
)
|
||||||
|
|
||||||
class ConfirmRequest(BaseModel):
|
def _create_confirm_request() -> type:
|
||||||
approved: bool
|
from src.module_loader import _require_warmed
|
||||||
script: Optional[str] = None
|
pydantic = _require_warmed("pydantic")
|
||||||
|
return pydantic.create_model(
|
||||||
|
"ConfirmRequest",
|
||||||
|
approved=(bool, ...),
|
||||||
|
script=(Optional[str], None),
|
||||||
|
)
|
||||||
|
|
||||||
|
_PYDANTIC_CLASS_FACTORIES: dict[str, callable] = {
|
||||||
|
"GenerateRequest": _create_generate_request,
|
||||||
|
"ConfirmRequest": _create_confirm_request,
|
||||||
|
}
|
||||||
|
|
||||||
|
def __getattr__(name: str) -> Any:
|
||||||
|
if name in _PYDANTIC_CLASS_FACTORIES:
|
||||||
|
cls = _PYDANTIC_CLASS_FACTORIES[name]()
|
||||||
|
globals()[name] = cls
|
||||||
|
return cls
|
||||||
|
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
|
||||||
|
|
||||||
#region: MMA Core
|
#region: MMA Core
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,130 @@
|
|||||||
|
"""Tests that src/models.py has NO top-level pydantic import.
|
||||||
|
|
||||||
|
Per the Main Thread Purity Invariant (spec.md:2.1), pydantic is heavy and
|
||||||
|
must not appear in the main-thread import chain. The two pydantic classes
|
||||||
|
(GenerateRequest, ConfirmRequest) are exposed via a module-level
|
||||||
|
__getattr__ proxy (PEP 562) that materializes them on first access.
|
||||||
|
|
||||||
|
These tests run in a fresh subprocess to ensure no warmup state leaks
|
||||||
|
from the test runner. We assert:
|
||||||
|
- pydantic is NOT imported as a side effect of `import src.models`
|
||||||
|
- GenerateRequest and ConfirmRequest can be imported and instantiated
|
||||||
|
- Accessing the proxy triggers the pydantic import
|
||||||
|
- The static audit no longer flags src/models.py for pydantic
|
||||||
|
"""
|
||||||
|
|
||||||
|
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_models_does_not_import_pydantic_at_module_level() -> None:
|
||||||
|
res = _run_in_subprocess("""
|
||||||
|
import sys
|
||||||
|
import src.models
|
||||||
|
print('pydantic' in sys.modules)
|
||||||
|
""")
|
||||||
|
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||||
|
assert res.stdout.strip() == "False", (
|
||||||
|
f"src.models triggered pydantic import: {res.stdout}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_generate_request_works_when_explicitly_imported() -> None:
|
||||||
|
res = _run_in_subprocess("""
|
||||||
|
from src.models import GenerateRequest
|
||||||
|
req = GenerateRequest(prompt="hello")
|
||||||
|
print(req.prompt)
|
||||||
|
print(req.auto_add_history)
|
||||||
|
""")
|
||||||
|
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||||
|
assert res.stdout.strip() == "hello\nTrue"
|
||||||
|
|
||||||
|
|
||||||
|
def test_confirm_request_works_when_explicitly_imported() -> None:
|
||||||
|
res = _run_in_subprocess("""
|
||||||
|
from src.models import ConfirmRequest
|
||||||
|
req = ConfirmRequest(approved=True, script="echo hi")
|
||||||
|
print(req.approved)
|
||||||
|
print(req.script)
|
||||||
|
""")
|
||||||
|
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||||
|
assert res.stdout.strip() == "True\necho hi"
|
||||||
|
|
||||||
|
|
||||||
|
def test_pydantic_only_loaded_after_explicit_class_access() -> None:
|
||||||
|
res = _run_in_subprocess("""
|
||||||
|
import sys
|
||||||
|
import src.models
|
||||||
|
assert 'pydantic' not in sys.modules, 'pydantic leaked into sys.modules at import time'
|
||||||
|
from src.models import GenerateRequest
|
||||||
|
print('pydantic' in sys.modules)
|
||||||
|
print(GenerateRequest.__bases__[0].__name__)
|
||||||
|
print(GenerateRequest.__bases__[0].__module__)
|
||||||
|
""")
|
||||||
|
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||||
|
lines = res.stdout.strip().splitlines()
|
||||||
|
assert lines[0] == "True", f"GenerateRequest access did not trigger pydantic import: {res.stdout}"
|
||||||
|
assert lines[1] == "BaseModel", f"GenerateRequest base is not BaseModel: {res.stdout}"
|
||||||
|
assert lines[2].startswith("pydantic"), f"GenerateRequest base is not from pydantic package: {res.stdout}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_proxy_caches_real_class_for_repeated_access() -> None:
|
||||||
|
res = _run_in_subprocess("""
|
||||||
|
from src.models import GenerateRequest
|
||||||
|
cls1 = GenerateRequest
|
||||||
|
cls2 = GenerateRequest
|
||||||
|
print(cls1 is cls2)
|
||||||
|
""")
|
||||||
|
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||||
|
assert res.stdout.strip() == "True", f"proxy did not cache the real class: {res.stdout}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_generate_request_validation_rejects_missing_prompt() -> None:
|
||||||
|
res = _run_in_subprocess("""
|
||||||
|
from src.models import GenerateRequest
|
||||||
|
try:
|
||||||
|
GenerateRequest()
|
||||||
|
print("NO_RAISE")
|
||||||
|
except Exception as e:
|
||||||
|
print(type(e).__name__)
|
||||||
|
""")
|
||||||
|
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||||
|
assert res.stdout.strip() != "NO_RAISE", "pydantic validation did not fire on missing required field"
|
||||||
|
assert "ValidationError" in res.stdout, f"expected ValidationError, got: {res.stdout}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_audit_sees_no_pydantic_violation_in_models() -> None:
|
||||||
|
res = _run_in_subprocess("""
|
||||||
|
import ast
|
||||||
|
from pathlib import Path
|
||||||
|
root = Path('.').resolve()
|
||||||
|
models_path = root / 'src' / 'models.py'
|
||||||
|
tree = ast.parse(models_path.read_text(encoding='utf-8'))
|
||||||
|
for node in tree.body:
|
||||||
|
if isinstance(node, ast.Import):
|
||||||
|
for alias in node.names:
|
||||||
|
if alias.name == 'pydantic' or alias.name.startswith('pydantic.'):
|
||||||
|
print('VIOLATION:', alias.name)
|
||||||
|
elif isinstance(node, ast.ImportFrom):
|
||||||
|
if node.module and (node.module == 'pydantic' or node.module.startswith('pydantic.')):
|
||||||
|
print('VIOLATION:', node.module)
|
||||||
|
print('OK')
|
||||||
|
""")
|
||||||
|
assert res.returncode == 0, f"stderr: {res.stderr}"
|
||||||
|
assert "OK" in res.stdout, f"audit script errored: {res.stderr}"
|
||||||
|
assert "VIOLATION" not in res.stdout, f"src/models.py still has top-level pydantic: {res.stdout}"
|
||||||
Reference in New Issue
Block a user