Browsecomp, baselined - #1848
Conversation
…1203) Our browsecomp lineage forked at #1087 and evolved independently. We own browsecomp and are the source of truth, so this drops upstream's parallel browsecomp work (#1109 'improve browsecomp', #1203 'fixes and efficiency improvements' — a ~768-line agent rewrite) before replaying our commits. Our tip already carries the substantive parts of those PRs (coerce_urls fix, policy_model_no_interleaved_reasoning, snap_dir, max_reset_count, vllm /tokenize endpoint) and supersedes the rest. Signed-off-by: Ritu Gala <rgala@nvidia.com>
Signed-off-by: Ewa Dobrowolska <edobrowolska@nvidia.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
Signed-off-by: Ewa Dobrowolska <edobrowolska@nvidia.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
… + x-client-source Ports from gym-gitlab onto our 4dfdf07 (keeping our outer run() retry): - snap_dir (fe9845e): save full-conversation JSONL at each context reset AND at trajectory end under {snap_dir}/sample_{task_index}/attempt_{attempt}_*.jsonl (the pre-reset context was previously lost). Gated by snap_dir config (off=None). - reset_count / num_tool_calls (fe9845e): tracked in the agent loop and surfaced on the response; harness verify now reads the agent's num_tool_calls (fixes the undercount where it summed only the trimmed final output) + adds reset_count. - /tokenize estimation (b66e37c): when save_model_call_using_vllm_tokenize_endpoint, count prompt tokens via vLLM /tokenize BEFORE the model call and reset (adaptive keep-rounds shrink) instead of paying for a discarded generation. - Tavily x-client-source header sourced from env TAVILY_CLIENT_SOURCE (no hardcoded value); sent only when set. config.yaml: add max_reset_count/snap_dir/save_model_call_using_vllm_tokenize_endpoint. Verified: py_compile clean; live ultra_v3 /tokenize returns {"tokens":[...]}. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
… reset Port the gitlab terminal-mode feature (bxyu 53a5f923/06ef9a9b/3af3bfbd) onto our 85d6f9e harness via 3-way merge, aligned to the bc_frankie bash-tool reference @ ee72d54: - app.py: _PageWriter disk pages + manifest.tsv, per-session workspace, /bash_command route, _run_bash_readonly. Read-only guard = bc_frankie deny-list + default-deny allow-list (byte-identical to ee72d54) with ulimit -f 0 kernel backstop. KEPT our x-client-source, num_tool_calls/reset_count, rate-limit logging; verify() combines metrics + _cleanup_workspace; extract() drops query=goal; bash_timeout_s 60. - prepare.py: bash_command tool (bc_frankie ee72d54 description w/ allow-list note), WORKSPACE_SYSTEM_ADDENDUM, disk-save search/browse descriptions. - config.yaml: workspace: per_session. - agent: context_reset_tokens (50000) overrides max_context_tokens*context_reset_pct via _reset_threshold(); matches the bc_frankie bash-tool 50k token-based baselines. - tests: test_bash_guard.py (7) + agent _reset_threshold tests (3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
…ncy metering Make the terminal-mode harness search backend pluggable: - ExaAIOHTTPClient: native aiohttp REST to api.exa.ai (no exa-py dep), mirrors TavilySearchAIOHTTPClient (global request(), retry + exa_rate_limit tagging), high-level search()/get_contents(). - Exa search = highlight snippets inline (never writes pages, even in terminal mode); Exa browse = /contents -> reuses the existing disk/inline page-writing. exclude_domains honored for both (Gym-consistent). - Config: search_provider (default tavily) + exa_api_key; tavily_api_key now optional; model_validator requires the chosen provider's key. config.yaml default. - Metering: provider added to the per-call record; one record per provider API call (per search query, per browse) surfaced in metrics.async_tavily_calls. - tests/test_exa_provider.py (13). No agent change; no new dependency. Validated vs live api.exa.ai (harness search/browse) and a 2-sample GLM-5.1 smoke (job 3649213): 2/2 reward, 443 exa search + 34 browse, 0 tool errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
…lists The previous commit's `ruff format` exploded the hand-wrapped _BASH_DENY_COMMANDS, _BASH_ALLOW_COMMANDS, and _BASH_KEYWORD_SKIP literals (one item per line) — code unrelated to the Exa change. Restore them to their 797b88e form so the Exa diff is surgical. No behavior change (38 harness tests still pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
…put_text verify() used body.response.output_text, the stock openai property that CONCATENATES every assistant message item in response.output — so when the model emits text alongside tool calls mid-trajectory (~49% of BrowseComp samples; ~18 had a stray "Exact Answer:"), the judge was fed a concatenation of multiple answers instead of the final one. This is also a P01 harness confound: bc_frankie grades only the final message. Add _last_assistant_text(response) (text of the LAST assistant message item) and use it in verify(); both the judge and regex graders take this one string. Forward-only (no re-grade). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
…trajectory.jsonl in snap_dir
The agent trims new_outputs to context_reset_keep_rounds at every reset and ended with
model_response.output = new_outputs, so the rollout jsonl kept only the last-reset window
(full history was scattered across per-reset snapshots). Maintain a parallel, never-trimmed
full_trajectory (mirrors every new_outputs append + the nudge mutation), set
model_response.output = full_trajectory, and write {snap_dir}/sample_<i>/attempt_<a>_trajectory.jsonl
(metadata header with reset_count/num_tool_calls/reset_steps + input prefix + every item).
Grading-safe with the Part A judge fix: the last assistant message item is identical whether
output is the full trajectory or the final window. Forward-only (no backfill of old runs).
Accepted: rollout jsonl grows ~10x. Tests: full-trajectory-survives-reset + _save_trajectory;
the 2 pre-existing api_response.status MagicMock failures are unrelated (fail on clean 797b88e too).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Ritu Gala <rgala@nvidia.com>
…_FULL=1 for full 1266) prepare() wrote all 1266 unconditionally; the only 400 path was pre-placing a curated file (the skip-gate), so the NEL path (mounts only the raw) fell through to prepare() and ran 1266. module.prepare() takes no args, so the toggle is an env var: add a pure _select_samples(df, run_full) (df.sample(n=400, random_state=42) — version-stable, CSV order fixed -> reproducible) + constants BROWSECOMP_SUBSET_N/SEED. Default -> 400; BROWSECOMP_RUN_FULL=1 -> 1266. assert len==1266 kept as a raw-download sanity check before subsetting. The seed-42 400 is a NEW set (!= the edobrowolska 400); acceptable since the judge-concat fix already forces a re-baseline. Tests: benchmarks/browsecomp/tests/test_prepare.py (default->400, full->1266, determinism, strict-subset). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
…attempt) _verify_answer_with_judge used temp=0.0 on attempt 0 (the de-facto judge temperature), escalating only on parse-error retries. Run the judge at temperature 1.0 on every attempt; parse-error retries re-sample at 1.0 (which already varies the output, so the escalate-from-0.0 logic is unnecessary). No test pins the judge temperature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
…n of interleave port 52363ba) 52363ba was a mixed commit: this browsecomp/config.yaml wiring plus a vllm_model/app.py change. The vllm code is already in upstream main (#1203 b66e37c), so only the browsecomp config portion is replayed here. Adds policy_model_no_interleaved_reasoning (_copy of policy_model with uses_interleaved_reasoning: false) and points the agent's model_server at it, to reproduce bc_frankie's reasoning-stripped behavior. Judge stays on policy_model. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
The debug-print instrumentation (c6396e1) reads api_response.status for the [browsecomp][tool_fail] log line, but the tool-call test mocks only set .ok/.read/.content.read, never an int .status -> 'MagicMock >= 400' TypeError. Set .status=200 on the mocks. Test-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
Signed-off-by: Ritu Gala <rgala@nvidia.com>
…frankie parity) The agent run() empty-answer retry keyed on NeMoGymResponse.output_text, the OpenAI-SDK property that concatenates EVERY retained assistant message. So a trajectory whose final turn at the step cap produced only <think> content (or a tool call) did NOT retry as long as any earlier turn had emitted text -- a much rarer trigger than bc_frankie's. bc_frankie keys on the LAST content-bearing assistant message: walk back from the cap to the most recent assistant message with non-empty content (browsecomp_agent.py:1054-1059), <think>-strip it, and retry the whole sample if empty (browsecomp_eval.py:305-309). Add BrowsecompAgent._last_message_text() (mirrors that walk-back) and feed the retry check from it instead of output_text. Everything downstream of the empty check is unchanged; scope is only the empty-answer trigger (retry count and exception handling untouched). Removes the last bc_frankie-vs-Gym retry- semantics asymmetry. Tests: 6 _last_message_text unit tests + 1 run()-level retry test (all pass; 2 pre-existing api_response.status MagicMock failures are unrelated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
…e parity)
The resources server returns every tool result as a one-field JSON envelope
({"results_string": "..."}), which JSON-escapes newlines - so the model sees
search/browse/bash output as a single escaped line instead of raw multi-line
text. bc_frankie feeds the raw text. This was the last remaining
materialized-prompt difference between the two harnesses (verified token-level
via apply_chat_template: 6 differing token spans, all from this envelope).
Unwrap strictly: only a JSON object with exactly the results_string key (all
three tool response models are exactly that shape). Error bodies and non-JSON
payloads pass through unchanged. Nudge suffix now appends to the raw text.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Ritu Gala <rgala@nvidia.com>
…lt 5) MAX_RESULTS was a hardcoded class attribute, unreachable from Hydra. Plumb it into TavilySearchResourcesServerConfig as max_results (default 5, unchanged behavior) so runs can override it per-provider, e.g. ++...browsecomp_advanced_harness.max_results=10 to match the bc_frankie Exa reference (which uses 10 results/query vs 5 on its own Tavily path). Tests: config default, exa passes num_results, tavily passes max_results. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Ritu Gala <rgala@nvidia.com>
mjmikulski
left a comment
There was a problem hiding this comment.
Excellent!
I have a few very small remarks that maybe you could fix or comment please before merging. Mainly this one: The reproducibility code could be made simpler and more robust by simply fixing the indices without testing random number generator.
| df = _df() | ||
| a = _select_samples(df, run_full=False) | ||
| b = _select_samples(df, run_full=False) | ||
| assert list(a["row"]) == list(b["row"]) # same rows every call |
There was a problem hiding this comment.
It only tests if:
- given the same python version
- given the same platform
the subsample is identical.
But it does not guarantee that two runs of the benchmark actually have the same subsample.
There was a problem hiding this comment.
i am ok to do this but my main concern is that this looks by benchmaxxing - we are selecting a specific 400 subset id rather than take a seed 42. But if you feel that is ok then i can hardcode the ids
There was a problem hiding this comment.
I see. What about doing both? I.e. seed 42 + assert on first 10 indices? Sth liek that:
assert indices[:10] == [11, 44, 7, ...], "Random gives unexpected results, subsample is not comparable"| browsecomp_eval.py performs, so both harnesses' seed-42 subset is the same 400.""" | ||
| if run_full: | ||
| return df | ||
| idx = random.Random(BROWSECOMP_SUBSET_SEED).sample(range(len(df)), BROWSECOMP_SUBSET_N) |
There was a problem hiding this comment.
Why not simply write down those 400 indexes in a CONSTANT:
FIXED_SUBSAMPLE_INDICES = [7, 12, 27, ...]That would guarantee that the same subset is used even if random changes.
A small python notice: even if random is stable, the .sample implementation may change.
There was a problem hiding this comment.
i am ok to do this but my main concern is that this looks by benchmaxxing - we are selecting a specfic 400 subset id. but if you feel that is ok then i can hardcode the ids
|
|
||
|
|
||
| # ---- deny-list cases (mined from real trajectories) ---- | ||
| _DENY_ALLOWED = [ |
There was a problem hiding this comment.
_DENY_ALLOWED 🤔
I took me some time to understand what it is :) It's a bit nit picking, but maybe more explicit _DENYLIST_SHOULD_PASS or sth like that?
| def test_last_assistant_text_returns_final_message_not_concat(): | ||
| resp = _response( | ||
| [ | ||
| _msg("Exact Answer: WRONG"), |
There was a problem hiding this comment.
That seems a bit misleading example, WRONG or RIGHT should be judge responses, not model responses. Or I misunderstand and this is a judge trajectory mocked here? 🤔
There was a problem hiding this comment.
oh sure let me clarify. This was a bug in the previous code.
Ideally what we want is that the judge only the last message from the policy model (eg. "Based on my searches, the correct answer is abc). However the previous code was passing all assistant.content to the judge, not just the last msg, which is the wrong thing to do and may sway the judge. This test makes sure we dont regress on this behavior.
There was a problem hiding this comment.
Yes, I understand the bug. My concern was that:
repsis a list of MODEL responses with some mockup values- the mockup values pretend to be JUDGE responses
But never mind, syntactically the test is correct. Semantically it is bizarre for me.
Summary
Consolidates our BrowseComp agent-harness work onto latest
main. This brings thebrowsecompbenchmark, thebrowsecomp_advanced_harnessresources server, and thebrowsecomp_agentup to the version we've been running for evals, adding a pluggablesearch backend, a read-only terminal/bash tool, token-based context reset, and
trajectory/metrics recording.
What's included
tavily|exa) in the advanced harness, withper-call cost/latency metering.
bashtool guarded bydeny/allow command lists (with a
ulimit -f 0kernel backstop; the guard is apragmatic speed-bump, not a security boundary).
context_reset_tokens, default 50k) as an alternativeto the percentage-based reset, plus
max_reset_count.snap_dirwrites per-sample reset/finaltrajectory
.jsonl; token counts estimated via the vLLM/tokenizeendpoint;reset-count / tool-call metrics on the response;
x-client-sourceheader.output_text(previously the judge saw all assistant turns joined together).
response.outputcarries the full untrimmed trajectory.prepare.pydefaults to a seeded 400-sample subset(seed 42); set
BROWSECOMP_RUN_FULL=1for the full 1266.policy_model_no_interleaved_reasoning: a_copyofpolicy_modelwithuses_interleaved_reasoning: false, wired as the agent's model server so reasoningisn't fed back into multi-turn history. (The
uses_interleaved_reasoningtoggleitself is already in
main; only the browsecomp config wiring is added here.)Relationship to #1087 / #1109 / #1203
Our harness forked from the original BrowseComp benchmark (#1087) and evolved
independently, in parallel with upstream #1109 and #1203. Since both lines heavily
rewrote
browsecomp_agent/app.py, a straight merge would be a large,hard-to-review conflict. This PR instead treats our version as the source of truth for
the browsecomp folders (
benchmarks/browsecomp,resources_servers/browsecomp_advanced_harness,responses_api_agents/browsecomp_agent), which supersedes the browsecomp changes from#1109 and #1203.
The substantive fixes/features from those PRs are retained in this version:
coerce_urlsbug fix (urls),policy_model_no_interleaved_reasoning,snap_dirsnapshot recording,max_reset_count,/tokenize-based token counting.Nothing outside the three browsecomp folders is changed.
Testing
benchmarks/browsecomp,browsecomp_advanced_harness, andbrowsecomp_agentpass (gym env test --resources-server browsecomp_advanced_harness,plus the agent/prepare tests).
ruff checkandruff format --checkclean (v0.9.9).