Async grpo OpenEnv harness rollout#6420
Conversation
dabfebb to
1903378
Compare
| return [] | ||
| last = trace[-1] | ||
| content = last["response"]["choices"][0]["message"].get("content", "") | ||
| return list(last["request"]["messages"]) + [{"role": "assistant", "content": content}] |
There was a problem hiding this comment.
Final tool calls dropped
Medium Severity
_messages_from_trace rebuilds only the final assistant content and omits tool_calls (and treats JSON null content as None). Episodes that end on a tool-call turn therefore hand session.verify an incomplete transcript, so harness rewards can be wrong for truncated or tool-heavy OpenCode runs.
Reviewed by Cursor Bugbot for commit 284da95. Configure here.
| # Sized to max_inflight so sessions run concurrently without queuing or per-rollout thread churn. | ||
| self._session_pool = ThreadPoolExecutor( | ||
| max_workers=max(1, self.max_inflight_tasks), thread_name_prefix="harness-session" | ||
| ) |
There was a problem hiding this comment.
Session pool never stops
Medium Severity
_HarnessRolloutLoop creates a long-lived ThreadPoolExecutor for harness sessions but never shuts it down when the asyncio loop stops. Those workers are non-daemon and can keep running wait_for_completion, so the child process often fails to exit cleanly and is force-terminated after the parent's join timeout.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 284da95. Configure here.
There was a problem hiding this comment.
yes, this needs a shutdown() on teardown.
| reward = float(verify.env_reward) if verify.env_reward is not None else None | ||
| sequences = _chain_to_sequences(turns, rollout_id, self._fork_threshold_tokens) | ||
| completion_ids = [tid for turn in turns for tid in turn.output_ids] | ||
| return completion, completion_ids, sequences, tool_call_count, tool_failure_count, reward |
There was a problem hiding this comment.
Empty traces skew advantages
High Severity
When the proxy trace is empty or _trace_output_ids recovers no token ids, _chain_to_sequences yields no training rows, but session.verify still supplies a rollout_reward that enters the group's advantage normalization. Sibling generations are then trained against rewards from conversations that contribute no samples.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 284da95. Configure here.
There was a problem hiding this comment.
When the proxy trace is empty or _trace_output_ids recovers no token ids
no idea in what case this is possible
There was a problem hiding this comment.
I dont think thats real case
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4a4af23. Configure here.
|
Thanks I think it looks good overall. I didn't try myself, but from reading the edits, nothing much to say. Check the cursor review, it's likely the reason of the CI failure. Once the CI green I approve and we merge |
sergiopaniego
left a comment
There was a problem hiding this comment.
thanks!!
approving but with 2 ideas:
- we could add a short harness-rollout section to
openenv.mdin this PR: white-box vs loop-owning,--return-tokens-as-token-ids, reward viasession.verify()→harness_reward. - we should add an example script. I think this could be added once we add support for HF sandbox + harnesses. wdyt? it's in the roadmap here -> huggingface/OpenEnv#940


WIP
Note
Medium Risk
Changes the rollout worker spawn path and scoring contract (new reward column and
_generate_onearity) while the defaultAsyncRolloutWorkerbehavior stays the same; harness mode runs synchronous OpenEnv sessions on a thread pool and depends on correct proxy traces and verify rewards for GRPO signal.Overview
Adds
HarnessRolloutWorker/openenv_harness.py, an Async GRPO rollout backend that runs OpenEnvResourceSessionFactorysessions instead of TRL’s built-in multi-turn tool loop. Rollouts can use white-box mode (adapter drives turns via vLLM through_sample_turn) or loop-owning mode (agent uses a proxy; TRL rebuildsTurnRecords from the trace and still runs_chain_to_sequencesfor training rows).session.verifysuppliesharness_reward, so training can run with emptyreward_funcs.The async rollout stack is extended to support this: spawned children take a configurable
_loop_cls,RolloutGroupcarriesrollout_rewards,_generate_onereturns an optional sixth reward value, and_provides_rollout_rewardlets subclasses register an extra reward column in_score_group.RolloutWorkerProtocolnow allowsmultiprocessing.Queueand renamesupdate_model_version’s argument; tests follow the new return tuple and stubrollout_rewards.Reviewed by Cursor Bugbot for commit e6d759d. Bugbot is set up for automated code reviews on this repo. Configure here.