fix(loop): actually consume a runner's self-reported workload_id

[nicholaslawrence-hub]

Summary

• fix(vllm-decode): restore dropped engine wiring + return run in factory #60 added run.workload_id = "vllm-decode" to _vllm_decode_factory's returned runner, but nothing ever reads it — run_loop's only lookup, getattr(cfg.engine, "workload_id", None), checks the wrong object (cfg.engine, the raw engine handle) at the wrong time (before cfg.engine is even populated from the runner, several lines later). The attribute was dead code.
• Practical effect: a workload_runner passed without an explicit cfg.workload always fell back to the hardcoded "vllm-decode" label for trace/report purposes, regardless of what the runner actually represents.
• Fix: re-check the runner's own workload_id once it has actually resolved, only when the caller didn't pin one explicitly (cfg.workload still wins).

Test plan

• pytest tests/test_run_loop_workload.py -q — 10 passed, including 2 new regression tests (override case + explicit-cfg.workload-wins precedence case)
• Full suite green (excluding pre-existing pyarrow-dependent failures unrelated to this change)

🤖 Generated with Claude Code

[github-actions]

🤖 Claude Code Review

Review

🐛 Bugs

Logic error in the guard condition (loop.py, line 246)

if cfg.workload is None and runner is not None:
    workload = getattr(runner, "workload_id", None) or workload

The condition checks cfg.workload is None, but the local variable workload may have already been set to the "vllm-decode" default earlier in the function — making the guard ineffective. The comment describes this exact problem ("fell through to the 'vllm-decode' default") but the fix doesn't address it. If workload was already assigned "vllm-decode" and cfg.workload was never explicitly set, cfg.workload could still be None here, so the guard would fire — but this relies on the distinction between cfg.workload and the local workload being maintained consistently throughout the function. That's a fragile implicit contract worth making explicit.

workload reassignment may be silently dropped

The new value is written to the local workload variable, but it's not clear from the diff that this variable is actually what gets passed to capture(... workload_id=workload ...) later. If any intermediate path re-reads cfg.workload instead of the local workload, the fix silently does nothing. The tests catch the happy path, but not this aliasing risk.

Test: setting attribute on a plain function may be fragile

def runner():
    return {"events": 1}

runner.workload_id = "custom-workload"

This works in CPython but getattr(runner, "workload_id", None) on a lambda or a functools.partial will silently return None. If production code ever wraps the runner, the attribute is lost with no warning. Consider asserting hasattr(runner, "workload_id") before passing it in, or using a simple callable class in tests to make intent explicit.

💡 Suggestions

The fix should be accompanied by a guard on the local default assignment

The cleaner fix would be to track whether workload was explicitly configured vs. defaulted:

workload = cfg.workload  # None if not set
# ... resolve runner ...
if workload is None and runner is not None:
    workload = getattr(runner, "workload_id", None)
if workload is None:
    workload = "vllm-decode"  # default, applied last

This makes precedence unambiguous and matches the documented intent.

Missing test: runner with workload_id = None

Neither test covers runner.workload_id = None. With the current or workload fallback:

getattr(runner, "workload_id", None) or workload

None and "" both fall through to the previous workload value, which is correct, but this should be an explicit test case since it's a plausible real-world state (factory didn't populate the field).

Missing test: runner without workload_id attribute at all

Covers the getattr default path — also a realistic case when a non-factory runner is passed directly.

Duplicate fake_capture setup between the two new tests — worth extracting to a shared fixture to reduce maintenance surface.

[github-actions]

Code Review by Gemini

No bugs or issues were found in the provided code changes. The fix correctly addresses the problem described in the commit message, and the new tests adequately cover the intended behavior.

The logic for re-checking the runner's workload_id after resolution, and only when cfg.workload is not explicitly set, is sound. The placement of the new code block ensures that runner is available and that cfg.workload takes precedence.

No changes are suggested for the provided diff.

[nicholaslawrence-hub]

Addressed this round's review (commit e4ba577):

Verified false — the two "bugs" claimed:

• "Logic error in the guard condition... fragile implicit contract" — checked: `cfg.workload` (the dataclass field) is never reassigned anywhere in `run_loop`, grep-verified repo-wide. It's a clean, unambiguous "did the caller explicitly pin a workload" signal, distinct from the local `workload` variable that gets progressively resolved. No fragility exists in the current code.
• "workload reassignment may be silently dropped... if any intermediate path re-reads cfg.workload instead of local workload" — checked every use of `workload` in the function (grep); every downstream consumer, including the `capture(..., workload_id=workload, ...)` call, reads the local variable. No such path exists.
• Also declined the "cleaner fix" restructuring suggestion (deferring the "vllm-decode" default until after runner resolution) — it would break `get_factory(workload)`'s registry lookup, which needs a non-None value before the runner exists to pick a factory in the first place. The two-stage guess-then-relabel structure is required by that chicken-and-egg constraint, not an oversight.

Real, applied — the test-quality suggestions:

• Replaced the function+monkeypatched-attribute runner double with an explicit `_Runner` class (a plain function's ad-hoc attribute is fragile if ever wrapped, e.g. `functools.partial`).
• De-duplicated the `fake_capture` setup shared by the workload_id tests into one helper.
• Added the two missing cases: `workload_id=None` explicit (a real state a factory can leave unset) and a runner with no `workload_id` attribute at all (the `getattr(..., None)` default path) — both now assert the correct fallback-to-default behavior.

Gemini's review found nothing to change.

No real inline/resolvable PR review threads exist (`gh api .../pulls/61/comments` returns `[]`).

Test run: `tests/test_run_loop_workload.py` (12 passed) + full suite green (excluding pre-existing `pyarrow`-dependent failures, unrelated to this change).

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Review

🐛 Bugs

Condition mismatch in production code vs. test intent

# loop.py
if cfg.workload is None and runner is not None:
    workload = getattr(runner, "workload_id", None) or workload

The guard checks cfg.workload is None, but the comment says this fires when "a caller passed workload_runner directly with no cfg.workload". However, workload (the local variable) was set earlier from cfg.workload or a default — meaning when cfg.workload is None, workload is already the fallback string (e.g. "vllm-decode"). The or workload fallback is therefore always the hardcoded default, which is correct, but the branch only executes when cfg.workload is None. If cfg.workload was None but workload was somehow set to something else by intervening logic (realistic if workload gets mutated between assignment and here), the condition silently misses it. Consider guarding on workload == DEFAULT_WORKLOAD or using a distinct sentinel rather than relying on cfg.workload is None as a proxy for "workload was not user-supplied":

_WORKLOAD_UNSET = object()
# earlier: workload = cfg.workload if cfg.workload is not None else _WORKLOAD_UNSET
if workload is _WORKLOAD_UNSET and runner is not None:
    workload = getattr(runner, "workload_id", None) or "vllm-decode"

_Runner sentinel uses value equality, not identity

if workload_id != "unset":
    self.workload_id = workload_id

A caller passing the actual string "unset" as a workload ID would incorrectly suppress the attribute. Use a module-level _UNSET = object() sentinel instead:

_UNSET = object()

class _Runner:
    def __init__(self, workload_id=_UNSET):
        if workload_id is not _UNSET:
            self.workload_id = workload_id

💡 Suggestions

Test coverage gap: runner sets workload_id = ""

The empty-string case is a real-world footgun — getattr(runner, "workload_id", None) or workload silently falls back to the default for falsy values including "". This should either be tested and documented as intentional, or the logic changed to if x is not None:

# Current: "" → falls back to default (silent)
# Probably wanted: only None → falls back
runner_wid = getattr(runner, "workload_id", None)
if runner_wid is not None:
    workload = runner_wid

_capturing_fake_capture is not a context manager

The inner fake_capture uses @contextmanager but never yields inside a try/finally — if the body after yield were to raise, teardown is silently skipped. Low risk in tests but worth noting for copy-paste into any fixture used across more tests.

from gitm import optimize inside each test body

Importing inside the test body (after monkeypatch.setattr) is intentional here to avoid a stale module reference, but this is fragile — if optimize caches loop at import time the patch won't apply. A module-level comment explaining this would reduce future confusion.

No test for the case where runner is None

The if cfg.workload is None and runner is not None branch is untested for runner=None with cfg.workload=None — i.e., confirming the default still applies when there's nothing to read from. This is the simplest path and worth one explicit test.

[github-actions]

Code Review by Gemini

The changes effectively address the bug where a runner's self-reported workload_id was not being consumed, leading to incorrect trace labeling. The new logic correctly prioritizes an explicit cfg.workload but falls back to the runner's workload_id when available and cfg.workload is not set. The added tests provide good coverage for the intended behavior and edge cases.

No bugs or issues were found in the proposed changes. The code is clear, and the comments explain the rationale well.

--- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -232,20 +232,30 @@ def run_loop(cfg: LoopConfig) -> dict[str, Any]:
             runner_error = f"no workload runner registered for {workload!r}"
 
     # If the workload built a live engine (e.g. vLLM), expose it so the
     # scheduler-stats sampler AND the Phase-4 live A/B can drive it. The runner
     # carries it as ``.engine`` (see the vllm-decode factory). Without this the
     # loop stays predict-only (DryRunApplicator, live=False) — the engine is
     # built but never handed to the applicator.
     if cfg.engine is None and runner is not None:
         cfg.engine = getattr(runner, "engine", None)
 
+    # A factory-built runner may know its own workload id better than the
+    # guessed/default one above (e.g. a caller passed ``workload_runner``
+    # directly with no ``cfg.workload``, so ``workload`` fell through to the
+    # "vllm-decode" default regardless of what the runner actually is). Only
+    # the runner's own object carries this — earlier ``cfg.engine`` is never
+    # populated yet — so it must be re-checked here, after resolution, not
+    # folded into the guess above.
+    if cfg.workload is None and runner is not None:
+        workload = getattr(runner, "workload_id", None) or workload
+
     # Sample the engine scheduler (queue depth, batch occupancy, preemptions)
     # over the same window as the CUPTI capture — engine-level telemetry the GPU
     # trace can't see. A no-op when no engine is attached (empty series).
     with (
         capture(trace_path, workload_id=workload, run_id=run_id) as trace,
         sample_scheduler_stats(cfg.engine) as sched_stats,
     ):
         if runner is not None:
             try:
                 runner()

[github-actions]

🤖 Claude Code Review

Review

🐛 Bugs

Falsy workload_id values silently swallowed — documented as "by design" but likely wrong

workload = getattr(runner, "workload_id", None) or workload

The or short-circuits on any falsy value: "", 0, False. The test test_runner_workload_id_empty_string_falls_back_to_default explicitly asserts this and calls it intentional, but an empty string is a legitimately broken state that deserves a warning, not silent fallback. A runner that sets workload_id = "" due to a misconfigured factory will produce invisible mislabeled traces with no diagnostic signal. Prefer:

_wid = getattr(runner, "workload_id", None)
if _wid is not None:
    workload = _wid or workload  # still fall back on "", but at least warn
    if not _wid:
        warnings.warn(f"Runner {runner!r} has falsy workload_id {_wid!r}; using default '{workload}'")

Resolution order: cfg.engine populated after the new block, but comment says "before"

The comment says "earlier cfg.engine is never populated yet" to justify placement, but the block directly above this diff sets cfg.engine = getattr(runner, "engine", None). If a runner carries both engine (with its own workload_id) and a top-level workload_id, the resolution precedence is now: runner's workload_id wins over engine.workload_id. This may or may not be intended, but it's not documented and could silently change behaviour for engine-bearing runners.

---

💡 Suggestions

_capturing_fake_capture duplicated six times via monkeypatch — use a fixture

Every test repeats the same three-line setup block:

captured: dict = {}
monkeypatch.setattr(loop, "capture", _capturing_fake_capture(captured))
monkeypatch.setattr(loop, "sync_device", lambda: None)

This is a natural pytest fixture:

@pytest.fixture
def captured_loop(monkeypatch, tmp_path):
    import gitm.scheduler.loop as loop
    captured: dict = {}
    monkeypatch.setattr(loop, "capture", _capturing_fake_capture(captured))
    monkeypatch.setattr(loop, "sync_device", lambda: None)
    return captured, tmp_path

Reduces surface area for copy-paste drift (e.g. one test forgetting to patch sync_device).

test_no_runner_and_no_explicit_workload_uses_default patches get_factory — coupling risk

This test special-cases the absence of vLLM by patching get_factory. If run_loop ever gains a second factory lookup site, or the symbol is renamed, the test silently stops exercising the intended path. A _Runner(_UNSET) with runner=None forced via a config flag would be more robust, if the API allows it.

Missing test: runner with workload_id as a non-string type

The sentinel _UNSET = object() comment says "a real workload_id could legitimately be any string", but the code does getattr(...) or workload which would accept any truthy object (an int, a list, etc.) and pass it downstream as the workload label. A test asserting that _Runner(42) either raises or coerces cleanly would catch type contract violations early.

Import inside test functions — consider module-level or fixture-level

import gitm.scheduler.loop as loop  # repeated in every test body
from gitm import optimize

These should be at module level or in a fixture to avoid repeated import overhead and to make monkeypatching order unambiguous (monkeypatching before the from gitm import optimize means optimize is already bound — if optimize is a re-export that caches loop.capture at import time, the patch would arrive too late in some import orderings).

[github-actions]

Code Review by Gemini

The changes correctly address the issue of the runner's workload_id not being consumed, ensuring that the trace and report use the correct workload identifier when cfg.workload is not explicitly set. The new tests are comprehensive and cover various scenarios, including explicit overrides, fallbacks, and different states of the workload_id attribute.

One minor improvement can be made to the initial assignment of the workload variable to fully align with the problem description in the commit message ("checks the wrong object (cfg.engine, the raw engine handle) at the wrong time (before cfg.engine is even populated from the runner)"). While the new code correctly re-evaluates workload later, the initial assignment can be simplified to avoid attempting to read cfg.engine.workload_id before cfg.engine is potentially populated from the runner.

Suggested Change:

--- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -197,7 +197,7 @@
     # If the caller didn't specify a workload, guess "vllm-decode" (the most
     # common case for a factory-built runner) or read it from the engine if
     # it's already live.
-    workload = cfg.workload or getattr(cfg.engine, "workload_id", None) or "vllm-decode"
+    workload = cfg.workload or "vllm-decode"
 
     # If the caller passed a workload_runner directly, use it. Otherwise, look
     # up a factory by workload name.

[nicholaslawrence-hub]

Addressed this round's review (commit d6a4419):

Real, applied:

• Extracted the repeated 3-line `monkeypatch(capture)`/`monkeypatch(sync_device)` setup shared by all 6 workload_id tests into a `captured_workload_id` pytest fixture — removes the copy-paste-drift risk flagged (e.g. one test forgetting to patch `sync_device`).
• `test_no_runner_and_no_explicit_workload_uses_default` no longer monkeypatches `get_factory` to force `runner=None` — it now relies on the same "no vLLM installed in this test environment" fact `test_no_data_guard_does_not_fabricate_claims` already depends on, removing the coupling-to-implementation-detail risk that was flagged.
• Reworded the code comment on the workload_id re-check in `loop.py` — it previously said "cfg.engine is never populated yet" in a spot directly below the block that populates it, which read as contradictory. Now states plainly that `cfg.engine` is populated by that point, and explicitly notes why `cfg.engine.workload_id` isn't also consulted here (no current runner sets both an engine and a top-level `workload_id`, so there's no real precedence question today — just one preferred, most-specific source when it exists).

Verified false / declined:

• "Condition mismatch... `cfg.workload is None` used as an unreliable proxy" — same claim re-raised from the prior round; re-verified via grep that `cfg.workload` (the field) is never reassigned anywhere in `run_loop`, so it's an exact, not proxy, signal. Left the clarifying inline comment from last round in place (unchanged) since it already covers this.
• "Falsy `workload_id` values silently swallowed... likely wrong, should warn" — this codebase has no logging convention anywhere in `loop.py`; introducing one for a single low-probability misconfiguration case is scope creep beyond this bug's fix, and the fallback-on-any-falsy behavior is already deliberately tested (`test_runner_workload_id_empty_string_falls_back_to_default`). Not changed.
• Gemini's suggested simplification of the original early guess (`workload = cfg.workload or "vllm-decode"`, dropping the `cfg.engine.workload_id` read) — declined: `gitm/api.py`'s `optimize()` docstring documents `engine=` as a fully supported standalone entry point independent of `workload=` (the embedded-API path); a caller passing a pre-tagged `engine` with no `workload` relies on exactly that early read. Removing it would silently break that documented (if currently untested) path.
• "Missing test: non-string `workload_id`" — declined; no type contract exists on `workload_id` anywhere else in the codebase, so asserting one here would be inventing validation for a scenario that isn't demonstrated to occur.
• "Imports inside test function bodies" — not a new issue; matches this test file's existing convention in every pre-existing test, not something introduced by this PR.

No real inline/resolvable PR review threads exist (`gh api .../pulls/61/comments` returns `[]`).

Test run: `tests/test_run_loop_workload.py` (14 passed) + full suite green (excluding pre-existing `pyarrow`-dependent failures, unrelated to this change).

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Review

🐛 Bugs

Falsy string suppression is undocumented behaviour, not a bug, but the implementation is fragile

workload = getattr(runner, "workload_id", None) or workload

The or operator silently drops any falsy workload_id (empty string, 0, False). The test test_runner_workload_id_empty_string_falls_back_to_default explicitly documents this as "by design," but a runner setting workload_id = "" to signal "no preference" versus workload_id = None is an indistinguishable accident. Prefer an explicit sentinel check:

_id = getattr(runner, "workload_id", None)
if _id is not None:
    workload = _id or workload  # still skip empty if you want, but intentionally

More defensively: if _id is not None and _id != "": workload = _id.

cfg.engine assignment and runner.workload_id read are order-dependent but not obviously so

The new block reads runner.workload_id after cfg.engine = getattr(runner, "engine", None), which is correct. But both blocks share the same if runner is not None guard with no assertion that the runner hasn't been mutated between the two lines. If anything in between modifies the runner (e.g., a lazy-init __getattr__), workload_id could be inconsistent with engine. Low probability, but worth a comment noting the ordering assumption.

