Skip to content

[ckpt]: nccl broadcast bucket size fix#7107

Open
parinayc20 wants to merge 1 commit into
verl-project:mainfrom
parinayc20:fix/nccl-broadcast-bucket-size
Open

[ckpt]: nccl broadcast bucket size fix#7107
parinayc20 wants to merge 1 commit into
verl-project:mainfrom
parinayc20:fix/nccl-broadcast-bucket-size

Conversation

@parinayc20

Copy link
Copy Markdown
Contributor

What does this PR do?

The NCCL checkpoint engine always broadcasts the full bucket_size buffer per bucket, even when the bucket was only partially filled (which is the common case for the last bucket of every send_weights call, and any time the parameter stream doesn't divide evenly into bucket_size). This wasted NCCL bandwidth is proportional to the unused tail of each bucket.

This PR fixes it by passing the bucket length as metadata, ensuring both sides agree on the broadcast size before the broadcast begins.

@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 updates the NCCL checkpoint engine to handle variable-sized buckets during weight transmission. Specifically, it slices the broadcast buffers to the actual offset length and includes this length in the metadata. It also updates the byte-tracking logic to accumulate the actual transmitted length instead of the fixed bucket size. There are no review comments to address.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

group_name=self.group_name,
bucket=send_buf,
metadata={"bucket_meta": bucket_meta, "is_last": False},
bucket=send_buf[:offset],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

recv_buf should be also restricted by offset: recv_buf[:offset]?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The recv_buf restriction happens in BroadcastOperation._run() before calling the final broadcast collective:
https://github.com/verl-project/verl/pull/7107/files#diff-2cdb965b958cf2a60dd21504e41bd098a583b4d2b82291a612dc4c5dbae85b77R88

The recv_buf cannot be restricted when using it to create the BroadcastOperation in the receive_weights() function itself, because the offset is not known until that point.

@parinayc20
parinayc20 requested a review from wuxibin89 July 23, 2026 07:05
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