openenv/tbench2: score the shared-server leg natively; retire the adapter compensation - #1790
Merged
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Zhichenzzz
added a commit
that referenced
this pull request
Jul 26, 2026
#1791 made resolve-ci-image depend on docker-build so that a docker PR runs its suites inside the freshly built image. docker-build only runs when docker files change, so on every other PR it is skipped -- and GitHub propagates a skip down the entire needs closure, including through resolve-ci-image's always() guard. stage-a-cpu carries no if: at all, so it was skipped, and stage-b/stage-c gate on stage-a-cpu succeeding, so the whole matrix went with it. The run still reports success, because a skipped job is not a failed one. Since #1791 merged, PRs 1792/1793/1794 each completed "green" with zero test jobs expanded; the last run that actually executed anything was #1790 at 07-24T21:17Z, before the merge. #1791's own CI was green and did run, because it changed docker/Dockerfile -- the regression could not appear in the PR that introduced it. Fix keeps the new behavior and restores the old graph: docker-paths and docker-build now always reach a conclusion instead of skipping, so nothing downstream is poisoned. With nothing to build, docker-build no-ops on ubuntu-latest rather than occupying a GPU runner, and reports built=false; resolve-ci-image reads that output instead of the job result. All seven stage-* jobs are byte-identical to their pre-#1791 definitions. docker-paths also loses its `if: github.event_name == 'pull_request'`, which had the same effect on the nightly cron: no PR context meant a skipped job and a silently empty scheduled run. It now reports changed=false for non-PR events.
…pter compensation With huggingface/OpenEnv#1012 bringing docker mode up to the canonical scoring contract (#965/#972 had it in local mode only), both legs speak one protocol: raw exec commands (the server resolves the task image's WORKDIR) and the standard `evaluate` action (canonical tests/test.sh, server-side). - delete the adapter-side compensation machinery the shared leg carried for older deployments: _apply_workdir, _CANONICAL_EVAL_CMD, the reward / test.sh-rc markers and their parsers, the OPENENV_TASK_WORKDIR / OPENENV_TB2_TESTS_SRC knobs, and the testsh_rc metrics plumbing. - the native_evaluate episode-wiring parameter goes with it: legs now differ only in run_body (how an env comes into being) and post_episode (the shared server keeps its trial-dir purge; a sandbox needs none). - runtime contract guard, replacing the source preflight that is impossible against a remote server: evaluate must carry the canonical harness marker (info.harness == "tests/test.sh"). An older deployment's bare-pytest reward looks valid but is untrustworthy -- it is dropped with a warning, so an out-of-date server surfaces as every sample dropping, never as a plausible reward curve. Composes with the existing error/None-reward drop. Behavior change: episodes whose task dir ships no tests/test.sh (scored by the server's pytest fallback, harness marker absent) are now dropped too -- all 89 official TB2 tasks and the synth pools ship test.sh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
nblintao
force-pushed
the
tao/openenv-native-evaluate
branch
from
July 29, 2026 14:49
d5b5de7 to
cad3e6e
Compare
The clone instruction still pointed at the #965/#972 merge — enough for the Daytona leg (local mode), but the shared docker leg scores through the docker-mode canonical contract that only exists from 04d259ea6. And the launcher preflight's failure message still said 'not upstream main', from the era when the fixes lived only on a fork branch — upstream main IS the correct install source now, as the README already says. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
nblintao
marked this pull request as ready for review
July 29, 2026 15:15
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
…s no-verdict semantics The docs framed the harness guard against 'an OLDER deployment (bare-pytest scoring)' — a description of one past deployment's behavior, with no counterpart left in this codebase to make sense of. Reframe as the forward-looking version contract it is: tbench2_env >= the OpenEnv#1012 merge, verified per episode. scan_golden.py also inherits the agent loop's no-verdict semantics: a server-side scoring failure or non-canonical harness now prints as ERR instead of a fake 0.0 that would misattribute an infra problem to the task in a golden baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed path Comments like 'scoring is identical on both legs' and 'no adapter-side canonical exec' only carried information while the compensation path existed to contrast against; with it gone they read as non sequiturs. Describe the present instead, keep 'canonical' only where it contrasts with something alive (the server's pytest fallback), keep the daytona test's no-purge assertion (a real cross-leg difference today) while dropping the test.sh negative assertions that guarded a path no code can produce anymore. The README's server-contract paragraph shrinks to the same terse style as the 2b install note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Shi-Dong
approved these changes
Jul 29, 2026
Comment on lines
-131
to
-151
| def _parse_reward_marker(output: str) -> float | None: | ||
| """Parse the reward.txt value the canonical-eval exec echoed on its marker line. | ||
|
|
||
| Returns None when no reward can be recovered -- no marker line, an empty | ||
| value (reward.txt absent, i.e. test.sh never wrote a verdict), or a | ||
| non-numeric value. These are infra/harness failures, not a task the agent | ||
| legitimately failed, so the caller drops the sample rather than scoring a | ||
| false 0.0 that would pollute the training signal. A genuine failure writes | ||
| reward.txt = 0 and is returned as 0.0. | ||
| """ | ||
| for line in output.splitlines()[::-1]: | ||
| if _REWARD_MARKER in line: | ||
| raw = line.split(_REWARD_MARKER, 1)[1].strip() | ||
| if not raw: | ||
| return None | ||
| try: | ||
| return float(raw) | ||
| except ValueError: | ||
| return None | ||
| return None | ||
|
|
Contributor
There was a problem hiding this comment.
Seems that the behavior is a little bit different in OpenEnv? If reward.txt is missing, OpenEnv gives 0.0 reward, whereas this deleted function gives None?
Contributor
Author
There was a problem hiding this comment.
Oh, good catch. Let me fix that in openenv. Could you review it? huggingface/OpenEnv#1025
|
|
||
| # Per-message WS recv timeout. Docker-mode tbench2 reset (container create), | ||
| # exec, and evaluate (pytest) each routinely exceed the EnvClient default of 60s. | ||
| _MESSAGE_TIMEOUT_S = float(os.getenv("OPENENV_MESSAGE_TIMEOUT_S", "600")) |
Contributor
There was a problem hiding this comment.
Seems that OpenEnv is giving the verifier 900 seconds by default?
Contributor
Author
There was a problem hiding this comment.
Updated to 1200s. It should always be greater than the timeout on openenv side. It would be better if this can be managed automically. Created an issue at huggingface/OpenEnv#1026
…fier budget The 600s default predates native evaluate: the server now legitimately runs tests/test.sh for the task's own [verifier].timeout_sec (default 900, up to 3600 in the official suite) inside ONE env op, so a 600s per-message timeout aborts a legal verify after the full trajectory was generated -- the most expensive point to fail -- while the server keeps burning the slot. Raise the default to 1200 and document the invariant on both knobs. Co-Authored-By: Claude Fable 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
2 participants
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 #1675, resolving the review thread about retiring the
native_evaluate=Falsebranch. Draft until the shared-server deployment upgrades (gate 1 of 2 — the upstream contract — cleared when huggingface/OpenEnv#1012 merged; see Landing order below).What
Both agent-function legs now speak ONE scoring protocol: raw exec commands (the server resolves the task image's real WORKDIR) and the standard
evaluateaction (canonicaltests/test.sh, run server-side). The adapter-side compensation machinery the shared leg carried for oldertbench2_envdeployments is deleted:_apply_workdir+ theOPENENV_TASK_WORKDIR/OPENENV_TB2_TESTS_SRCknobs_CANONICAL_EVAL_CMD+ the reward / test.sh-rc markers and their parserstestsh_rcmetrics plumbingnative_evaluateepisode-wiring parameter itself — legs now differ only inrun_body(how an env comes into being) andpost_episode(the shared server keeps its trial-dir purge; a per-episode sandbox needs none)Plus two consistency fixes in the same spirit:
scan_golden.pyadopts the loop's no-verdict semantics (a server-side scoring failure or non-canonical harness prints asERR, not as a fake0.0in a golden baseline), and the docs/README describe the server requirement as a version contract (tbench2_env ≥ the OpenEnv#1012 merge,04d259ea6) instead of narrating a particular old deployment's behavior.Why deletion needed an upstream PR first
The premise "the fixes are already in OpenEnv" held for local mode only (huggingface/OpenEnv#965 + #972 — what the Daytona leg runs). Docker mode — what the shared server runs — still scored via bare
pytest tests/from/taskand executed agent commands there, so flipping the shared leg to native scoring would have traded canonical-in-/app for bare-pytest-in-/task: a fidelity regression, not a cleanup. huggingface/OpenEnv#1012 brings docker mode up to the same contract; this PR is its miles-side consumer.Runtime contract guard (replaces an impossible preflight)
The Daytona leg preflights the installed server source before launch; against a remote shared server that's impossible — and an old deployment doesn't fail, it returns a plausible-looking (mis-scored) reward for every episode. The guard:
evaluatemust carry the canonical harness marker (info.harness == "tests/test.sh", emitted by current upstream). Anything else — an older server's bare-pytest info, a server-side scoring error, a missing reward — drops the sample with a warning. An out-of-date deployment therefore surfaces as every sample dropping loudly, never as a believable reward curve. Composes with #1675's existing error/None-reward drop.Known behavior change: task dirs shipping no
tests/test.sh(scored by the server's pytest fallback, which carries no harness marker) are also dropped — deliberate, since that path is indistinguishable from an old server; all 89 official TB2 tasks and the synth pools shiptest.sh.Validation
Post-merge re-validation (2026-07-29, after OpenEnv#1012 landed — everything below runs against a
TB2_MODE=dockerserver freshly installed from merged upstream main, i.e. exactly what the upgraded deployment will run; also re-proves the README install instructions as written):pytest examples/experimental/openenv/tests/ -q→ 21 passed, incl. a test asserting an old-server-shapedevaluatereply (valid-looking reward,{tests_passed, exit_code}info) yieldsreward=None./app/personal-site, the image's real WORKDIR), chess-best-move 1.0, no-solution control 0.0, imageless task dir rejected loudly at reset.run_episode, real episode path): stub policy control → reward 0.0 (guard passes a canonical server; genuine failure scored, not dropped); DeepSeek-V4-Flash as policy → fix-git solved in 13 turns, 1/1, 0 errors.Earlier rounds (against the pre-merge #1012 branch) additionally demonstrated the negative case live: the same stub against the currently-deployed older shared server →
evaluate produced no canonical verdict (error='', harness=''); dropping episode, reward None — the exact silent mis-scoring this guard exists to block.Not covered without GPU (deferred to a 4-GPU smoke at cutover): the session-server TITO wiring + GRPO loop of
run()— shared code across both legs, already exercised by #1675's 4×H200 validation.Landing order (why draft)
fix(tbench2_env): bring Docker mode up to the canonical scoring contract huggingface/OpenEnv#1012 merges (docker-mode canonical contract).Done — merged as04d259ea6.tbench2_envfrom upstream ≥04d259ea6and restarts — this also closes the old deployment's reward-hacking hole (tests/ and solution/ visible to the agent in/task)./task), and this NEW client drops everything against the OLD server — so step 2 and 3 should happen together, coordinated with whoever operates the deployment.🤖 Generated with Claude Code