Select fully async rollout with --fully-async - #1924
Merged
yueming-yuan merged 1 commit intoJul 30, 2026
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
guapisolo
marked this pull request as draft
July 29, 2026 03:47
yueming-yuan
force-pushed
the
review/pr-1716-fully-async-flag
branch
from
July 30, 2026 01:04
6005e24 to
382c92f
Compare
yueming-yuan
changed the base branch from
yueming/fully-async-eval-backend-refactor
to
yueming/fully-async-class-api
July 30, 2026 01:04
yueming-yuan
force-pushed
the
review/pr-1716-fully-async-flag
branch
from
July 30, 2026 01:10
382c92f to
b63a7e9
Compare
Rollout selection belongs in the argument surface, not in an environment variable that rewrites sys.argv before parsing. --rollout-function-path now defaults to None, so "the user chose one" is a plain is-None check instead of a comparison against a computed default, and resolve_rollout_function_path() is the single place that maps arguments to a rollout function. miles_validate_args rejects the configurations that cannot work: no class-based rollout API, a competing --rollout-function-path, or --colocate, which the async driver cannot honor. Evaluation keeps the standard rollout function, since fully async does not serve eval. train.py asserts the flag is off, so picking the wrong driver fails loudly. Co-authored-by: yueming-yuan <yym022502@gmail.com>
yueming-yuan
force-pushed
the
review/pr-1716-fully-async-flag
branch
from
July 30, 2026 01:17
b63a7e9 to
32fe00b
Compare
yueming-yuan
marked this pull request as ready for review
July 30, 2026 01:43
yueming-yuan
requested review from
Shi-Dong,
Zhichenzzz,
fzyzcjy,
jybsuper,
maocheng23 and
yueming-yuan
as code owners
July 30, 2026 01:43
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
yueming-yuan
merged commit Jul 30, 2026
67c5ca8
into
yueming/fully-async-class-api
31 of 41 checks passed
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.
Motivation
Selecting the fully async rollout should be part of the argument surface, not an environment variable. The previous approach (
MILES_FULLY_ASYNC=1) rewrotesys.argvbeforeparse_args()to inject--rollout-function-path, which meant a hand-rolled argparse prefix matcher to detect conflicts, validation split across two entrypoints, and configuration that never appears in--helpor in the logged config table.Retargeted onto #1717 (fully-async rewritten as
FullyAsyncRolloutFn).What this PR does
Adds
--fully-async, resolved inmiles_validate_argswhere the rest of the cross-argument checks already live:_validate_fully_async(args)asserts:MILES_EXPERIMENTAL_ROLLOUT_REFACTOR=1, otherwise the class entry point cannot be loaded at all;--rollout-function-path— detected by comparing againstdefault_rollout_function_path()(the default is now a named helper instead of an inline conditional), so no argv scanning;--colocate— the async driver rejects colocation, and failing at argument time beats failing after the placement group is built.It then sets
rollout_function_path, and pointseval_function_pathat the standard inference rollout unless the user set it explicitly —FullyAsyncRolloutFnraises on eval, and eval otherwise inherits the rollout path. That makes the flag a complete switch: the launch scripts and the e2e harness no longer spell out two function paths by hand.train.pyasserts the flag is off, so choosing the wrong driver fails loudly instead of silently running synchronously.Testing
Launch scripts, docs, and the 30B fully-async e2e case (#1717) converted to the flag, so the e2e run exercises it end to end.
pre-commit run --all-filespasses.🤖 Generated with Claude Code