autoresearch: generate and gate non-catalog vLLM optimization candidates#56
autoresearch: generate and gate non-catalog vLLM optimization candidates#56nicholaslawrence-hub wants to merge 25 commits into
Conversation
Code Review by Gemini--- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -19,6 +19,7 @@
FallbackProposer,
TableProposer,
autoresearch,
classify_bottleneck,
)
from gitm.agents.policy import Policy, select_interventions
from gitm.kernels.library import load_library
+from gitm.kernels.spec import knob_kind
from gitm.optimizer.apply import (
Applicator,
DryRunApplicator,
|
🤖 Claude Code ReviewCode Review:
|
nicholaslawrence-hub
force-pushed
the
autoresearch-clean
branch
from
e49d4f0 to
5c1e1ab
Compare
Code Review by GeminiThe changes introduce a new autoresearch agent, integrate it into the scheduler loop, and add comprehensive documentation and tests. The design for agentic search, bottleneck classification, and knob generation is well-thought-out, with clear separation of concerns and fallback mechanisms. One bug was identified in the integration with the scheduler loop. BugFile: Issue: The Proposed change: --- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -11,6 +11,7 @@ from __future__ import annotations
import json
import os
import re
+import time
import uuid
from dataclasses import asdict, dataclass
from pathlib import Path
@@ -24,7 +25,7 @@
LiveEngineApplicator,
apply_intervention,
)
from gitm.optimizer.attribution import attribute
from gitm.optimizer.deviation import deviation_summary, deviation_trace, write_deviation_jsonl
from gitm.optimizer.monitor import (
AttributionContext,
@@ -32,7 +33,7 @@
Residuals,
TraceSummary,
build_attribution_context,
)
from gitm.optimizer.preconditions import GateContext, Preconditions
from gitm.optimizer.replay import predict_delta
from gitm.optimizer.report import Claim, Provenance, build_report
@@ -40,6 +41,7 @@
from gitm.tracer.schema import Trace
from gitm.tracer.trace import capture_trace
from gitm.workloads.vllm import VLLMConfig, VLLMRunner
+from gitm.kernels.spec import knob_kind
#: The loop's configuration.
@@ -507,6 +509,7 @@
# Phase 4b - agentic autoresearch through the catalog gate/rollback path.
if time.time_ns() - started_ns < int(budget_s * 1e9):
proposer = FallbackProposer(EngineArgsProposer(), TableProposer())
+
def _unenactable(spec: Any) -> str | None:
if (
cfg.engine is not None
|
🤖 Claude Code ReviewReview🐛 Bugs
base = d if isinstance(d, int | float) and d not in (0, False) else 1
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))
applicable=applied is not None,When a spec is rejected by the gate, if not r.applicable:
rejected.append(...)
continue
if r.rolled_back:
rolled_back.append(...)This means a spec that was applied but rolled back is added to
max_attempts = max(self._n * 4, len(eligible) * 4)If
kind, choices = _field_kind_and_choices(f.type)With ⚡ Performance
📊 Reproducibility
💡 Suggestions
and knob_kind(spec.knob) == "structural"
return IDLE_STALL if sc_score >= mem_score else MEMORY_BOUNDExact tie ( No test for
|
🤖 Claude Code ReviewReview🐛 Bugs
is_list = nargs in ("+", "*") or (isinstance(nargs, int) and nargs > 1)
kind, choices = _knob_domain(f.type, domain)With PEP 563 deferred evaluation, ⚡ Performance
📊 Reproducibility
💡 Suggestions
Test file imports private symbols ( |
Code Review by GeminiThe changes introduce a new Here are some minor improvements and observations:
|
nicholaslawrence-hub
force-pushed
the
autoresearch-clean
branch
from
29b51bd to
01cd4a2
Compare
Code Review by GeminiThe changes introduce an "autoresearch" agent that proposes non-catalog vLLM optimization candidates. This agent classifies bottlenecks, targets the largest residual operations, and generates intervention proposals from the vLLM Overall, the design is robust, with clear separation of concerns, extensibility through Issues and SuggestionsBugFile: The --- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -19,6 +19,7 @@ from gitm.agents.autoresearch import (
autoresearch,
classify_bottleneck,
)
+from gitm.optimizer.apply import knob_kind
from gitm.agents.policy import Policy, select_interventions
from gitm.kernels.library import load_library
from gitm.optimizer.apply import (
Suggested change: Add the following import statement to from gitm.optimizer.apply import knob_kind |
🤖 Claude Code ReviewReview🐛 Bugs
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))If
kind, choices = _knob_domain(f.type, domain)In Python ≥ 3.10 with
applicable=applied is not None,
Off-by-one in max_attempts = max(self._n * 4, len(eligible) * 4)If
if not any(weights):
if time.time_ns() - started_ns < int(budget_s * 1e9):
...
ar_run = autoresearch(...)
else:
ar_run = AutoresearchRun(bottleneck_class=classify_bottleneck(trace), results=[])
🔒 Security
def _unenactable(spec: Any) -> str | None:
if (
cfg.engine is not None
and live_restart_fn is None
and knob_kind(spec.knob) == "structural"
):
⚡ Performance
def _searchable(self) -> list[Knob]:
return [k for k in self._source.knobs() if k.name not in self._catalog and _value_grid(k)]
📊 Reproducibility
💡 SuggestionsThe (run_dir / "autoresearch.json").write_text(json.dumps(...))
base = d if isinstance(d, int | float) and d not in (0, False) else 1For a knob like
assert mem == {"cpu_offload_gb", "block_size"} # both memory-affine by nameThis test asserts that No timeout / budget enforcement inside |
nicholaslawrence-hub
force-pushed
the
autoresearch-clean
branch
from
01cd4a2 to
1934e94
Compare
Code Review by Gemini### gitm/agents/autoresearch.py
No issues found.
### gitm/scheduler/loop.py
```diff
--- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -19,6 +19,7 @@
DryRunApplicator,
LiveEngineApplicator,
apply_intervention,
+ knob_kind,
)
from gitm.optimizer.attribution import attribute
from gitm.optimizer.deviation import deviation_summary, deviation_trace, write_deviation_jsonl
Issue: The gitm/tracer/vllm_stats.py--- a/gitm/tracer/vllm_stats.py
+++ b/gitm/tracer/vllm_stats.py
@@ -83,66 +83,27 @@ def _first_attr(obj: Any, *paths: str) -> Any:
def _len_or_none(x: Any) -> int | None:
try:
return len(x)
except TypeError:
return None
def _schedulers(engine: Any) -> list[Any]:
"""Resolve the engine's scheduler(s) as a list (vLLM may keep one per PP stage)."""
sched = _first_attr(
- engine,
- # vLLM V0.
- "scheduler",
- "engine.scheduler",
- "llm_engine.scheduler",
- # vLLM V1 (in-process, VLLM_ENABLE_V1_MULTIPROCESSING=0): the scheduler
- # lives inside the EngineCore, not on the LLMEngine. Paths vary across
- # v0.2x point releases — GPU-validate the exact one on the target build.
- "llm_engine.engine_core.engine_core.scheduler",
- "llm_engine.engine_core.scheduler",
- "engine_core.engine_core.scheduler",
- "engine_core.scheduler",
+ engine, "scheduler", "engine.scheduler", "llm_engine.scheduler"
)
if sched is None:
return []
return list(sched) if isinstance(sched, list | tuple) else [sched]
-def _v1_scheduler_stats(scheduler: Any) -> dict[str, Any]:
- """vLLM V1 stats via ``scheduler.make_stats()`` — V1 doesn't keep the V0
- running/waiting deques in the same shape, but exposes a stats object with
- ``num_running_reqs`` / ``num_waiting_reqs`` / ``kv_cache_usage``. Best-effort;
- returns only the fields it actually found. GPU-validate the attr names on the
- target vLLM build (they've shifted across v0.2x).
- """
- make = getattr(scheduler, "make_stats", None)
- if not callable(make):
- return {}
- try:
- stats = make()
- except Exception:
- return {}
- if stats is None:
- return {}
- out: dict[str, Any] = {}
- for field_name, attr in (("num_running", "num_running_reqs"),
- ("num_waiting", "num_waiting_reqs")):
- v = getattr(stats, attr, None)
- if isinstance(v, int):
- out[field_name] = v
- ku = getattr(stats, "kv_cache_usage", None)
- if isinstance(ku, int | float):
- out["gpu_cache_usage"] = float(ku)
- return out
-
-
def _max_num_seqs(engine: Any) -> int | None:
val = _first_attr(
engine,
"scheduler_config.max_num_seqs",
"engine.scheduler_config.max_num_seqs",
"llm_engine.scheduler_config.max_num_seqs",
"vllm_config.scheduler_config.max_num_seqs",
)
return int(val) if isinstance(val, int) and val > 0 else None
@@ -174,16 +127,6 @@ def read_scheduler_stats(engine: Any, *, t_ns: int = 0) -> SchedulerSample | Non
if usage is not None:
sample.gpu_cache_usage = usage
saw_any = True
- # vLLM V1: fill running / waiting / cache from the scheduler's stat object
- # where the VO deques weren't exposed (they read empty on V1)
- for sch in schedulers:
- for field_name, val in _v1_scheduler_stats(sch).items():
- if getattr(sample, field_name) is None:
- setattr(sample, field_name, val)
- saw_any = True
-
# Total unfinished — a stable public method on LLMEngine across versions.
getter = _first_attr(
engine,
"get_num_unfinished_requests",
"engine.get_num_unfinished_requests",
"llm_engine.get_num_unfinished_requests",
)Issue: The removal of gitm/workloads.pyNo issues found. tests/test_autoresearch.pyNo issues found. tests/test_vllm_stats_v1.py--- a/tests/test_vllm_stats_v1.py
+++ /dev/null
@@ -1,50 +0,0 @@
-"""Read scheduler stats from a vLLM V1-shaped engine.
-
-V1 keeps the scheduler inside EngineCore and exposes a stats object via
-make_stats() (num_running_reqs / num_waiting_reqs / kv_cache_usage) instead of
-the V0 running/waiting deques. Fake that shape to pin the read path off-GPU; the
-exact attr names still need GPU validation on the target vLLM build.
-"""
-
-from __future__ import annotations
-
-from types import SimpleNamespace
-
-from gitm.tracer.vllm_stats import read_scheduler_stats
-
-
-def _v1_engine(running: int, waiting: int, cache: float, max_seqs: int = 256):
- stats = SimpleNamespace(
- num_running_reqs=running, num_waiting_reqs=waiting, kv_cache_usage=cache
- )
- scheduler = SimpleNamespace(make_stats=lambda: stats)
- # llm.llm_engine.engine_core.engine_core.scheduler (in-process V1)
- return SimpleNamespace(
- llm_engine=SimpleNamespace(
- engine_core=SimpleNamespace(engine_core=SimpleNamespace(scheduler=scheduler))
- ),
- scheduler_config=SimpleNamespace(max_num_seqs=max_seqs),
- )
-
-
-def test_reads_v1_stats_object():
- s = read_scheduler_stats(_v1_engine(running=3, waiting=12, cache=0.87), t_ns=0)
- assert s is not None
- assert s.num_running == 3
- assert s.num_waiting == 12
- assert s.gpu_cache_usage == 0.87
- assert s.batch_occupancy == 3 / 256 # occupancy derived from V1 num_running
-
-
-def test_none_engine_still_none():
- assert read_scheduler_stats(None) is None
-
-
-def test_v1_engine_without_stats_is_none():
- # a V1-shaped engine whose scheduler exposes nothing readable -> None, no crash.
- empty = SimpleNamespace(
- llm_engine=SimpleNamespace(
- engine_core=SimpleNamespace(engine_core=SimpleNamespace(scheduler=object()))
- )
- )
- assert read_scheduler_stats(empty) is NoneIssue: The removal of tests/test_workload_bootstrap.pyNo issues found. |
🤖 Claude Code ReviewReview🐛 Bugs
run.engine = llm
llm.gitm_throughput_fn = _throughput
llm.gitm_restart_fn = _restart
return run
def _vllm_synthetic_runner(...):There's no blank line between the
weights = [1.0 if _affine(...) else self._epsilon for k, _grid in eligible]
if not any(weights):
return []When
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))
mean = sum(values) / len(values)
if mean <= 0:
return None
🔒 Security
kind, choices = _knob_domain(f.type, domain)With ⚡ Performance
📊 Reproducibility
rng = random.Random(self._seed)This means repeated calls with the same
💡 Suggestions
specs = self._primary.propose(bottleneck_class, target_op=target_op)
return specs or self._secondary.propose(...)If
assert mem == {"cpu_offload_gb", "block_size"} # both memory-affine by name
|
Code Review by Gemini--- a/gitm/agents/autoresearch.py
+++ b/gitm/agents/autoresearch.py
@@ -100,7 +100,7 @@
"""True if ``op`` names (a substring of) a real kernel in the trace.
Guards the ``applies_to_kernels`` tagging: the residual op label comes from
the predicted graph, so only tag a proposal with it when it actually matches
a captured kernel name — otherwise ``predict_delta`` coverage would be 0 and
the proposal would rank as worthless rather than untargeted.
"""
- return any(op in k.name for k in trace.kernels())
+ return any(op in k.name for k in trace.kernels()) if op else False
# --- candidate table --------------------------------------------------------Reasoning for the change: --- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -17,6 +17,7 @@
autoresearch,
classify_bottleneck,
)
from gitm.agents.policy import Policy, select_interventions
from gitm.kernels.library import load_library
from gitm.optimizer.apply import (
Applicator,
DryRunApplicator,
LiveEngineApplicator,
apply_intervention,
)
from gitm.optimizer.attribution import attribute
from gitm.optimizer.deviation import deviation_summary, deviation_trace, write_deviation_jsonl
+from gitm.kernels.spec import knob_kind
from gitm.optimizer.monitor import (
Residuals,
SchedulerSample,Reasoning for the change: --- a/gitm/tracer/vllm_stats.py
+++ b/gitm/tracer/vllm_stats.py
@@ -83,20 +83,10 @@
def _len_or_none(x: Any) -> int | None:
try:
return len(x)
except TypeError:
return None
-def _schedulers(engine: Any) -> list[Any]:
- """Resolve the engine's scheduler(s) as a list (vLLM may keep one per PP stage)."""
- sched = _first_attr(
- engine, "scheduler", "engine.scheduler", "llm_engine.scheduler"
- )
- if sched is None:
- return []
- return list(sched) if isinstance(sched, list | tuple) else [sched]
-
-
def _max_num_seqs(engine: Any) -> int | None:
val = _first_attr(
engine,
"scheduler_config.max_num_seqs",
"engine.scheduler_config.max_num_seqs",
"llm_engine.scheduler_config.max_num_seqs",
"vllm_config.scheduler_config.max_num_seqs",
)Reasoning for the change: --- a/gitm/tracer/vllm_stats.py
+++ b/gitm/tracer/vllm_stats.py
@@ -135,6 +125,19 @@
"""
sample = SchedulerSample(t_ns=t_ns)
saw_any = False
+
+ def _schedulers(engine: Any) -> list[Any]:
+ """Resolve the engine's scheduler(s) as a list (vLLM may keep one per PP stage)."""
+ sched = _first_attr(
+ engine,
+ "scheduler",
+ "engine.scheduler",
+ "llm_engine.scheduler",
+ )
+ if sched is None:
+ return []
+ return list(sched) if isinstance(sched, list | tuple) else [sched]
+
schedulers = _schedulers(engine)
if schedulers:
totals: dict[str, int] = {}Reasoning for the change: --- a/gitm/workloads.py
+++ b/gitm/workloads.py
@@ -632,6 +643,9 @@
f"restart candidate for {knob!r} failed to build — likely a "
f"two-engine V1 distributed clash: {exc}"
) from exc
+ # Dynamically attach gitm-specific functions to the LLM object.
+ # This is generally discouraged; consider a wrapper class or explicit passing.
run.engine = llm
llm.gitm_throughput_fn = _throughput
llm.gitm_restart_fn = _restart
return runReasoning for the change: |
🤖 Claude Code ReviewCode Review🐛 Bugs
# loop.py ~L659
"bottleneck_class": ar_run.bottleneck_class,If the budget check at the top of Phase 4b is somehow bypassed (e.g. an exception in the
base = d if isinstance(d, int | float) and d not in (0, False) else 1For
assert mem == {"cpu_offload_gb", "block_size"} # The test passes
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))Functionally correct, but the mean is computed twice (once in 🔒 Security
parser = engine_args_cls.add_cli_args(argparse.ArgumentParser())If ⚡ Performance
📊 Reproducibility
rng = random.Random(self._seed)
💡 SuggestionsNo error handling if (run_dir / "autoresearch.json").write_text(...)A disk-full or permissions error here will abort the loop before Phase 5. Wrap in try/except and log a warning — the JSON is observability, not correctness.
|
Code Review by Gemini## gitm/agents/autoresearch.py
No issues found.
## gitm/scheduler/loop.py
### Bug: Missing import for `knob_kind`
The function `_unenactable` uses `knob_kind(spec.knob)` but `knob_kind` is not defined or imported in this file. This will lead to a `NameError` at runtime. `knob_kind` is likely intended to be imported from `gitm.optimizer.apply`.
```diff
--- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -11,6 +11,7 @@
from dataclasses import asdict, dataclass
from pathlib import Path
from typing import Any
+from gitm.optimizer.apply import knob_kind
from gitm._paths import runs_dir, traces_dir
from gitm.agents.autoresearch import (
gitm/tracer/vllm_stats.pyPotential Issue: Removal of vLLM V1 scheduler statsThe changes remove specific logic for reading vLLM V1 scheduler statistics ( Consider adding a comment or documentation to clarify the supported vLLM versions for metric collection, or re-introduce conditional logic if V1 support is desired. --- a/gitm/tracer/vllm_stats.py
+++ b/gitm/tracer/vllm_stats.py
@@ -83,50 +83,11 @@
def _schedulers(engine: Any) -> list[Any]:
"""Resolve the engine's scheduler(s) as a list (vLLM may keep one per PP stage)."""
sched = _first_attr(
- engine,
- # vLLM V0.
- "scheduler",
- "engine.scheduler",
- "llm_engine.scheduler",
- # vLLM V1 (in-process, VLLM_ENABLE_V1_MULTIPROCESSING=0): the scheduler
- # lives inside the EngineCore, not on the LLMEngine. Paths vary across
- # v0.2x point releases — GPU-validate the exact one on the target build.
- "llm_engine.engine_core.engine_core.scheduler",
- "llm_engine.engine_core.scheduler",
- "engine_core.engine_core.scheduler",
- "engine_core.scheduler",
+ engine, "scheduler", "engine.scheduler", "llm_engine.scheduler"
)
if sched is None:
return []
return list(sched) if isinstance(sched, list | tuple) else [sched]
-def _v1_scheduler_stats(scheduler: Any) -> dict[str, Any]:
- """vLLM V1 stats via ``scheduler.make_stats()`` — V1 doesn't keep the V0
- running/waiting deques in the same shape, but exposes a stats object with
- ``num_running_reqs`` / ``num_waiting_reqs`` / ``kv_cache_usage``. Best-effort;
- returns only the fields it actually found. GPU-validate the attr names on the
- target vLLM build (they've shifted across v0.2x).
- """
- make = getattr(scheduler, "make_stats", None)
- if not callable(make):
- return {}
- try:
- stats = make()
- except Exception:
- return {}
- if stats is None:
- return {}
- out: dict[str, Any] = {}
- for field_name, attr in (("num_running", "num_running_reqs"),
- ("num_waiting", "num_waiting_reqs")):
- v = getattr(stats, attr, None)
- if isinstance(v, int):
- out[field_name] = v
- ku = getattr(stats, "kv_cache_usage", None)
- if isinstance(ku, int | float):
- out["gpu_cache_usage"] = float(ku)
- return out
-
-
def _max_num_seqs(engine: Any) -> int | None:
val = _first_attr(
engine,
"scheduler_config.max_num_seqs",
"engine.scheduler_config.max_num_seqs",
@@ -174,16 +134,6 @@
if usage is not None:
sample.gpu_cache_usage = usage
saw_any = True
- # vLLM V1: fill running / waiting / cache from the scheduler's stat object
- # where the VO deques weren't exposed (they read empty on V1)
- for sch in schedulers:
- for field_name, val in _v1_scheduler_stats(sch).items():
- if getattr(sample, field_name) is None:
- setattr(sample, field_name, val)
- saw_any = True
-
# Total unfinished — a stable public method on LLMEngine across versions.
getter = _first_attr(
engine,
"get_num_unfinished_requests",
"engine.get_num_unfinished_requests",
gitm/workloads.pyNo issues found. tests/test_autoresearch.pyNo issues found. tests/test_run_loop_workload.pyNo issues found. tests/test_vllm_stats_v1.pyNo issues found (file was deleted). tests/test_workload_bootstrap.pyNo issues found. |
🤖 Claude Code ReviewReview🐛 Bugs
if not any(weights):
return []
base = d if isinstance(d, int | float) and d not in (0, False) else 1
raw = [base * m for m in _GRID_MULTIPLIERS] # → [0.5, 2, 4]For
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))
mean = sum(values) / len(values)
if mean <= 0:
return None
🔒 Security
⚡ Performance
📊 Reproducibility
💡 Suggestions
except Exception:
passA broken annotation object will silently fall back to a string-match kind. At minimum log a debug warning so annotation-parse failures are visible during development.
ranked = select_interventions(trace, proposals, policy or Policy(), ...)Fine for now, but if Test file is cut off —
|
🤖 Claude Code ReviewReview🐛 Bugs
run.engine = llm
llm.gitm_throughput_fn = _throughput
llm.gitm_restart_fn = _restart
return run
def _vllm_synthetic_runner(...) # ← no blank line, likely a merge artifactThe diff shows the new
run = autoresearch(make_trace(events=events), applicator=applica # cut offThe test file is incomplete — this will be a
🔒 Security
⚡ Performance
📊 Reproducibility
💡 Suggestions
No error handling if
|
Code Review by Gemini--- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -11,6 +11,7 @@ from __future__ import annotations
import json
import os
import re
import time
import uuid
from dataclasses import asdict, dataclass
from pathlib import Path
from typing import Any
from gitm._paths import runs_dir, traces_dir
from gitm.agents.autoresearch import (
AutoresearchRun,
EngineArgsProposer,
FallbackProposer,
TableProposer,
autoresearch,
classify_bottleneck,
)
from gitm.agents.policy import Policy, select_interventions
from gitm.kernels.library import load_library
from gitm.optimizer.apply import (
Applicator,
DryRunApplicator,
LiveEngineApplicator,
apply_intervention,
+)
+from gitm.optimizer.apply import knob_kind
from gitm.optimizer.attribution import attribute
from gitm.optimizer.deviation import deviation_summary, deviation_trace, write_deviation_jsonl
from gitm.optimizer.monitor import (Reasoning for the change: --- a/gitm/tracer/vllm_stats.py
+++ b/gitm/tracer/vllm_stats.py
@@ -83,20 +83,59 @@ def _first_attr(obj: Any, *paths: str) -> Any:
def _len_or_none(x: Any) -> int | None:
try:
return len(x)
except TypeError:
return None
def _schedulers(engine: Any) -> list[Any]:
"""Resolve the engine's scheduler(s) as a list (vLLM may keep one per PP stage)."""
sched = _first_attr(
- engine, "scheduler", "engine.scheduler", "llm_engine.scheduler"
+ engine,
+ # vLLM V0.
+ "scheduler",
+ "engine.scheduler",
+ "llm_engine.scheduler",
+ # vLLM V1 (in-process, VLLM_ENABLE_V1_MULTIPROCESSING=0): the scheduler
+ # lives inside the EngineCore, not on the LLMEngine. Paths vary across
+ # v0.2x point releases — GPU-validate the exact one on the target build.
+ "llm_engine.engine_core.engine_core.scheduler",
+ "llm_engine.engine_core.scheduler",
+ "engine_core.engine_core.scheduler",
+ "engine_core.scheduler",
)
if sched is None:
return []
return list(sched) if isinstance(sched, list | tuple) else [sched]
+def _v1_scheduler_stats(scheduler: Any) -> dict[str, Any]:
+ """vLLM V1 stats via ``scheduler.make_stats()`` — V1 doesn't keep the V0
+ running/waiting deques in the same shape, but exposes a stats object with
+ ``num_running_reqs`` / ``num_waiting_reqs`` / ``kv_cache_usage``. Best-effort;
+ returns only the fields it actually found. GPU-validate the attr names on the
+ target vLLM build (they've shifted across v0.2x).
+ """
+ make = getattr(scheduler, "make_stats", None)
+ if not callable(make):
+ return {}
+ try:
+ stats = make()
+ except Exception:
+ return {}
+ if stats is None:
+ return {}
+ out: dict[str, Any] = {}
+ for field_name, attr in (("num_running", "num_running_reqs"),
+ ("num_waiting", "num_waiting_reqs")):
+ v = getattr(stats, attr, None)
+ if isinstance(v, int):
+ out[field_name] = v
+ ku = getattr(stats, "kv_cache_usage", None)
+ if isinstance(ku, int | float):
+ out["gpu_cache_usage"] = float(ku)
+ return out
+
+
def _max_num_seqs(engine: Any) -> int | None:
val = _first_attr(
engine,
"scheduler_config.max_num_seqs",
"engine.scheduler_config.max_num_seqs",
"llm_engine.scheduler_config.max_num_seqs",
"vllm_config.scheduler_config.max_num_seqs",
)
return int(val) if isinstance(val, int) and val > 0 else None
@@ -174,14 +204,13 @@ def read_scheduler_stats(engine: Any, *, t_ns: int = 0) -> SchedulerSample | Non
if usage is not None:
sample.gpu_cache_usage = usage
saw_any = True
- # vLLM V1: fill running / waiting / cache from the scheduler's stat object
- # where the VO deques weren't exposed (they read empty on V1)
- for sch in schedulers:
- for field_name, val in _v1_scheduler_stats(sch).items():
- if getattr(sample, field_name) is None:
- setattr(sample, field_name, val)
- saw_any = True
+ # vLLM V1: fill running / waiting / cache from the scheduler's stat object
+ # where the V0 deques weren't exposed (they read empty on V1)
+ for sch in schedulers:
+ for field_name, val in _v1_scheduler_stats(sch).items():
+ if getattr(sample, field_name) is None: # Only set if not already found by V0 path
+ setattr(sample, field_name, val)
+ saw_any = True
# Total unfinished — a stable public method on LLMEngine across versions.
getter = _first_attr(Reasoning for the change: --- /dev/null
+++ b/tests/test_vllm_stats_v1.py
@@ -0,0 +1,50 @@
+"""Read scheduler stats from a vLLM V1-shaped engine.
+
+V1 keeps the scheduler inside EngineCore and exposes a stats object via
+make_stats() (num_running_reqs / num_waiting_reqs / kv_cache_usage) instead of
+the V0 running/waiting deques. Fake that shape to pin the read path off-GPU; the
+exact attr names still need GPU validation on the target vLLM build.
+"""
+
+from __future__ import annotations
+
+from types import SimpleNamespace
+
+from gitm.tracer.vllm_stats import read_scheduler_stats
+
+
+def _v1_engine(running: int, waiting: int, cache: float, max_seqs: int = 256):
+ stats = SimpleNamespace(
+ num_running_reqs=running, num_waiting_reqs=waiting, kv_cache_usage=cache
+ )
+ scheduler = SimpleNamespace(make_stats=lambda: stats)
+ # llm.llm_engine.engine_core.engine_core.scheduler (in-process V1)
+ return SimpleNamespace(
+ llm_engine=SimpleNamespace(
+ engine_core=SimpleNamespace(engine_core=SimpleNamespace(scheduler=scheduler))
+ ),
+ scheduler_config=SimpleNamespace(max_num_seqs=max_seqs),
+ )
+
+
+def test_reads_v1_stats_object():
+ s = read_scheduler_stats(_v1_engine(running=3, waiting=12, cache=0.87), t_ns=0)
+ assert s is not None
+ assert s.num_running == 3
+ assert s.num_waiting == 12
+ assert s.gpu_cache_usage == 0.87
+ assert s.batch_occupancy == 3 / 256 # occupancy derived from V1 num_running
+
+
+def test_none_engine_still_none():
+ assert read_scheduler_stats(None) is None
+
+
+def test_v1_engine_without_stats_is_none():
+ # a V1-shaped engine whose scheduler exposes nothing readable -> None, no crash.
+ empty = SimpleNamespace(
+ llm_engine=SimpleNamespace(
+ engine_core=SimpleNamespace(engine_core=SimpleNamespace(scheduler=object()))
+ )
+ )
+ assert read_scheduler_stats(empty) is NoneReasoning for the change: |
The value grid was blind: a ½×/2×/4× ladder that invents values for enum knobs and list-valued args, which then fail to apply and waste rollback-gated trials. Read each field's valid domain from its vLLM CLI argument instead: - argparse `choices=` → the real enum domain, so the grid proposes only values that can apply (not an invented ladder point). - argparse `type=` → sharpens int/float when the annotation is opaque. - `nargs` → skip list-valued args (not scalar knobs). Split the extraction into `_knobs_from_engine_args` / `_argparse_domains` / `_knob_domain` so it's tested against a fake EngineArgs-like class without vLLM present. Honest limit: argparse carries no numeric min/max, so unconstrained ints/floats still use the ladder. Offline (no vLLM) path unchanged — frozen fallback catalog. Docs reconciled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
On platforms whose CUPTI doesn't attribute CUDA-graph-replayed kernels, the tracer records an empty trace and the loop honestly reports no-data. Setting GITM_VLLM_ENFORCE_EAGER=1 builds the engine (and every restart-A/B candidate, via the shared _base_kwargs) with enforce_eager=True — decode kernels launch individually so CUPTI captures them. Also skips torch.compile + graph capture, so engine builds are much faster, fitting more candidates in a given budget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Real H100/Colab runs showed most generated structural candidates rolling back unmeasured (apply failed, not a measured loss) — several because the knob (tensor_parallel_size, prefill_context_parallel_size, ...) only makes sense with more than one GPU, and the single-GPU box's engine build has no topology to satisfy. Every such trial is a wasted restart-A/B on a candidate that was never enactable. Filter EngineArgs fields whose name indicates multi-GPU-only relevance (_MULTI_GPU_HINTS: parallel_size, tensor_parallel, pipeline_parallel, data_parallel, context_parallel, expert_parallel) out of the searchable surface when the box has fewer than 2 GPUs. GPU count is autodetected via torch.cuda.device_count() (default 1 when it can't tell — undercounting only skips a possibly-valid knob, the safe direction) or overridable explicitly via VLLMKnobSource(gpu_count=...) / EngineArgsProposer(gpu_count=...). Offline fallback catalog is unaffected (no multi-GPU knobs in it). 9 new tests. Docs reconciled: notes the filter, and the remaining honest gap (checkpoint- or VRAM-dependent knobs like quantization=awq or the restart A/B's second-engine memory requirement aren't covered by this filter). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nicholaslawrence-hub
force-pushed
the
autoresearch-clean
branch
from
8494aa8 to
49e23dd
Compare
Code Review by Gemini--- a/gitm/agents/autoresearch.py
+++ b/gitm/agents/autoresearch.py
@@ -500,6 +500,9 @@
"""Read each EngineArgs field's valid domain from ``add_cli_args`` (best-effort).
vLLM builds its CLI from the same dataclass, so the argparse actions are the
authoritative source of choices/types — far better than string-matching the
annotation. Returns ``{}`` if the class has no ``add_cli_args`` or it raises.
"""
try:
import argparse
parser = engine_args_cls.add_cli_args(argparse.ArgumentParser()) # type: ignore[attr-defined]
+ # NOTE: Accessing `_actions` is an internal implementation detail of argparse.
+ # This is done to introspect the CLI argument definitions (choices, type, nargs)
+ # which are not exposed via a public API on the dataclass itself.
except Exception:
return {}
out: dict[str, _ArgDomain] = {} |
🤖 Claude Code ReviewReview🐛 Bugs
weights = [1.0 if _affine(...) else self._epsilon for k, _grid in eligible]
if not any(weights):
return []When
🔒 Security
if r.measured_delta is None and r.apply_error:
evidence += f"; apply failed: {r.apply_error}"
⚡ Performance
📊 Reproducibility
💡 Suggestions
kind, choices = _knob_domain(f.type, domain)vLLM uses
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))This is only reached when No test for |
nicholaslawrence-hub
force-pushed
the
autoresearch-clean
branch
from
49e23dd to
bd1e9a8
Compare
🤖 Claude Code ReviewReview🐛 Bugs
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))If
if not any(weights):
🔒 Security
restart_mode=os.environ.get("GITM_RESTART_MODE", "parallel"),
⚡ Performance
📊 Reproducibility
rng = random.Random(self._seed)
💡 Suggestions
apply_error=applied.error if applied else None,The No test for serial-restart rollback path |
Code Review by Gemini## `benchmarks/README.md`
```diff
--- a/benchmarks/README.md
+++ b/benchmarks/README.md
@@ -1,125 +1,105 @@
-# We Gave Our Runtime Four GPU Workloads and No Instructions. It Diagnosed Each and Proved Every Fix.
+# Benchmarks
-*The trace picks the lever. A gated A/B proves it.*
-
-**+32.4% on HFT, byte-identical on 400M events. +37% on nuScenes. Up to +427% on KITTI. Zero human tuning, every failed change rolled back.**
-
-*July 2026*
-
-[Reproduce our benchmarks](#try-it-yourself)
-
----
+We Gave Our Runtime Four GPU Workloads and No Instructions. It Diagnosed Each and Proved Every Fix.
+The trace picks the lever. A gated A/B proves it.
++32.4% on **HFT Limit Order Books**, byte-identical on 400M events. +37% on **nuScenes**. Up to +427% on **KITTI**. Zero human tuning, every failed change rolled back.
We think most GPU fleets run far below the hardware they are paying for, and that the waste is a systems problem, not a kernel problem. The GPU is rarely slow because the math is slow. It is slow because it waits: launching thousands of tiny kernels one at a time, stalling at synchronization points, moving memory it did not need to move, or running work in series that could have overlapped.
-
Our runtime does one thing: it closes the loop between diagnosis and proof.
- **Observe.** Profile the workload from its own telemetry (CUPTI, NVML). No model-code changes or source access.
- **Diagnose.** Read the dominant bottleneck off the trace: launch-bound, compute-bound, or serialized.
- **Apply.** The candidate intervention selected for that bottleneck.
- **Verify.** A gated A/B on real hardware, best-of-N to suppress jitter.
- **Keep or roll back.** Keep only if the candidate is both faster and output-equivalent. Otherwise revert and report nothing.
Every number in this post is a kept decision, not a guess.
-
-A good engineer can tune one workload once. A fleet is thousands of workloads changing weekly: new models, new request mixes, new kernels, new traffic shapes, new failure modes. That is why this has to be a runtime loop, not a consulting pass. The loop earns its place because it runs continuously, per job, where hand-tuning gives up. Any one result below is useful; the point is that the same loop found all of them automatically, verified each one, and rolled back the ones that failed.
+A good engineer can tune one workload at once. A fleet is thousands of workloads changing weekly: new models, new request mixes, new kernels, new traffic shapes, new failure modes. That is why this has to be a runtime loop, not a consulting pass. The loop earns its place because it runs continuously, per job, where hand-tuning gives up. Any one result below is useful; the point is that the same loop found all of them automatically, verified each one, and rolled back the ones that failed.
Two honest notes before the numbers. These are benchmark workloads, not a customer's production fleet, and what they prove is the loop, that the runtime can read what actually limits a workload and apply a safe, verified fix without touching model code. Separately, our commercial wedge is inference serving, and the same loop is now pointed there in a vLLM decode benchmark we describe at the end. We are not pretending these four workloads stand in for that one. They show the loop; the next one runs it on what we sell into.
-## Where the Waste Actually Lives
-
+Where the Waste Actually Lives
Two readings from the trace tell us most of what we need. The first is whether the GPU is **launch-bound** or **compute-bound**: burning time issuing many small kernels, or genuinely busy doing large-matrix arithmetic? The second is **serialized concurrency**, our reading of how much work runs with no overlap.
A serialized concurrency of 1.000 means your GPU is effectively single-threaded at the workload level: thousands of kernels standing in a single-file line, each waiting on the one before it, a piece of hardware built for massive parallelism running its work one at a time.
These two readings decide the lever. A launch-bound workload wants its launches amortized. A compute-bound workload does not care about launch overhead at all, so amortizing launches buys nothing; it wants the arithmetic itself made cheaper. Pick the wrong lever and you do real work for a 0% result.
-
-## Track 1: HFT Limit Order Books (the cleanest proof)
-
+Track 1: HFT Limit Order Books (the cleanest proof)
The strongest result first: **+32.4% throughput with byte-identical output, on 400 million events.**
-optimize hft: 400,000,000 events on NVIDIA A100 80GB PCIe
--
|
nicholaslawrence-hub
force-pushed
the
autoresearch-clean
branch
from
bd1e9a8 to
7e97a38
Compare
Trimmed multi-paragraph docstrings/comments added across the recent commits (residual attribution, prerequisite veto, joint candidates, coverage unification, relative catalog values) down to what a reader actually needs, in apply.py, deviation.py, monitor.py, replay.py, vllm_knobs.py, spec.py, and loop.py. No behavior change.
…e from autoresearch's static table Three new curated library.yaml entries, all backed by real evidence: - async_scheduling: measured +1.7% (kept) on an actual L4/opt-125m trace via autoresearch's generative search. Overlaps CPU-side scheduling with GPU execution, directly targeting idle_stall (launch-bound gaps). Low risk, no numerics change. - cpu_offload_gb / preemption_mode=swap: promoted from autoresearch's static _RULES fallback table (memory_bound) to reviewed catalog entries with real applicability/safety/source metadata, instead of living only as a bare (knob, value, rationale) tuple. autoresearch proposes *outside* the catalog by design — cpu_offload_gb and preemption_mode are removed from _RULES["memory_bound"] (now empty, same as idle_stall already was) so the disjoint-from-catalog invariant (test_proposed_knobs_are_disjoint_from_catalog) holds. This also empties the offline/no-vLLM fallback's memory_bound coverage, since both of its only two candidates graduated — same gap idle_stall's fallback already had. Updated 6 tests that exercised the old memory_bound fallback path to use compute_bound (still has compilation_config) or to assert the now-honest empty result, rather than paper over the gap. Also compresses the comments/docstrings touched in autoresearch.py along the way.
A markdown table cell had a raw Windows-1252 em-dash byte (0x97) instead of a proper UTF-8-encoded em-dash — every other em-dash in the file was already correctly encoded (0xE2 0x80 0x94), just this one stray byte from what was likely a copy-paste. Both the gemini-review and review CI checks read the full PR diff as UTF-8 and crashed with UnicodeDecodeError on this exact byte, which is why they were failing -- not any of the code-quality findings those bots had posted.
Code Review by GeminiThe changes introduce a new autoresearch agent, dynamic knob value resolution, and improved engine lifecycle management for vLLM workloads. The new features are well-integrated with the existing policy and rollback mechanisms, and the documentation and tests cover the new functionality comprehensively. One minor issue was identified: Issue: Duplicate Test FunctionThe test function File: --- a/tests/test_vllm_knobs_and_restart.py
+++ b/tests/test_vllm_knobs_and_restart.py
@@ -242,26 +422,6 @@
assert out["summary"]["scheduler_stats"] is not None
def test_report_kernel_time_residual_uses_weighted_total_and_clamps():
from gitm.optimizer.monitor import KernelResidual, Residuals
from gitm.scheduler.loop import _agg_kt_residual
res = Residuals(
per_kernel=[
KernelResidual(op="tiny", layer=None, r_kt=9999.0, r_mt=None, t_obs_s=200e-6, t_pred_s=1e-8),
KernelResidual(op="main", layer=None, r_kt=0.1, r_mt=None, t_obs_s=11e-6, t_pred_s=10e-6),
]
)
assert _agg_kt_residual(res) == 1.0
sane = Residuals(
per_kernel=[
KernelResidual(op="a", layer=None, r_kt=9.0, r_mt=None, t_obs_s=12e-6, t_pred_s=10e-6),
KernelResidual(op="b", layer=None, r_kt=0.2, r_mt=None, t_obs_s=12e-6, t_pred_s=10e-6),
]
)
assert _agg_kt_residual(sane) == pytest.approx(0.2)
def test_ar_target_residual_uses_the_search_target_not_a_hardcoded_zero():
from gitm.agents.autoresearch import AutoresearchRun, ResidualTarget
from gitm.scheduler.loop import _ar_target_residual
# No target found (nothing exceeded its predicted ceiling) -> honest 0.0.
empty = AutoresearchRun(bottleneck_class="idle_stall", results=[], target=None)
assert _ar_target_residual(empty) == 0.0
# A real target -> its residual surfaces, clamped like every other residual.
modest = AutoresearchRun(
bottleneck_class="idle_stall", results=[],
target=ResidualTarget(op="attn_score_value", residual=0.42, n_kernels=8),
)
assert _ar_target_residual(modest) == pytest.approx(0.42)
huge = AutoresearchRun(
bottleneck_class="idle_stall", results=[],
target=ResidualTarget(op="attn_score_value", residual=17.8, n_kernels=8),
)
assert _ar_target_residual(huge) == 1.0Suggested change: |
🤖 Claude Code ReviewReview🐛 Bugs
base = d if isinstance(d, int | float) and d not in (0, False) else 1For
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))
mean = sum(values) / len(values)
if mean <= 0:
return None
mem_score = max(mem_score, roofline_frac / _MEMCPY_THRESHOLD)
out += self._extra_candidates(...)
return out if self._max is None else out[:self._max]The cap is applied after appending joint candidates, so the effective cap for the main grid candidates is 🔒 Security
⚡ Performance
📊 Reproducibility
💡 Suggestions
|
… them out GenerativeProposer.propose() appended joint (prerequisite+dependent) candidates after the per-knob value grid, then sliced the combined list to max_candidates from the front. A large single-knob grid (the common case — real EngineArgs surfaces have many affine knobs) could fill the whole cap on its own, silently truncating every joint candidate off the end with no visibility into why. Put joint candidates first instead: they're deliberately targeted (unlocking a prerequisite-gated knob atomically), so they shouldn't lose a slot to the more generic grid search. Adds a regression test with a 50-knob filler surface confirming a joint candidate survives a max_candidates=10 cap that the filler grid alone would otherwise fill.
Code Review by Gemini--- a/gitm/agents/autoresearch.py
+++ b/gitm/agents/autoresearch.py
@@ -709,7 +709,7 @@
if prereq is None or prereq.kind != "bool":
continue
for knob in knobs:
- if knob.name == prereq_name or needle not in knob.name.lower():
+ if knob.name == prereq_name or needle not in knob.name.lower(): # Bug: This condition should be `if needle in knob.name.lower()` to match dependent knobs.
continue
if not _affine(knob, bottleneck_class, keywords):
continue |
🤖 Claude Code ReviewCode Review🐛 Bugs
vals = sorted({int(round(x)) for x in raw if round(x) >= 1})
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))The
mem_score = max(mem_score, roofline_frac / _MEMCPY_THRESHOLD)
value: 4 # fallback; also the outcome when current swap_space is 0 (common
# default) — 0 * any multiplier stays 0, so the sweep collapses to this.The comment acknowledges
value_multiplier_grid: [0.95, 1.03, 1.10]
value_max: 0.97
🔒 Security
parser = engine_args_cls.add_cli_args(argparse.ArgumentParser())
⚡ Performance
self._catalog = (
set(catalog_knobs)
if catalog_knobs is not None
else {s.knob for s in load_library()}
)If multiple proposers are instantiated (e.g.
📊 Reproducibility
rng = random.Random(self._seed)Re-seeding on every call means two consecutive calls with the same
💡 Suggestions
No timeout on |
CI status updateAll checks are now green: Root cause of the CI failures: both On the flood of prior bot comments: the ~30 comments accumulated on this PR are largely stale, duplicated across many re-runs, or false when checked against the current code. A few examples verified while triaging:
One real, actionable finding did survive triage (fixed in No inline/resolvable review threads exist on this PR (verified via 🤖 Addressed by Claude Code |
main (#60) added run.workload_id = "vllm-decode" (read by loop.py to infer the workload id off the engine) and a regression test guarding the factory's return path. This branch had since added gitm_baseline_restart_fn (used by LiveEngineApplicator's restart_mode="serial") in the same wiring block. Kept both. The merged-in regression test (test_vllm_decode_factory_returns_wired_ runner) called gitm_restart_fn with the old 3-arg (engine, knob, value) signature; updated to the dict-based (engine, {knob: value}) signature this branch introduced for joint candidates. benchmarks/README.md and tests/test_run_loop_workload.py's other additions merged cleanly (no conflicts, purely additive).
Code Review by Gemini```diff
--- a/gitm/optimizer/apply.py
+++ b/gitm/optimizer/apply.py
@@ -76,14 +77,14 @@ def apply_intervention(
try:
applicator.apply(spec)
except Exception as exc:
applicator.restore(snapshot)
- _audit(audit, "revert", spec, cause=f"apply failed, restored: {exc}")
+ _audit(audit, "revert", spec, cause=f"apply failed, restored: {exc}", knobs=_knob_values(spec))
return ApplyResult(False, rolled_back=True, measured_delta=None,
error=f"apply failed, restored: {exc}")
_audit(audit, "apply", spec, cause="applied live", knobs=_knob_values(spec))
# Step 3: measure. A crash mid-measurement also rolls back.
try:
delta = applicator.measure(spec)
except Exception as exc:
applicator.restore(snapshot)
- _audit(audit, "revert", spec, cause=f"measure failed, restored: {exc}")
+ _audit(audit, "revert", spec, cause=f"measure failed, restored: {exc}", knobs=_knob_values(spec))
return ApplyResult(False, rolled_back=True, measured_delta=None,
error=f"measure failed, restored: {exc}") |
🤖 Claude Code ReviewReview🐛 Bugs
base = d if isinstance(d, int | float) and d not in (0, False) else 1For
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))
mean = sum(values) / len(values)
if mean <= 0:
return None
if knob.classes:
return 1But
weights = [
1.0 if _affine(...) else self._epsilon
for k, _grid in eligible
]
if not any(weights):
return []With
🔒 Security
⚡ Performance
self._catalog = ... {s.knob for s in load_library()}
📊 Reproducibility
rng = random.Random(self._seed)Each
💡 Suggestions
Missing |
All three apply_intervention() "revert" audit calls (apply-raises, measure-raises, regression rollback) omitted knobs=_knob_values(spec), so a rollback's audit entry recorded only the cause string, not which knob(s) were involved — the sibling "apply" entry already had this. Fixed all three call sites and extended test_apply_rollback.py to assert the knobs detail on every revert path, including a new test covering the apply-raises/measure-raises cases directly. autoresearch: explicit Knob.classes tag scores max, not flat 1 _affinity_strength() gave an explicit, authored Knob.classes tag a flat score of 1, while a knob matched by several affinity keywords incidentally could score higher (e.g. 2+) — inverting the intended confidence ordering. An authored tag is ground truth and must never be outranked by a coincidental multi-keyword name match, so it now scores max(len(keywords), 1), the highest score reachable for that call. Updated the corresponding assertion in test_autoresearch.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed this round's review feedback (commit 574f5cd): Real, fixed:
Verified false / already addressed, no action taken:
No real inline/resolvable PR review threads exist ( Full test run ( 🤖 Addressed by Claude Code |
🤖 Claude Code ReviewCode Review🐛 Bugs
base = d if isinstance(d, int | float) and d not in (0, False) else 1If
mem_score = max(mem_score, roofline_frac / _MEMCPY_THRESHOLD)
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))
applies_to_kernels: This deserializes as
value_multiplier_grid: [0.95, 1.03, 1.10]
value_max: 0.97
🔒 Security
parser = engine_args_cls.add_cli_args(argparse.ArgumentParser())If ⚡ Performance
max_attempts = max(self._n * 4, len(eligible) * 4)When 📊 Reproducibility
rng = random.Random(self._seed)The RNG resets to the same seed on every
return IDLE_STALL if sc_score >= mem_score else MEMORY_BOUNDTies always resolve to 💡 Suggestions
"idle_stall": [],
"memory_bound": [],
Missing |
_value_grid() substituted base=1 for an int/float knob whose real default is 0 (e.g. cpu_offload_gb), fabricating a grid like [0, 1, 2, 4] that doesn't represent a real multiplicative search around the knob's actual value. A zero default has no meaningful ratio-based grid, so it now returns [] instead of inventing one. Updated the test fixtures that relied on the old fabricated-grid behavior (cpu_offload_gb -> nonzero default) since they were exercising the value-grid path, not testing zero specifically. _searchable() re-ran self._source.knobs() (a fresh knob-surface parse, e.g. VLLMKnobSource's argparse introspection) on every call; propose() calls it twice per invocation (main loop + _extra_candidates), doubling that work for no reason since source/catalog are fixed for the proposer's lifetime. Now memoized per instance. Also: one-line comment documenting that classify_bottleneck's sc/mem tie resolves to idle_stall (was previously undocumented). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review by GeminiThe changes introduce an autoresearch agent that proposes non-catalog vLLM optimization candidates. This involves bottleneck classification, targeting the largest residual, generating candidates from knob surfaces, and integrating this into the existing intervention loop. Here's a review of the changes:
|
|
Addressed this round's Claude Code Review batch (commit e0d11bc): Real, fixed:
Verified false / already addressed, no action taken:
No real inline/resolvable PR review threads exist ( Test run: 🤖 Addressed by Claude Code |
Code Review by GeminiThe changes introduce a new autoresearch agent, integrating it into the existing optimization loop. The agent classifies bottlenecks, targets the largest residuals, and proposes non-catalog vLLM optimization candidates. These candidates are then routed through the same gate and rollback path as catalog interventions, ensuring safety. The changes also include significant updates to the intervention specification to support relative and joint knob changes, and robust handling of live engine restarts. Here are some observations and suggested improvements: Issues and Improvements
|
🤖 Claude Code ReviewReview🐛 Bugs
🔒 Security
⚡ Performance
📊 Reproducibility
💡 Suggestions
|
|
This round's Gemini review: no action needed. Every item in the review is explicitly marked "No change needed" by Gemini itself, except one suggested diff — removing `cpu_offload_gb`/`preemption_mode` from `_FALLBACK_KNOBS` in `autoresearch.py` on the theory that the offline fallback path bypasses the catalog-exclusion filter and could re-propose knobs that already graduated to `library.yaml`. Verified false: catalog exclusion happens downstream in `_ProposerBase._searchable()`, which filters every knob (regardless of source — real EngineArgs introspection or `_FALLBACK_KNOBS`) against `self._catalog`, built as `{s.knob for s in load_library()}`. Both `cpu_offload_gb` and `preemption_mode` are in that set (their catalog entries' `knob:` field), so the fallback path already excludes them — no separate code path bypasses the filter. This exact behavior has its own regression test: `test_engineargs_offline_fallback_runs_without_vllm` asserts `p.propose("memory_bound") == []` on the fallback path specifically because these two graduated to the catalog. CI status: all checks (`lint`, `pytest` 3.10/3.11/3.12, `gemini-review`, `review`) now pass on commit e0d11bc. `gh pr view` reports `mergeable: MERGEABLE` (no conflicts) — the remaining `mergeStateStatus: BLOCKED` is a required-review/branch-protection gate, not a code or CI issue. No real inline/resolvable PR review threads exist (`gh api .../pulls/56/comments` still returns `[]`). 🤖 Addressed by Claude Code |
|
Addressed this round's 3 review comments (commit aea634d): Real, fixed:
Verified false / already addressed, no action taken:
No real inline/resolvable PR review threads exist (`gh api .../pulls/56/comments` still returns `[]`). Targeted test suite (`test_apply_rollback.py`, `test_vllm_knobs_and_restart.py`, `test_run_loop_workload.py`, `test_vllm_embodiment.py`, `test_autoresearch.py`, `test_catalog_unify.py`) is green. 🤖 Addressed by Claude Code |
🤖 Claude Code ReviewReview🐛 Bugs
if not isinstance(d, int | float) or d in (0, False):
return []
mem_score = max(mem_score, roofline_frac / _MEMCPY_THRESHOLD)The roofline fraction (fraction of kernel time that is memory-bound, 0–1) is divided by
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))
value_multiplier_grid: [0.95, 1.03, 1.10]
value_max: 0.97The grid contains
value: 4 # fallback; also the outcome when current swap_space is 0 (common
# default) — 0 * any multiplier stays 0, so the sweep collapses to this.The comment admits the multiplier grid degenerates to nothing useful (0×anything=0), but the fix (use an absolute grid instead) isn't implemented. ⚡ Performance
max_attempts = max(self._n * 4, len(eligible) * 4)With a large knob surface (real 📊 Reproducibility
rng = random.Random(self._seed)The RNG is re-seeded from
_FALLBACK_KNOBS: tuple[Knob, ...] = (
Knob("cpu_offload_gb", "int", default=0),
Knob("preemption_mode", "enum", ...),
...
)The doc says "autoresearch only proposes outside the catalog." The 💡 Suggestions
text = str(annotation).lower()With
"idle_stall": [],
"memory_bound": [],
|
Code Review by Geminiautoresearch: generate and gate non-catalog vLLM optimization candidates
The changes introduce a significant new feature with a robust design, clear separation of concerns, and comprehensive test coverage. The implementation of dynamic/relative interventions, joint candidates, and the detailed engine restart/shutdown logic is particularly well-executed. The documentation is also thorough.
Here are a few minor suggestions for improvement:
### `docs/autoresearch.md`
The `Fallback table` section mentions that `idle_stall` and `memory_bound` have `(none by default)`. While the commit body explains that `cpu_offload_gb` and `preemption_mode` graduated to `library.yaml`, this isn't explicitly stated in the user-facing documentation table. Clarifying this would improve readability and understanding of why these classes have no default autoresearch rules.
```diff
--- a/docs/autoresearch.md
+++ b/docs/autoresearch.md
@@ -199,9 +199,9 @@
| Class | Knob | Value | Rationale |
|-------|------|-------|-----------|
| `idle_stall` | *(none by default)* | — | dependent prefill/DBO knobs are filtered unless represented by reviewed catalog entries |
-| `memory_bound` | *(none by default)* | — | `cpu_offload_gb` and `preemption_mode` graduated to `library.yaml` (see below) |
+| `memory_bound` | *(none by default)* | — | `cpu_offload_gb` and `preemption_mode` graduated to `library.yaml` |
| `compute_bound` | `compilation_config` | 3 | torch.compile level 3: kernel fusion + piecewise CUDA graphs |
The **rationales are plausibility arguments, not measured claims**. Each proposal
carries `expected_delta_mean = 0.05` (`lo = 0.0`, `hi = 0.15`) — a modest,
honestly-unproven range — and a `source` that says so. Only the measured A/B
turns a proposal into a real number.
|
|
This round's 2 review comments: no code changes needed. Claude Code Review — every "bug" is either a rehash already verified false in earlier rounds (`_roofline_memory_fraction` normalization, `largest_residual` max()-on-empty, `value_multiplier_grid` "no schema/consumer"), or the bot's own analysis concludes it's a non-issue:
Gemini review — suggests removing "(see below)" from the doc line I just added last round. Purely cosmetic wording, no functional or clarity difference either way; not acted on. No real inline/resolvable PR review threads exist (`gh api .../pulls/56/comments` still returns `[]`). CI on aea634d: all checks pass (lint, pytest ×3, gemini-review, review); `mergeable: MERGEABLE`. 🤖 Addressed by Claude Code |
…m-decode path _shutdown() called gc.collect() + torch.cuda.empty_cache() but not torch.cuda.ipc_collect(), unlike gitm/workloads.py's _vllm_decode_factory shutdown, which already calls all three. Brought the two in line so a restart-apply releases GPU memory as completely as a fresh workload boot. docs: memory_bound fallback-table rows were stale after the cpu_offload_gb/preemption_mode catalog graduation _RULES["memory_bound"] is empty in code (those two knobs moved to library.yaml), matching how the idle_stall row already reads "(none by default)" — but the memory_bound rows still listed them as if they were still _RULES entries. Updated to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
nicholaslawrence-hub
force-pushed
the
autoresearch-clean
branch
from
aea634d to
de4e0e9
Compare
Code Review by Gemini--- a/gitm/kernels/library.yaml
+++ b/gitm/kernels/library.yaml
@@ -109,7 +109,7 @@
value: 0.92 # offline/predict-only fallback when there's no live engine to scale
# A fraction near 1.0, not a count — the shared *0.5/*4 ladder would be
# invalid here, so a gentler one, clamped below 1.0.
- value_multiplier_grid: [0.95, 1.03, 1.10]
+ value_multiplier_grid: [0.95, 0.97]
value_max: 0.97
applies_to_kernels: *all_ops # raises the batch-slot ceiling for every op
expected_delta_mean: 0.04 |
🤖 Claude Code ReviewReview🐛 Bugs
if not isinstance(d, int | float) or d in (0, False):
return [] # zero/non-numeric default: no meaningful multiplicative grid
if knob.kind == "bool":
return [not bool(knob.default)]
if knob.kind in ("int", "float"):
if not isinstance(d, int | float) or d == 0: # don't conflate False
return []
op, values = max(by_op.items(), key=lambda kv: sum(kv[1]) / len(kv[1]))
# also the outcome when current swap_space is 0 (common
# default) — 0 * any multiplier stays 0, so the sweep collapses to this.This admits the dynamic sweep silently degrades to a single hardcoded value when
kind, choices = _knob_domain(f.type, domain)
if not any(weights):
return []
🔒 Security
⚡ Performance
self._catalog = ... {s.knob for s in load_library()}If
📊 Reproducibility
💡 Suggestions
apply_error=applied.error if applied else None,When
|
2 participants
Changes
Add
gitm.agents.autoresearchEngineArgsknob discovery and value-grid generationWire autoresearch into
gitm.scheduler.loopas Phase 4bautoresearch.jsonbottleneck_classandn_autoresearchAdd docs in
docs/autoresearch.mdAdd tests for autoresearch generation, targeting, gating, and loop integration