Skip to content

fix(args): treat the fully-async rollout path as the mode that selects it - #3587

Merged
Shi-Dong merged 3 commits into
mainfrom
shi/normalize-fully-async-selection
Sep 22, 2026
Merged

Shi-Dong merged 3 commits into
mainfrom
shi/normalize-fully-async-selection

Conversation

@Shi-Dong

@Shi-Dong Shi-Dong commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

--fully-async selects FullyAsyncRolloutFn, but the class can also be named directly:

--rollout-function-path miles.rollout.fully_async_rollout.FullyAsyncRolloutFn

That spelling loads the same persistent producer while args.fully_async stays False, so the run silently skips everything the mode implies:

  • the mode's validation — --colocate, --partial-rollout, legacy rollout v1, --pause-generation-mode abort, multi-LoRA, and the rest are all checked inside if args.fully_async:
  • the guard in train.py that refuses the sync driver, so the producer can run with no async driver at all

examples/infra_features/fully_async/run_qwen3_5_4b_fully_async_eval.py uses exactly this spelling today. The mismatch surfaces during training rather than at launch.

Change

Normalize the path spelling into the flag before any validation runs: when --rollout-function-path names FullyAsyncRolloutFn, enable --fully-async and let the flag own the selection. The two spellings are one selection, so the existing "pass only one" assertion now fires only when the path names a different function.

The path string becomes a shared constant, so the override and the normalization cannot drift apart. Only the exact class is recognized: a subclass still passes --fully-async explicitly.

Behavior

Arguments Before After
--fully-async mode on unchanged
--rollout-function-path <FullyAsyncRolloutFn> producer on, mode off mode on
both, agreeing AssertionError mode on
--fully-async + a different path AssertionError unchanged
any other path plugin unchanged

A run that was already correct is unaffected. A run that was silently wrong now either works or fails at launch with the mode's own message.

Tests

Five cases in tests/fast/utils/test_arguments.py: the path enables the mode, the mode's constraints then apply (--colocate is rejected), the agreeing pair is accepted, a conflicting pair still raises, and an ordinary plugin path is untouched.

Verified locally:

  • the five new cases pass; on unmodified main the path-plus---colocate combination is accepted and returns fully_async=False
  • tests/fast/rollout/test_fully_async_rollout.py and tests/fast/test_train_async.py: 34 passed
  • all pre-commit hooks pass

No GPU run.

Unrelated file: scripts/ci/runner_utilization_report.py

That file landed in #3586 unformatted, so the repo-wide pre-commit job fails on every branch cut from main since — including this one, on a file it does not touch. The last commit here is black alone (the pinned 24.3.0, applied through the repo's own hook); the file's AST is unchanged. pre-commit run --all-files passes with it and fails without it. Drop that commit if main is fixed first.

…s it

--fully-async selects FullyAsyncRolloutFn, but naming that class through
--rollout-function-path selected the same producer while leaving the mode
off. Such a run skipped every --fully-async check (colocate, partial
rollout, legacy rollout v1, pause mode, multi-LoRA) and the guard in
train.py that sends the mode to the async driver, so a misconfiguration
surfaced during training instead of at launch.

Normalize the path spelling into the flag before validation runs. A
subclass still has to pass --fully-async, since only the exact class is
recognized.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@Shi-Dong

Copy link
Copy Markdown
Collaborator Author

@claude review always

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

Comment thread miles/utils/arguments.py Outdated
Same three points in two lines: the path is the selection the flag makes,
a plugin path would skip the checks below and train.py's guard, and only
the exact class is recognized.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

The file landed in #3586 unformatted, so `pre-commit` fails repo-wide on
every branch cut from main since; this PR inherited that red check. Only
black reformats it, and the file's AST is unchanged.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

@Shi-Dong
Shi-Dong merged commit 8ec0a11 into main Sep 22, 2026
26 checks passed
@Shi-Dong
Shi-Dong deleted the shi/normalize-fully-async-selection branch September 22, 2026 05:29
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