Skip to content

fix(verl): avoid replaying completion hooks across async steps - #643

Open
Tan jiahao (JiahaoTanXX) wants to merge 1 commit into
microsoft:mainfrom
JiahaoTanXX:fix/async-carry-over-hooks
Open

Tan jiahao (JiahaoTanXX) wants to merge 1 commit into
microsoft:mainfrom
JiahaoTanXX:fix/async-carry-over-hooks

Conversation

@JiahaoTanXX

Copy link
Copy Markdown
Contributor

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_succeeded or on_failed, potentially repeating grading and reward writes.

Keep a completion_hook_processed flag on the carried EnqueuedRollout. 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-cpu dependencies:

  • 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.
  • Before the fix, the initial eight-case regression produced 6 failed / 2 passed on unmodified 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, and git 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.

Copilot AI balanced review requested due to automatic review settings October 14, 2026 07:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 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

No deployments
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