Skip to content

Make the coding-agent RL loop train on ROCm/MI355X - #415

Open
lizamd wants to merge 11 commits into
vllm-project:mainfrom
lizamd:rocm-agent-rl
Open

lizamd wants to merge 11 commits into
vllm-project:mainfrom
lizamd:rocm-agent-rl

Conversation

@lizamd

@lizamd lizamd commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Includes @aoshen02's local multi-turn smoke test (aoshen02#4) with his agreement, then makes the loop actually train on ROCm/MI355X.

Why it couldn't train

The smoke test runs end to end but cannot learn on any platform:

--n-samples-per-prompt 1
--rollout-temperature 0.0

GRPO computes its advantage within a prompt's sample group. One sample gives an advantage of identically zero; greedy decoding makes n samples identical, so group variance is zero too. Both have to change — fixing either alone still gives no gradient.

What's in here

  • Working GRPO defaults, with the measurements behind them in the commit messages
  • Prompt truncation instead of returning an empty TurnRecord on overflow (the empty reply made the CLI exit 1, so the overflow and the agent_exit_code=1 after it were one bug)
  • Tool-call repair before vLLM's parser runs — its Hermes parser doesn't raise, it logs and returns tools_called=False, so post-hoc recovery hid the failure instead of fixing it
  • max_turns_per_sid wired through; it was fully implemented but generate.py never passed it
  • Dockerfile.sympy-23950 fixed — it no longer builds anywhere since bullseye went EOL and its security Release file expired
  • IMAGE and WEIGHT_TRANSPORT parameterised

Context budget

Turn 1 spent 24118 of a 40960-token window before any conversation accumulated: 6315 system prompt, 18730 tool schemas across 19 tools. A SWE task uses three (Bash, Read, Edit = 3848). Dropping the rest takes turn-1 to 9878 and usable turns from 13 to 59. A 262144-token window hides this; it appears immediately on a smaller model.

Verification

vllm/vime-rocm:coding-agent-rl, 8×MI355X, Qwen3-32B: 4 episodes, nccl weight sync 6.5s, 0 context overflows, 0 agent_exit_code=1, 0 OOM.

The training behaviour behind the defaults was measured on the pre-rebase tree — a 30-step run where pass rate rose 0.254 → 0.596 against a 0.3125 baseline (t = +4.19, df=28), entropy rising rather than collapsing. That has not been re-collected on this rebase; the run above only confirms the branch starts, rolls out, and syncs weights.

🤖 Generated with Claude Code

aoshen02 and others added 9 commits September 8, 2026 20:56
max_turns_per_sid is fully implemented in adapters/common.py -- constructor
argument, per-sid counter, a 429 once the cap is passed -- but generate.py
never passed it, so it stayed None everywhere outside unit tests.

Wire it through SweConfig from VIME_MAX_TURNS_PER_SID, following the existing
fork_merge_threshold idiom.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Context overflow was not a tuning problem. Instrumenting prompt size per turn
showed the fixed cost dominates: turn 1 already spent 24118 of a 40960-token
window before any conversation accumulated, and individual turns jumped past
100k (max observed 108518). Overflow appeared as early as turn 1, so no turn
cap can prevent it, and clamping tool results does nothing -- no observation
exceeded 4000 characters.

The old path returned an empty TurnRecord on overflow, so the CLI got a
zero-token reply and exited 1. The overflow and the agent_exit_code=1 that
followed it were one bug. Prompts are now truncated in the middle -- the head
keeps the system prompt and tool schemas, the tail keeps recent turns -- with a
reserve so there is always room to generate.

Malformed tool calls are repaired before vLLM's parser runs rather than after.
vLLM's Hermes parser does not raise on bad JSON; it logs and returns
tools_called=False, so post-hoc recovery left the parser's own exception in the
log and hid the failure rather than fixing it. Repairing first lets the strict
parse succeed. Blocks that cannot be repaired pass through untouched and still
drop -- reconstructing a truncated tool call is guesswork.

Prompt-budget instrumentation (per-turn size, system prompt and per-tool token
cost) is kept but gated behind DEBUG.

Measured over 64 episodes / 859 turns:

  context overflow      33 -> 0
  agent_exit_code=1        -> 0
  parse failures      2339 log lines (960 eps) -> 9 actual failures (1.0%)
  truncated_ratio          0.0 (the reserve does not cut generation short)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
run_h200.sh targets CUDA and runs rollout only (--debug-rollout-only,
--num-rollout 1). This pair runs actual training on 8x MI355X.

What matters beyond the platform swap:

- --entrypoint bash. The ROCm image's ENTRYPOINT is `sleep`, so a launcher
  without it starts a container that ignores its command. The CUDA launcher
  has the same shape and would hit this on any image with an ENTRYPOINT.
- --update-weight-transport disk: the image's vLLM predates
  WeightTransferTrainerFactory.
- Context 40960 and Qwen3 stop tokens: 262144 exceeds Qwen3's
  max_position_embeddings, and the CUDA script's stop-token id belongs to a
  different tokenizer.
- Defaults are the ones that actually trained, with the evidence in comments:
  n-samples-per-prompt 8 and temperature 1.0 (either at its old value makes the
  GRPO advantage identically zero), lr 3e-6, entropy-coef 0.01.
- global-batch-size is derived, not free -- vime asserts it equals
  rollout_batch_size * n_samples_per_prompt // num_steps_per_rollout.
- Orchestration tools are dropped from the prompt. Measured, the schema cost
  18730 tokens across 19 tools while a SWE task uses three (Bash 685+2765,
  Read, Edit = 3848); Workflow alone was 5736. Turn-1 prompt 24118 -> 9878,
  usable turns 13 -> 59. The existing --disallowedTools entries proved the
  mechanism: a disallowed tool's schema is not sent at all.
- Every knob is env-parameterised and forwarded explicitly; the launcher
  silently drops anything missing from its -e list.

TP=8 is load-bearing rather than only a memory choice: dp_schedule.py sets
align_to = dp_size, and per-turn sample counts are data-dependent, so a smaller
TP can fail with "could only produce N mbs after maximal splitting".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Subset of sympy-10.jsonl kept to the instances Qwen3-32B solves sometimes but
not always, measured over 80 episodes (10 tasks x 8 samples):

  24539  4/8 = 50.0%      23824  3/8 = 37.5%
  22914  2/8 = 25.0%      23950  1/8 = 12.5%
  the other six          0/8 =  0.0%

GRPO derives its advantage within a prompt's sample group, so a task that is
always solved or never solved contributes exactly zero gradient while still
costing full rollout time. Training on these four keeps every group
informative; on the full ten, six of ten groups are dead weight.

These pass rates are specific to Qwen3-32B -- a different model needs the
measurement redone. Group size changes the answer too: at n=4 the same model
measured 2/10 trainable, because a task with a true 20% pass rate reads as 0/4
about 41% of the time. 24539 and 22914 both looked dead before n was raised.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two bugs found while verifying that the documented defaults are the ones that
actually reach train.py.

