Repository navigation
feat(benchmarks): explicit agent pin on benchmark datasets; scaffold generates the decoupled layout - #2724
Conversation
95a419f to
5d014c0
Compare
5d014c0 to
39f01c8
Compare
39f01c8 to
b7b7bca
Compare
b7b7bca to
3d07e79
Compare
3d07e79 to
4868e89
Compare
4868e89 to
babadb8
Compare
|
/ok to test babadb8 |
|
/ok to test babadb8 |
babadb8 to
c708032
Compare
4ab0966 to
ee44065
Compare
ee44065 to
584e5de
Compare
|
/ok to test d94c88e |
|
/ok to test bc44b78 |
|
/ok to test 0e751be |
|
|
||
| dataset = datasets[0] | ||
|
|
||
| agent_name = dataset.agent or candidate_agent_server_instance_names[0] |
There was a problem hiding this comment.
take this config:
shared_rs:
datasets:
- agent: agent_b
agent_a:
resources_server: shared_rs
agent_b:
resources_server: some_other_rsThis line selects agent_b because the explicit pin wins without validating the relationship. Collation later writes only task_source: shared_rs, so runtime follows the actual resources-server edge and selects agent_a.
benchmark listing/preparation → agent_b
actual rollout → agent_a
we should reject the pin unless the named agent references the declaring resources server
There was a problem hiding this comment.
Confirmed and fixed in fcce242. Resolution now goes through a single shared resolver (resolve_dataset_agent in global_config.py), which rejects any pin that doesn't name the declaring agent itself or an agent referencing the declaring resources server — so this config is now a hard error at discovery, preparation, validation, and dispatch alike. Test: test_rs_declared_pin_must_reference_the_declaring_rs.
| ) | ||
| agent_name = candidates[0] | ||
|
|
||
| benchmarks_dict[agent_name] = BenchmarkConfig( |
There was a problem hiding this comment.
Suppose two benchmark declarations both select shared_agent:
benchmark_a → shared_agent
benchmark_b → shared_agent
The assignments become:
benchmarks_dict["shared_agent"] = benchmark_a
benchmarks_dict["shared_agent"] = benchmark_b
where benchmark_b overrides benchmark_a silently
There was a problem hiding this comment.
Confirmed and fixed in fcce242. benchmarks_dict is now keyed by the declaring instance (unique by construction; each instance is limited to one benchmark dataset) instead of the resolved agent, so both declarations are prepared. Test: test_two_declarations_resolving_to_one_agent_both_prepare.
| @@ -93,10 +105,22 @@ def from_initial_config_dict( | |||
|
|
|||
| dataset = datasets[0] | |||
There was a problem hiding this comment.
Consider this configuration:
shared_rs:
datasets:
- agent: agent_b
agent_a:
resources_server: shared_rs
agent_b:
resources_server: shared_rsDiscovery honors the pin and selects agent_b. Collation then emits only:
{"task_source": "shared_rs"}At rollout time, resolve_task_sources() asks which agents reference shared_rs. It finds both agent_a and agent_b, no longer has the pin, and rejects the row as ambiguous.
We should propagate the pin into the runtime routing, equivalent to:
agent_map:
shared_rs: agent_balong with the discovery -> collate -> rollout test for this case
There was a problem hiding this comment.
Confirmed and fixed in fcce242. Rather than materializing an implicit agent_map, resolve_task_sources() now resolves through the same shared resolver, reading the agent: pin back off the declaring instance in the merged config — so this config routes to agent_b end to end (an explicit +agent_map still overrides). Conflicting pins across one instance's datasets are a hard error, since rows carry only task_source. Discovery → collate → rollout test added: test_pin_survives_discovery_collate_and_rollout (real collate stamping, real dispatch resolution).
|
/ok to test |
@adil-a, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test bf8f5fa |
…declares datasets on the resources server A benchmark's score depends on the harness, so the manifest should pin it explicitly rather than have it inferred from which config block declares the dataset. Benchmark datasets gain an optional agent: key; resolution order is explicit pin > declaring agent block (legacy inference, kept for un-migrated manifests) > unique agent referencing the declaring resources server. An RS-declared benchmark dataset with 2+ referencing agents and no pin is a hard error naming the fix (resolves the bigcodebench base-agent/benchmark-agent ambiguity). gym env init now scaffolds the decoupled layout, so nothing new is born in the old format: datasets are declared on the resources server block (kept on the agent only when reusing a verifier from another config, where the RS implementation key is unknown); scaffolded benchmark datasets carry the agent: pin; manifest validation accepts a dataset-bearing resources server; the generated data .gitignore covers the per-declaration prepared files. Replay over all 225 runnable config closures: no observable change beyond the diffs already enumerated for the previous PR in the stack. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Adil Asif <adasif@nvidia.com>
Caught by the live smoke matrix: gym eval prepare / eval run --benchmark had its own agent-blocks-only dataset scan, so a benchmark dataset declared on a resources server block (the decoupled layout) was invisible — prepare never ran and collation then crashed on the missing file with a raw AttributeError (BenchmarkDatasetConfig has no registry identifiers). prepare_benchmark now scans both declaring block types with the same agent resolution as BenchmarkConfig (explicit agent: pin > declaring agent block > unique agent referencing the declaring RS), and the missing-file download branch gives benchmark datasets a clear run-gym-eval-prepare error instead of the AttributeError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Adil Asif <adasif@nvidia.com>
Caught by the live smoke matrix: the new dataset-level agent: key leaked into the dataset dump embedded in *_metrics.json, so every committed sidecar conflicted with freshly computed metrics and benchmark e2e runs refused to proceed. The pin is routing config, not dataset identity — excluding it keeps all pre-pin sidecars valid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Adil Asif <adasif@nvidia.com>
… not the default Every current benchmark config resolves without a pin (declaring agent block, or unique agent referencing the declaring resources server). Reword the field description and resolver comment so pins read as the disambiguator for genuinely ambiguous configs rather than a recommended default. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Adil Asif <adasif@nvidia.com>
…datasets The scaffolded config always has exactly one agent referencing the resources server, so the harness resolves from that edge. The agent key remains the escape hatch for ambiguous configs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Adil Asif <adasif@nvidia.com>
…validation, and dispatch Addresses review on #2724: - An `agent:` pin is now validated everywhere it is honored: it must name the declaring agent itself, or an agent referencing the declaring resources server. Previously discovery honored any pin while dispatch followed the agent -> resources-server edge, silently running a different agent. - resolve_task_sources reads the pin back off the declaring instance, so a pin that disambiguates a shared resources server at discovery also routes its collated rows (which carry only task_source) at rollout time. - prepare_benchmark keys benchmarks by declaring instance instead of resolved agent, so two declarations resolving to one agent no longer silently drop all but the last. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Adil Asif <adasif@nvidia.com>
|
/ok to test 151f1ed |
|
@marta-sd closing the loop on this one, late. You were right on both counts. bigcodebench's two-agent closure is an artifact: its benchmark config defines its own agent instead of So the pin is going away: #3209 removes the The bigcodebench |
Problem
gym env initstill generates the old layout. New environments are born withdatasets:under the agent block — every scaffold run creates more configs the migration has to fix later.Change
1. Benchmark agent resolution becomes an explicit, validated rule set (with an optional
agent:key as the disambiguator for genuinely ambiguous configs — unambiguous configs, which is all of them today, need no pin).How a benchmark finds its agent now, first hit wins:
agent:keyresources_server: {name: <that server>}→ that agentagent:2.
gym env initgenerates the decoupled layout. Scaffolded configs declaredatasets:on the resources server block; scaffolded benchmark datasets need noagent:pin (the scaffold has exactly one agent referencing the resources server); the generated data.gitignorecovers the per-declaration prepared files from #2717. One exception: when scaffolding reuses a verifier from another config file, the dataset stays on the agent block (we don't know the other file's internal keys, so a partial override can't be merged safely — still fully supported).3. Manifest validation accepts the new layout.
gym env validatepreviously required exactly one dataset-bearing agent; it now equally accepts one dataset-bearing resources server, resolving the agent by the same rules as above.What does NOT change
Where
agent_refis headedSame timeline as #2717: this stack removes
agent_reffrom data files. This PR ensures nothing new is ever generated in the old format; the next PR migrates what exists; the cleanup PR removes the field from the data contract (it remains only in run outputs, as a record of which agent actually ran each row).Tests
90 passed across benchmark/scaffold suites. New tests pin all four resolution rules and the scaffolded config layout (datasets on the RS block, none on the agent block).
Relationship to other PRs
Part 4 of the dataset↔agent decoupling work (RFC: gym-dataset-preparation, Open Question 1), now an individual PR targeting
main(#2710/#2713/#2717 merged). The repo-wide migration in #2732 builds on this PR's resolver and should merge after it.🤖 Generated with Claude Code