feat: route NeMo-Gym rollout rows across sharded actors - #3372
Draft
ananthsub wants to merge 1 commit into
Draft
Conversation
Replace the single run_rollouts call with a shard-aware dispatch: bucket prompt groups by the shard hosting their agent, then merge the K resulting streams into the one row stream the accumulator already consumes. Bucketing is by whole group, never by row. A group split across shards would deadlock any verifier that scores rollouts against their peers, since those buffer a group in one process and block until it is complete. Rows keep the batch-global _rowidx stamped before bucketing, so group assembly and ordering downstream are untouched. A bare actor handle in task_to_env reads as the one-shard, one-replica case it already is, so call sites that predate sharding keep working and the unsharded path is unchanged: one bucket, one stream, and the actor's own timing metrics passed through verbatim. With several shards, each reports its numbers under its own prefix and the roll-up takes the maximum, since shards run concurrently and the step is gated by the slowest rather than by their sum. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do ?
Routes rollout rows to the right actor and merges the results back into one stream. With K actors now buildable (PR 5 of this stack), a training step has to decide which rows go where and then reassemble what comes back.
Dispatch is by whole prompt group, never by row.
_bucket_nemo_gym_rows_by_shardwalks the batch in groups ofnum_generations, resolves the group'sagent_refto a shard, and sends the entire group to one actor. Splitting a group across actors would deadlock any verifier that scores rollouts against their peers —genrm_comparebuffers a whole group in one process and blocks until it is complete, so each half would wait forever for rollouts sitting in the other. A group whose rows name agents in different shards is rejected rather than split; group members inherit the prompt'sagent_refso this should not happen, but the check is cheap and catches upstream regressions.Rows keep the
_rowidxstamped over the whole batch before bucketing, so the accumulator downstream never learns that shards exist.Fan-in preserves streaming.
run_rolloutsis an async generator, so there is nothing to gather:_merge_nemo_gym_shard_streamsreplaces one stream with an interleave of K, yielding rows as they complete and feeding them to the existing accumulator unchanged. With one bucket it degenerates to that single stream, which is what keeps the unsharded path identical. Leaving the merge early — a downstream error, or the consumer closing the generator — cancels the in-flightanexttasks rather than stranding them.Timing metrics are reported per shard and rolled up.
_merge_nemo_gym_timing_metricsemits each shard's numbers under{timer_prefix}/shard/{name}/...and takes the maximum for the rolled-up value rather than the sum, because shards run concurrently and the step is gated by the slowest one.Two smaller changes fall out of this:
task_to_env["nemo_gym"]now accepts either aNemoGymShardSetor a bare actor handle, the latter read as the one-shard, one-replica case (as_nemo_gym_shard_set). This is what lets the router land before any entrypoint builds a real shard set.shutdown_environmentshad assumed every environment is a Ray actor handle. A shard set is not — it owns actors — so teardown now asks an owner object to shut itself down and does not escalate toray.kill, since there is no single handle to kill and the owner has already reported which shard would not stop.Issues
None.
Usage
No user-facing change. Unsharded jobs take the same path as before: one bucket over one stream.
Before your PR is "Ready for review"
Pre checks:
Additional Information
agent_ref) and cannot change mid-job — shard contents freeze whenRunHelperserializes each child's config atPopentime. Which replica within that shard is chosen at dispatch, round-robin.postprocess_results_pctthe worst shard's share rather than a batch-wide average. A true batch-wide percentage needs each shard's rollout total, which the actor pops before returning; exposing it is a small upstream Gym change and is not worth coupling to this stack. Per-shard percentages are reported and are exact.