Skip to content

Commit 8ad0a21

Browse files
rustyconoverclaude
andcommitted
perf: cache per-class argument specs; make worker logging and profiling reachable
extract_argument_specs derives a function class's arguments from its annotations, its FunctionArguments, and the Arg descriptors on its MRO — none of which change once the class exists. It was nonetheless re-derived on every catalog listing and every overload match. Profiling one run of the upstream integration suite through the Cedar proxy: 58,058 calls costing 15.8s, driving 131,653 typing.get_type_hints calls, 2.4M _eval_type calls and 648k compile() calls — about a quarter of the worker's CPU. catalog_schema_contents_functions cost 38ms per call where every other catalog method was 0.1-2ms, entirely because of this. After: 16.7ms per call, and 179x on the function itself in isolation. Memoized with functools.cache, which is thread safe (workers serve on a pool) and unbounded — the keys are function classes the catalog already holds for the process's lifetime, so it retains nothing new. The cached value is a tuple and the public function returns a fresh list, so a caller that mutates the result cannot corrupt the cache; ArgumentSpec is a frozen dataclass, so sharing the specs themselves is safe. Two supporting changes, both about reaching diagnostics from outside: VGI_WORKER_LOG_LEVEL / _FORMAT / _LOGGERS, applied inside configure_worker_logging rather than in one CLI. The vgi_rpc.access log already records duration_ms, method and row/byte counts for every RPC, but turning it on required --log-logger and --log-format on the command line, which anything launching a worker without owning its argv cannot do. Placed in the shared function because vgi-serve and the fixture servers had byte-identical logging blocks and wiring it into one would be a trap. vgi/profiling.py: VGI_WORKER_PROFILE=<dir> writes pstats on exit. It uses a single profiler deliberately — under --http the worker runs on waitress, so a main-thread-only profile would show time in select() and nothing else, and since Python 3.12 cProfile is sys.monitoring-based and process-global, so one profiler covers the pool threads. A profiler per thread raises "Another profiling tool is already active" as each waitress thread starts, which stops the server accepting connections at all. SIGTERM is handled because its default action skips interpreter shutdown, losing the profile on every normal stop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6110504 commit 8ad0a21

5 files changed

Lines changed: 222 additions & 0 deletions

File tree

‎vgi/_test_fixtures/http_server.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
from vgi._test_fixtures.worker import ExampleWorker
2828
from vgi.logging_config import LogFormat, LogLevel, configure_worker_logging
2929
from vgi.meta_worker import MetaWorker
30+
from vgi.profiling import maybe_start_profile
3031

