refactor(v1)!: make serving its own config block - #2159
Conversation
`EnvServerConfig` was three things at once — the env, how it's served, and the v0
bridge — and eval/serve/prime-rl inherited it, so pool knobs sat flattened on an
eval config and a trainer's env entry. Three blocks now, composed rather than
inherited:
[env] what runs (unchanged)
[serve] how it's hosted pool, address, per-worker episode bound
[legacy] the v0 bridge id, args, extra_env_kwargs
A run config declares the blocks it needs (`EvalConfig`, the serve CLI, a
trainer's source entry) plus its own fields, and shares only the narrowing
helpers — `GEPAConfig` already worked this way, with no `[serve]` at all since it
runs in-process.
The run keeps owning its concurrency: `-c` bounds episodes in flight and seeds
each worker's bound under `--server`, while `serve.max_concurrent` pins a worker
below it when set (`EvalConfig.worker_max_concurrent`).
BREAKING: `--pool.*` → `--serve.pool.*`, `--address` → `--serve.address`,
`--id`/`--args`/`--extra-env-kwargs` → `--legacy.*`. Each retired top-level key
raises with a pointer home instead of a bare `extra_forbidden`, the way the
retired `--taskset.*`/`--harness.*` axes already do.
| def refuse_mixed_run(env: EnvConfig, legacy: LegacyEnvConfig) -> None: | ||
| """Refuse a v0 id next to a v1 taskset: `is_legacy` would be False and the v0 env | ||
| would never load, so the id would sit there silently inert.""" | ||
| if legacy.id is not None and env.taskset.id: |
There was a problem hiding this comment.
🟡 Medium configs/legacy.py:41
refuse_mixed_run rejects a v0 legacy.id alongside a v1 env.taskset.id but does not reject a v0 legacy.id alongside a nonempty v1 env.id. In that case, is_legacy returns True (legacy id set, no taskset), so the server runs the v0 env, but run_env_id returns env.env_id — preferring the v1 id. The run silently executes one environment while labeling and saving results under a different env's id. Consider also rejecting a nonempty env.id when legacy.id is set, or otherwise making the selection unambiguous.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/configs/legacy.py around line 41:
`refuse_mixed_run` rejects a v0 `legacy.id` alongside a v1 `env.taskset.id` but does not reject a v0 `legacy.id` alongside a nonempty v1 `env.id`. In that case, `is_legacy` returns `True` (legacy id set, no taskset), so the server runs the v0 env, but `run_env_id` returns `env.env_id` — preferring the v1 id. The run silently executes one environment while labeling and saving results under a different env's id. Consider also rejecting a nonempty `env.id` when `legacy.id` is set, or otherwise making the selection unambiguous.
| """Elastic env-server pool: start at one worker and scale up on demand.""" | ||
|
|
||
| type: Literal["elastic"] = "elastic" | ||
| max_workers: int | None = None |
There was a problem hiding this comment.
🟡 Medium configs/serve.py:25
ElasticPoolConfig.max_workers accepts 0 or negative values because it lacks the ge=1 constraint that StaticPoolConfig.num_workers has. With max_workers=0, EnvServerPool still starts one worker but treats the configured cap as immediately reached, so the server silently runs one worker despite an impossible maximum. Add ge=1 to enforce a valid upper bound.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/configs/serve.py around line 25:
`ElasticPoolConfig.max_workers` accepts `0` or negative values because it lacks the `ge=1` constraint that `StaticPoolConfig.num_workers` has. With `max_workers=0`, `EnvServerPool` still starts one worker but treats the configured cap as immediately reached, so the server silently runs one worker despite an impossible maximum. Add `ge=1` to enforce a valid upper bound.
|
|
||
|
|
||
| class EvalConfig(EnvServerConfig): | ||
| class EvalConfig(BaseConfig): |
There was a problem hiding this comment.
🟠 High cli/eval.py:26
Removing id from the inheritance chain leaves consumers that reference config.id broken. run_legacy_eval accesses config.id before scheduling rollouts, so every valid --legacy.id ... evaluation now raises AttributeError and never runs. Similarly, push_traces reads config.id, so pushed evaluations fail after execution. Update these consumers to use config.legacy.id or config.env_id, or restore a compatibility id property on EvalConfig.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/configs/cli/eval.py around line 26:
Removing `id` from the inheritance chain leaves consumers that reference `config.id` broken. `run_legacy_eval` accesses `config.id` before scheduling rollouts, so every valid `--legacy.id ...` evaluation now raises `AttributeError` and never runs. Similarly, `push_traces` reads `config.id`, so pushed evaluations fail after execution. Update these consumers to use `config.legacy.id` or `config.env_id`, or restore a compatibility `id` property on `EvalConfig`.
Follows @mikasenghaas's review on #2157: a per-worker bound is a serving knob, not part of describing the env. Pulling that thread all the way —
EnvServerConfigwas three things at once (the env, how it's served, the v0 bridge) andEvalConfig/ the serve CLI / prime-rl's env entry all inherited it, so pool knobs sat flattened on an eval config and on a trainer's source entry.Three blocks now, composed rather than inherited:
A run config declares the blocks it needs and adds its own fields, sharing only the narrowing helpers —
GEPAConfigalready worked exactly this way, and has no[serve]at all since it runs in-process.EnvServerConfigis gone.The run keeps owning its concurrency.
-cbounds episodes in flight and seeds each worker's bound under--server;serve.max_concurrentpins a worker below the run when you want that (EvalConfig.worker_max_concurrent). So an in-process eval never has to reach into a block namedserve, and one number stays honest when a run spawns workers.Breaking:
--pool.*→--serve.pool.*,--address→--serve.address,--id/--args/--extra-env-kwargs→--legacy.*. Every retired top-level key raises with a pointer home rather than a bareextra_forbidden, the way the retired--taskset.*/--harness.*axes already do:Nothing in this repo set the moved keys, and the
configs/gepa/*.tomlmax_concurrententries are GEPA's own run-level bound, untouched.Downstream: prime-rl's
EnvConfigsubclassesvf.EnvServerConfig, so it needs the matching composition change — that lands with thedeps/verifiersbump, not here.Stacked
On #2157 (episode/agent concurrency), which introduced the per-worker bound this moves. Review that one first; this branch is the follow-up Mika's comment asked for.
Verified
pytest tests/v1 -m "not e2e"(64) and the v0 config/serve/gepa/env-server/display tests (94),ruff check,ruff format --check,ty check verifiers— all clean. Live E2Es not run here (no endpoint in this environment).-c 64+--serve.pool.type static --serve.pool.num-workers 4→ worker bound 64;--serve.max-concurrent 16→ 16);--legacy.id+--legacy.args '{"a": 1}'resolves withis_legacy/env_idintact; a v0 id next to a v1 taskset is refused; all five retired keys point home.-c 2over 6 episodes peaks at 2 episodes / 2 runs,max_concurrent_agents=Noneat-c 3peaks at 3 episodes / 6 runs.evaluate-environmentsskill reference updated for the new tree (that reference documentedEnvServerConfigfield by field).Note
Refactor v1 eval/serve config to use dedicated
[serve]and[legacy]blocksServingConfig(pool, address, max_concurrent) andLegacyEnvConfig(id, args, extra_env_kwargs) as explicit config blocks, replacing the flatEnvServerConfigbase class onEvalConfigandServeConfig.[serve]block now owns all pool and address settings;serve.max_concurrentis used as the per-worker episode bound, falling back to run-levelmax_concurrent.[legacy]block; mixed v0/v1 configurations are rejected at validation time.--id/--argsto--legacy.id/--legacy.argsand from top-level pool flags to--serve.*.EnvServerConfigis removed from the public API; callers must migrate toServingConfigandLegacyEnvConfig. Deprecated top-level keys (pool,address,id,args, etc.) now raise targeted errors.📊 Macroscope summarized 8ba0afe. 13 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.