Skip to content

feat: build one client per rollout, share renderers process-wide - #2218

Merged
mikasenghaas merged 27 commits into
mainfrom
feat/per-rollout-clients
Aug 3, 2026
Merged

feat: build one client per rollout, share renderers process-wide#2218
mikasenghaas merged 27 commits into
mainfrom
feat/per-rollout-clients

Conversation

@mikasenghaas

@mikasenghaas mikasenghaas commented Jul 31, 2026

Copy link
Copy Markdown
Member

Summary

Supersedes #2210. The rollout is now the client's owner: ModelContext.client carries the endpoint config, and every rollout builds, uses, and closes its own Client. This removes the 128-connection ceiling #2210 measured — in-flight capacity scales with rollout count structurally instead of against one shared pool — and fixes the training side's mirror-image problem at the same time.

  • One Client per rollout. Rollout resolves its client in __init__ and closes it in close()/abort(); the interception server calls session.client. No rollout shares transport, connection state, or retries with another.
  • Elastic renderer pool. The train client's tokenizers come from a process-wide ElasticRendererPool (the renderer-side counterpart to ElasticInterceptionPool): one warmed when a client is built, one more grown per multiplex concurrent rollouts (TrainClientConfig.multiplex, default 256 — a render is milliseconds against a multi-second turn, so one tokenizer absorbs many rollouts). Renderers carry no rollout state, so pools are shared by build inputs (model, renderer config, template kwargs) while transports are not.
  • Uniform transport, no client-side retries. Both clients share the OpenAI SDK's own defaults (DEFAULT_TIMEOUT, DEFAULT_LIMITS, MAX_RETRIES = 0 in clients.base), so a rollout behaves the same whether its turns are relayed or rendered — and a failed call surfaces to the harness SDK and the trace instead of being silently reattempted.
  • Cache and mutation bugs removed. EnvServer._clients (a duplicate renderer pool per distinct model string, per worker, with no eviction) and Env._agent_clients are deleted. claude_code no longer mutates ctx.client.base_url — a shared-client corruption affecting every other rollout on the endpoint; clients.base.join_url() joins upstream URLs without duplicating the API version segment instead (probed live: pinference serves Anthropic messages at /api/v1/messages, 404s /api/v1/v1/messages).

Scalability

Measured end to end on gsm8k-v1 against a local Qwen/Qwen3-4B-Instruct-2507 (uv run inference, dp=2 on 2× RTX PRO 6000, 4096 ctx). One wave per run — -n 1 -r C -c C — so every rollout starts at once and each run pays its own cold start; that shape is what exposes per-rollout construction costs. All runs 100% ok, zero errors.

Wall time — the per-rollout eval client scales cleanly, and the train client now matches it:

concurrency eval client train (fixed 8-slot pool) train (elastic pool)
512 38.5 s 45.2 s 39.1 s
1024 70.9 s 73.9 s 72.6 s
2048 134.7 s 139.7 s 134.6 s

Resource footprint is flat in concurrency. From 512 to 2048 concurrent rollouts, peak fds (~800–1050), sockets (~340–450), and threads (~300–350) stay in the same band, with zero syn-sent backlog — per-rollout clients are not the bottleneck at any tested level. Harness subprocess spawn, not the client, gates how many calls are in flight.

The fixed pool's cost was a cold-start convoy, and the elastic pool eliminates it. The old pool was built lazily behind a threading.Lock with one asyncio.to_thread per caller, so every rollout arriving before the build queued on the default executor — the same one episode persistence uses. Bucketing model calls by rollout start order at 2048 concurrency:

q1 q2 q3 q4 calls >20s
fixed 8-slot pool 23.43 s 5.09 4.00 3.56 288
elastic pool 6.64 s 3.78 3.69 3.58 0

Peak RSS drops with it (2169 → 1441 MB at c=512, 3303 → 2551 MB at c=2048): the elastic pool only ever grew to 1–2 tokenizers where the fixed pool always paid for 8.

Breaking

  • TrainClientConfig.pool_sizemultiplex, with inverted meaning: pool_size was "how many renderers", multiplex is "concurrent rollouts per renderer". Carrying an old value across silently changes behaviour (pool_size = 8multiplex = 8 is a tokenizer per 8 rollouts). Drop it to take the default, or set it to concurrency / desired_renderers.
  • Agent(config, client=...) / make_agent(config, client=...) are removed — put the endpoint on AgentConfig.client instead. An agent no longer owns or closes a client, so async with agent governs only its interception server.
  • Harness hooks see a config-valued ctx.client: ModelContext.client is a ClientConfig, not a live Client. A custom harness that read (or mutated) the live client must use the config fields; a live client is resolve_client(ctx.client), or the rollout's own at RolloutSession.client.

Verification

