Support DSV4 disaggregated launcher - #1310
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for non-colocated actor and rollout configurations in the DeepSeek-V4 training script, adding new arguments (colocate, actor_num_nodes, actor_num_gpus_per_node, and rollout_num_gpus) along with validation logic in __post_init__. It also updates the parallel configuration logic, environment variables, and chat template arguments, and introduces a conditional enable_miles_router flag. The feedback recommends replacing bare assert statements with explicit ValueError exceptions to prevent uninformative errors and avoid issues when Python is run with optimization flags.
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.
| if self.actor_num_nodes is None: | ||
| if self.colocate: | ||
| self.actor_num_nodes = self.num_nodes | ||
| else: | ||
| assert self.num_nodes % 2 == 0 | ||
| self.actor_num_nodes = self.num_nodes // 2 | ||
| assert self.actor_num_nodes <= self.num_nodes | ||
| assert self.actor_num_gpus_per_node <= self.num_gpus_per_node | ||
| if self.rollout_num_gpus is None: | ||
| if self.colocate: | ||
| self.rollout_num_gpus = self.actor_num_nodes * self.actor_num_gpus_per_node | ||
| else: | ||
| self.rollout_num_gpus = (self.num_nodes - self.actor_num_nodes) * self.num_gpus_per_node | ||
| if not self.colocate: | ||
| assert self.rollout_num_gpus > 0 |
There was a problem hiding this comment.
Using bare assert statements for input validation in __post_init__ can lead to uninformative AssertionError exceptions and may be optimized away if Python is run with the -O flag. It is better to raise a ValueError with a descriptive error message to improve usability and robustness.
if self.actor_num_nodes is None:
if self.colocate:
self.actor_num_nodes = self.num_nodes
else:
if self.num_nodes % 2 != 0:
raise ValueError(
f"num_nodes ({self.num_nodes}) must be even for non-colocated training "
"when actor_num_nodes is not specified."
)
self.actor_num_nodes = self.num_nodes // 2
if self.actor_num_nodes > self.num_nodes:
raise ValueError(f"actor_num_nodes ({self.actor_num_nodes}) cannot exceed num_nodes ({self.num_nodes}).")
if self.actor_num_gpus_per_node > self.num_gpus_per_node:
raise ValueError(
f"actor_num_gpus_per_node ({self.actor_num_gpus_per_node}) cannot exceed "
f"num_gpus_per_node ({self.num_gpus_per_node})."
)
if self.rollout_num_gpus is None:
if self.colocate:
self.rollout_num_gpus = self.actor_num_nodes * self.actor_num_gpus_per_node
else:
self.rollout_num_gpus = (self.num_nodes - self.actor_num_nodes) * self.num_gpus_per_node
if not self.colocate and self.rollout_num_gpus <= 0:
raise ValueError("rollout_num_gpus must be greater than 0 when colocate is False.")| if args.enable_r3: | ||
| misc_args += "--use-rollout-routing-replay " | ||
| if args.enable_miles_router: | ||
| assert args.enable_r3 |
There was a problem hiding this comment.
Using a bare assert statement here can raise an uninformative AssertionError if enable_miles_router is enabled without enable_r3. Raising a ValueError with a clear, actionable error message is much more user-friendly.
| assert args.enable_r3 | |
| if not args.enable_r3: | |
| raise ValueError("enable_miles_router requires enable_r3 to be True.") |
ee0ea84 to
13b201f
Compare
13b201f to
4705f59
Compare
Summary
--rollout-num-nodesto derive actor nodes and rollout GPU counts from--num-nodesand--num-gpus-per-node--use-rollout-routing-replaywithout enabling Miles routerthinking_modechat-template kwargSGLANG_HEALTH_CHECK_TIMEOUT=120to avoid false rollout health failures during long detokenizer gapsValidation
pre-commit run --all-filesRelated PRs