Repository navigation
fix(verl): avoid replaying completion hooks across async steps - #643
Open
Tan jiahao (JiahaoTanXX) wants to merge 1 commit into
Open
Tan jiahao (JiahaoTanXX) wants to merge 1 commit into
Tan jiahao (JiahaoTanXX) wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the documented lifecycle semantics and includes comprehensive regression coverage.
0 open findings
What changed in this PR
Prevents completion hooks from replaying when asynchronous rollout groups carry across training steps.
Changes:
- Persists hook-processing state on carried rollouts.
- Adds cross-step success, failure, and error-path regressions.
- Documents best-effort and restart semantics.
| File | Description |
|---|---|
agentlightning/verl/agl_rollout_manager.py |
Gates terminal hooks using persisted carry-over state. |
tests/verl/test_async_carry_over_hooks.py |
Tests hook execution across manager recreation and failures. |
docs/35-asynchronous-training.md |
Documents completion-hook lifecycle guarantees. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
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.
When an async prompt group has one finished rollout and one slow sibling, another group can complete first and let collection return. The finished rollout's hook has already run, but the next training step creates a new manager and resets its local finished-ID set. Observing the carried rollout again then replays
on_succeededoron_failed, potentially repeating grading and reward writes.Keep a
completion_hook_processedflag on the carriedEnqueuedRollout. Hooks still run as soon as a terminal state is observed, and the existing best-effort handling of callback/event-write errors is preserved. A failed event read before callback invocation leaves the flag unset. The flag travels with carry-over metadata rather than a manager-local cache; it is not durable exactly-once delivery across trainer restarts.Related to #612. This is a separate lifecycle fix based on
main, independent of the batch-polling endpoint design discussed there. I also merged #612's current head (21d4cb5) into a temporary checkout with this patch: the merge was clean and all 167 CPU tests passed, including its batch-polling tests and these new cross-step regressions.Validation on macOS arm64, Python 3.12.13, with frozen
dev/verl-cpudependencies:python -m pytest -q --durations=10 tests: 160 passed, including 11 new cases. The tests use the real manager, server/store, reward-event writer and completed-rollout conversion with simulated HTTP transport. They recreate managers across three steps, serialize carry-over metadata, cover success/failure, callback exceptions, failed writes and accepted writes with lost responses, and verify rewards, timestamps and rollout cleanup.main; both no-hook controls passed.ruff check .,ruff format --check .,python scripts/check_headers.py,pre-commit run --all-files --show-diff-on-failure, andgit diff --check: passed.pyright: 0 errors, 0 warnings.mkdocs build --strict: passed.uv build --no-sources: sdist and wheel built; verified the wheel contains the marker and the sdist contains the regression tests.No model inference, GPU training or Kubernetes execution was performed. AI assistance: developed and validated with OpenAI Codex.