Commit fcaab5d
committed
[train] Async batch collation (double-buffering) for the SFT trainer
# What does this PR do?
The SFT training loop builds each step's batch with a CPU-side slice + collate on the main thread, which blocks the GPU. The per-step slice is fully deterministic from `(global_step, batch_size, len(tokenized))` *given the current data order*, and the order only changes at epoch boundaries (`rng.shuffle`). So step `N+1`'s batch can be collated on a background thread while step `N`'s forward/backward runs on the GPU, hiding the collate latency under GPU compute.
Note this overlaps the in-memory **slice + collation** that assembles each training batch from the already-tokenized dataset; it is not disk/network data prefetching (that is a separate, orthogonal optimization).
This is **default-safe and byte-identical to the serial path**: the collated-ahead batch is exactly what the synchronous loop would have produced for the same step.
## Changes
- **`AsyncBatchCollator`, `skyrl/train/utils/async_batch_collator.py`** — a generic single-slot async double-buffer: a `max_workers=1` `ThreadPoolExecutor` with a one-slot future and a strict `submit`/`get` contract that asserts the in-flight step matches the step the loop expects, turning any mis-wiring into a loud failure rather than a silent training-order corruption. Teardown is exception-safe: `clear()` discards the in-flight batch (and any exception the orphaned worker raised) on a reset/teardown path so it cannot mask the caller's own unwind, and `shutdown()` always joins the executor even if draining the in-flight batch raises.
- **`SFTTrainer.train()`, `skyrl/train/sft_trainer.py`** — wire the async collator into the data-loading block.
- **`SFTConfig.async_batch_collation` (default `True`), `skyrl/train/config/sft_config.py`** — set `False` to A/B against the serial batch-building path. Defaulting to `True` flips the default SFT batch-building path to threaded; it is behavior-preserving (byte-identical batches; see tests) and only changes timing.
### Correctness guarantees
- **Cross-epoch collate-ahead is withheld**: `_can_collate_ahead` mirrors the loop's own reshuffle predicate, so step `N+1` is never collated against the pre-shuffle order. The first step of each new epoch is collated synchronously against the post-shuffle order.
- **Drain before reshuffle**: the in-flight slot is drained (`clear()`) before any reshuffle, so a worker thread can never read `tokenized` while it is being shuffled.
- **No leaked thread, even on exception**: the executor is shut down in the loop's `finally` block (drains the in-flight batch + joins the worker). Teardown is double-fault-safe — if the loop body raises *and* an in-flight collation worker also raised, the original loop exception is what propagates (the worker's exception is swallowed during the reset), and the worker thread is still joined.
## Scope
This is a controller-side **SFT-trainer** optimization. The other trainers do not need it:
- In **synchronous RL** (`trainer.py`), the per-step training batch is built from the *just-generated* rollout (`convert_to_training_input`), so it cannot be collated ahead of generation.
- The **fully async RL** trainer (`fully_async_trainer.py`) already overlaps generation with training via its `asyncio` queue.
- `SFTTrainer.run_eval()` has the same collate-then-dispatch shape that `train()` optimizes here, and (since the eval slice has no wrap-around/reshuffle) would be an even cleaner `AsyncBatchCollator` fit, but it is **intentionally left serial**: eval is off by default (`eval_interval=0`) and infrequent, and the per-batch model forward dominates the small eval collate, so the payoff is marginal. It can adopt the async collator later without API changes.
`AsyncBatchCollator` is written as a generic, reusable component (it holds no trainer state), so a future deterministic per-step producer can reuse it.
## Test plan
- [x] `tests/train/test_async_batch_collation.py` — **7 integration cases + 5 `AsyncBatchCollator` unit tests**:
- The integration cases run the real `SFTTrainer.train()` loop twice over the same dummy dataset/seed (async collation OFF vs ON) across **two epoch boundaries** and assert every `batch` handed to `train_step` is tensor-equal step-for-step — for **both** `DefaultCollator` (FSDP left-pad) and `PackedDataCollator` (Megatron FFD packing), plus an uneven wrap-around case, a mid-epoch resume case, an eval-enabled case, and a `num_epochs`-derived `num_steps` case. Examples have distinct token ids, so a stale pre-shuffle batch would diverge from the baseline and fail. (Mutation-checked: disabling either the cross-epoch withhold or the epoch-boundary drain makes the integration tests fail.)
- A teardown double-fault case: a loop-body `train_step` failure coincides with an in-flight collation worker that *also* raises, asserting the original loop exception propagates (not the worker's re-raised error) and that no `sft-batch-collate` worker thread survives after `train()` returns.
- `AsyncBatchCollator` invariant unit tests: step-mismatch assertion, single-slot guard, worker-exception propagation through `get()`, and `clear()` drain.
- The collate path is pure Python + torch (no numpy), so the packed-collator test needs no Megatron runtime.
- [x] `ruff==0.11.9` + `black==24.10.0` clean.
```bash
uv run --isolated --extra dev --extra fsdp pytest tests/train/test_async_batch_collation.py -v
```1 parent e1b995f commit fcaab5d
4 files changed
Lines changed: 814 additions & 108 deletions
File tree
- skyrl/train
- config
- utils
- tests/train
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
199 | 199 | | |
200 | 200 | | |
201 | 201 | | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
202 | 211 | | |
203 | 212 | | |
204 | 213 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
| 58 | + | |
58 | 59 | | |
59 | 60 | | |
60 | 61 | | |
| |||
1553 | 1554 | | |
1554 | 1555 | | |
1555 | 1556 | | |
1556 | | - | |
1557 | | - | |
1558 | | - | |
1559 | | - | |
1560 | | - | |
1561 | | - | |
1562 | | - | |
1563 | | - | |
1564 | | - | |
1565 | | - | |
1566 | | - | |
1567 | | - | |
1568 | | - | |
1569 | | - | |
1570 | | - | |
1571 | | - | |
1572 | | - | |
1573 | | - | |
1574 | | - | |
1575 | | - | |
1576 | | - | |
1577 | | - | |
1578 | | - | |
1579 | | - | |
1580 | | - | |
1581 | | - | |
1582 | | - | |
1583 | | - | |
1584 | | - | |
1585 | | - | |
1586 | | - | |
1587 | | - | |
1588 | | - | |
1589 | | - | |
1590 | | - | |
1591 | | - | |
1592 | | - | |
1593 | | - | |
1594 | | - | |
1595 | | - | |
1596 | | - | |
1597 | | - | |
1598 | | - | |
1599 | | - | |
1600 | | - | |
1601 | | - | |
1602 | | - | |
1603 | | - | |
1604 | | - | |
1605 | | - | |
1606 | | - | |
1607 | | - | |
1608 | | - | |
1609 | | - | |
1610 | | - | |
1611 | | - | |
1612 | | - | |
1613 | | - | |
1614 | | - | |
1615 | | - | |
1616 | | - | |
1617 | | - | |
1618 | | - | |
1619 | | - | |
1620 | | - | |
1621 | | - | |
1622 | | - | |
1623 | | - | |
1624 | | - | |
1625 | | - | |
1626 | | - | |
1627 | | - | |
1628 | | - | |
1629 | | - | |
1630 | | - | |
1631 | | - | |
1632 | | - | |
1633 | | - | |
1634 | | - | |
1635 | | - | |
1636 | | - | |
1637 | | - | |
1638 | | - | |
1639 | | - | |
1640 | | - | |
1641 | | - | |
| 1557 | + | |
| 1558 | + | |
| 1559 | + | |
| 1560 | + | |
| 1561 | + | |
| 1562 | + | |
| 1563 | + | |
| 1564 | + | |
| 1565 | + | |
| 1566 | + | |
| 1567 | + | |
| 1568 | + | |
| 1569 | + | |
| 1570 | + | |
| 1571 | + | |
| 1572 | + | |
| 1573 | + | |
| 1574 | + | |
| 1575 | + | |
| 1576 | + | |
| 1577 | + | |
| 1578 | + | |
| 1579 | + | |
| 1580 | + | |
| 1581 | + | |
| 1582 | + | |
| 1583 | + | |
| 1584 | + | |
| 1585 | + | |
| 1586 | + | |
| 1587 | + | |
| 1588 | + | |
| 1589 | + | |
| 1590 | + | |
| 1591 | + | |
| 1592 | + | |
| 1593 | + | |
| 1594 | + | |
| 1595 | + | |
| 1596 | + | |
| 1597 | + | |
| 1598 | + | |
| 1599 | + | |
| 1600 | + | |
| 1601 | + | |
1642 | 1602 | | |
1643 | | - | |
1644 | | - | |
1645 | | - | |
| 1603 | + | |
| 1604 | + | |
| 1605 | + | |
| 1606 | + | |
| 1607 | + | |
| 1608 | + | |
| 1609 | + | |
| 1610 | + | |
| 1611 | + | |
| 1612 | + | |
| 1613 | + | |
| 1614 | + | |
| 1615 | + | |
| 1616 | + | |
| 1617 | + | |
| 1618 | + | |
| 1619 | + | |
| 1620 | + | |
| 1621 | + | |
| 1622 | + | |
| 1623 | + | |
| 1624 | + | |
| 1625 | + | |
| 1626 | + | |
| 1627 | + | |
| 1628 | + | |
| 1629 | + | |
| 1630 | + | |
| 1631 | + | |
| 1632 | + | |
| 1633 | + | |
| 1634 | + | |
| 1635 | + | |
| 1636 | + | |
| 1637 | + | |
| 1638 | + | |
| 1639 | + | |
| 1640 | + | |
| 1641 | + | |
| 1642 | + | |
| 1643 | + | |
| 1644 | + | |
| 1645 | + | |
| 1646 | + | |
| 1647 | + | |
| 1648 | + | |
| 1649 | + | |
| 1650 | + | |
| 1651 | + | |
| 1652 | + | |
| 1653 | + | |
| 1654 | + | |
| 1655 | + | |
| 1656 | + | |
| 1657 | + | |
| 1658 | + | |
| 1659 | + | |
| 1660 | + | |
| 1661 | + | |
| 1662 | + | |
| 1663 | + | |
| 1664 | + | |
| 1665 | + | |
| 1666 | + | |
| 1667 | + | |
| 1668 | + | |
1646 | 1669 | | |
| 1670 | + | |
| 1671 | + | |
| 1672 | + | |
| 1673 | + | |
| 1674 | + | |
| 1675 | + | |
| 1676 | + | |
| 1677 | + | |
| 1678 | + | |
| 1679 | + | |
| 1680 | + | |
| 1681 | + | |
| 1682 | + | |
| 1683 | + | |
| 1684 | + | |
| 1685 | + | |
| 1686 | + | |
| 1687 | + | |
| 1688 | + | |
| 1689 | + | |
| 1690 | + | |
| 1691 | + | |
| 1692 | + | |
| 1693 | + | |
| 1694 | + | |
| 1695 | + | |
| 1696 | + | |
| 1697 | + | |
| 1698 | + | |
| 1699 | + | |
| 1700 | + | |
| 1701 | + | |
| 1702 | + | |
| 1703 | + | |
| 1704 | + | |
| 1705 | + | |
| 1706 | + | |
1647 | 1707 | | |
1648 | | - | |
1649 | | - | |
1650 | | - | |
1651 | | - | |
1652 | | - | |
| 1708 | + | |
| 1709 | + | |
| 1710 | + | |
| 1711 | + | |
| 1712 | + | |
1653 | 1713 | | |
1654 | | - | |
1655 | | - | |
1656 | | - | |
1657 | | - | |
1658 | | - | |
1659 | | - | |
1660 | | - | |
1661 | | - | |
1662 | | - | |
1663 | | - | |
1664 | | - | |
1665 | | - | |
1666 | | - | |
1667 | | - | |
| 1714 | + | |
| 1715 | + | |
| 1716 | + | |
| 1717 | + | |
| 1718 | + | |
| 1719 | + | |
| 1720 | + | |
| 1721 | + | |
| 1722 | + | |
| 1723 | + | |
| 1724 | + | |
| 1725 | + | |
| 1726 | + | |
| 1727 | + | |
| 1728 | + | |
| 1729 | + | |
| 1730 | + | |
| 1731 | + | |
| 1732 | + | |
| 1733 | + | |
| 1734 | + | |
| 1735 | + | |
| 1736 | + | |
| 1737 | + | |
| 1738 | + | |
| 1739 | + | |
| 1740 | + | |
| 1741 | + | |
| 1742 | + | |
| 1743 | + | |
| 1744 | + | |
1668 | 1745 | | |
1669 | 1746 | | |
1670 | 1747 | | |
| |||
0 commit comments