uv run ruff check and ruff format --check pass on the touched files. Directly exercised: two clients from one config are distinct objects; the Anthropic path resolves to /api/v1/messages while chat stays /api/v1/chat/completions and the count_tokens aux route stays correct; a bare-origin base (https://api.anthropic.com, the endpoints.toml shape) keeps the dialect's /v1; the dedup is gated to version-shaped segments so a base ending in /chat doesn't swallow /chat/completions.

Elastic pool behaviour, directly asserted: shared() returns without blocking while the first renderer warms in the background; 10 concurrent acquires at multiplex=4 grow to exactly 3 renderers with loads [4, 4, 2]; all release to 0; further acquires reuse slots without growing; configs differing only in multiplex share one pool (first client's policy wins); an acquired renderer renders.

🤖 Generated with Claude Code

Note

Build one client per rollout and share renderers process-wide via an elastic pool

  • ModelContext.client now holds a ClientConfig (endpoint config) instead of a live Client; each Rollout calls resolve_client() at construction and closes the client on teardown.
  • Introduces ElasticRendererPool in train.py: renderer instances are keyed by model/config and shared across concurrent rollouts up to a configurable multiplex limit, replacing the old pool_size field on TrainClientConfig.
  • Shared client creation and caching is removed from Agent, Env, EnvServer, and the eval/GEPA runners; all now pass endpoint config and defer client construction to rollout time.
  • EvalClient and TrainClient are refactored to accept BaseClientConfig/TrainClientConfig directly, using build_async_openai() in base.py for explicit timeouts, connection limits, and URL joining.
  • Behavioral Change: TrainClientConfig.pool_size is replaced by multiplex (default 256); existing configs using pool_size will break.

Changes since #2218 opened

  • Removed documentation string from the client field of the RolloutSession dataclass [01f3a3e]
  • Added asynchronous cleanup logic to close all cached clients when server run loop exits [c7c19d2]

Macroscope summarized b6b0e0c.


Note

High Risk
Breaking changes to agent construction and ModelContext.client typing affect all rollouts and custom harnesses; train configs must migrate pool_size to multiplex with inverted semantics.

Overview
Model transport is owned by each rollout, not by agents or eval workers. ModelContext.client is now endpoint config (ClientConfig); Rollout calls resolve_client() at construction and closes the client in close()/abort(). The interception path uses RolloutSession.client instead of a shared live client on the context.

Breaking API: make_agent / Agent no longer accept client= — set AgentConfig.client. Eval, GEPA, and env-server paths stop resolving and caching clients (Env._agent_clients, EnvServer._clients, eval-runner client.close()).

Eval/train clients take config objects directly; shared httpx/OpenAI defaults and join_url() live in clients/base.py. EvalClient uses explicit timeouts/limits and version-segment URL joining (replacing the claude_code harness mutating base_url).

Training: TrainClientConfig.pool_size becomes multiplex (rollouts per renderer, default 256). ElasticRendererPool shares renderers process-wide with threaded encode locking; TrainClient acquires a slot per turn.

Docs/tests updated for config-on-agent and per-rollout clients.

Reviewed by Cursor Bugbot for commit c7c19d2. Bugbot is set up for automated code reviews on this repo. Configure here.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread verifiers/v1/configs/client.py
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread verifiers/v1/clients/train.py
Conflict in verifiers/v1/rollout.py: main renamed RolloutRun -> Rollout and
made RolloutSession's limits argument required; this branch had added the
per-rollout client as the session's second positional argument. Kept both.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread verifiers/v1/clients/eval.py Outdated
Comment thread verifiers/v1/clients/train.py Outdated
Comment thread verifiers/v1/env.py Outdated
hallerite
hallerite previously approved these changes Jul 31, 2026

@hallerite hallerite left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments

@hallerite
hallerite self-requested a review July 31, 2026 23:07
The fixed 8-tokenizer pool was built lazily behind a threading lock, one
asyncio.to_thread per caller: every rollout arriving before the build queued a
task on the default executor and blocked there. At 2048 concurrent rollouts the
first quartile of model calls averaged 23.4s against 3.6s for the rest, and the
convoy starved the same executor episode writes use.

ElasticRendererPool warms one tokenizer when a client is built and grows one per
`multiplex` concurrent rollouts, mirroring ElasticInterceptionPool. Each slot is
a size=1 RendererPool, so the lock that makes concurrent renders safe (and the
thread offload) comes from renderers itself.

The v0 bridge keeps its own static pool at the v0 default of 1, as on main.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread verifiers/v1/clients/train.py Outdated
Comment thread verifiers/v1/clients/train.py Outdated
mikasenghaas and others added 8 commits August 1, 2026 01:59
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The join stripped any leading path segment the base happened to end with, so a
base ending in `/chat` would swallow `/chat/completions`. The invariant is
narrower — don't repeat the API version — so gate the dedup on a `v\d+` segment.
Verified live: pinference serves Anthropic messages at `/api/v1/messages` and
404s `/api/v1/v1/messages`; bare-origin bases (`https://api.anthropic.com`)
join unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lient

Timeouts, limits, and the retry policy are client behavior, not endpoint
schema; configs.client keeps only the config classes and API-key resolution.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`multiplex` is scaling policy, not a build input — keying the shared renderer
pool on it would duplicate a model's tokenizers for configs that differ only
there; the first client's value wins instead. Also restores one-line docstrings
on ModelContext and RolloutSession.client.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread verifiers/v1/clients/train.py Outdated
mikasenghaas and others added 3 commits August 1, 2026 02:58
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pool outlives event loops (process-wide _shared), but its warm task is
loop-bound: an asyncio.run() that builds a TrainClient and exits before any
turn leaves the task cancelled. acquire()'s suppress(Exception) doesn't catch
CancelledError and skipped clearing _warm_task, so every later acquire on a
fresh loop re-raised forever. Distinguish the two cancellations: a cancelled
warm task falls through to rebuilding under the lock; a cancelled acquire
still re-raises its own cancellation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread verifiers/v1/clients/train.py Outdated
mikasenghaas and others added 4 commits August 1, 2026 03:10
The endpoint is config, not a borrowed resource — its example was stranded
under Borrowed Resources after the client= parameter went away.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mikasenghaas and others added 5 commits August 3, 2026 09:25
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread verifiers/v1/clients/train.py
mikasenghaas and others added 2 commits August 3, 2026 18:12
Conflicts:
- tests/v1/conftest.py: keep the config-valued ModelContext.client, take
  main's CI model bump (deepseek-v4-flash-0731)
- verifiers/v1/env.py: keep main's deep-merged agent sampling (#2226) plus
  this branch's per-agent client fallback

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mikasenghaas
mikasenghaas marked this pull request as ready for review August 3, 2026 18:15
@mikasenghaas
mikasenghaas requested a review from stu-cao August 3, 2026 18:16

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 01f3a3e. Configure here.

Comment thread verifiers/v1/serve/server.py
@macroscopeapp

macroscopeapp Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces significant architectural changes including a new process-wide ElasticRendererPool with shared state, new concurrency primitives (threading and asyncio locks), and fundamentally different client lifecycle management. The complexity of these new abstractions and their runtime behavior changes warrant human review.

You can customize Macroscope's approvability policy. Learn more.

EnvServer.run() used to close the base class's client cache; the cache
moved to LegacyEnvServer, so the close moves with it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
"""Renders prompts to token ids and calls a vLLM `/inference/v1/generate` engine."""
@dataclass
class RendererSlot:
"""One renderer and the rollouts currently holding it. Encoding mutates a fast

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like it should be handled at the renderer level

sampling_params.update(raw_sampling)
renderer = self._renderer_pool(
model,
pool = ElasticRendererPool(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, it's because you introduce the ElasticRendererPool here in verifiers, but perhaps it would still make sense to offload some of the code into renderers proper?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, i think elastic scaling of renderers seems like a vf concern. renderers imo should mainly provide the Renderer and RendererConfig

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fair

@hallerite hallerite left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments, but overall lgtm

@mikasenghaas
mikasenghaas merged commit 576506d into main Aug 3, 2026
13 checks passed
@hallerite
hallerite deleted the feat/per-rollout-clients branch August 3, 2026 19:34
eligotts added a commit that referenced this pull request Aug 4, 2026
…fload

Reconciled train.py with main's process-shared renderer pool (#2218):
adopted RendererSlot/ElasticRendererPool and the slot.run call structure
(which subsumes this branch's _maybe_offload thread-hop), dropped main's
multimodal bridging gate (raw refs make mm bridging safe — that is this
PR's feature), and re-applied previous_multi_modal_data bridge kwargs
inside the pooled bridge closure.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
eligotts added a commit that referenced this pull request Aug 4, 2026
…ages

Reconciled with main's renderer pool (#2218) the same way as the offload
branch: adopted RendererSlot/ElasticRendererPool and slot.run, dropped
the multimodal bridging gate (this PR's feature), re-applied
previous_multi_modal_data bridge kwargs. graph.py keeps the raw-mm
sidecar validators over main's BaseModel MessageNode; trace.py takes the
already-reconciled shape (main upstreamed the sidecar EXCLUDE_FIELDS).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mikasenghaas added a commit that referenced this pull request Aug 6, 2026
Since #2218 each rollout built and closed its own httpx client, so a
wide run churns TCP connections at the rollout rate — the load pattern
that wedges a hyper-based vllm-router. Move client ownership to the
interception server: one client per distinct endpoint config, assigned
to each session at register and closed with the server. Rollouts
multiplexed onto a server (multiplex, default 32) now share one bounded
keepalive pool, so connections are reused warm instead of reopened per
rollout — shared resources without unbounded fan-in.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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