Repository navigation
fix(adapters): make the declared LLMClient interface sufficient to run - #235
Merged
Merged
Conversation
LLMClient declares three abstract methods, but a client implementing exactly those could not complete a single row. Two more were required and neither was declared: * the invocation stage calls start()/stop() unconditionally, through a `type: ignore[attr-defined]` — the code admitting the attribute was not on the interface. A client without them died at the first row with AttributeError. * the default executor is async, so ainvoke() is what a run actually calls. Without it every row was skipped, and the run reported a provider problem rather than a missing method. Both now have defaults on the base class: start/stop no-op, and the async methods delegate to their sync counterparts. Implement what is abstract and it runs; override the async ones for real concurrency, since delegating serialises requests. examples/13_custom_client.py had never run. It failed at instantiation on the missing structured_invoke, so the injection bug fixed in #230 was not even reachable — nothing executes examples in CI. It now implements the full interface and completes. Adds docs/guides/custom-clients.md and a SUMMARY entry. The API had no documentation at all: its only public presence was that example, so the one discoverable path to it demonstrated something that could not work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ptimizeroracle
pushed a commit
that referenced
this pull request
Aug 11, 2026
Every number we would print in a README or a launch post is now a test that
fails when the number stops being true. The three added here are the ones
worth publishing:
orchestration overhead 28.7k rows/s measured; floor asserted at 5k, which
still means 5M rows in under 17 minutes
streaming bounds memory asserted as the property rather than the megabytes:
the streamed run peaks below the one that loads
everything (784 MB vs 2,477 MB at 1M on a laptop)
scheduling efficiency 84-95% of the provider-bound floor on four real
providers, reproduced deterministically with a 40ms
stub and asserted at 70%
Thresholds sit well below laptop measurements on purpose. CI runners are
slower and shared, and a flaky performance test teaches everyone to ignore
red — these are floors under a claim, not records.
Mutation-verified the one that matters: a client that does not override
ainvoke falls back to the blocking sync path, keeps working, and silently
loses all concurrency — the #235 regression class. The efficiency test reports
4% against a 70% floor and fails.
Why this file rather than a benchmark script: a script proves a number once,
on one machine, on one day. This proves it on every pull request. The
distinction matters because the two claims I got wrong this week — a router
"3.5x slower" and models reordering batches "routinely" — were both
assertions with nothing executing them, which is the same defect this project
keeps finding in its own code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ptimizeroracle
added a commit
that referenced
this pull request
Aug 11, 2026
…ignment (#256) * test(structured): cover the structured-output path, and pin its misalignment The conformance suite covered batching, concurrency, streaming, resume, cost and failure reporting — but not structured output, which is a different code path: the model returns a Pydantic object and the disaggregator maps its items onto rows without going through JsonBatchStrategy at all. Nothing there was checkable by the existing tests, because shape assertions cannot see it. Six rows of well-formed objects look identical whether or not they landed on the right rows. The fake's answers now carry the marker of the row that asked for them, which makes the mapping visible. That immediately found one: a model returning its batch items in a different order silently gives rows each other's answers. marker token category row-0000 row-0002 cat-row-0002 <- row 0 holds row 2's answer row-0001 row-0001 cat-row-0001 row-0002 row-0000 cat-row-0000 MISALIGNED: 4/6 failed=0 skipped=0 success=True The plain JSON batch path survives the identical reordering, because its items carry ids and the parser sorts by them. Structured output maps items[i] to row i and trusts the order. Recorded as xfail(strict=True) rather than left out: the test states the behaviour ondine should have, and CI will report the moment it starts passing. See #255 for the fix options. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(structured): mark the reordering gap as latent, not observed The xfail reason claimed models reorder batch responses routinely. That was an assertion, not a measurement, and the measurements contradict it: gpt-4o-mini preserved order across 140 real rows (40 at batch 10 with an explicit echo instruction, 100 at batch 25 with duplicated items and no ordering hint), with zero misassignment in both. The asymmetry the test guards is still real — the structured path trusts position while the JSON batch path verifies ids — but it defends a failure that has not been reproduced against a real provider, and the reason text now says so. * test(verification): put the published throughput claims under test Every number we would print in a README or a launch post is now a test that fails when the number stops being true. The three added here are the ones worth publishing: orchestration overhead 28.7k rows/s measured; floor asserted at 5k, which still means 5M rows in under 17 minutes streaming bounds memory asserted as the property rather than the megabytes: the streamed run peaks below the one that loads everything (784 MB vs 2,477 MB at 1M on a laptop) scheduling efficiency 84-95% of the provider-bound floor on four real providers, reproduced deterministically with a 40ms stub and asserted at 70% Thresholds sit well below laptop measurements on purpose. CI runners are slower and shared, and a flaky performance test teaches everyone to ignore red — these are floors under a claim, not records. Mutation-verified the one that matters: a client that does not override ainvoke falls back to the blocking sync path, keeps working, and silently loses all concurrency — the #235 regression class. The efficiency test reports 4% against a 70% floor and fails. Why this file rather than a benchmark script: a script proves a number once, on one machine, on one day. This proves it on every pull request. The distinction matters because the two claims I got wrong this week — a router "3.5x slower" and models reordering batches "routinely" — were both assertions with nothing executing them, which is the same defect this project keeps finding in its own code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: ptimizeroracle <git@binblok.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #230. Fixing that bug made
with_custom_llm_client()reach the pipeline — and revealed that a client implementing the declared interface still could not complete a single row.Two undeclared requirements
LLMClientdeclares three abstract methods. Two more were required, and neither was stated:start()/stop()— the invocation stage calls them unconditionally, through a# type: ignore[attr-defined]. That comment is the code admitting the attribute is not on the interface. A client without them died at the first row:ainvoke()— the default executor is async, so this is what a run actually calls, notinvoke(). Without it every row was skipped and the run reported "Check the model name, credentials, and provider" — a provider problem, for a missing method.Neither the ABC nor mypy could say so. You found out at runtime, with a misleading message.
Fix
Defaults on the base class:
start/stopno-op, and the async methods delegate to their sync counterparts. Implement what is abstract and it runs.Delegating serialises requests — the sync call blocks the event loop — so the docstring says to override
ainvoke()with a real async client for concurrency. Correct by default, fast when you need it.The example had never run
examples/13_custom_client.pyfailed at instantiation on the missingstructured_invoke, so the #230 injection bug was not even reachable through it. Nothing executes examples in CI, so it sat broken.Documentation
docs/guides/custom-clients.md— the API had no documentation at all. Its only public presence was that example, so the single discoverable path to the feature demonstrated something that could not work.The guide covers the interface, the async method that actually runs, optional lifecycle hooks, what you get for free (batching, concurrency, retries, cost tracking, checkpointing), the shared-instance guarantee from #230, and a stub-for-tests recipe. Added to
SUMMARY.mdso it appears in GitBook nav.Test
test_client_with_only_abstract_methods_runs— a client implementing exactly the three abstract methods runs a pipeline end to end. It fails when either default is removed.That is the contract worth pinning: the interface says what it needs, and needing it is enough.
Full suite: 1165 passing, mypy clean.
ai assistance: i directed this work with help from claude code.