Repository navigation
fix(verl): prevent stale/failed rollouts from breaking batch completion - #649
Anvarbek Ziyodov (Mr-Perfectuz) wants to merge 2 commits into
Conversation
Reject leftover server results, allow successful retries to overwrite empty failed placeholders, and soften the completed==queued assert so training does not crash when counts drift (microsoft#355).
|
Anvarbek Ziyodov (@Mr-Perfectuz) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
1 similar comment
|
Anvarbek Ziyodov (@Mr-Perfectuz) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
🟡 Changes recommended
The grace-poll branch exits immediately after initialization, preventing successful retries from being collected once all IDs have results.
1 open issue
What changed in this PR
Addresses #355 on stable/v0.2.x by hardening VERL batch completion against stale results and failed rollouts.
Changes:
- Drains stale completions, rejects unknown IDs, and reconciles batch counts.
- Allows successful retries to replace failed placeholders and introduces bounded grace polling.
- Adds bookkeeping regression tests and lightweight VERL import stubs.
| File | Description |
|---|---|
| tests/verl/test_daemon_completed_rollouts.py | Tests failure detection, stale-ID removal, and retry replacement. |
| tests/verl/conftest.py | Provides a VERL stub for unit tests. |
| tests/verl/__init__.py | Marks the test directory as a package. |
| agentlightning/verl/daemon.py | Updates completion filtering, reconciliation, and retry handling. |
Review effort: Balanced
| if failed_grace_polls_remaining is None: | ||
| # A few extra polls (~15s) for in-flight retries to land. | ||
| failed_grace_polls_remaining = 3 | ||
| elif failed_grace_polls_remaining > 0: |
There was a problem hiding this comment.
Good catch — fixed.
The grace counter init used elif, so the first all-present-but-failed poll set the counter and then fell through to break without sleeping. It is now an independent if, and test_async_run_until_finished_grace_polls_when_only_failure_present covers fail-on-first / succeed-on-next.
Co-authored-by: Cursor <cursoragent@cursor.com>

Summary
stable/v0.2.x(AgentModeDaemonwas removed frommain).rollout_id, with a short grace poll window so in-flight retries can land.completed == queuedasserts with reconciliation that drops stale IDs and warns instead of crashing training.This is a fresh take on the closed #480 approach: failed rollouts can still satisfy the wait loop (so permanent backend failures do not hang forever), but they no longer permanently block a later successful retry, and leftover results from previous steps cannot inflate the completed count.
Test plan
pytest -q tests/verl/test_daemon_completed_rollouts.py(4 passed)Completed N/MwithN > Mafter vLLM 400s and confirm the assert no longer firesRelated issues
Made with Cursor