Repository navigation
[https://nvbugs/6428124][fix] Include use_host_stop_criteria in the PP send/recv payload in… #16134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3883,10 +3883,18 @@ def _ring_broadcast_sample_state( | |
| if not self.dist.is_last_pp_rank: | ||
| # Receive tokens from previous pp rank (w.r.t model forward direction) | ||
| with nvtx_range("recv_sample_state"): | ||
| sample_state.host, py_result_diffs = self.dist.recv_object( | ||
| src=self.dist.prev_pp_rank, | ||
| tag=tag, | ||
| ) | ||
| (sample_state.host, py_result_diffs, | ||
| use_host_stop_criteria) = self.dist.recv_object( | ||
| src=self.dist.prev_pp_rank, | ||
| tag=tag, | ||
| ) | ||
|
|
||
| # The last PP rank owns the sampling shape. Its fast host stop | ||
| # criteria path leaves host.finish_reasons=None, so the flag | ||
| # governing update_requests's branching must ride the ring with | ||
| # the host state or non-last ranks index into an empty list. | ||
| if hasattr(sample_state, "use_host_stop_criteria"): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please drop it and keep only the waiver removal for |
||
| sample_state.use_host_stop_criteria = use_host_stop_criteria | ||
|
|
||
| for request, py_result_diff in zip(requests, py_result_diffs): | ||
| request.py_result.apply_diff(py_result_diff) | ||
|
|
@@ -3904,7 +3912,8 @@ def _ring_broadcast_sample_state( | |
| self.wait_on_pp_send_handles(self.send_handles, microbatch_id) | ||
| with nvtx_range("send_sample_state"): | ||
| self.send_handles[microbatch_id] = self.dist.isend_object( | ||
| (sample_state.host, py_result_diffs), | ||
| (sample_state.host, py_result_diffs, | ||
| getattr(sample_state, "use_host_stop_criteria", False)), | ||
| dest=self.dist.next_pp_rank, | ||
| tag=tag, | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
SampleStatesubclass definesuse_host_stop_criteria, so thishasattris always False and the assignment never executes — paired withgetattr(sample_state, "use_host_stop_criteria", False)on the send side, the flag round-trips as a constantFalse. If a flag really is needed here it has to be a declared field on the sample-state dataclass, set by the sampler; ahasattrguard just hides that it isn't.