Skip to content

refactor(lm): define minimal engine protocols - #10709

Open
isaacbmiller wants to merge 2 commits into
mainfrom
isaac/lm15-engine-contract-prep
Open

isaacbmiller wants to merge 2 commits into
mainfrom
isaac/lm15-engine-contract-prep

Conversation

@isaacbmiller

@isaacbmiller isaacbmiller commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

1/6 — Make completion the minimum engine contract

Stack: #10709 → #10712 → #10713 → #10652 → #10708 → #10719.
Review this diff against main; later PRs are reviewed against their immediate parent. Nothing is merged.

What changes

  • Engine requires only synchronous complete(Request) -> Response; AsyncEngine requires asynchronous complete. Streaming and cleanup remain optional capabilities, documented beside the protocols.
  • Annotate LM's engine and async_engine constructor parameters with these protocols.
  • A separate test-only commit serializes the custom-proposer GEPA fixture: its shared scripted LM must consume lookup/submit pairs per example. The exact CI failure was reproduced on unchanged main and this PR by controlling worker scheduling; no assertion or production behavior changes.
LM(..., engine=..., async_engine=...)       TYPE ANNOTATIONS CHANGE
  _check_engines                           IMPLEMENTATION UNCHANGED
    named selection → validate name, reject async counterpart
    custom engine → validate callable and sync/async method kind

LM.copy(...) → the same existing validation

Inference                                 UNCHANGED
  BaseLM.__call__ / acall
    dual-contract execution
      native lm15 / LiteLLM / custom engine

What does not change

No request construction, adapter behavior, candidate execution, caching, provider routing, dependencies, or inference changes. messages=, forward/aforward, legacy engines and historical cache readers remain. Custom engines remain borrowed; DSPy does not acquire an obligation to close them.

The protocols are not runtime_checkable: runtime construction still checks callability and coroutine kind, which a structural isinstance check cannot establish. PR4 adds separate runtime-checkable streaming protocols and the missing-stream guard; neither is backported here. Async-only custom LMs also arrive in PR4.

How to review

  1. Read dspy/clients/engines/base.py: only complete is required.
  2. Read the two constructor annotations in dspy/clients/lm.py.
  3. Confirm _check_engines and _is_coroutine_function are unchanged from the parent.

The contract simplification reduced 85 to 16 changed lines (18.82%), without changing its tests or using code golf. The independent CI fixture correction adds three changed lines: the current whole PR is 19 lines (22.35%), not under 20%. The earlier speculative validate_engines extraction was removed. Shared validation moves only in PR4, where BaseLM and LM actually both consume it; this reduction does not claim that the entire stack shrank by 81%.

Exact-head verification

On bf18778d6: 306 focused engine/compatibility/execution/error/lifecycle tests passed; Ruff, ty check --python /home/user/workspace/repo/.venv/bin/python, and git diff --check passed. Tests ran with a fresh HOME, env -i, local model metadata and PYTHON_DOTENV_DISABLED=1. The simplification also matched 1,875 original constructor/copy/validator outcomes in adversarial probes.

On 33df97cee: 117 GEPA/engine/execution tests passed, plus Ruff and git diff --check. The controlled scheduling probe reproduced the original 416-character failing Context on both main and PR1; serializing just the custom-proposer fixture passed without weakening assertions. Exact-head GitHub CI passes and Greptile reports 5/5.

GitHub checks and the latest reviewed commit identify current CI and Greptile coverage. Older 5/5 reviews do not cover this new head automatically. The entire stack remains gated on source fixes and compatibility acceptance tracked in CMP-1607.

Maintainer-directed, Amp-assisted preparation for Maxime Rivest's #10401 migration. This is our minimal contract-preparation layer, not a claim that Max's original PR used this split.

@greptile-apps

greptile-apps Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] No merge-blocking issues identified in the reviewed changes.

Summary

Makes complete the only required method on Engine and AsyncEngine, and adds these protocol types to the LM constructor. Streaming and cleanup remain optional, and runtime engine validation is unchanged.

Since the previous review, only the custom-proposer test changed: it now uses one thread so the shared scripted LM receives lookup and submit calls consecutively for each example. The current head builds on the previously reviewed commit, and both share the same merge base with the target branch.

No actionable issues found. No behavioral tests were run for this review.

  • Custom engines only need a completion method to meet the protocol.
  • The custom-proposer GEPA test runs its scripted calls in order.

Reviews (5) · Last reviewed commit: "Stabilize scripted GEPA context fixture ..." · Reviewed by Greptile

Require only complete(Request) on completion engines. Streaming and cleanup are optional capabilities. Annotate public constructor parameters without moving or changing existing validation.

Amp-Thread-ID: https://ampcode.com/threads/T-01a135b1-7d59-73cc-a17c-57b7b3454c03
Co-authored-by: Isaac Miller <isaac@cmpnd.ai>
@isaacbmiller
isaacbmiller force-pushed the isaac/lm15-engine-contract-prep branch from 0224a3a to bf18778 Compare October 28, 2026 00:52
@isaacbmiller isaacbmiller changed the title refactor(lm): define minimal engine protocols and share validation refactor(lm): define minimal engine protocols Oct 28, 2026
Keep the shared lookup/submit script on one evaluation thread. A legal
interleaving reproduced the CI failure on both main and this stack;
all existing context assertions remain unchanged.

Amp-Thread-ID: https://ampcode.com/threads/T-01a135b1-7d59-73cc-a17c-57b7b3454c03
Co-authored-by: Isaac Miller <isaac@cmpnd.ai>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants