feat(trtllm): support agentic training for SWE rollout - #3130
Conversation
bc92d0a to
c6b1fd9
Compare
c6b1fd9 to
231a945
Compare
292c5e2 to
c2f4d83
Compare
yuki-97
left a comment
There was a problem hiding this comment.
thanks @hchings , left some comments.
this PR support gym path, it's worth to add a functional test and nightly test for this, you can take 3eedbc6 as an example for nightly test.
Reviewed via hchings#4 (delta against shuyix/trtllm), scoped to the agentic-SWE + TRT-LLM HTTP server changes. Scripts and recipe YAMLs skipped per the PR note. Comments target the shared code paths.
c2f4d83 to
6947764
Compare
terrykong
left a comment
There was a problem hiding this comment.
Thanks @hchings — the TRT-LLM agentic path is a valuable addition, and the convergence-parity scoping (with perf deferred) is a reasonable split. This pass was reviewed via hchings#4 (the true delta against shuyix/trtllm, since #3130 is stacked on the not-yet-merged #2420), scoped to the 14 files that PR adds. Same head SHA, so comments anchor here.
Rebase needed: the PR currently has merge conflicts against main (mergeable: CONFLICTING) — please rebase on main (or the refreshed #2420 base) and resolve before merge.
A few non-blocking notes (details in the inline comments where actionable):
- Verified positive: the new
test_replace_prefix_tokens_qwen3_think_shift_picks_assistant_eos_not_user_eosis a genuine regression test — we executed both the old positional and new EOS-count algorithms on its inputs; the old one drops the intervening user turn, the new one preserves it. - We also verified the open logprob question on the earlier review thread is not a bug on this code path (evidence permalinks in the thread reply) — no change needed there.
- The pure helpers in
trtllm_http_server.py(_parse_tool_calls,_to_int_ids,_compute_splice_inputs, ...) currently have no unit coverage; they are CPU-testable with a stub tokenizer. We have a verified-passing 8-test suite for them — happy to share it on this PR if useful. - Nightly/functional coverage for the gym+trtllm path is still on your TODO — +1 to landing at least one entry with (or right after) this PR.
- For the perf follow-up PRs:
asyncio.to_thread(llm.generate, ...)uses asyncio's default executor (max ~32 threads), which caps concurrent HTTP generations well belowmax_batch_size: 256— worth revisiting when you optimize throughput (e.g. a dedicated executor orllm.generate_async).
Generated by Claude Code
|
did you measure logp error with stripped thinking blocks (Qwen3)? wouldn't it be high without something like an attn mask in this case if you are recomputing? |
bdaadcb to
ec1a8dd
Compare
15a5c98 to
c679c58
Compare
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
77affc7 to
74769c6
Compare
|
/ok to test 74769c6 |
terrykong
left a comment
There was a problem hiding this comment.
Reviewing 5bd494e4 "fix validation, aligned w/ vllm's behavior" with one goal: match how vLLM does this today, and keep this PR to the status quo.
The short version: this change actually diverges from vLLM rather than mirroring it, and as written it's inert. Detail + receipts in the inline comment. Suggested path is to revert to the unconditional assert for this PR (which is the vLLM behavior) and unblock validation config-side, deferring real per-split sampling to a follow-up.
Generated by Claude Code
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
74769c6 to
7c33404
Compare
terrykong
left a comment
There was a problem hiding this comment.
lgtm modulo proof that examples/swe_bench/grpo_qwen3_30b_async_swe_trtllm.yaml can run (was concern about validation sampling params causing error so need to make sure it's also passing the same sampling params)
|
/ok to test 7c33404 |
|
/ok to test 7c33404 |
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
|
/ok to test e62fb40 |
|
/ok to test bffcfbe |
Signed-off-by: Erin Ho <erinh@nvidia.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
|
/ok to test 07dbfd4 |
What does this PR do ?
Add support of agentic training for SWE with TRTLLM rollout.
This MR builds on top of #2420 (merged).
[‼️ Notes for reviewers ‼️ ]
Scope of testing:
1.
Qwen3.5-35B-A3B4 nodes X GB200/GB300Retested w/ latest commits as of
7/25.2.
Qwen3-30B-Thinking, 16 nodes X H100seos-fix-0630-r3.Last tested til commit 0591b0e.
Tentatively remove Thinking scripts in 07dbfd4 and will add them back in a follow-up MR.
Issues
TRTLLM-13972
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information