Repository navigation
Replace monkey-patching of train._make_llm_client with dependency injection #64
Copy link
Copy link
Closed
Description
Context
PR #62 wires clawloop run --dry-run by mutating module-level state in clawloop/train.py:
# clawloop/cli.py:_install_dry_run_clients
_train._make_llm_client = _mock_make
_train.ENV_BUILDERS[env_type] = _stub_builderThis works for the CLI's one-shot process model but creates implicit contracts that future refactors can silently break:
- Every dry-run caller depends on
_make_llm_clientbeing a module-level reassignable function. Iftrain.pyever inlines it, moves it into a class, or wraps it in a decorator, dry-run silently stops mocking. ENV_BUILDERSis mutated globally, so the modification persists for the lifetime of the process. Two dry-run runs in the same process would patch on top of each other.- The next "intercept this" feature (judge-LLM mocking, deterministic seeds, etc.) will follow this monkey-patch precedent and compound the problem.
Proposal
Replace the monkey-patching with explicit dependency injection through train():
def train(
config: TrainConfig,
*,
make_llm_client: Callable[[LLMClientConfig], LLMClient] = _make_llm_client,
env_builders: dict[str, EnvBuilder] = ENV_BUILDERS,
) -> tuple[AgentState, StateID]:
...Each _build_* helper takes make_llm_client as a parameter instead of importing it from module scope. The CLI's cmd_run builds a mock factory and an overridden builder dict, then calls train(config, make_llm_client=mock_factory, env_builders=stub_builders).
Benefits:
- No global mutation; multiple parallel
train()calls are safe - The injection points are documented in the
train()signature — discoverable via type hints / docs - Tests that already do
with patch(\"clawloop.train._make_llm_client\") as mock_make(tests/test_train_config.py::TestTrainEndToEnd) can switch to passing the mock directly - Future intercepts (judge LLM, seeds, sampling client) follow the same pattern: add a kwarg, default to the production thing, document
Scope
- Modifies the public signature of
train(). All current call sites (examples/train_runner.py,clawloop/cli.py:cmd_run,tests/test_train_config.py::TestTrainEndToEnd) need updating. - Each
_build_*helper inclawloop/train.py(~6 functions) takes the factory as a parameter. - Deletes
_install_dry_run_clients's monkey-patching block; the CLI builds factories and passes them in. _StubAdapterstays as-is;ENVS_USING_MAKE_LLM_CLIENTbecomes a property of how the CLI builds the override dict.
Acceptance
-
train()acceptsmake_llm_clientandenv_buildersas keyword-only parameters with sensible defaults - No code outside
train.pymutatestrain._make_llm_clientortrain.ENV_BUILDERS -
test_dry_run_role_*and existing TestTrainEndToEnd patterns work withoutunittest.mock.patch - All existing
--dry-runsmoke tests still pass -
examples/train_runner.py(the deprecated shim) keeps working unchanged via the CLI passthrough
Related
- PR fix: address PR #60 review comments on clawloop run --dry-run + taubench #62 (this comment was raised in self-review using the A Philosophy of Software Design framework: principle 6, strategic vs tactical programming)
Metadata
Metadata
Assignees
Labels
No labels
Activity
refactor: replace monkey-patching with dependency injection — closes a…