---

💡 Suggestions

captured_workload_id fixture imports inside the fixture body

import gitm.scheduler.loop as loop

Importing inside a fixture means import errors surface at test collection as cryptic fixture failures rather than clear import errors. Move to module level or at minimum add a comment explaining why it's deferred (e.g., to allow monkeypatching to take effect first — though monkeypatch doesn't require that here).

fake_capture silently ignores fingerprint and run_id inconsistencies

The fixture captures workload_id but discards fingerprint. If a future bug causes fingerprint to be derived from workload_id incorrectly, these tests won't catch it. Consider asserting fingerprint is non-empty at minimum.

_Runner.__call__ return value is never validated in any test

def __call__(self) -> dict:
    return {"events": 1}

No test asserts that the runner's return value influences any output. If run_loop silently drops it on type mismatch, these tests won't catch the regression. A single assertion on a field downstream of the runner output would close this gap.

Test for runner=None path (test_no_runner_and_no_explicit_workload_uses_default) relies on an environment assumption

Relies on the same "no vLLM installed" test-environment fact

This is a hidden environment dependency. If vLLM ever becomes available in CI (e.g., via a transitive dependency), this test silently changes behaviour. Use an explicit monkeypatch of get_factory with a comment explaining the tradeoff, or mark with pytest.mark.skipif.

Missing pytest.mark for the six new tests

If these are meant to be fast/unit tests, adding @pytest.mark.unit (or equivalent) would let CI run them selectively without invoking the full GPU integration suite. The existing test_no_data_guard_does_not_fabricate_claims doesn't have it either, but this is a natural point to establish the convention since you're adding import pytest anyway.

Comment verbosity in production code

The 12-line comment block in loop.py is thorough but explains implementation archaeology rather than the invariant. Consider condensing to 3–4 lines and moving the extended rationale to the linked issue (#60) or a CHANGELOG entry. Source comments should state what and why, not what we tried before.

[github-actions]

Code Review by Gemini

No bugs or critical issues were found in the provided code changes. The fix correctly addresses the problem described in the commit message, and the new tests provide comprehensive coverage for the intended behavior and edge cases.

The logic for re-evaluating the workload variable based on the runner's workload_id is correctly implemented, respecting the precedence of an explicitly set cfg.workload. The comments in loop.py clearly explain the rationale behind the change and the design decisions. The new test cases in test_run_loop_workload.py effectively validate the fix and its interactions with various workload_id states (unset, None, empty string, explicit value) and explicit cfg.workload settings.