Skip to content

fix(async): publish weights before the next fully-async drain - #3343

Merged
Shi-Dong merged 2 commits into
radixark:mainfrom
modal-projects:fix/fully-async-weight-update-order
Sep 22, 2026
Merged

Shi-Dong merged 2 commits into
radixark:mainfrom
modal-projects:fix/fully-async-weight-update-order

Conversation

@nanjiangwill

Copy link
Copy Markdown
Contributor

Summary

  • preserve the existing next-rollout lookahead for ordinary pipelined async training
  • on fully-async update steps, publish the new weights before starting the next consumer drain
  • retain fully-async lookahead on steps where the weight version cannot change

Why

The fully-async rollout function owns a persistent producer that continues generating independently of get(). The driver still inherited the older finite-batch pipeline behavior: it started the next get() before training and waited for that batch before publishing weights.

Under fully async, get() is a consumer drain rather than the lifetime of generation. Waiting for it delays publication, does not quiesce the persistent producer, and makes the drain capture the previous weight version for staleness filtering.

This change separates those semantics without depending on an endpoint or transfer protocol. The weight-update protocol remains responsible for pausing, updating, versioning, and resuming engines.

Tests

  • ordinary async still finishes its in-flight next rollout before publication
  • fully async with an update every step starts the next drain after publication
  • fully async preserves safe lookahead between less-frequent weight updates

Tested with:

pytest -q tests/fast/test_train_async.py
pre-commit run --files train_async.py tests/fast/test_train_async.py

@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 pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@Shi-Dong

Copy link
Copy Markdown
Collaborator

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread train_async.py

@Shi-Dong Shi-Dong left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed with local execution at c4f919f: 7/7 driver tests, with an exact fail-before on base (the two new fully-async ordering tests fail; the ordinary pipeline test passes unchanged). Re-verified after #3587 landed on main: the merge is conflict-free, and on the combined tree the driver tests (7/7) and the new argument-resolution tests (5/5) pass — the path-without-flag scenario from the review thread is closed by #3587 (path selection now enables the mode), so that thread can be resolved with a pointer to it.

@Shi-Dong
Shi-Dong merged commit 948c1ba into radixark:main Sep 22, 2026
20 checks passed
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