A comment placed between backslash-continued lines silently truncates the
command. `--rollout-batch-size ${RB:-4} \` followed by a comment line ends the
continuation, so every argument after it was dropped and Megatron fell back to
its own defaults. `bash -n` does not catch this -- the result is still valid
shell. Rationale comments now sit above the command block instead of inside it.

The launcher forwarded `-e LR="${LR:-1e-6}"` and friends, so it injected its
own default into the container and run_rl.sh's `${LR:-3e-6}` never saw an unset
variable. Editing a default in one file therefore had no effect. The launcher
now forwards `"${LR}"`, passing an empty string when unset, which the runner's
`${LR:-default}` falls back on -- one place to change a default, not two.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This image no longer builds on a current host:

  E: Release file for http://deb.debian.org/debian-security/dists/
     bullseye-security/InRelease is expired (invalid since 22h 53min 27s)

Debian bullseye has reached end of life and its security Release file has
expired, so `apt-get update` fails and takes the build with it. Anyone trying
to reproduce this benchmark today hits it.

The install was never needed: python:3.10-bullseye already ships git 2.30.2,
which is the only thing the layer was there to provide. Removing it makes the
build independent of Debian's repository state rather than working around the
expiry with Acquire::Check-Valid-Until.

Verified: image builds clean, 1.83GB.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The launcher runs under `set -euo pipefail`. Forwarding "${MAX_TURNS}" to let
run_rl.sh own the default therefore aborted the launcher outright with
"MAX_TURNS: unbound variable" instead of passing an empty value.

"${VAR-}" expands to empty when unset without tripping set -u, and run_rl.sh's
"${VAR:-default}" falls back on the empty string -- so defaults still live in
exactly one place.

Verified on 8x MI355X with Qwen3-32B, one full rollout+train iteration:

  defaults reaching train.py   lr 3e-06, entropy_coef 0.01,
                               n_samples_per_prompt 8, global_batch_size 32
  32 episodes                  multiple tasks scored reward=1.00
  context overflow             0
  agent_exit_code=1            0
  OOM / batch-alignment        0
  training step                Timer train end 607.9s
                               loss -0.759, entropy 0.407, grad_norm 1.539

One error class remains and is NOT addressed here: vLLM returned
400 "Out of range float values are not JSON compliant: nan" 30 times across 9
sessions. It is pre-existing rather than a side effect of prompt truncation --
7 of those 9 sessions were never truncated (truncation fired on 6 sessions,
overlap of 2). Episodes still completed and scored, so it degrades throughput
rather than correctness.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The runner hardcoded vllm/vime-rocm:latest, whose vLLM (0.22.1rc1, built
2026-07-15) predates several APIs current vime needs -- notably
vllm.entrypoints.launchers.cli_args, which makes the tree fail to start at all.

IMAGE and WEIGHT_TRANSPORT are now overridable, defaulting to
rocm/pytorch-private:vime-09-08 (vLLM 0.28.1rc1) and nccl. On that image the
older workarounds are unnecessary: ENTRYPOINT is [] with CMD /bin/bash rather
than sleep, WeightTransferTrainerFactory exists so weight sync no longer has to
fall back to disk, and the render group is present.

Note --update-weight-transport takes nccl or disk; ipc is a vLLM-internal
weight_transfer_config backend, not a vime choice.

Verified on the new image: 4 episodes, 2 scored reward=1.00, nccl weight sync
in 6.5s, 0 context overflows, 0 agent_exit_code=1, 0 OOM.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@read-the-docs-community

read-the-docs-community Bot commented Sep 9, 2026

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a local multi-turn smoke test environment for coding agents, including local Docker sandbox execution, run scripts, and enhanced tool-call parsing to handle invalid JSON escapes. The review feedback highlights several critical areas for improvement: addressing hardcoded paths to ensure portability, preventing subprocess leaks on cancellation, ensuring failed executions are properly traced, fixing potential negative budget calculations for small context windows, storing session-specific state correctly, and refining token count calculations and regex patterns for robust parsing.

# ROCm/MI355X (gfx950) port of run_h200.sh.
set -euo pipefail

ROOT=${ROOT:-/mnt/m2m_nobackup/lizli102/vime-agent-smoke}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Parameterize or generalize the hardcoded path /mnt/m2m_nobackup/lizli102/vime-agent-smoke to make the script portable for other users.

Suggested change
ROOT=${ROOT:-/mnt/m2m_nobackup/lizli102/vime-agent-smoke}
ROOT=${ROOT:-${HOME}/vime-agent-smoke}

Comment on lines +23 to +28
-v /home/lizli102/vime:/root/vime \
-v "${ROOT}/models:/work/models" \
-v "${ROOT}/assets:/work/assets" \
-v "${ROOT}/tasks:/work/tasks:ro" \
-v "${ROOT}/runs:/work/runs" \
-v /home/lizli102:/host \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Avoid hardcoding the user home directory /home/lizli102 in volume mounts. Use ${HOME} or a parameter instead to ensure portability.

Suggested change
-v /home/lizli102/vime:/root/vime \
-v "${ROOT}/models:/work/models" \
-v "${ROOT}/assets:/work/assets" \
-v "${ROOT}/tasks:/work/tasks:ro" \
-v "${ROOT}/runs:/work/runs" \
-v /home/lizli102:/host \
-v "${HOME}/vime:/root/vime" \
-v "${ROOT}/models:/work/models" \
-v "${ROOT}/assets:/work/assets" \
-v "${ROOT}/tasks:/work/tasks:ro" \
-v "${ROOT}/runs:/work/runs" \
-v "${HOME}:/host" \

Comment on lines +105 to +115
async def _run(*argv: str, check: bool = False) -> ExecResult:
process = await asyncio.create_subprocess_exec(
*argv,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
)
stdout, stderr = await process.communicate()
result = process.returncode, stdout.decode(errors="replace"), stderr.decode(errors="replace")
if check and process.returncode != 0:
raise RuntimeError(f"command failed ({process.returncode}): {' '.join(argv)}\n{result[2]}")
return result

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If _run is cancelled (e.g., due to a timeout in asyncio.wait_for inside exec), the spawned subprocess is not terminated and will leak as a zombie or background process. Use a try...finally block or catch asyncio.CancelledError to terminate the process.

Suggested change
async def _run(*argv: str, check: bool = False) -> ExecResult:
process = await asyncio.create_subprocess_exec(
*argv,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
)
stdout, stderr = await process.communicate()
result = process.returncode, stdout.decode(errors="replace"), stderr.decode(errors="replace")
if check and process.returncode != 0:
raise RuntimeError(f"command failed ({process.returncode}): {' '.join(argv)}\n{result[2]}")
return result
@staticmethod
async def _run(*argv: str, check: bool = False) -> ExecResult:
process = await asyncio.create_subprocess_exec(
*argv,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
)
try:
stdout, stderr = await process.communicate()
except asyncio.CancelledError:
try:
process.terminate()
await process.wait()
except ProcessLookupError:
pass
raise
result = process.returncode, stdout.decode(errors="replace"), stderr.decode(errors="replace")
if check and process.returncode != 0:
raise RuntimeError(f"command failed ({process.returncode}): {' '.join(argv)}\n{result[2]}")
return result

Comment on lines +56 to +64
result = await asyncio.wait_for(self._run(*argv, check=check), timeout=timeout)
self._trace(
"exec",
user=user,
cmd=cmd,
returncode=result[0],
stdout=result[1],
stderr=result[2],
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When check=True is passed to exec, any command failure will raise a RuntimeError inside _run, bypassing the self._trace call entirely. This prevents failed executions from being logged in the sandbox events trace. Call _run with check=False and manually check the return code after tracing.

Suggested change
result = await asyncio.wait_for(self._run(*argv, check=check), timeout=timeout)
self._trace(
"exec",
user=user,
cmd=cmd,
returncode=result[0],
stdout=result[1],
stderr=result[2],
)
result = await asyncio.wait_for(self._run(*argv, check=False), timeout=timeout)
self._trace(
"exec",
user=user,
cmd=cmd,
returncode=result[0],
stdout=result[1],
stderr=result[2],
)
if check and result[0] != 0:
raise RuntimeError(f"command failed ({result[0]}): {' '.join(argv)}\n{result[2]}")
return result

Comment thread vime/agent/adapters/common.py Outdated
Comment on lines +538 to +540
_reserve = min(1024, max(256, session.max_context_tokens // 8))
_budget = session.max_context_tokens - _reserve
if len(prompt_ids) > _budget:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If session.max_context_tokens is small (e.g., less than 256), _reserve will be larger than max_context_tokens, resulting in a negative _budget. This causes incorrect slicing and potential errors. Cap _reserve at half of max_context_tokens to ensure _budget remains positive.

        _reserve = min(1024, max(256, session.max_context_tokens // 8))
        _reserve = min(_reserve, session.max_context_tokens // 2)
        _budget = session.max_context_tokens - _reserve

Comment thread vime/agent/adapters/common.py Outdated
Comment on lines +517 to +518
if _n <= 1 and not getattr(adapter, "_logged_schema", False):
adapter._logged_schema = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

_logged_schema is set on the adapter instance, which is a shared singleton across all sessions. This prevents the schema from being logged for any subsequent sessions/tasks that might have different tools or system prompts. Store _logged_schema on the session object instead.

Suggested change
if _n <= 1 and not getattr(adapter, "_logged_schema", False):
adapter._logged_schema = True
if _n <= 1 and not getattr(session, "_logged_schema", False):
session._logged_schema = True

Comment thread vime/agent/adapters/common.py Outdated
try:
_tools = (body or {}).get("tools") or []
_sys = (body or {}).get("system")
_st = len(adapter.tokenizer.encode(json.dumps(_sys))) if _sys else 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

_sys is a string representing the system prompt. Using json.dumps(_sys) wraps it in quotes and escapes characters, which inflates the token count and makes the logged system prompt size inaccurate. Encode the raw string directly.

Suggested change
_st = len(adapter.tokenizer.encode(json.dumps(_sys))) if _sys else 0
_st = len(adapter.tokenizer.encode(_sys)) if _sys else 0

Comment on lines +19 to +22
def _trace_dir() -> Path:
path = Path(os.environ["VIME_LOCAL_SANDBOX_TRACE_DIR"]) / _instance_id.get()
path.mkdir(parents=True, exist_ok=True)
return path

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If VIME_LOCAL_SANDBOX_TRACE_DIR is not set in the environment, _trace_dir() will raise a KeyError and crash the script. Handle this gracefully by providing a default fallback path (e.g., /tmp/vime-trace).

Suggested change
def _trace_dir() -> Path:
path = Path(os.environ["VIME_LOCAL_SANDBOX_TRACE_DIR"]) / _instance_id.get()
path.mkdir(parents=True, exist_ok=True)
return path
def _trace_dir() -> Path:
path = Path(os.environ.get("VIME_LOCAL_SANDBOX_TRACE_DIR", "/tmp/vime-trace")) / _instance_id.get()
path.mkdir(parents=True, exist_ok=True)
return path

Comment thread vime/agent/parsing.py


_HERMES_RE = re.compile(r"<tool_call>\s*(\{.*?\})\s*</tool_call>", re.DOTALL)
_ESCAPE_FIX_RE = re.compile(r'\\(?!["\\\\/bfnrtu])')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _ESCAPE_FIX_RE negative lookahead prevents matching \u entirely. However, if the model outputs an invalid \u escape (such as \user or \utility in file paths), json.loads will still fail with a JSONDecodeError. Update the regex to only skip \u if it is actually followed by 4 hex digits.

Suggested change
_ESCAPE_FIX_RE = re.compile(r'\\(?!["\\\\/bfnrtu])')
_ESCAPE_FIX_RE = re.compile(r'\\(?!["\\\\/bfnrt]|u[0-9a-fA-F]{4})')

lizamd and others added 2 commits September 9, 2026 04:03
Three real defects in the truncation and instrumentation added by this branch:

- A small max_context_tokens drove the reserve above the window itself: at 256
  the budget came out 0 and the head/tail slices below went silently wrong. The
  reserve is now also capped at half the window. Behaviour at the sizes actually
  used is unchanged (40960 still reserves 1024).
- _logged_schema was set on the adapter, which is shared across sessions, so
  only the first session ever logged its tool schema. It now lives on the
  session.
- The system prompt was tokenised through json.dumps, counting the added quotes
  and escapes. It is now encoded directly when it is already a string.

The launcher no longer names a user directory: /host mounts ${HOME} and the repo
mount is derived from the script's own location. ROOT keeps a ${HOME} fallback
but documents what it is for -- it holds tens of GB of weights per model, so it
wants node-local scratch; putting it on a network filesystem bottlenecks
rollouts. VIME_SMOKE_ROOT or ROOT overrides it.

Remaining review comments are on sandbox.py and generate.py from the base
smoke-test commit and are left for a separate change.

Verified after the change: 4 episodes, 2 scored reward=1.00, nccl weight sync,
0 context overflows, 0 agent_exit_code=1, 0 OOM.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g optional

Three defects in the local Docker sandbox, all reachable on every tool call.

_run left its child alive when cancelled. exec() wraps it in asyncio.wait_for,
so any timeout abandoned a running docker CLI process; over a training run those
accumulate. It now kills and reaps the child before re-raising.

check=True raised inside _run, before exec() reached self._trace, so a failing
command -- exactly the one whose trace is worth keeping -- produced no trace at
all. The check moved after tracing. __aenter__ no longer passes check either; it
inspects the return code directly so a failed container start still records its
returncode and stderr before raising.

VIME_LOCAL_SANDBOX_TRACE_DIR was optional in sandbox.py (skip tracing when
unset) but required in generate.py, which raised KeyError and killed the rollout
over a debugging aid. generate.py now follows sandbox.py and returns None.

Verified: 4 episodes, 3 scored reward=1.00, 0 context overflows, 0
agent_exit_code=1, 0 OOM, nccl weight sync.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants