Repository navigation
fix(agent): report summed policy usage instead of null or zeroed details - #4761
Conversation
|
/ok to test 07df8b9 |
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for sweeping these six agents onto the documented usage contract; the tau2 ContextVar recorder, the osworld _PolicyUsage, and the exact-sum tests (including the discarded-attempt and user-simulator cases) look solid. I reviewed only the last commit, since the branch still carries #4753's commit.
Needs a change (1 inline, 2 anchors): a finished policy response with missing or malformed usage is recorded as unknown, but the sum then skips it, so the rollout reports the other calls' subtotal as a complete total. rollout-evidence.mdx / rollout-health.mdx say a finished response whose usage was lost should make the usage unknown. See the inline comments on tau2 and osworld.
Follow-ups (not blocking):
vcqa_agent,image_tools_agentandconversational_tool_use_simulationhave the same shape (accumulate_response_usage(total, None)returns the total). It is no worse than before this PR, so a shared completeness helper in a follow-up would keep all agents consistent.- tau2: a small test that gathers two
Tau2Agent.runcalls with different usages and asserts each row keeps only its own sum would pin the ContextVar isolation. - osworld timeout and Ray-failure rows report
usage: null(as the description says); a one-line comment at those return sites would make that intentional-looking.
PR hygiene: please rebase onto main once #4753 merges so the diff shrinks to this commit. The live rollout evidence covers tau2 and conversational simulation only, and the description gives reasons for the other four agents.
| except ValidationError: | ||
| # A malformed usage block is unknown usage, not a reason to fail the rollout. | ||
| usage = None | ||
| usages.append(usage) |
There was a problem hiding this comment.
When a finished policy response has no usable usage this appends None, and the fold in run() then skips it (accumulate_response_usage(total, None) returns total), so the row reports the other calls' subtotal as if it were complete. rollout-evidence.mdx says to omit usage when a finished response's usage was lost. Could you keep response.usage as None when any recorded entry is None (as scicode_agent does with usage_complete) and add a test with one known and one unknown call, in both orders?
There was a problem hiding this comment.
Thanks, fixed in 45cd858. run() now reports NeMoGymResponseUsage.sum_from_list(...) only when every recorded policy call has usage, and None otherwise, so a lost usage block no longer leaves a subtotal. An empty record also stays None: the recorder only sees calls made through Tau2ToolValidatingAsyncOpenAI, so an empty record can't prove that no policy call finished.
Tests: test_run_usage_is_unknown_when_a_finished_policy_call_lost_its_usage (the unknown call first and last, through /run), plus the isolation test you suggested: two Tau2Agent.run calls under asyncio.gather with different usages, each row keeping only its own sum.
| except ValidationError: | ||
| # A malformed usage block is unknown usage, not a reason to fail the policy call. | ||
| usage = None | ||
| self.total = accumulate_response_usage(self.total, usage) |
There was a problem hiding this comment.
Same gap here: a malformed or missing usage block becomes None and accumulate_response_usage(self.total, None) leaves the earlier subtotal in place, so the result looks like a complete sum. Could _PolicyUsage track a complete flag and report None once any finished response lacked usable usage, with a test for known and unknown calls in both orders?
There was a problem hiding this comment.
Fixed in 45cd858. _PolicyUsage now tracks complete, and reported() returns None once any finished response lacked usable usage; the child result uses reported().
Tests: test_child_result_usage_is_unknown_when_a_finished_response_lost_its_usage (known and unknown calls in both orders, checked through _build_response). The malformed-block test now also checks that a later known call doesn't restore a total. The timeout and Ray-failure returns have a comment saying their usage stays unknown.
Six agents reported rollout usage wrongly. tau2, osworld_agent and anyterminal_agent never set response.usage although the policy calls' usage was available; vcqa_agent, conversational_tool_use_simulation and image_tools_agent summed turns but overwrote cached/reasoning details with 0, also when a provider reported none. - tau2: Gym's tool-validating client records every finished agent-model response, including ones tau2 or the malformed-tool-call retry discards; user-simulator calls (other model name) are excluded. run() sets the sum on success and failure rows. - osworld_agent: Gym's own policy callers record each finished response, including one rejected for its finish reason, and the Ray result carries the sum. Runners that call models through their own clients (m3_agent, pointer_agent) keep usage unknown. - anyterminal_agent: keep the inner agent's usage from response.json, as anyswe_agent does. - vcqa_agent, conversational_tool_use_simulation, image_tools_agent: sum with accumulate_response_usage, which keeps an unknown detail unknown. A usage block that does not parse counts as unknown in tau2 and osworld; it never fails the policy call or the rollout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: qiaochuz <qiaochuz@nvidia.com>
… usage Review on #4761: tau2 and osworld recorded a finished policy response with missing or malformed usage as unknown, then summed the other calls anyway, so the row reported a subtotal as the rollout's total. rollout-evidence.mdx says a finished response whose usage was lost makes the usage unknown. tau2 reports the sum only when every recorded policy call has usage, and keeps an empty record unknown (the recorder sees only calls made through its own client). osworld's _PolicyUsage tracks completeness and reports None once any finished response lacked usable usage. Tests cover known and unknown calls in both orders, and two concurrent tau2 runs keeping their own sums. Comments mark osworld's timeout and Ray-failure rows as unknown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: qiaochuz <qiaochuz@nvidia.com>
07df8b9 to
45cd858
Compare
|
/ok to test 45cd858 |
|
@yaoyu-33 thanks for the review. Round 1 is in 45cd858:
|
|
/ok to test 45cd858 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39856697805 |
What does this PR do?
Makes six more agents report rollout token usage per the documented contract: the sum over the policy-model
responses that finished, with an unknown detail kept unknown. Before this PR they reported either nothing or
fabricated zeros:
response.usagebefore this PRtau2nullosworld_agentnullgym_pyautogui,prompt_agent*, nemotron/qwen omni runners);m3_agent/pointer_agentstaynullanyterminal_agentnull(inner agent's usage dropped)anyswe_agentalready doesvcqa_agentcached_tokens/reasoning_tokens= 0 from the 2nd call onnullwhen a call reported noneconversational_tool_use_simulationimage_tools_agentnemo_gym/reward_profile.pycopiesresponse.usageinto each rollout row, so these rows had no token counts orwrong cache/reasoning counts. Contract:
rollout-evidence.mdx("Report the agent's responseusageas the sum overmodel responses that finished … Omit usage only when a finished response's usage was lost"); policy calls only per
rollout-health.mdx"Binding Policy-Model Calls" andswitchyard.mdx.Follow-up to #4716 and #4753, NVBug 6900411. No separate issue: the
6900411 sweeps found these agents.
Uses
chat_completion_usage_to_response_usagefrom #4753 (merged). Rebased ontomainafter #4753 merged, so thediff is this PR's two commits only: the original change and the review-round fix.
Cause
tau2/app.pybuilds the response dict withoutusage; the per-message counts only feed min/mean/max metrics.The messages are not a complete record anyway: tau2-bench regenerates an empty agent message (up to 5 attempts,
agent/llm_agent.py) and Gym's malformed-tool-call retry discards attempts.osworld_agent/app.py:_build_model_fnreturns only the text and_build_messages_model_fnonly the normalizedmessage, so
resp.usageis dropped;_build_responsesets no usage.anyterminal_agent/app.py:_inner_responsesrebuilds the response from the savedoutputandtoolsonly.vcqa_agent,conversational_tool_use_simulation(_merge_usage),image_tools_agent(_response_usage_add)hand-roll the merge and write
cached_tokens = 0/reasoning_tokens = 0.Fix
ContextVarset inTau2Agent.run(). Gym'sTau2ToolValidatingAsyncOpenAI(already installed astau2's client) records each finished response whose
modelis the agent model name Gym configures, so retriedand discarded responses count and user-simulator calls (other model name, same server) do not.
run()adds thesum to success and failure rows: a rollout that failed on a malformed tool call still spent those tokens.
_PolicyUsageis passed into both Gym-owned callers and records right aftercreate()returns,before finish-reason or normalization rejections (that response finished). The Ray task returns
usage;_build_responsesets it.m3_agentandpointer_agentcall models through their own Anthropic clients(
pointer_agentalso through gate/planner/verifier/summarization models), so Gym cannot attribute those calls;their usage stays
null, as does the error path.prompt_tokens: null) counts as unknown; it neverfails the policy call or the rollout. A finished call whose usage is unknown makes the rollout's usage
null, notthe other calls' subtotal (
rollout-evidence.mdx: omit usage when a finished response's usage was lost). tau2 alsokeeps an empty record
null: its recorder sees only calls made through its own client, so an empty record does notprove that no policy call finished.
saved.usage.accumulate_response_usage, likesimple_agent.details →
nulldetails. anyterminal: usage kept, andnullwithoutresponse.json. osworld: sum over threeGym-owned calls incl. one rejected for
finish_reason=length, andnullfor a runner that calls models through its own client. tau2: clientrecords a discarded malformed attempt and not a user call; end-to-end through the real tau2-bench orchestrator
(empty agent message regenerated, user simulator on the same server) asserting the exact sum; failure row usage.
tau2 and osworld: a malformed usage block is recorded as unknown without raising, and a known plus an unknown call
(both orders) gives
null; two concurrent tau2 runs keep their own sums.Before the fix
This branch's tests against
main's (aa5d277e3) sixapp.pyfiles:After the fix
Validation
Agent suites (
gym env test +entrypoint=responses_api_agents/<agent>): all six green (counts above).pre-commit run --all-files: clean.Rollout evidence (hosted provider models, model-call capture on, one example row,
maincontrol vs this branch):main 0/2, this branch 2/2. Policy calls = tau2 requests with the agent model name / conversational-simulation
requests carrying the domain tools; the other captured calls are the user simulator and judge.
mainNemotron-3-Nano-30B-A3B)usagenullusage29649 / 5840 / 35489nemotron-3.5-super-vl-preview)usage25396 / 2008, cached 0, reasoning 0usageidentical(Separate runs per checkout, so the dialogues and counts differ; each row is compared with its own capture. Run on
an earlier revision of this commit with the same accounting; the final commit adds only the malformed-usage handling
and the rebase onto current
main.)Not run live:
osworld_agent(VM image),anyterminal_agentandvcqa_agent(Apptainer),image_tools_agent(no in-repo example data for this agent). Their changes are covered by the unit tests above.
Review round 1
yaoyu-33's review: tau2 and osworld summed the known calls when another finished call's usage was unknown. Both now
report
nullin that case (commitfix(agent): report unknown usage when a finished policy call lost its usage).Also added: a tau2 test with two concurrent runs, and comments on osworld's timeout and Ray-failure returns.
vcqa_agent,image_tools_agentandconversational_tool_use_simulationstill sum the known calls when one lacksusage, as before this PR; per the review, a shared completeness helper for every agent follows separately.
Compatibility
Rows from these agents now carry token counts where they had
null, and cache/reasoning sums (ornull) wherethey had 0. tau2 failure rows now carry the usage of the calls that finished. No config or API change.
🤖 Generated with Claude Code