3132
if TYPE_CHECKING:
3233
from vgi_rpc.external import UploadUrl
@@ -209,12 +210,15 @@ def _run(
209210

210211
env_debug = os.environ.get("VGI_WORKER_DEBUG", "").lower() in ("1", "true", "yes")
211212
effective_debug = debug or env_debug
213+
# VGI_WORKER_LOG_* overrides are applied inside configure_worker_logging,
214+
# so this server and `vgi-serve` are configurable the same way.
212215
effective_level = configure_worker_logging(
213216
debug=effective_debug,
214217
log_level=log_level,
215218
log_loggers=log_logger,
216219
log_format=log_format,
217220
)
221+
maybe_start_profile()
218222

219223
bucket = s3_bucket or os.environ.get("VGI_HTTP_S3_BUCKET")
220224
external_location = None

‎vgi/argument_spec.py‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import warnings
1919
from collections.abc import Sequence
2020
from dataclasses import dataclass
21+
from functools import cache
2122
from typing import Annotated, Any, get_args, get_origin, get_type_hints
2223

2324
import pyarrow as pa
@@ -519,6 +520,10 @@ def extract_argument_specs(
519520
2. Type annotation with `PYTHON_TO_ARROW` mapping
520521
3. Default to pa.null() with warning for unknown types
521522
523+
Memoized per class — see :func:`_extract_argument_specs_cached`. A fresh
524+
list is returned each call so a caller that mutates it cannot corrupt the
525+
cache; the `ArgumentSpec`s inside are frozen dataclasses and safe to share.
526+
522527
Args:
523528
cls: Function class with `Arg` descriptors.
524529
@@ -527,6 +532,52 @@ def extract_argument_specs(
527532
then named).
528533
529534
"""
535+
return list(_extract_argument_specs_cached(cls))
536+
537+
538+
@cache
539+
def _extract_argument_specs_cached(cls: type) -> tuple[ArgumentSpec, ...]:
540+
"""Memoized :func:`_extract_argument_specs`, keyed on the class.
541+
542+
A function class's argument specs are fixed once the class exists — they are
543+
derived from its annotations, its ``FunctionArguments``, and the `Arg`
544+
descriptors on its MRO, none of which change at runtime. Deriving them was
545+
nonetheless repeated on **every** catalog listing and every overload match:
546+
profiling one run of the upstream integration suite through the proxy showed
547+
58,058 calls costing 15.8s, which drove 131,653 ``typing.get_type_hints``
548+
calls, 2.4M ``_eval_type`` calls and 648k ``compile`` calls — roughly a
549+
quarter of the worker's CPU. ``catalog_schema_contents_functions`` was 38ms
550+
per call against 0.1-2ms for every other catalog method, entirely because of
551+
this.
552+
553+
Unbounded (``functools.cache``) is the right size: the keys are function
554+
classes, which the catalog already holds for the process's lifetime, so the
555+
cache adds no retention that did not already exist. It is also thread safe,
556+
which matters because workers serve requests on a pool.
557+
558+
Returns a tuple so the cached value cannot be mutated through a caller's
559+
reference.
560+
561+
Args:
562+
cls: Function class with `Arg` descriptors.
563+
564+
Returns:
565+
The class's argument specs, sorted.
566+
"""
567+
return tuple(_extract_argument_specs(cls))
568+
569+
570+
def _extract_argument_specs(
571+
cls: type,
572+
) -> list[ArgumentSpec]:
573+
"""Derive a class's argument specs, without caching.
574+
575+
Args:
576+
cls: Function class with `Arg` descriptors.
577+
578+
Returns:
579+
List of `ArgumentSpec` objects, sorted by position.
580+
"""
530581
specs: list[ArgumentSpec] = []
531582
seen_names: set[str] = set()
532583

‎vgi/logging_config.py‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
from __future__ import annotations
1212

1313
import logging
14+
import os
1415
import sys
1516
from enum import StrEnum
1617

@@ -46,6 +47,68 @@ class LogFormat(StrEnum):
4647
]
4748

4849

50+
def _env_overrides(
51+
log_level: LogLevel,
52+
log_loggers: list[str] | None,
53+
log_format: LogFormat,
54+
) -> tuple[LogLevel, list[str] | None, LogFormat]:
55+
"""Apply ``VGI_WORKER_LOG_*`` environment overrides to the CLI options.
56+
57+
The access log (``vgi_rpc.access``) already records ``duration_ms``,
58+
``method`` and row/byte counts for every RPC, but reaching it required
59+
passing ``--log-logger vgi_rpc.access --log-format json`` on the command
60+
line. Anything that launches a worker without owning its argv — a container
61+
image, a test harness spawning workers from a dozen call sites — could not
62+
turn it on. These variables make the same configuration reachable the way
63+
``VGI_SIGNING_KEY`` and ``VGI_ENABLE_DESCRIBE`` already are.
64+
65+
Applied here rather than in a single CLI so that *every* worker entry point
66+
honours them identically — ``vgi-serve`` and the fixture servers had
67+
byte-identical logging blocks, and an override wired into only one of them
68+
would be a trap.
69+
70+
Each variable wins when set, matching ``VGI_ENABLE_DESCRIBE``: Typer cannot
71+
distinguish "user passed the default" from "user passed nothing", so
72+
deferring to an explicit environment setting is the only unambiguous rule.
73+
74+
Args:
75+
log_level: The ``--log-level`` value.
76+
log_loggers: The ``--log-logger`` values, if any.
77+
log_format: The ``--log-format`` value.
78+
79+
Returns:
80+
The effective ``(level, loggers, format)``.
81+
82+
Raises:
83+
SystemExit: If a variable names a level or format that does not exist.
84+
A typo would otherwise silently leave logging as it was, which is
85+
indistinguishable from the feature not working.
86+
"""
87+
raw_level = os.environ.get("VGI_WORKER_LOG_LEVEL")
88+
if raw_level:
89+
try:
90+
log_level = LogLevel(raw_level.upper())
91+
except ValueError:
92+
valid = ", ".join(level.value for level in LogLevel)
93+
sys.exit(f"VGI_WORKER_LOG_LEVEL={raw_level!r} is not a log level (expected one of: {valid})")
94+
95+
raw_format = os.environ.get("VGI_WORKER_LOG_FORMAT")
96+
if raw_format:
97+
try:
98+
log_format = LogFormat(raw_format.lower())
99+
except ValueError:
100+
valid = ", ".join(fmt.value for fmt in LogFormat)
101+
sys.exit(f"VGI_WORKER_LOG_FORMAT={raw_format!r} is not a log format (expected one of: {valid})")
102+
103+
raw_loggers = os.environ.get("VGI_WORKER_LOG_LOGGERS")
104+
if raw_loggers:
105+
names = [name.strip() for name in raw_loggers.split(",") if name.strip()]
106+
if names:
107+
log_loggers = names
108+
109+
return log_level, log_loggers, log_format
110+
111+
49112
def configure_worker_logging(
50113
*,
51114
debug: bool = False,
@@ -55,6 +118,10 @@ def configure_worker_logging(
55118
) -> int:
56119
"""Configure stdlib logging for a VGI worker process.
57120
121+
``VGI_WORKER_LOG_LEVEL``, ``VGI_WORKER_LOG_FORMAT`` and
122+
``VGI_WORKER_LOG_LOGGERS`` override the corresponding arguments when set —
123+
see :func:`_env_overrides`.
124+
58125
Args:
59126
debug: If True, force DEBUG on all default loggers (overrides *log_level*).
60127
log_level: Logging level when *debug* is False.
@@ -65,6 +132,7 @@ def configure_worker_logging(
65132
The effective numeric log level.
66133
67134
"""
135+
log_level, log_loggers, log_format = _env_overrides(log_level, log_loggers, log_format)
68136
effective_level = logging.DEBUG if debug else getattr(logging, log_level.value)
69137

70138
handler = logging.StreamHandler(sys.stderr)

‎vgi/profiling.py‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
# Copyright 2025, 2026 Query Farm LLC - https://query.farm
2+
3+
"""Opt-in CPU profiling for VGI worker processes.
4+
5+
A worker is normally a subprocess someone else launched — a container, a test
6+
harness, a DuckDB client spawning it over stdio — so the usual ways of profiling
7+
Python (``python -m cProfile``, an interactive session) are not available to
8+
whoever needs the answer. Setting one environment variable is.
9+
10+
Shared by every worker entry point rather than living in one CLI, so that
11+
``vgi-serve`` and the fixture servers profile identically.
12+
"""
13+
14+
from __future__ import annotations
15+
16+
import os
17+
import pathlib
18+
import sys
19+
20+
__all__ = ["maybe_start_profile"]
21+
22+
23+
def maybe_start_profile() -> None:
24+
"""Start a ``cProfile`` profiler when ``VGI_WORKER_PROFILE`` names a path.
25+
26+
Set ``VGI_WORKER_PROFILE`` to a directory (or a file path) and the worker
27+
writes ``pstats`` data on exit, readable with :mod:`pstats`, snakeviz, or
28+
``python -m pstats``. A directory gets one file per process
29+
(``worker-<pid>.prof``), which is what you want when the thing under
30+
investigation spawns more than one worker.
31+
32+
**All threads are covered by the single profiler.** That matters because
33+
under ``--http`` a worker runs on waitress, which serves every request on a
34+
pool thread — a main-thread-only profile would faithfully report time spent
35+
in ``select()`` and nothing about the work being measured. Since Python 3.12
36+
``cProfile`` is built on :mod:`sys.monitoring`, whose tool registration is
37+
process-global rather than per-thread, so one profiler observes the pool
38+
threads too.
39+
40+
The corollary is that there can be only **one** active profiler in the
41+
process: ``sys.monitoring`` allows a given tool id to be claimed once, so a
42+
profiler-per-thread raises ``ValueError: Another profiling tool is already
43+
active`` in every waitress thread — and because that happens as the thread
44+
starts, the server never accepts a connection at all.
45+
46+
``atexit`` alone would not be enough: a worker is normally stopped with
47+
``SIGTERM``, whose default action terminates without unwinding, so the
48+
profile would be lost on every normal shutdown. A handler turns it into an
49+
ordinary exit — but only if nothing else has claimed ``SIGTERM``, since
50+
stealing another component's shutdown path would be a worse bug than a
51+
missing profile.
52+
"""
53+
dest = os.environ.get("VGI_WORKER_PROFILE")
54+
if not dest:
55+
return
56+
57+
import atexit
58+
import cProfile
59+
import signal
60+
61+
path = pathlib.Path(dest)
62+
if path.is_dir() or dest.endswith(os.sep):
63+
path.mkdir(parents=True, exist_ok=True)
64+
path = path / f"worker-{os.getpid()}.prof"
65+
else:
66+
path.parent.mkdir(parents=True, exist_ok=True)
67+
68+
profiler = cProfile.Profile()
69+
profiler.enable()
70+
dumped = False
71+
72+
def _dump() -> None:
73+
# atexit and the SIGTERM handler can both reach this; dumping twice
74+
# would truncate the file the first call just wrote.
75+
nonlocal dumped
76+
if dumped:
77+
return
78+
dumped = True
79+
try:
80+
profiler.disable()
81+
profiler.dump_stats(str(path))
82+
except Exception as exc: # noqa: BLE001 — never fail a shutdown over telemetry
83+
sys.stderr.write(f"vgi: could not write profile: {exc}\n")
84+
return
85+
sys.stderr.write(f"vgi: wrote profile to {path}\n")
86+
87+
atexit.register(_dump)
88+
89+
def _on_sigterm(signum: int, frame: object) -> None:
90+
raise SystemExit(128 + signum)
91+
92+
if signal.getsignal(signal.SIGTERM) in (signal.SIG_DFL, None):
93+
signal.signal(signal.SIGTERM, _on_sigterm)

‎vgi/serve.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
from typing import TYPE_CHECKING, Any, Literal
3636

3737
from vgi.logging_config import LogFormat, LogLevel
38+
from vgi.profiling import maybe_start_profile
3839

3940
if TYPE_CHECKING:
4041
import falcon
@@ -364,12 +365,17 @@ def serve(
364365
) -> None:
365366
env_debug = os.environ.get("VGI_WORKER_DEBUG", "").lower() in ("1", "true", "yes")
366367
effective_debug = debug or env_debug
368+
# VGI_WORKER_LOG_* overrides are applied inside configure_worker_logging,
369+
# so every worker entry point honours them identically.
367370
effective_level = configure_worker_logging(
368371
debug=effective_debug,
369372
log_level=log_level,
370373
log_loggers=log_logger,
371374
log_format=log_format,
372375
)
376+
# Before the worker class is imported, so module-import cost lands in
377+
# the profile too — for many workers that is the bulk of startup.
378+
maybe_start_profile()
373379

374380
# Resolve env var overrides
375381
describe = _resolve_describe(describe)

0 commit comments

Comments
 (0)