[https://nvbugs/6426852][fix] Add explicit gc.collect() between del weights and… - #16144
[https://nvbugs/6426852][fix] Add explicit gc.collect() between del weights and…#16144trtllm-agent wants to merge 1 commit into
gc.collect() between del weights and…#16144Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds an explicit ChangesCUDA IPC weight update fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
9821ab7 to
ff85e5d
Compare
ff85e5d to
8a94d16
Compare
Partial-update loops on MoE models (Qwen3-30B-A3B) issue many per-suffix update_weights RPCs. Each RPC's worker side reconstructs CUDA IPC tensors via rebuild_cuda_tensor (cudaIpcOpenMemHandle) into 'weights', then drops the dict. Without an explicit gc.collect(), the underlying storages (backed by IPC mappings) can linger in unreachable cycles until the next full GC, so the following torch.cuda.ipc_collect() finds nothing to reclaim and the CUDA driver's IPC handle table saturates before the finalize step, producing cudaErrorMapBufferObjectFailed on later RPCs. Add gc.collect() between 'del weights' and 'torch.cuda.ipc_collect()' in WorkerExtension.update_weights so IPC-mapped storages are actually released before the CUDA IPC pool sweep in each iteration. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
8a94d16 to
701d5eb
Compare
allisonlim-nv
left a comment
There was a problem hiding this comment.
LGTM; forces GC before ipc_collect() so IPC weight mappings are released in the same RPC.
|
NVBug 6426852 is closed as Bug - Fixed. The linked bug appears resolved elsewhere or for a reason that does not prove this PR is redundant. This PR should be judged on its own merits; repair-bot is not auto-closing it. |
brnguyen2
left a comment
There was a problem hiding this comment.
The same failure was fixed on main by #16226 (merged 2026-07-20), which took a different approach (resource-cleanup wait in ray_executor.py) and removed the waiver plus updated the tests. This PR predates that and hasn't been rebased. Please confirm the per-RPC gc.collect() is still needed on top of #16226 — with a reproduction on the same MoE config — or close it. If it is kept: a full gc.collect() on every partial-update RPC is not free; note the per-RPC cost, since the finalize branch was deliberately written to collect once.
| # already gone; force GC so cudaIpcOpenMemHandle storages are | ||
| # released before the sweep, or the IPC handle table saturates | ||
| # across repeated RPCs. | ||
| gc.collect() |
There was a problem hiding this comment.
del weights isn't the only live reference at this point: the loop above leaves tensor, func, args, list_args, and all_handles bound in the frame, so the last IPC-mapped storage survives this gc.collect()/ipc_collect() pair and is only released when the method returns. If the handle-table pressure is real, drop those too:
| gc.collect() | |
| del weights, tensor, list_args, all_handles | |
| gc.collect() |
Summary
del weightsalone left the mapped storages in unreachable cycles; many partial-update RPCs on MoE model exhausted CUDA driver's IPC handle table.gc.collect()betweendel weightsandtorch.cuda.ipc_collect()in the per-RPC branch so IPC-mapped storages are released in the same iteration, complementing the prior producer-side lazy-replica fix.Test plan
Links
Summary by CodeRabbit
Bug Fixes
Tests